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 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 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

(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?

(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

(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

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

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

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

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.

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.

(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.

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]);

(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. :)

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.

Modified few comments based on #Comment 27.

(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.

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

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

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

Note that any fix that lands will need uplifting.

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 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 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.

(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)

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

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

Pushed an interdiff of these patches to Aurora to fix the intermittent failures there. https://hg.mozilla.org/releases/mozilla-aurora/rev/bc0b13fce4ed