968709 - QEMU Bluetooth tests: pair with remote devices

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Implement Marionette tests for Gecko Bluetooth API to be run on B2G emulator:
- Test the pair procedure between FxOS emulator and virtual remote device.
- Verify pair related properties and methods of BluetoothAdapter

Comment 1

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

Attachment: Add a marionette test for pairing APIs of BluetoothAdapter. (v0)

Comment 2

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

Attachment: Github pull request for gaia. Allow system app to listen to bluetooth-pairing-request (v0)

Comment 3

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

Comment on attachment 8410213 [details]
Github pull request for gaia.  Allow system app to listen to bluetooth-pairing-request (v0)

Move this patch to gaia Bug 1007545.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8408880 [details] [diff] [review]
Add a marionette test for pairing APIs of BluetoothAdapter. (v0)

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

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +27,5 @@
> +function pair(aAdapter, aDeviceAddress) {
> +  let deferred = Promise.defer();
> +
> +  let request = aAdapter.pair(aDeviceAddress);
> +  request.onsuccess = function () {

nit: you might want to have yet another utility API wrapDomRequestAsPromise(). [1]

[1]: http://mxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#80

@@ +55,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function getPairedDevices(aAdapter) {

nit: if pair/unpair/getPairedDevices are to be used frequently in other Bluetooth test cases, you should really consider moving them into head.js.

@@ +61,5 @@
> +
> +  let request = aAdapter.getPairedDevices();
> +  request.onsuccess = function () {
> +    log("  getPairedDevices - Success");
> +    let paired = request.result.slice();

Why?  It's already a js array, isn't it?

@@ +108,5 @@
> +      promises.push(startDiscovery(aAdapter));
> +      return Promise.all(promises)
> +        .then(function(aResults) {
> +          is(aResults[0].device.address, aRemoteAddress, "BluetoothDevice.address");
> +          return Promise.resolve(aResults[0].device.address);

nit: |return aResults[0].device.address;| or |return aRemoteAddress;|.  We don't really have to wrap everything with |Promise.resolve()| again.

@@ +111,5 @@
> +          is(aResults[0].device.address, aRemoteAddress, "BluetoothDevice.address");
> +          return Promise.resolve(aResults[0].device.address);
> +        });
> +    })
> +    .then(function(addr) {

nit: aRemoveAddress

@@ +118,5 @@
> +      promises.push(pair(aAdapter, addr));
> +      return Promise.all(promises);
> +    })
> +    .then(() => getPairedDevices(aAdapter))
> +    .then((paird) => unpair(aAdapter, paird.pop().address))

nit: aPairedDevices

Comment 5

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

Attachment: Add a marionette test for pairing APIs of BluetoothAdapter. (v1), f=vyang

Comment 6

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)

Thank you for your comments.
I've uploaded a new patch Attachment #8426859 [details] [diff] based on your comments.

> @@ +61,5 @@
> > +
> > +  let request = aAdapter.getPairedDevices();
> > +  request.onsuccess = function () {
> > +    log("  getPairedDevices - Success");
> > +    let paired = request.result.slice();
> 
> Why?  It's already a js array, isn't it?
Yes, it's an Array.
The function called slice() because it wants to return a shallow copy of the array of paried devices.

Returns a copy here to avoid the original array be modified by functions like Array.pop() or Array.sort().

The similar usage can also be found in gaia.  e.g. settings/js/bluetooth.js, bluetooth/js/deviceList.js ...

Comment 7

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8426859 [details] [diff] [review]
Add a marionette test for pairing APIs of BluetoothAdapter. (v1), f=vyang

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

r=me with nits addressed and question answered. Thanks!

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +31,5 @@
> +      aAdapter.setPairingConfirmation(aPairingEvent.address, true);
> +      break;
> +    case 'pincode':
> +      let pincode = BT_PAIRING_PINCODE;
> +      defaultAdapter.setPinCode(aPairingEvent.address, pincode);

Shouldn't we use aAdapter?

@@ +35,5 @@
> +      defaultAdapter.setPinCode(aPairingEvent.address, pincode);
> +      break;
> +    case 'passkey':
> +      let passkey = BT_PAIRING_PASSKEY;
> +      defaultAdapter.setPasskey(aPairingEvent.address, passkey);

Ditto.

@@ +62,5 @@
> +          is(aResults[0].device.address, aRemoteAddress, "BluetoothDevice.address");
> +          return aResults[0].device.address;
> +        });
> +    })
> +    .then(function(aRemoveAddress) {

should be 'aRemoteAddress'

@@ +70,5 @@
> +      return Promise.all(promises);
> +    })
> +    .then(() => getPairedDevices(aAdapter))
> +    .then((aPairedDevices) => unpair(aAdapter, aPairedDevices.pop().address))
> +    .then(() => stopDiscovery(aAdapter))

