Closed Bug 968709 Opened 6 years ago Closed 5 years ago

QEMU Bluetooth tests: pair with remote devices

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.0, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S4 (20june)
feature-b2g 2.0
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jaliu, Assigned: jaliu)

References

Details

(Whiteboard: [p=1])

Attachments

(2 files, 12 obsolete files)

24.45 KB, patch
Details | Diff | Splinter Review
2.01 KB, patch
Details | Diff | Splinter Review
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
Assignee: nobody → jaliu
Depends on: 968624
Blocks: 860695
Target Milestone: --- → 1.4 S6 (25apr)
Status: NEW → ASSIGNED
Attachment #8408880 - Flags: feedback?(echou)
Whiteboard: [p=1]
Target Milestone: 1.4 S6 (25apr) → 2.0 S2 (23may)
Depends on: 1007545
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.
Attachment #8410213 - Attachment is obsolete: true
Attachment #8410213 - Flags: feedback?(echou)
Attachment #8408880 - Flags: feedback?(echou) → review?(echou)
feature-b2g: --- → 2.0
Attachment #8408880 - Flags: feedback?(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
Attachment #8408880 - Flags: feedback?(vyang) → feedback+
Attachment #8408880 - Attachment is obsolete: true
Attachment #8408880 - Flags: review?(echou)
Attachment #8426859 - Flags: review?(echou)
(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 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?
Attachment #8426859 - Flags: review?(echou) → review+
(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.
master : https://github.com/mozilla-b2g/gaia/commit/6a391274cd436f8f0d1fad2db8c6b4805703259c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
(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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
Keywords: checkin-needed
This bug depends on Bug 968624.
I'll mark it as 'checkin-needed' when Bug 968624 land to m-c.
Keywords: checkin-needed
landed as https://hg.mozilla.org/integration/b2g-inbound/rev/349ca77481c0
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
sorry had to backout this checkin for permanent timeouts like https://tbpl.mozilla.org/php/getParsedLog.php?id=40530672&tree=B2g-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: in-moztrap-
Attachment #8427529 - Attachment is obsolete: true
Attachment #8433842 - Flags: review?(echou)
Attachment #8433842 - Flags: feedback?(vyang)
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.
(In reply to Jamin Liu [:jaliu] from comment #16)

It looks fine one try server.
https://tbpl.mozilla.org/?tree=Try&rev=d39e5327d035
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.
Attachment #8433843 - Flags: feedback?(vyang) → feedback+
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.
Attachment #8433842 - Flags: feedback?(vyang) → feedback-
(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.
Make a few changes based on #Comment 20.
I'd like to thank Vicamo for his help.
Attachment #8433842 - Attachment is obsolete: true
Attachment #8433842 - Flags: review?(echou)
Attachment #8434730 - Flags: review?(echou)
Attachment #8434730 - Flags: feedback?(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]);
Attachment #8434730 - Flags: feedback?(vyang) → feedback-
Attachment #8434730 - Attachment is obsolete: true
Attachment #8434730 - Flags: review?(echou)
Attachment #8434781 - Flags: review?(echou)
Attachment #8434781 - Flags: feedback?(vyang)
Attachment #8434781 - Flags: feedback?(vyang) → feedback+
(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 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. :)
Attachment #8433843 - Flags: review?(echou) → review+
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.
Attachment #8434781 - Flags: review?(echou) → review+
Modified few comments based on #Comment 27.
Attachment #8434781 - Attachment is obsolete: true
(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.
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Note that any fix that lands will need uplifting.
Depends on: 1023167
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'.
Attachment #8435463 - Attachment is obsolete: true
Attachment #8440641 - Flags: review?(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
Attachment #8440641 - Flags: review?(echou) → review?(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.
Attachment #8440641 - Flags: review?(vyang) → review-
(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)
Attachment #8441237 - Flags: review?(vyang) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61e8191c387f
https://hg.mozilla.org/mozilla-central/rev/08237be918f5
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Pushed an interdiff of these patches to Aurora to fix the intermittent failures there.

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc0b13fce4ed
You need to log in before you can comment on or make changes to this bug.