1006754 - Unit tests for NDEFUtils

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Kamil Leszczuk [:kamituel]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36




Expected results:

NfcManagerUtils (or possibly NDEFUtils when 1005820 gets resolved) needs unit tests. As of now, only encodeHandoverSelect() has proper unit test cases (1005886, merged today). 

Of course, it's possible that while implementing test cases, some refactoring will be done (one idea is to extract MAC address parsing from encodeHandoverSelect() and encodeHandoverRequest() to a method of its own and test it separately).

I'd like to work on that. Please assign this bug to me.

Comment 1

[Kamil Leszczuk [:kamituel]]

1007237 will provide unit tests for encodeHandoverRequest().

Comment 2

[Kamil Leszczuk [:kamituel]]

I have several questions regarding NfcManagerUtils.parseHandoverNDEF() set of methods:

1. We do support parsing collision detection record. However, we don't use it anywhere. Android doesn't even attempt to parse it, they just ignore it [1]. Should we get rid of it?

2. Android handles a special case for Nokia BH-505 headsets, as they apparently use different record format [2]. I don't think we support it. Record's type is "nokia.com:bt" instead of Hs/Hr. Should I fill a bug for that? (I don't own this headset, so I'm not able to test it).

3. We do some complex parsing/matching - for instance, first we look into first record to get all Alternative Carrier fields and then we attempt to match those with another records in message. Hovewer, we never use all of that functionality - what we do immediately after parseHandoverNDEF() is we call .searchForBluetoothAC() which returns *first* record which is Bluetooth OOB record. So maybe we shouldn't bother and parse the whole message, but only find the first OOB record and return it? This would have advantage of eliminating .searchForBluetoothAC() as it would be superfluous.

4. We do discard the whole message if we find record different than Bluetooth OOB. Is that the desired behaviour? Android will still parse this kind of message and only break if no OOB records at all where found in it.

I'm asking all those questions because I've finished writing unit tests for this method (as of current implementation) - https://github.com/kamituel/gaia/commit/c0055e65720998b11297709acdb3907ff67d6c94.
Now I'd like to refactor it a bit. In order to do that, I need to know what functionality I can safely remove (if any).

[1] com.android.nfc.handover.HandoverManager.tryHandoverRequest()
[2] com.android.nfc.handover.HandoverManager.parse()

Comment 3

[Kamil Leszczuk [:kamituel]]

Update for point 2 - bug 1011387 has been filled: https://bugzilla.mozilla.org/show_bug.cgi?id=1011387.

Comment 4

[arno]

(In reply to Kamil Leszczuk from comment #2)
> 1. We do support parsing collision detection record. However, we don't use
> it anywhere. Android doesn't even attempt to parse it, they just ignore it
> [1]. Should we get rid of it?

According to the specs this field needs to be present so I suggest we also check this. The only thing you can simplify is the assembly of the two bytes to the CR value, but I don't see any harm to leave that code in there.

> 2. Android handles a special case for Nokia BH-505 headsets, as they
> apparently use different record format [2]. I don't think we support it.
> Record's type is "nokia.com:bt" instead of Hs/Hr. Should I fill a bug for
> that? (I don't own this headset, so I'm not able to test it).

Yes, see my comment for bug 1011387.

> 3. We do some complex parsing/matching - for instance, first we look into
> first record to get all Alternative Carrier fields and then we attempt to
> match those with another records in message. Hovewer, we never use all of
> that functionality - what we do immediately after parseHandoverNDEF() is we
> call .searchForBluetoothAC() which returns *first* record which is Bluetooth
> OOB record. So maybe we shouldn't bother and parse the whole message, but
> only find the first OOB record and return it? This would have advantage of
> eliminating .searchForBluetoothAC() as it would be superfluous.

I suggest to leave this logic. Right now we don't support Wifi Direct but FFOS might in the future. Right now our code can only handle BT ACs but parsing the complete message will make it easier to support other carriers in the future.

> 4. We do discard the whole message if we find record different than
> Bluetooth OOB. Is that the desired behaviour? Android will still parse this
> kind of message and only break if no OOB records at all where found in it.

Not sure I understand this question. If there is no OOB record there is nothing we can do.

Comment 5

[Kamil Leszczuk [:kamituel]]

Ad 1 and 3 - thanks for explanation, I'll follow your advice.

> 
> > 4. We do discard the whole message if we find record different than
> > Bluetooth OOB. Is that the desired behaviour? Android will still parse this
> > kind of message and only break if no OOB records at all where found in it.
> 
> Not sure I understand this question. If there is no OOB record there is
> nothing we can do.

I messed up this question ;) sorry. I meant that when we're parsing embedded NDEF's and we stumble upon anything different that NDEF with .type = "ac", we throw an error/return null. Even if this message contains valid "ac" record pointing to Bluetooth OOB record (so it could be used). For the same message, Android would parse it and attempt to pair.