Question: In practice, stopDiscovery() would be called even before executing pair(). I suggest that we should try our best to simulate the call flow. Make sense?

Comment 8

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

Attachment: (final) Add a marionette test for pairing APIs of BluetoothAdapter. (v2), r=echou, f=vyang

Comment 9

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #7)
> Question: In practice, stopDiscovery() would be called even before executing
> pair(). I suggest that we should try our best to simulate the call flow.
> Make sense?

That's a good idea.
I changed the order between stopDiscovery() and pair() in Attachment #8427529 [details] [diff].
Thank you.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #10)
> master :
> https://github.com/mozilla-b2g/gaia/commit/
> 6a391274cd436f8f0d1fad2db8c6b4805703259c

ups that was for bug 1007545 actually

Comment 12

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

This bug depends on Bug 968624.
I'll mark it as 'checkin-needed' when Bug 968624 land to m-c.

Comment 13

[Carsten Book [:Tomcat]]

landed as https://hg.mozilla.org/integration/b2g-inbound/rev/349ca77481c0

Comment 14

[Carsten Book [:Tomcat]]

sorry had to backout this checkin for permanent timeouts like https://tbpl.mozilla.org/php/getParsedLog.php?id=40530672&tree=B2g-Inbound

Comment 15

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

Attachment: (part 2) Add a marionette test for pairing APIs of BluetoothAdapter. (v3)

Comment 16

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

Attachment: (part 1) Add a marionette test case to make sure the URL of Marionette client is correct. (v1)

Comment 17

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

Bluetooth tests could work correctly by running dom/bluetooth/tests/marionette/manifest.ini without Attachment #8433843 [details] [diff].

However, [test_dom_BluetoothAdapter_pair.js] would fail by running testing/marionette/client/marionette/tests/unit-tests.ini. The reason is that some tests in unit-tests.ini would change the URL of Marionette client and wouldn't restore it. It may cause issues to the test which use system message. Restore the URL before running BT tests since the test result shouldn't be affected by the order of test sets.

Comment 18

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

(In reply to Jamin Liu [:jaliu] from comment #16)

It looks fine one try server.
https://tbpl.mozilla.org/?tree=Try&rev=d39e5327d035

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8433843 [details] [diff] [review]
(part 1) Add a marionette test case  to make sure the URL of Marionette client is correct. (v1)

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

Marionette test cases can be run with random order, so this temporary work-around doesn't always work in every case.  But since it's committed to dom/bluetooth only, I'm fine with this.  A follow-up for a solid solution will be really appreciated.  Thank you.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8433842 [details] [diff] [review]
(part 2) Add a marionette test for pairing APIs of BluetoothAdapter. (v3)

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

f- for using return statement in onreject callbacks.

::: dom/bluetooth/tests/marionette/head.js
@@ +251,5 @@
> +      // is(aAdapter.discovering, false, "BluetoothAdapter.discovering");
> +      log("  Start discovery - Success");
> +    }, function reject(aEvent) {
> +      ok(false, "Start discovery - Fail");
> +      return aEvent.target.error;

Use 'throw aEvent.target.error;' instead, or it will become a RESOLVED promise.  Applies to all similar cases below.

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-

nit: Just found this mode line is wrong.  Definitely not C++. ;)

