1240989 - Fix Bluetooth marionette tests test_dom_BluetoothDevice.js and test_dom_BluetoothAdapter_discovery.js on B2G emulator-kk

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Bluetooth, defect, P2)

Description

[Will Wang [:WillWang]]

Fix Bluetooth marionette tests test_dom_BluetoothDevice.js and test_dom_BluetoothAdapter_discovery.js on B2G emulator-kk.

I fire this bug from original bug 1175389 to extract above 2 test cases for better handling of code review and check-in.
We may refer to the original bug for earlier developing record.

Comment 1

[Will Wang [:WillWang]]

Attachment: Patch: Fix test_dom_BluetoothDevice.js and test_dom_BluetoothAdapter_discovery.js

(refer to bug 1175389 comment 36)

Hi Ben,

This patch fix following marionette test cases:
1. test_dom_BluetoothDevice.js (refer to bug 1175389 comment 8)
2. test_dom_BluetoothAdapter_discovery.js (refer to bug 1175389 comment 7)

and also have them enabled in the manifest.ini.

This patch is tested for 100 times and the test log can refer to bug 1175389 comment 39.

Could you help to review this patch?
Thanks!

Comment 2

[Ben Tian (inactive)]

Comment on attachment 8709783 [details] [diff] [review]
Patch: Fix test_dom_BluetoothDevice.js and test_dom_BluetoothAdapter_discovery.js

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

::: dom/bluetooth/tests/marionette/head.js
@@ +601,4 @@
>      }
>    };
>  
> +  runEmulatorCmdSafe("bt notify");

