1035651 - (webbt-test-device) Write marionette tests for BluetoothDevice

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Write marionette tests for BluetoothDevice API.

Comment 1

[Ben Tian (inactive)]

Jamin, please help on this bug. Thanks.

Comment 2

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v1)

Comment 3

[Ben Tian (inactive)]

Comment on attachment 8453706 [details] [diff] [review]
Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v1)

Review of attachment 8453706 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth2/tests/marionette/head.js
@@ +600,5 @@
>  
>  /**
> + * Wait for 'devicefound' events of specified devices.
> + *
> + * Resolve if that every specified devices has been found.  Never reject.

nit:
- 'Resolve if all specified devices have been found.'
- remove extra space before Never.

@@ +613,5 @@
> + *        An array which contains addresses of remote devices.
> + *
> + * @return A deferred promise.
> + */
> +function waitForSpecifiedDevicesFound(aDiscoveryHandle, aRemoteAddresses) {

Where is the function called? It's not called in test_dom_BluetoothDevice_API2.js.

@@ +624,5 @@
> +  aDiscoveryHandle.ondevicefound = function onDeviceFound(aEvent) {
> +    ok(aEvent instanceof BluetoothDeviceEvent,
> +      "aEvent should be a BluetoothDeviceEvent");
> +
> +    if (aRemoteAddresses.indexOf(aEvent.device.address) != -1) {

We should filter out duplicate devices.

@@ +627,5 @@
> +
> +    if (aRemoteAddresses.indexOf(aEvent.device.address) != -1) {
> +      devicesArray.push(aEvent);
> +    }
> +    if (devicesArray.length == aRemoteAddresses.length) {

Move this |if| into |if (addresses.indexOf != -1)|.

@@ +647,5 @@
> + *        Another array which contains uuid strings.
> + *
> + * @return A boolean value.
> + */
> +function isUuidsEqual(aUuids_1, aUuids_2) {

- Rename function to |isEqualArray| since it can be used not only for uuid arrays. 
- Rename parameters to |aArray1| and |aArray2|.

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothDevice_API2.js
@@ +131,5 @@
> +
> +      ok(Array.isArray(uuids), "type checking for 'fetchUuids'.");
> +
> +      ok(isUuidsEqual(uuids, device.uuids),
> +        "Bluetoodh.device.uuids should equal to the result from 'fetchUuids'");

Typo: 'device.uuids' should ...

@@ +135,5 @@
> +        "Bluetoodh.device.uuids should equal to the result from 'fetchUuids'");
> +
> +      if (isUuidsEqual(originalUuids, device.uuids) == false) {
> +        is(hasReceivedUuidsChanged, true, "Uuids has changed.");
> +      }

We should also check condition of unchanged uuids array.

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v2)

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

Thank you for your comments.
I've uploaded a new patch base on your comments.

(In reply to Ben Tian [:btian] from comment #3)
> Comment on attachment 8453706 [details] [diff] [review]
> @@ +613,5 @@
> > + *        An array which contains addresses of remote devices.
> > + *
> > + * @return A deferred promise.
> > + */
> > +function waitForSpecifiedDevicesFound(aDiscoveryHandle, aRemoteAddresses) {
> 
> Where is the function called? It's not called in
> test_dom_BluetoothDevice_API2.js.

It wouldn't be called in this test case.
I wrote this one as an utility function in case other developer may need it later.
After having an offline discussion with you, I decided to remove this function from patch Attachment #8455870 [details] [diff]. I plan to add it back when we really have a chance to use it in test case.

> @@ +647,5 @@
> > + *        Another array which contains uuid strings.
> > + *
> > + * @return A boolean value.
> > + */
> > +function isUuidsEqual(aUuids_1, aUuids_2) {
> 
> - Rename function to |isEqualArray| since it can be used not only for uuid
> arrays. 
> - Rename parameters to |aArray1| and |aArray2|.

I prefer to keep the original name since the function can't compare two arrays of any type correctly.

Since Array_Compare function may need to compare arrays which contain another array, the recursive mechanism should be implement for that case. 

Furthermore, the function have to handle the compare different types of object.
For examples, we have to implement the method to compare javascript dictionary, function, NaN, and the object which contains arrays. It's easy to implement these comparison methods, however, it's hard to add proper description for this function due to the controversial definition of 'equal' of some objects.

Since we only need to compare two uuids in our test suits, I wrote a uuids comparison function here.
Of course, it's open for discussion. :)

> @@ +135,5 @@
> > +        "Bluetoodh.device.uuids should equal to the result from 'fetchUuids'");
> > +
> > +      if (isUuidsEqual(originalUuids, device.uuids) == false) {
> > +        is(hasReceivedUuidsChanged, true, "Uuids has changed.");
> > +      }
> 
> We should also check condition of unchanged uuids array.

Yes. Thank you for reminding me.

Comment 6

[Ben Tian (inactive)]

Agree, but the naming is still unconventional. How about function name |isEqualUuidArray| and parameters |aUuidArray1| and |aUuidArray2|? Two reasons: 1) the names state they are for arrays and 2) we seldom use '_' to name parameter/variables.

(In reply to Jamin Liu [:jaliu] from comment #5)
> I prefer to keep the original name since the function can't compare two
> arrays of any type correctly.
> 
> Since Array_Compare function may need to compare arrays which contain
> another array, the recursive mechanism should be implement for that case. 
> 
> Furthermore, the function have to handle the compare different types of
> object.
> For examples, we have to implement the method to compare javascript
> dictionary, function, NaN, and the object which contains arrays. It's easy
> to implement these comparison methods, however, it's hard to add proper
> description for this function due to the controversial definition of 'equal'
> of some objects.
> 
> Since we only need to compare two uuids in our test suits, I wrote a uuids
> comparison function here.
> Of course, it's open for discussion. :)

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8455870 [details] [diff] [review]
Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v2)

Review of attachment 8455870 [details] [diff] [review]:
-----------------------------------------------------------------

Please see comment 6 and my comment below.

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothDevice_API2.js
@@ +4,5 @@
> +
> +///////////////////////////////////////////////////////////////////////////////
> +// Test Purpose:
> +//   To verify that all properties of BluetoothDevice is correct when they've
> +//   been discoveried by adapter and delivered by BluetoothDeviceEvent.

typo: discovered.

@@ +5,5 @@
> +///////////////////////////////////////////////////////////////////////////////
> +// Test Purpose:
> +//   To verify that all properties of BluetoothDevice is correct when they've
> +//   been discoveried by adapter and delivered by BluetoothDeviceEvent.
> +//   Tester have to put the B2G devices to an environment which is surrounded by

nit: Tester's' ... put B2G devices 'in' an environment

@@ +7,5 @@
> +//   To verify that all properties of BluetoothDevice is correct when they've
> +//   been discoveried by adapter and delivered by BluetoothDeviceEvent.
> +//   Tester have to put the B2G devices to an environment which is surrounded by
> +//   N discoverable remote devices. To pass this test, the number N has to be
> +//   greater than EXPECTED_NUMBER_OF_REMOTE_DEVICES.

'be greater than or equals to'

@@ +133,5 @@
> +
> +      ok(isUuidsEqual(uuids, device.uuids),
> +        "device.uuids should equal to the result from 'fetchUuids'");
> +
> +      bool hasUuidsChanged = !isUuidsEqual(originalUuids, device.uuids);

I suggest to rename to |isUuidsChanged| since |hasUuidsChanged| misses verb between 'has' and 'UuidsChanged'.

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v3)

Revise Attachment #8455870 [details] [diff] based on #Comment 7.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8455947 [details] [diff] [review]
Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v3)

Review of attachment 8455947 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit addressed. Thanks.

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothDevice_API2.js
@@ +141,5 @@
> +    })
> +  .then(function() {
> +      log("[7] Stop discovery ... ");
> +      return aAdapter.stopDiscovery();
> +  })

nit: indent this section.

Comment 10

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (final) Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v4), r=btian

Thank Ben for reviewing the patch.

Comment 11

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/ef6a44c2fa8b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef6a44c2fa8b