1240992 - Fix Bluetooth marionette test [test_dom_BluetoothAdapter_pair.js] on B2G emulator-kk

Status: RESOLVED WONTFIX

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

Description

[Will Wang [:WillWang]]

Fix Bluetooth marionette test [test_dom_BluetoothAdapter_pair.js]on B2G emulator-kk

I fire this bug from original bug 1175389 to extract above test case 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: Log: 0128-change-appOrigin

The Job mentioned in bug 1175389 comment 13(and 9,10,11) is done.

That is, now the marionette test(which has a system app origin and won't pass the check) can dynamically change app origin of Bluetooth certified app to system app to pass the check.

Method for reference:
https://developer.mozilla.org/en-US/docs/SpecialPowers

Bluetooth app origin pref in b2g.js:
https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js?from=b2g.js#1123-1125

Log:

01-28 16:55:52.050 I/Gecko   (   64): MARIONETTE LOG: INFO: ==== original dom.bluetooth.app-origin is:app://bluetooth.gaiamobile.org
01-28 16:55:52.070 I/Gecko   (   64): MARIONETTE LOG: INFO: ==== now change dom.bluetooth.app-origin to:app://system.gaiamobile.org
......
01-28 16:55:52.190 I/Gecko   (   64): MARIONETTE LOG: INFO: navigator.mozBluetooth is available
01-28 16:55:52.190 I/Gecko   (   64): MARIONETTE LOG: INFO: Wait for creating bluetooth adapter...
01-28 16:55:52.250 I/GeckoBluetooth(   64): BluetoothAdapter: ==== enter ctor
01-28 16:55:52.250 I/GeckoBluetooth(   64): IsBluetoothCertifiedApp: ==== enter
01-28 16:55:52.250 I/GeckoBluetooth(   64): IsBluetoothCertifiedApp: ==== appOrigin= [app://system.gaiamobile.org]
01-28 16:55:52.250 I/GeckoBluetooth(   64): BluetoothAdapter: ==== IsBluetoothCertifiedApp check: yes

Full log is as attached.

Comment 2

[Will Wang [:WillWang]]

Attachment: Patch: Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

Hi Ben,

Here is the patch which can pass this pairing marionette test,
could you help to review?
Thanks!

Comment 3

[Ben Tian (inactive)]

Comment on attachment 8715762 [details] [diff] [review]
Patch: Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

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

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +50,4 @@
>  MARIONETTE_TIMEOUT = 60000;
>  MARIONETTE_HEAD_JS = 'head.js';
>  
> +const ADDRESS_OF_TARGETED_REMOTE_DEVICE = "ba:be:ba:d0:00:01";

Why change the device address?

@@ +66,4 @@
>    log("adapter.name: " + aAdapter.name);
>  
>    return Promise.resolve()
> +    .then(addEmulatorRemoteDevice())

Does the remote device's address have to be |ADDRESS_OF_TARGETED_REMOTE_DEVICE|? If so, how do you ensure the requirement?

@@ +85,4 @@
>      })
>      .then(function(discoveryHandle) {
>        log("[3] Wait for the specified 'devicefound' event ... ");
> +      let promises = [];

Wrap the discovery process into a method to reuse, as I mentioned in bug 1240989 comment 11.

@@ +92,2 @@
>      })
> +    .then(function(aResult) {

aResult's' as patch in bug 1240989 comment 10.

@@ +128,4 @@
>        log("[8] Unpair ... ");
>        let promises = [];
>        promises.push(waitForAdapterEvent(aAdapter, "deviceunpaired"));
> +      promises.push(aAdapter.unpair(ADDRESS_OF_TARGETED_REMOTE_DEVICE));

Why this change?

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8715762 [details] [diff] [review]
Patch: Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

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

Some more comments.

::: dom/bluetooth/tests/marionette/head.js
@@ +759,5 @@
> +      function onReject() {
> +        log("  - 'accept' reject.");
> +        cleanupPairingListener(aAdapter.pairingReqs);
> +      });
> +

nit: remove extra newline.

@@ +847,5 @@
>  }
>  
>  function startBluetoothTest(aReenable, aTestCaseMain) {
> +
> +  // Change app origin of Bluetooth to pass IsBluetoothCertifiedApp() check in gecko

Set app origin for pair test case only.

Also revise comment as
  // Change bluetooth app origin to gain permission for pairing request reply

@@ +848,5 @@
>  
>  function startBluetoothTest(aReenable, aTestCaseMain) {
> +
> +  // Change app origin of Bluetooth to pass IsBluetoothCertifiedApp() check in gecko
> +  log("original dom.bluetooth.app-origin is:"+ SpecialPowers.getCharPref("dom.bluetooth.app-origin") );

Remove this line since we don't care about old app origin.

@@ +849,5 @@
>  function startBluetoothTest(aReenable, aTestCaseMain) {
> +
> +  // Change app origin of Bluetooth to pass IsBluetoothCertifiedApp() check in gecko
> +  log("original dom.bluetooth.app-origin is:"+ SpecialPowers.getCharPref("dom.bluetooth.app-origin") );
> +  SpecialPowers.setCharPref("dom.bluetooth.app-origin","app://system.gaiamobile.org");

nit: this line is too long, also add space after ','

@@ +850,5 @@
> +
> +  // Change app origin of Bluetooth to pass IsBluetoothCertifiedApp() check in gecko
> +  log("original dom.bluetooth.app-origin is:"+ SpecialPowers.getCharPref("dom.bluetooth.app-origin") );
> +  SpecialPowers.setCharPref("dom.bluetooth.app-origin","app://system.gaiamobile.org");
> +  log("now change dom.bluetooth.app-origin to:"+ SpecialPowers.getCharPref("dom.bluetooth.app-origin") );

nit: this line is too long. Also revise log to:
  "Change bluetooth app origin to: ", ...

Comment 5

[Will Wang [:WillWang]]

Attachment: Patch(v2): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

Hi Ben,

Here is the patch which revises all points you mentioned above,
and it's tested.

Could you help to review?
Thanks!

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8719401 [details] [diff] [review]
Patch(v2): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

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

::: dom/bluetooth/tests/marionette/head.js
@@ +581,4 @@
>   * @param aExpectedNumberOfDevices
>   *        The number of remote devices we expect to discover.
>   *
> + * @return A promise object.

Why revise the comment?

@@ -751,5 @@
>      ok(typeof passkey === 'string', "type checking for passkey.");
>      log("  - Received 'onpairingconfirmationreq' event with passkey: " + passkey);
>  
> -    let device = evt.device;
> -    if (!aSpecifiedBdAddress || device.address == aSpecifiedBdAddress) {

We still need to match device address, right?

@@ +868,4 @@
>    return deferred.promise;
>  }
>  
> +function setBTAppOrigin(aOrigin) {

Leave comment to explain function usage as |waitForAdapterAttributeChanged|.

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +94,2 @@
>      })
> +    .then(function(aResults) {

nit: do we use a* for all arguments here? Any reason to not conform with existing style?

@@ +110,3 @@
>        log("[5] Pair and wait for confirmation ... ");
>  
> +      addEventHandlerForPairingRequest(aAdapter, ADDRESS_OF_TARGETED_REMOTE_DEVICE);

Why do we still need to predefine device address? Can't we extract from device.address? Doesn't the last promise's return pass device to this promise?

@@ +144,5 @@
>        return aAdapter.stopDiscovery();
> +    })
> +    .then(function() {
> +      log("[12] Change bluetooth app origin back ... ");
> +      setBTAppOrigin("app://bluetooth.gaiamobile.org");

Why not restore app origin after |startBluetoothTest|, no matter it's resolved or not?

Also remember old app origin to restore instead of reassignment.

Comment 7

[Will Wang [:WillWang]]

Hi Ben,

Thanks for your review, my opinion is as following.

(In reply to Ben Tian [:btian] from comment #6)
> Comment on attachment 8719401 [details] [diff] [review]
> Patch(v2): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch
> 
> Review of attachment 8719401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/tests/marionette/head.js
> @@ +581,4 @@
> >   * @param aExpectedNumberOfDevices
> >   *        The number of remote devices we expect to discover.
> >   *
> > + * @return A promise object.
> 
> Why revise the comment?
> 

My idea is to slightly reflect the literally changed return value which is not a "deferred.promise" anymore, as two other functions in head.js did.


> @@ -751,5 @@
> >      ok(typeof passkey === 'string', "type checking for passkey.");
> >      log("  - Received 'onpairingconfirmationreq' event with passkey: " + passkey);
> >  
> > -    let device = evt.device;
> > -    if (!aSpecifiedBdAddress || device.address == aSpecifiedBdAddress) {
> 
> We still need to match device address, right?
> 
Per offline discussion and IMO, we may not need this:
1. BluetoothPairingEvent in API v2 doesn't have an address member [1]
2. Original(deleted) condition accepts a null address
3. Current test in emulator have only one remote device

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


> @@ +868,4 @@
> >    return deferred.promise;
> >  }
> >  
> > +function setBTAppOrigin(aOrigin) {
> 
> Leave comment to explain function usage as |waitForAdapterAttributeChanged|.
> 
I will add this, thanks for your advice.

> ::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
> @@ +94,2 @@
> >      })
> > +    .then(function(aResults) {
> 
> nit: do we use a* for all arguments here? Any reason to not conform with
> existing style?
> 
So far I observed that most marionette tests use aResults instead within similar context,
especially 5/6 tests in bluetooth module use aResults as well.(except for this pairing test)
Therefore I would like to modify to keep naming aligned.


> @@ +110,3 @@
> >        log("[5] Pair and wait for confirmation ... ");
> >  
> > +      addEventHandlerForPairingRequest(aAdapter, ADDRESS_OF_TARGETED_REMOTE_DEVICE);
> 
> Why do we still need to predefine device address? Can't we extract from
> device.address? Doesn't the last promise's return pass device to this
> promise?
> 
IMO using predefined device address can remove/avoid unnecessary argument passing to increase readability and decrease the complexity. (or you see some sort of need?)


> @@ +144,5 @@
> >        return aAdapter.stopDiscovery();
> > +    })
> > +    .then(function() {
> > +      log("[12] Change bluetooth app origin back ... ");
> > +      setBTAppOrigin("app://bluetooth.gaiamobile.org");
> 
> Why not restore app origin after |startBluetoothTest|, no matter it's
> resolved or not?
> 
> Also remember old app origin to restore instead of reassignment.
Thanks you for reminding this good point, I will improve this!

Comment 8

[Ben Tian (inactive)]

(In reply to Will Wang [:WillWang] from comment #7)
> My idea is to slightly reflect the literally changed return value which is
> not a "deferred.promise" anymore, as two other functions in head.js did.

I see. 

> Per offline discussion and IMO, we may not need this:
> 1. BluetoothPairingEvent in API v2 doesn't have an address member [1]
> 2. Original(deleted) condition accepts a null address
> 3. Current test in emulator have only one remote device
> 
> [1]
> https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/
> BluetoothPairingEvent#BluetoothPairingEvent

Remove redundant |aSpecifiedBdAddress| since pairing event carries no device address.

> So far I observed that most marionette tests use aResults instead within
> similar context,
> especially 5/6 tests in bluetooth module use aResults as well.(except for
> this pairing test)
> Therefore I would like to modify to keep naming aligned.

Got it. Thanks for the revision.

> IMO using predefined device address can remove/avoid unnecessary argument
> passing to increase readability and decrease the complexity. (or you see
> some sort of need?)

But it doesn't match real use case. I still prefer to extract address from device event instead of predefined one.

> > Why not restore app origin after |startBluetoothTest|, no matter it's
> > resolved or not?
> > 
> > Also remember old app origin to restore instead of reassignment.
> Thanks you for reminding this good point, I will improve this!

One option is to store result of original promise sequence (say A), call A.then(/* restore app origin */), and return A finally. Also is adapter of new app origin created both before and after pairing test, to ensure change take effect?

Comment 9

[Will Wang [:WillWang]]

Attachment: Patch(v3): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

Hi Ben,

Here is the patch which is revised as comment above,
and it's tested for many other cases (promise rejected, |setBtAppOrigin| misused, ...).
could you help to review?

Thanks!

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8720687 [details] [diff] [review]
Patch(v3): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

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

::: dom/bluetooth/tests/marionette/head.js
@@ +867,5 @@
> +  if (aOrigin) { // Set
> +    defaultBtAppOrigin = SpecialPowers.getCharPref("dom.bluetooth.app-origin");
> +    SpecialPowers.setCharPref("dom.bluetooth.app-origin", aOrigin);
> +  } else { // Restore
> +    if (defaultBtAppOrigin) {

Store original app origin in test_dom_BluetoothAdapter_pair.js instead of here. Your flow results in non-reentry of the method and doesn't reset |defaultBtAppOrigin|.

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +13,1 @@
>  //   ADDRESS_OF_TARGETED_REMOTE_DEVICE.

Revise the comment since ADDRESS_OF_TARGETED_REMOTE_DEVICE is removed.

@@ +52,4 @@
>  MARIONETTE_TIMEOUT = 60000;
>  MARIONETTE_HEAD_JS = 'head.js';
>  
> +const ADDRESS_OF_TARGETED_REMOTE_DEVICE = "ba:be:ba:d0:00:01";

Remove the redundant constant.

@@ +109,4 @@
>        log("  - BluetoothDevice.paired: " + device.paired);
>        log("  - BluetoothDevice.uuids: " + device.uuids);
>  
> +      remoteDeviceAddress = device.address;

Remember address after/before pair() since it's address to pair rather than address discovered.

@@ +150,5 @@
>        log("[11] Stop discovery ... ");
>        return aAdapter.stopDiscovery();
> +    })
> +    .then(function() {
> +      log("[12-A] Change bluetooth app origin back ... ");

Revise log to "[12] Change ..."

@@ +154,5 @@
> +      log("[12-A] Change bluetooth app origin back ... ");
> +      setBtAppOrigin();
> +    })
> +    .catch(function() {
> +      log("[12-B] One of promise rejected, change bluetooth app origin back ... ");

Revise log to

  One of the promises is rejected, change bluetooth app origin back ...

since here may run after any of the promises.

@@ +156,5 @@
> +    })
> +    .catch(function() {
> +      log("[12-B] One of promise rejected, change bluetooth app origin back ... ");
> +      setBtAppOrigin();
> +      throw 'Clean end, throw to fail the test.';

Leave comment to indicate this line rejects the promise chain.

Comment 11

[Will Wang [:WillWang]]

Attachment: Patch(v4): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

Hi Ben,

Could you help to review this revised patch?

- After investigating the python script of marionette test, the script basically start a VM, then run each marionette test in a linear sequence, and shutdown the VM in the end. That is, no two tests will run in parallel, which means some of race condition issues are eased.
- Remove the "pre-unpair" procedure since now the device we are going to pair is always newly added. fresh.
- Remove several procedures which are already included in other test and not needed by this pairing test.
- Revise comments.
- Other improvements.

Thanks!

Comment 12

[Will Wang [:WillWang]]

Marionette log for above patch FYR:

 0:00.00 LOG: MainThread INFO Using workspace for temporary data: "/home/will/repooooo/b2ggg/emulator-kk"
 0:00.00 LOG: MainThread INFO Profile destination is TMP
waiting for system-message-listener-ready...
...done
 1:29.51 LOG: MainThread INFO starting httpd
 1:29.51 LOG: MainThread INFO running httpd on http://10.247.24.131:52338/
 1:31.38 LOG: MainThread INFO Android sdk version '19'; will use this to filter manifests
 2:12.34 LOG: MainThread mozversion INFO application_buildid: 20160219175252
 2:12.34 LOG: MainThread mozversion INFO application_display_name: B2G
 2:12.34 LOG: MainThread mozversion INFO application_id: {3c2e2abc-06d4-11e1-ac3b-374f68613e61}
 2:12.34 LOG: MainThread mozversion INFO application_name: B2G
 2:12.34 LOG: MainThread mozversion INFO application_remotingname: b2g
 2:12.34 LOG: MainThread mozversion INFO application_vendor: Mozilla
 2:12.34 LOG: MainThread mozversion INFO application_version: 46.0a1
 2:12.34 LOG: MainThread mozversion INFO device_firmware_date: 1439793977
 2:12.34 LOG: MainThread mozversion INFO device_firmware_version_incremental: eng.will.20150817.144534
 2:12.34 LOG: MainThread mozversion INFO device_firmware_version_release: 4.4.2
 2:12.34 LOG: MainThread mozversion INFO device_id: generic
 2:12.34 LOG: MainThread mozversion INFO gaia_changeset: 97266c579c544f5ba57a701f39893cc86d46774a
 2:12.34 LOG: MainThread mozversion INFO gaia_date: 1449093766
 2:12.34 LOG: MainThread mozversion INFO platform_buildid: 20160219175252
 2:12.34 LOG: MainThread mozversion INFO platform_version: 46.0a1
 2:12.34 SUITE_START: MainThread 1
 2:13.60 TEST_START: MainThread test_dom_BluetoothAdapter_pair.js
 2:36.07 TEST_END: MainThread PASS
 2:36.24 LOG: MainThread INFO START LOG:
 2:36.24 LOG: MainThread INFO INFO TEST-START: /home/will/repooooo/b2ggg/emulator-kk/gecko/dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js Fri Feb 26 2016 18:46:18 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO   - Change bluetooth app origin to: [app://system.gaiamobile.org] Fri Feb 26 2016 18:46:24 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO navigator.mozBluetooth is available Fri Feb 26 2016 18:46:24 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO Wait for creating bluetooth adapter... Fri Feb 26 2016 18:46:24 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO bluetoothManager.defaultAdapter.state: disabled Fri Feb 26 2016 18:46:25 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO Original state of bluetooth is disabled Fri Feb 26 2016 18:46:25 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO Enable Bluetooth ... Fri Feb 26 2016 18:46:25 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO Checking adapter attributes ... Fri Feb 26 2016 18:46:25 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO adapter.address: 56:34:12:00:54:52 Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO adapter.name: Full Android on Emulator Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO [1] Pair and wait for confirmation ...  Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO   - Add event handlers for pairing requests. Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO   - Received 'onpairingconfirmationreq' event with passkey: 872640 Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO   - 'accept' resolve. Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO [2] Get paired devices and verify 'devicepaired' event ...  Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO   - Verify 'devicepaired' event Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO [3] Pair again...  Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO [4] Unpair ...  Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO [5] Get paired devices and verify 'deviceunpaired' event ...  Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO   - Verify 'deviceunpaired' event Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO [6] Unpair again...  Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO [7] Change bluetooth app origin back ...  Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO   - Change bluetooth app origin to: [app://bluetooth.gaiamobile.org] Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO Disable Bluetooth ... Fri Feb 26 2016 18:46:26 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO INFO TEST-END: /home/will/repooooo/b2ggg/emulator-kk/gecko/dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js Fri Feb 26 2016 18:46:30 GMT+0800 (UTC)
 2:36.24 LOG: MainThread INFO END LOG:
 2:36.41 LOG: MainThread INFO
SUMMARY
-------
 2:36.41 LOG: MainThread INFO passed: 1
 2:36.41 LOG: MainThread INFO failed: 0
 2:36.41 LOG: MainThread INFO todo: 0
 2:39.51 SUITE_END: MainThread
Summary
=======

Ran 1 tests
Expected results: 1
Unexpected results: 0

OK

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8724009 [details] [diff] [review]
Patch(v4): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

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

Almost there. Please see my questions below.

::: dom/bluetooth/tests/marionette/head.js
@@ +855,5 @@
>    return deferred.promise;
>  }
>  
> +/**
> + * Set Bluetooth's App origin to another one

Remove 'to another one', since |aOrigin| may equal to old origin.
Also decapitalize 'a'pp origin.

@@ +858,5 @@
> +/**
> + * Set Bluetooth's App origin to another one
> + *
> + * @param aOrigin
> + *        A Bluetooth's App origin you want to set.

nit: 'The' app origin to set

@@ +860,5 @@
> + *
> + * @param aOrigin
> + *        A Bluetooth's App origin you want to set.
> + *
> + * @return default App origin of Bluetooth

'Old' app origin of bluetooth, since the old one is not necessarily the default one.

@@ +863,5 @@
> + *
> + * @return default App origin of Bluetooth
> + */
> +function setBtAppOrigin(aOrigin) {
> +  if (!aOrigin) {

When do we use it? Should we assign default value of |aOrigin| instead?

@@ +867,5 @@
> +  if (!aOrigin) {
> +    return;
> +  }
> +
> +  let defaultBtAppOrigin = SpecialPowers.getCharPref("dom.bluetooth.app-origin");

Rename the variable since old app origin is not necessarily default one.

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +9,5 @@
>  //
>  // Test Procedure:
> +//   [0] Set Bluetooth permission, enable default adapter and change bluetooth
> +//       app origin to gain permission for pairing request reply
> +//   [1] Pair and wait for confirmation.

Add step 1:

  [1] Add a remote device for pairing.
  [2] Pair to the remote device and wait for confirmation.

@@ +68,3 @@
>    return Promise.resolve()
>      .then(function() {
> +      return addEmulatorRemoteDevice();

Print log

  [1] Add a remote device for pairing ...

@@ +85,3 @@
>      })
>      .then(function(deviceEvent) {
> +      log("[3] Pair again... ");

nit: space before ...

@@ +101,3 @@
>      })
>      .then(function(deviceEvent) {
> +      log("[6] Unpair again... ");

nit: space before ...

@@ +107,5 @@
> +      log("[7] Change bluetooth app origin back ... ");
> +      setBtAppOrigin(defaultBtAppOrigin);
> +    })
> +    .catch(function() {
> +      log("One of the promises is rejected, change bluetooth app origin back ...");

nit: space after ... for consistency

Comment 14

[Will Wang [:WillWang]]

Attachment: Patch(v5): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

Hi Ben,

Thanks for your detailed review! 

About the comment:
> @@ +863,5 @@
> > + *
> > + * @return default App origin of Bluetooth
> > + */
> > +function setBtAppOrigin(aOrigin) {
> > +  if (!aOrigin) {
> 
> When do we use it? Should we assign default value of |aOrigin| instead?
I use it as a guard statement simply for null input,
and now I don't tend to add an additional effect(assign default value) to this set function, for least astonishment. Would you agree?

Attached file is the revised patch,
could you help to review?
Thanks!

Comment 15

[Ben Tian (inactive)]

(In reply to Will Wang [:WillWang] from comment #14)
> > When do we use it? Should we assign default value of |aOrigin| instead?
> I use it as a guard statement simply for null input,
> and now I don't tend to add an additional effect(assign default value) to
> this set function, for least astonishment. Would you agree?

So how do you want to revise? The comment 14 patch still keeps the if-condition.

Comment 16

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #15)
> So how do you want to revise? The comment 14 patch still keeps the
> if-condition.

Oh I see your point. But still the question: when do we use it? Would anywhere trigger the if-condition?

Comment 17

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #16)
> (In reply to Ben Tian [:btian] from comment #15)
> > So how do you want to revise? The comment 14 patch still keeps the
> > if-condition.
> 
> Oh I see your point. But still the question: when do we use it? Would
> anywhere trigger the if-condition?

BTW, I think your if-condition also results in 'astonishment', doesn't it?

Comment 18

[Ben Tian (inactive)]

Comment on attachment 8725216 [details] [diff] [review]
Patch(v5): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

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

Please see my question on the if-condition in |setBtAppOrigin|.

::: dom/bluetooth/tests/marionette/head.js
@@ +858,5 @@
> +/**
> + * Set Bluetooth's app origin
> + *
> + * @param aOrigin
> + *        The Bluetooth's app origin you want to set.

nit: I prefer 'The app origin of Bluetooth' since it's 'the app origin' instead of 'the bluetooth'.

@@ +864,5 @@
> + * @return Old app origin of Bluetooth
> + */
> +function setBtAppOrigin(aOrigin) {
> +  if (!aOrigin) {
> +    return;

Please explain the rationale behind. I still don't get why and when we need this.

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +10,5 @@
>  // Test Procedure:
> +//   [0] Set Bluetooth permission, enable default adapter and change bluetooth
> +//       app origin to gain permission for pairing request reply
> +//   [1] Add a remote device for pairing.
> +//   [2] Pair and wait for confirmation.

nit: 'Pair to the remote device' to be clearer

@@ +73,3 @@
>      })
> +    .then(function(address) {
> +      log("[2] Pair and wait for confirmation ... ");

nit: 'Pair to the remote device' to be clearer

Comment 19

[Will Wang [:WillWang]]

Attachment: Patch(v6): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

Hi Ben,

Here is the revised patch,
thanks for your review!

Comment 20

[Will Wang [:WillWang]]

Marionette log for above patch FYR:

╭─will@will-desktop ~/repooooo/b2ggg/emulator-kk  ‹master*› 
╰─➤  ./mach marionette-webapi gecko/dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
 0:00.00 LOG: MainThread INFO Using workspace for temporary data: "/home/will/repooooo/b2ggg/emulator-kk"
 0:00.00 LOG: MainThread INFO Profile destination is TMP
waiting for system-message-listener-ready...
...done
 1:26.68 LOG: MainThread INFO starting httpd
 1:26.68 LOG: MainThread INFO running httpd on http://10.247.24.131:34817/
 1:28.60 LOG: MainThread INFO Android sdk version '19'; will use this to filter manifests
 2:09.81 LOG: MainThread mozversion INFO application_buildid: 20160219175252
 2:09.81 LOG: MainThread mozversion INFO application_display_name: B2G
 2:09.81 LOG: MainThread mozversion INFO application_id: {3c2e2abc-06d4-11e1-ac3b-374f68613e61}
 2:09.81 LOG: MainThread mozversion INFO application_name: B2G
 2:09.81 LOG: MainThread mozversion INFO application_remotingname: b2g
 2:09.81 LOG: MainThread mozversion INFO application_vendor: Mozilla
 2:09.81 LOG: MainThread mozversion INFO application_version: 46.0a1
 2:09.81 LOG: MainThread mozversion INFO device_firmware_date: 1439793977
 2:09.81 LOG: MainThread mozversion INFO device_firmware_version_incremental: eng.will.20150817.144534
 2:09.81 LOG: MainThread mozversion INFO device_firmware_version_release: 4.4.2
 2:09.81 LOG: MainThread mozversion INFO device_id: generic
 2:09.81 LOG: MainThread mozversion INFO gaia_changeset: 97266c579c544f5ba57a701f39893cc86d46774a
 2:09.81 LOG: MainThread mozversion INFO gaia_date: 1449093766
 2:09.81 LOG: MainThread mozversion INFO platform_buildid: 20160219175252
 2:09.81 LOG: MainThread mozversion INFO platform_version: 46.0a1
 2:09.81 SUITE_START: MainThread 1
 2:11.03 TEST_START: MainThread test_dom_BluetoothAdapter_pair.js
 2:30.16 TEST_END: MainThread PASS
 2:30.27 LOG: MainThread INFO START LOG:
 2:30.27 LOG: MainThread INFO INFO TEST-START: /home/will/repooooo/b2ggg/emulator-kk/gecko/dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js Mon Mar 07 2016 15:02:37 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO   - Change bluetooth app origin to: [app://system.gaiamobile.org] Mon Mar 07 2016 15:02:43 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO navigator.mozBluetooth is available Mon Mar 07 2016 15:02:43 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO Wait for creating bluetooth adapter... Mon Mar 07 2016 15:02:43 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO bluetoothManager.defaultAdapter.state: disabled Mon Mar 07 2016 15:02:43 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO Original state of bluetooth is disabled Mon Mar 07 2016 15:02:43 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO Enable Bluetooth ... Mon Mar 07 2016 15:02:43 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO Checking adapter attributes ... Mon Mar 07 2016 15:02:44 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO adapter.address: 56:34:12:00:54:52 Mon Mar 07 2016 15:02:44 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO adapter.name: Full Android on Emulator Mon Mar 07 2016 15:02:44 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO [1] Add a remote device for pairing ...  Mon Mar 07 2016 15:02:44 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO [2] Pair to the remote device and wait for confirmation ...  Mon Mar 07 2016 15:02:44 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO   - Add event handlers for pairing requests. Mon Mar 07 2016 15:02:44 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO   - Received 'onpairingconfirmationreq' event with passkey: 872640 Mon Mar 07 2016 15:02:45 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO   - 'accept' resolve. Mon Mar 07 2016 15:02:46 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO [3] Get paired devices and verify 'devicepaired' event ...  Mon Mar 07 2016 15:02:46 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO   - Verify 'devicepaired' event Mon Mar 07 2016 15:02:46 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO [4] Pair again ...  Mon Mar 07 2016 15:02:46 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO [5] Unpair ...  Mon Mar 07 2016 15:02:47 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO [6] Get paired devices and verify 'deviceunpaired' event ...  Mon Mar 07 2016 15:02:47 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO   - Verify 'deviceunpaired' event Mon Mar 07 2016 15:02:47 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO [7] Unpair again ...  Mon Mar 07 2016 15:02:47 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO [8] Change bluetooth app origin back ...  Mon Mar 07 2016 15:02:48 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO   - Change bluetooth app origin to: [app://bluetooth.gaiamobile.org] Mon Mar 07 2016 15:02:48 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO Disable Bluetooth ... Mon Mar 07 2016 15:02:48 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO INFO TEST-END: /home/will/repooooo/b2ggg/emulator-kk/gecko/dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js Mon Mar 07 2016 15:02:51 GMT+0800 (UTC)
 2:30.27 LOG: MainThread INFO END LOG:
 2:30.39 LOG: MainThread INFO 
SUMMARY
-------
 2:30.39 LOG: MainThread INFO passed: 1
 2:30.39 LOG: MainThread INFO failed: 0
 2:30.39 LOG: MainThread INFO todo: 0
 2:33.24 SUITE_END: MainThread 
Summary
=======

Ran 1 tests
Expected results: 1
Unexpected results: 0

OK

Comment 21

[Ben Tian (inactive)]

Comment on attachment 8727273 [details] [diff] [review]
Patch(v6): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

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

r=me with manifest.ini revision in [1], including skip for ICS emulator. 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/tests/marionette/manifest.ini#25

Comment 22

[Will Wang [:WillWang]]

Attachment: Patch(v7): Bug-1240992-Fix-Bluetooth-marionette-test-test_dom_B.patch

Since currently m-c is under build break[1], the try testing[2] will be stopped at build stage.
I attach the latest revised patch here for reference, with r+ carried.


[1] 
Bug 1245091 – [meta] b2g build issues
https://bugzilla.mozilla.org/show_bug.cgi?id=1245091

[2] 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0b45ddf4a7&exclusion_profile=false&filter-tier=1&filter-tier=2&filter-tier=3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
There are 2 patches sent to try for both bug 1240997 and this bug.

Comment 23

[Ben Tian (inactive)]

Resolve as WONTFIX since B2G is discontinued.