@@ +55,5 @@
> +    .then(() => addEmulatorRemoteDevice())
> +    .then(function(aRemoteAddress) {
> +      let promises = [];
> +      promises.push(waitForAdapterEvent(aAdapter, "devicefound"));
> +      promises.push(startDiscovery(aAdapter));

nit: you might want to add an API 'startDiscoveryAndWaitDevicesFound(aAdapter, aRemoveAddresses)'.

@@ +64,5 @@
> +        });
> +    })
> +    .then(function(aRemoteAddress) {
> +      let promises = [];
> +      promises.push(stopDiscovery(aAdapter));

nit: I think stopDiscovery has nothing to do with the pairing procedure, so it should probably be in his own then-block.  Or, with startDiscoveryAndWaitDevicesFound, call to stopDiscovery automatically when all expected devices are found.

@@ +66,5 @@
> +    .then(function(aRemoteAddress) {
> +      let promises = [];
> +      promises.push(stopDiscovery(aAdapter));
> +      promises.push(waitForAdapterEvent(aAdapter, "pairedstatuschanged"));
> +      promises.push(pair(aAdapter, aRemoteAddress));

nit: and a 'pairDeviceAndWait(aAdapter, aRemoteAddress)' will be convenient for me.

Comment 21

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
> Comment on attachment 8433843 [details] [diff] [review]
> (part 1) Add a marionette test case  to make sure the URL of Marionette
> client is correct. (v1)
> 
> Review of attachment 8433843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Marionette test cases can be run with random order, so this temporary
> work-around doesn't always work in every case.  But since it's committed to
> dom/bluetooth only, I'm fine with this.  A follow-up for a solid solution
> will be really appreciated.  Thank you.

Thank you for your comment.
The follow-up bug 1020732 has been opened.

Comment 22

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

Attachment: (part 2) Add a marionette test for pairing APIs of BluetoothAdapter. (v4)

Make a few changes based on #Comment 20.
I'd like to thank Vicamo for his help.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8434730 [details] [diff] [review]
(part 2) Add a marionette test for pairing APIs of BluetoothAdapter. (v4)

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

Please adjust your git/hg config to include 8-line context in generated patches.

::: dom/bluetooth/tests/marionette/head.js
@@ +285,5 @@
> +
> +/**
> + * Wait for 'devicefound' event of specific devices.
> + *
> + * Resolve if that every specific devices has been found.  Never reject.

nit: should be 'specified' I think.

@@ +301,5 @@
>    let deferred = Promise.defer();
>  
> +  aAdapter.addEventListener("devicefound", function onevent(aEvent) {
> +    var addrArray = [];
> +    if(aRemoteAddresses.indexOf(aEvent.device.address) > -1) {

if (... != -1)

@@ +302,5 @@
>  
> +  aAdapter.addEventListener("devicefound", function onevent(aEvent) {
> +    var addrArray = [];
> +    if(aRemoteAddresses.indexOf(aEvent.device.address) > -1) {
> +      addrArray.push(aEvent.device.address);

addrArray always has at most one element.

@@ +338,5 @@
> +  promises.push(startDiscovery(aAdapter));
> +  return Promise.all(promises)
> +    .then(function(aResults) {
> +      return aResults[0];
> +    });

nit: .then(aResults => aResults[0]);

Comment 24

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

Attachment: (part 2) Add a marionette test for pairing APIs of BluetoothAdapter. (v5)

Comment 25

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

(In reply to Jamin Liu [:jaliu] from comment #22)
> Created attachment 8434730 [details] [diff] [review]
It passed Mnw test on try server.
https://tbpl.mozilla.org/?tree=Try&rev=7d349e62e247

Comment 26

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8433843 [details] [diff] [review]
(part 1) Add a marionette test case  to make sure the URL of Marionette client is correct. (v1)

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

I'm good if Vicamo is happy with this. :)

Comment 27

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8434781 [details] [diff] [review]
(part 2) Add a marionette test for pairing APIs of BluetoothAdapter. (v5)

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

r=me with nits addressed. Thanks.

::: dom/bluetooth/tests/marionette/head.js
@@ +314,5 @@
>    return deferred.promise;
>  }
>  
>  /**
> + * Start dicovering Bluetooth devices and wait for 'devicefound' events.

typo: di's'covering

::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_pair.js
@@ +54,5 @@
> +    .then((aRemoteAddress) =>
> +          startDiscoveryAndWaitDevicesFound(aAdapter, [aRemoteAddress]))
> +    .then((aRemoteAddresses) =>
> +          stopDiscovery(aAdapter).then(() => aRemoteAddresses))
> +    .then((aRemoteAddresses) => pairDeviceAndWait(aAdapter, aRemoteAddresses.pop()))

Just curious: what's the result of pairDeviceAndWait(aAdapter, aRemoteAddresses.pop())) if aRemoteAddresses contains more than 1 device address? To my knowledge only the first device element will be passed into function pairDeviceAndWait(), is that right? Please ignore what I'm going to say if my above assumption was wrong. However if it is the case I mentioned, then please add a comment to explain in detail.

Comment 28

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

Attachment: (final) (part 2) Add a marionette test for pairing APIs of BluetoothAdapter. (v6), r=echou, f=vyang

Modified few comments based on #Comment 27.

Comment 29

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

(In reply to Eric Chou [:ericchou] [:echou] (GSMA MAE @ Shanghai, 6/10 ~ 6/13) from comment #27)
> Just curious: what's the result of pairDeviceAndWait(aAdapter,
> aRemoteAddresses.pop())) if aRemoteAddresses contains more than 1 device
> address? To my knowledge only the first device element will be passed into
> function pairDeviceAndWait(), is that right?
Yes, it's correct. 'aRemoteAddresses' is an array and it may contain more than one address of device.

> Please ignore what I'm going to
> say if my above assumption was wrong. However if it is the case I mentioned,
> then please add a comment to explain in detail.
Thank you for your suggestions.

Comment 30

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

Attachment: (final) (part 1) Add a marionette test case to make sure the URL of Marionette client is correct. (v1), r=echou, f=vyang

Comment 31

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

Attachment: (final) (part 2) Add a marionette test for pairing APIs of BluetoothAdapter. (v6), r=echou, f=vyang

Comment 32

[Eric Chou [:ericchou] [:echou]]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/5f7e58de5d56
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/8d2cc1387119

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f7e58de5d56
https://hg.mozilla.org/mozilla-central/rev/8d2cc1387119

Comment 34

[Ed Morley [:emorley]]

Backed out for intermittent test failures (bug 1023167). In addition to fixing the intermittent failure, please can you correct the test assert 'Flase' typo - thanks :-)

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/46e2472531aa
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/c4419d8eb525

Comment 35

[Ed Morley [:emorley]]

Note that any fix that lands will need uplifting.

Comment 36

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

Attachment: (part 1) Add a marionette test case to make sure the URL of Marionette client is correct. (v2)

Navigating to system app would take longer than expected on tinderbox and the action may exceed the default timeout intermittently. Set a appropriate timeout on 'test_navigate_to_default_url.py'.

Comment 37

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8440641 [details] [diff] [review]
(part 1) Add a marionette test case to make sure the URL of Marionette client is correct. (v2)

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

Redirect to vicamo since he is more familiar with test cases than me.

::: dom/bluetooth/tests/marionette/test_navigate_to_default_url.py
@@ +6,5 @@
> +
> +class testNavigateToDefault(MarionetteTestCase):
> +    def setUp(self):
> +        MarionetteTestCase.setUp(self)
> +        # Sets a appropriate timeout for this test.

typo: sets 'an' appropriate

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8440641 [details] [diff] [review]
(part 1) Add a marionette test case to make sure the URL of Marionette client is correct. (v2)

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

Please also have test run(s?) on tbpl before landing again.

::: dom/bluetooth/tests/marionette/test_navigate_to_default_url.py
@@ +14,5 @@
> +    def test_navigate_to_default_url(self):
> +        try:
> +            self.marionette.navigate("app://system.gaiamobile.org/index.html")
> +        except:
> +            self.assertTrue(False, "Can not navigate to system app.")

Comment 34 is not addressed.

Comment 39

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

Attachment: (part 1) Add a marionette test case to make sure the URL of Marionette client is correct. (v3)

Comment 40

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

(In reply to Jamin Liu [:jaliu] from comment #39)
> Created attachment 8441237 [details] [diff] [review]
> (part 1) Add a marionette test case to make sure the URL of Marionette
> client is correct. (v3)

Attachment #8440641 [details] [diff]
- Fix the issue which was described in Bug 1023167.
  Please refer to Comment 34 and Comment 36

Attachment #8441237 [details] [diff]
- Fix the typo: sets 'an' appropriate.

Try server:
https://tbpl.mozilla.org/?tree=Try&rev=0c47fe18b951 (part1 only)
https://tbpl.mozilla.org/?tree=Try&rev=405578f72ab6 (part1 + part2)

Comment 41

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

Attachment: (final) (part 1) Add a marionette test case to make sure the URL of Marionette client is correct. (v3) r=vyang

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/61e8191c387f
https://hg.mozilla.org/integration/b2g-inbound/rev/08237be918f5

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61e8191c387f
https://hg.mozilla.org/mozilla-central/rev/08237be918f5

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Pushed an interdiff of these patches to Aurora to fix the intermittent failures there.

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc0b13fce4ed