Comment 6

[arno]

(In reply to Kamil Leszczuk from comment #5)
> I messed up this question ;) sorry. I meant that when we're parsing embedded
> NDEF's and we stumble upon anything different that NDEF with .type = "ac",
> we throw an error/return null. Even if this message contains valid "ac"
> record pointing to Bluetooth OOB record (so it could be used). For the same
> message, Android would parse it and attempt to pair.

Ah, ok. It makes sense to just silently ignore AC records we don't understand.

Comment 7

[Kamil Leszczuk [:kamituel]]

Class name has changed as result of refactoring done in 1005820.

Comment 8

[Kamil Leszczuk [:kamituel]]

Attachment: NDEFUtils unit tests and minor refactor

Hi Arno! Before I ask @Alive, it'd like to ask you for review first, as you know this code inside out. 

I followed your advice, and I tried not to change too much before 2.0 release. It is a refactor, though, so it's good to be extra careful with it. 

As you will see, I've written extensive unit tests, in large part for parseHandoverNDEF(), because I intended to work on it a bit. After refactor, it passes all tests. The only functional difference I've made is that now this test case passes (while previously it would fail):

>    test('Decodes even when records other than "cr"/"ac" are present',
>      function() {
> 
>      var hin = NDEFUtils.encodeHandoverRequest(mac, 1);
>      var payload = Array.apply([], hin[0].payload);
>      // 0x63, 0x63 is "cc" record which isn't supported
>      // (probably doesn't even exist ;))
>      payload = payload.concat([0x51, 0x02, 0x02, 0x63, 0x63, 0x00, 0x01]);
>      // Previously last records isn't last anymore, so remove ME bit.
>      payload[8] = 0x11;
>      hin[0].payload = new Uint8Array(payload);
>      var hout = NDEFUtils.parseHandoverNDEF(hin);
> 
>      assert.isNotNull(hout);
>    });

Other tests include those for searchForBluetoothAC(), parseBluetoothSSP() and new helper method formatMAC() (complementary to parseMAC()).

(Travis build is still in progress, but I think it's okay to review at any time now. If it fails, I'll fix it before r?'ing Alive).

Comment 9

[arno]

Comment on attachment 8425591 [details] [review]
NDEFUtils unit tests and minor refactor

LGTM. I left one comment/suggestion on Github. Also, Travis failed.

Comment 10

[Kamil Leszczuk [:kamituel]]

Comment on attachment 8425591 [details] [review]
NDEFUtils unit tests and minor refactor

Build failed again, this time with "No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.". I'm not sure what's happening to Travis recently, but it's not because of my commit.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

Just a note since the change is a little big I will read it carefully later. Sorry for that.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Kami, per Alive's comment https://bugzilla.mozilla.org/show_bug.cgi?id=1013853#c2
Do you think we should const the 'application/vnd.bluetooth.ep.oob' string into NDEF?
Would you like to handle Bug 1013853 here or rebase on my patch?

I am okay either way, even mark Bug 1013853 as invalid, duplicated,... is fine for me.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8425591 [details] [review]
NDEFUtils unit tests and minor refactor

Could you see Yoshi's comment and reach a consensus with Yoshi for bt oob?

Comment 15

[Kamil Leszczuk [:kamituel]]

Not sure if I need to ask for review again - Alive, since you've marked it as r+, I won't do that.
Yoshi's changes from 1013853 are incorporated now.

I've tried to build it with Travis three times actually, getting different errors every time, but always not related to my patch. So I think it's not related.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/58016de39df214460b4a2deb254a6a553d44aba3