1007237 - encodeHandoverRequest() unit tests and improvements

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:

encodeHandoverRequest() should be unit tested and those tests should verify it's compliance with NFC Forum's "Bluetooth Secure Simple Pairing Using NFC" specification.

Additionally, I plan to refactor it in a similar fashion I refactored encodeHandoverSelect() (https://bugzilla.mozilla.org/show_bug.cgi?id=1005886).

Also, Bluetooth MAC addresss (MAC-48) parsing should be moved to separate method (now, there is code duplication in encodeHandoverSelect() and encodeHandoverRequest()) and unit tested.

Comment 1

[Kamil Leszczuk [:kamituel]]

parseMAC() I plan to extract from encodeHandover*() methods will be useful in NfcHandoverManager unit tests.

Comment 2

[Kamil Leszczuk [:kamituel]]

@Arno, why is encodeHandoverRequest() putting "b" (HEX: 0x62, DEC: 98) as a payload ID for OOB NDEF record?

Cccording to "Bluetooth Secure Simple Pairing Using NFC", it should be "0" instead (HEX: 0x30, DEC: 40) right? Spec states it in two places at least: figure 1 and table 4 (on page 13 and 15 respectively).

For it to be more interesting, Android seems to be using the same value we are: http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android-apps/4.1.1_r1/com/android/nfc/handover/HandoverManager.java#672

Comment 3

[arno]

The ID can be anything you like (even more than one byte). It needs to be referenced from the Handover Select/Request record. It provides a simple way to reference a record within a record. Note that the 98 also shows up in the first NDEF record.

Comment 4

[Kamil Leszczuk [:kamituel]]

Makes sense, thank you for explanation. Would you mind if I change it to "0" though? Spec uses this value, so it might be a bit more readable; and we're already using "0" in a twin method, encodeHandoverSelect().

Also, encodeHandoverRequest() accepts "rnd" parameter. Do you think we could remove it and generate random value internally? It could be beneficial, because:
1. Client of this method wouldn't be able to *not use* random values.
2. I could test it's randomness in encodeHandoverSelect() unit tests. Otherwise, it should be tested in every place this method is used (which is not many now, but still).

Comment 5

[Kamil Leszczuk [:kamituel]]

Work in progress - removed "rnd" parameter:

https://github.com/kamituel/gaia/commit/57388316aa432aeb610beebdb85e1a3ddbda106f

Comment 6

[arno]

(In reply to Kamil Leszczuk from comment #4)
> Makes sense, thank you for explanation. Would you mind if I change it to "0"
> though? Spec uses this value, so it might be a bit more readable; and we're
> already using "0" in a twin method, encodeHandoverSelect().

Of course. No problem.

> Also, encodeHandoverRequest() accepts "rnd" parameter. Do you think we could
> remove it and generate random value internally?

Well, not sure about that one. For unit testing it would be better to pass it as a parameter so you can check the marshalling. Otherwise you will not know what value was used during marshalling. You wrote "test it's randomness". If by that you mean calling this function n times to see if the value changes, then that wouldn't be a good test.

Comment 7

[Kamil Leszczuk [:kamituel]]

Attachment: encodeHandoverRequest() unit tests and improvements

- Unit tests for encodeHandoverSelect().
- Removed 'rnd' parameter, unit tested by stubbing Math.random()
- Extracted MAC parsing to parseMAC()
- Extracted CPS validation to validateCPS()
- Use hexadecimal number format instead of decimal (to be consistent with encodeHandoverSelect()

Comment 8

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

Comment on attachment 8419767 [details] [review]
encodeHandoverRequest() unit tests and improvements

r=me

Comment 9

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/c52bfa501f6c927a3f93ba7f1b5f7228f016cda4