Use |runEmulatorCmdSafe| as [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/tests/marionette/head.js#160

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_discovery.js
@@ +51,4 @@
>  
>    let discoveryHandle = null;
>    return Promise.resolve()
> +    .then(runEmulatorCmdSafe("bt remote add"))

Why not use |addEmulatorRemoteDevice| [2]?

[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/tests/marionette/head.js#158

::: dom/bluetooth/tests/marionette/test_dom_BluetoothDevice.js
@@ +16,4 @@
>  //   [2] Wait for 'devicefound' events.
>  //   [3] Type checking for BluetoothDeviceEvent and BluetoothDevice.
>  //   [4] Attach the 'onattributechanged' handler for the first device.
> +//   [5] Stop discovery.

Why remove UUID test?

@@ +52,4 @@
>    log("adapter.name: " + aAdapter.name);
>  
>    return Promise.resolve()
> +    .then(runEmulatorCmdSafe("bt remote add"))

Why not use |addEmulatorRemoteDevice| [2]?

[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/tests/marionette/head.js#158

@@ +67,4 @@
>  
>        for (let i in deviceEvents) {
>          let deviceEvt = deviceEvents[i];
> +        isnot(deviceEvt.address, null, "deviceEvt.address");

Why this change? |deviceEvt.address| should be null per [3], right?

[3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDiscoveryHandle#ondevicefound

Comment 3

[Will Wang [:WillWang]]

Attachment: rrrun.log-0122-fetchUuid

Thanks you Ben! 
Firstly I reply two of these in short:

(In reply to Ben Tian [:btian] from comment #2)
> Comment on attachment 8709783 [details] [diff] [review]
> Patch: Fix test_dom_BluetoothDevice.js and
> test_dom_BluetoothAdapter_discovery.js
> 
> Review of attachment 8709783 [details] [diff] [review]:
> -----------------------------------------------------------------

> ::: dom/bluetooth/tests/marionette/test_dom_BluetoothDevice.js
> @@ +16,4 @@
> >  //   [2] Wait for 'devicefound' events.
> >  //   [3] Type checking for BluetoothDeviceEvent and BluetoothDevice.
> >  //   [4] Attach the 'onattributechanged' handler for the first device.
> > +//   [5] Stop discovery.
> 
> Why remove UUID test?
It's a misunderstanding in discussion and now I am dealing with this, and have already found some clues for a rejected promise of device.fetchUuids().
Log is as attached for reference.

> @@ +67,4 @@
> >  
> >        for (let i in deviceEvents) {
> >          let deviceEvt = deviceEvents[i];
> > +        isnot(deviceEvt.address, null, "deviceEvt.address");
> 
> Why this change? |deviceEvt.address| should be null per [3], right?
> 
> [3]
> https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/
> BluetoothDiscoveryHandle#ondevicefound
It's also a misunderstanding in discussion, now this is fixed if changing the assertion check to empty string as other address checkings, although the wiki specify it should be "null".

Comment 4

[Ben Tian (inactive)]

(In reply to Will Wang [:WillWang] from comment #3)
> > Why remove UUID test?
> It's a misunderstanding in discussion and now I am dealing with this, and
> have already found some clues for a rejected promise of device.fetchUuids().
> Log is as attached for reference.

So you'll restore UUID test, right?

> > Why this change? |deviceEvt.address| should be null per [3], right?
> > 
> > [3]
> > https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/
> > BluetoothDiscoveryHandle#ondevicefound
> It's also a misunderstanding in discussion, now this is fixed if changing
> the assertion check to empty string as other address checkings, although the
> wiki specify it should be "null".

Yeah you're right. I've revised wiki according to BluetoothDeviceEvent.webidl and BluetoothAdapterEvent.webidl.

Comment 5

[Will Wang [:WillWang]]

(In reply to Ben Tian [:btian] from comment #4)
> (In reply to Will Wang [:WillWang] from comment #3)
> > > Why remove UUID test?
> > It's a misunderstanding in discussion and now I am dealing with this, and
> > have already found some clues for a rejected promise of device.fetchUuids().
> > Log is as attached for reference.
> 
> So you'll restore UUID test, right?
> 
Yes, I am fixing this right now.


> > > Why this change? |deviceEvt.address| should be null per [3], right?
> > > 
> > > [3]
> > > https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/
> > > BluetoothDiscoveryHandle#ondevicefound
> > It's also a misunderstanding in discussion, now this is fixed if changing
> > the assertion check to empty string as other address checkings, although the
> > wiki specify it should be "null".
> 
> Yeah you're right. I've revised wiki according to
> BluetoothDeviceEvent.webidl and BluetoothAdapterEvent.webidl.
Thanks for your feedback!

Comment 6

[Will Wang [:WillWang]]

For the API device.fetchUuids():

I found this API seems not used by any gaia app. 
Bluetooth app don't use this API.
Only system app uses an API which relate to Bluetooth UUID. [1]

I am curious when is the suitable time for using this since API v2 is enabled?
Is this API designed for 3rd party app?

Hi Jocelyn,
May you help to feedback your opinion? Thanks :)


[1]
system/js/bluetooth_transfer.js:102:      var req = adapter.getConnectedDevices(serviceUuid);

Comment 7

[Ben Tian (inactive)]

(In reply to Will Wang [:WillWang] from comment #6)
> I am curious when is the suitable time for using this since API v2 is
> enabled?
> Is this API designed for 3rd party app?

It's part of API v2. The method is designed for apps to query all provided services and decide which to connect/disconnect by themselves. In API v1 gecko may have to decide for apps and extra effort/bugs are introduced.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothDevice#fetchUuids.28.29

> [1]
> system/js/bluetooth_transfer.js:102:      var req =
> adapter.getConnectedDevices(serviceUuid);

This method is irrelevant to |fetchUuid|. It queries all connected devices of certain service uuid.

Comment 8

[Will Wang [:WillWang]]

Attachment: Log: 0127 fetchUuid

For the API device.fetchUuids():

Per offline discussion, following log indicates fetchUuids() method encounter the same SDP issue mentioned in bug 1241052:

01-27 17:55:27.076 W/bt-sdp  (  770): SDP - Wrong type: 0x00 in attr_rsp

Comment 9

[Will Wang [:WillWang]]

Set bug depend to bug 1241052 as above comment mentioned

Comment 10

[Will Wang [:WillWang]]

Attachment: Patch: Bug-1240989-Fix-Bluetooth-marionette-tests-test_dom_.patch

Hi Ben,

Here is the revised patch which can pass the 2 tests(except for the "[6] Verify the UUIDs" part in test_dom_BluetoothDevice.js which need more SDP handling).

Could you help to give feedback?

Thanks!

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8714756 [details] [diff] [review]
Patch: Bug-1240989-Fix-Bluetooth-marionette-tests-test_dom_.patch

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

f=me with comment addressed.

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_discovery.js
@@ +92,5 @@
>      })
>      .then(function(aResults) {
>        log("[6] Wait for 'devicefound' events ... ");
> +      let promises = [];
> +      promises.push(waitForDevicesFound(aResults[1], EXPECTED_NUMBER_OF_REMOTE_DEVICES));

nit: this line is too long (> 80).

@@ +94,5 @@
>        log("[6] Wait for 'devicefound' events ... ");
> +      let promises = [];
> +      promises.push(waitForDevicesFound(aResults[1], EXPECTED_NUMBER_OF_REMOTE_DEVICES));
> +      promises.push(runEmulatorCmdSafe("bt notify"));
> +      return Promise.all(promises);

Suggest to wrap the discovery process into a method to reuse.

::: dom/bluetooth/tests/marionette/test_dom_BluetoothDevice.js
@@ +142,4 @@
>        is(isUuidsChanged, hasReceivedUuidsChanged, "device.uuids has changed.");
>  
>        device.onattributechanged = null;
> +    },function(reason){

nit: space before function and {

@@ +142,5 @@
>        is(isUuidsChanged, hasReceivedUuidsChanged, "device.uuids has changed.");
>  
>        device.onattributechanged = null;
> +    },function(reason){
> +      log("[6] Verify the UUIDs: promise rejected, reason="+reason);

nit: space before and after = and +.

Also please fail test case if promise is rejected.

Comment 12

[Ben Tian (inactive)]

Resolve as WONTFIX since B2G is discontinued.