Bug 1035651 (webbt-test-device)

Write marionette tests for BluetoothDevice

RESOLVED FIXED in 2.0 S6 (18july)

Status

Firefox OS
Bluetooth
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ben Tian (inactive), Assigned: jaliu)

Tracking

unspecified
2.0 S6 (18july)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webbt-api])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Write marionette tests for BluetoothDevice API.
(Reporter)

Updated

4 years ago
Whiteboard: [webbt-api]
(Reporter)

Comment 1

4 years ago
Jamin, please help on this bug. Thanks.
Assignee: nobody → jaliu
Blocks: 1005848
Depends on: 1006315
(Reporter)

Updated

4 years ago
Alias: webbt-test-device
(Assignee)

Updated

4 years ago
Target Milestone: --- → 2.0 S6 (18july)
(Assignee)

Comment 2

4 years ago
Created attachment 8453706 [details] [diff] [review]
Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v1)
Attachment #8453706 - Flags: review?(btian)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 1037291
(Reporter)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Created attachment 8455870 [details] [diff] [review]
Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v2)
Attachment #8453706 - Attachment is obsolete: true
Attachment #8453706 - Flags: review?(btian)
Attachment #8455870 - Flags: review?(btian)
(Assignee)

Comment 5

4 years ago
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.
(Reporter)

Comment 6

4 years ago
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. :)
(Reporter)

Comment 7

4 years ago
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'.
(Assignee)

Comment 8

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

Revise Attachment #8455870 [details] [diff] based on #Comment 7.
Attachment #8455870 - Attachment is obsolete: true
Attachment #8455870 - Flags: review?(btian)
Attachment #8455947 - Flags: review?(btian)
(Reporter)

Comment 9

4 years ago
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.
Attachment #8455947 - Flags: review?(btian) → review+
(Assignee)

Comment 10

4 years ago
Created attachment 8456606 [details] [diff] [review]
(final) Write a marionette test for BluetoothDevice based on Bluetooth API v2 (v4), r=btian

Thank Ben for reviewing the patch.
Attachment #8455947 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef6a44c2fa8b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.