Implement Marionette tests for Gecko Bluetooth API to be run on B2G emulator: - start/stop discovery - remote devices inspection

test cases implementation: https://github.com/jhlin/mozilla-central/commit/43c3cc812a71ec14d8adfb58be30bda122438c5b

Please note that to pass the tests, attachment 743499 [details] [diff] [review] is needed.

to reflect the truth that John is not working on BT anymore. Macro, please check who should own this issue

Comment on attachment 8364234 [details] [diff] [review] [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v2) Review of attachment 8364234 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good to me. Please ask for vicamo's review as well. He is good at writing Marionette test cases. ::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_discovery.js @@ +63,5 @@ > + > + emulator.run("bt set " + REMOTE_DEVICE_ADDRESS + " discoverable " + aDiscoverable, function(result) { > + if (result[0] === "OK") { > + log(" Set discoverable of remote device - Success"); > + deferred.resolve(); nit: unnecessary indentation @@ +77,5 @@ > +function startDiscovery(aAdapter) { > + let deferred = Promise.defer(); > + > + let request = aAdapter.startDiscovery(); > + request.addEventListener("success", function() { The callbacks(onsuccess/onerror) of DOMRequest are usually written as request.onsuccess = function() { ... } @@ +146,5 @@ > + > + emulator.run("bt remove-remote all", function(result) { > + if (result[0] === "OK") { > + log(" Remove device - Success"); > + deferred.resolve(); nit: unnecessary indentation @@ +176,5 @@ > + .then(function() { return setRemoteDeviceName(); }) > + //.then(function() { return setRemoteDeviceDiscoverable(false); }) > + .then(function() { return startDiscovery(aAdapter); }) > + .then(function() { return discoveryDeferred.promise; }) > + //.then(function() { return pair(); }) Not sure if it's suitable to call "Pair()" here since this file is named test_dom_BluetoothAdapter_"discovery".js

Comment on attachment 8373158 [details] [diff] [review] [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v2) Review of attachment 8373158 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_discovery.js @@ +23,5 @@ > + > +const REMOTE_DEVICE_ADDRESS = "56:34:12:00:54:60"; > +const REMOTE_DEVICE_NAME = "User_friendly_name_for_remote_device"; > + > +let discoveryDeferred = Promise.defer(); You can use |Promise.all()|. return removeAllRemoteDevices() .then(() => addRemoteDevice()) .then(() => setRemoteDeviceName()) .then(function() { let promises = []; promises.push(waitForAdapterEvent(aAdapter, "devicefound")); promises.push(startDiscovery(aAdapter)); return Promise.all(promises); }) .then(function(aResults) { // aResults[0] should be a BluetoothEvent // aResults[1] should probably be <undefined> because we may not resolve anything meaningful in startDiscovery(). }) ... @@ +25,5 @@ > +const REMOTE_DEVICE_NAME = "User_friendly_name_for_remote_device"; > + > +let discoveryDeferred = Promise.defer(); > + > +function addRemoteDevice() { I think this should definitely be in "head.js". Many Bluetooth tests will be relying on the interaction between two devices, won't they? @@ +28,5 @@ > + > +function addRemoteDevice() { > + let deferred = Promise.defer(); > + > + runEmulatorCmd("bt remote add " + REMOTE_DEVICE_ADDRESS, function(result) { See bug 860697 comment 13, you'll need a |runEmulatorCmdSafe| utility function. @@ +41,5 @@ > + > + return deferred.promise; > +} > + > +function setRemoteDeviceName() { I'd wish for a utility function |addRemoteDevice(<a dictionary containing customized property-value pairs>)|. @@ +57,5 @@ > + > + return deferred.promise; > +} > + > +function setRemoteDeviceDiscoverable(aDiscoverable) { Move to "head.js" as an utility function |setRemoteDeviceDiscoverable(aAddress, aDiscoverable)|. @@ +73,5 @@ > + > + return deferred.promise; > +} > + > +function startDiscovery(aAdapter) { ditto. @@ +105,5 @@ > + ok(true, "Device found"); > + discoveryDeferred.resolve(); > +} > + > +function pair(aAdapter, aDevice) { Considering this function is not used and it's fairly simple, please just remove it for now and place a "TODO: bug xxx" nearby where it's supposed to be called. @@ +121,5 @@ > + > + return deferred.promise; > +} > + > +function stopDiscovery(aAdapter) { Move to "head.js". @@ +140,5 @@ > + > + return deferred.promise; > +} > + > +function removeRemoteDevice() { ditto.

Depending on bug 860697 for utility functions.

Comment on attachment 8375446 [details] [diff] [review] [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v3) Review of attachment 8375446 [details] [diff] [review]: ----------------------------------------------------------------- r- because you didn't address previous review comments, especially about the |discoveryDeferred| things. ::: dom/bluetooth/tests/marionette/head.js @@ +127,5 @@ > + * result -- an array of emulator response lines. > + * > + * @return A deferred promise. > + */ > +function setRemoteDeviceName(aAddress, aName) { nit: missing documentation for arguments aAddress and aName.

Comment on attachment 8377046 [details] [diff] [review] [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v4) Review of attachment 8377046 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/tests/marionette/head.js @@ +28,3 @@ > // Service Classes: Capturing, Object Transfer, Telephony > // Device Class: Phone, Smart phone > const EMULATOR_CLASS = 0x58020c; Please also define some constants which are used in https://github.com/mozilla-b2g/platform_external_qemu/blob/master/hw/bt.h#L31: const BDADDR_ALL = "ff:ff:ff:ff:ff:ff"; const BDADDR_LOCAL = "ff:ff:ff:00:00:00"; @@ +28,5 @@ > // Service Classes: Capturing, Object Transfer, Telephony > // Device Class: Phone, Smart phone > const EMULATOR_CLASS = 0x58020c; > > +const REMOTE_DEVICE_ADDRESS = "11:22:33:44:55:66"; From https://github.com/mozilla-b2g/platform_external_qemu/pull/57, I'm to return the <bd_addr> of newly allocated remote device instead. So, no, there won't be a property called address. Retrieve it from the emulator output lines. @@ +91,5 @@ > + * Otherwise, the function would report a test failure. > + * > + * @return A deferred promise. > + */ > +function addRemoteDevice(aProperies) { function addEmulatorRemoteDevice(aProperties) { let address; let promise = runEmulatorCmdSafe("bt remote add") .then(function(aResults) { address = aResults[0]; }); for (let key in aProperties) { let value = aProperties[key]; promise = promise.then(function() { return setEmulatorDeviceProperty(address, key, value); }); } promise ＝ promise.then(function() { return address; }); return promise; } @@ +126,5 @@ > + * > + * @return A deferred promise. > + */ > +function removeAllRemoteDevice() { > + return runEmulatorCmdSafe("bt remote remove all"); From https://github.com/mozilla-b2g/platform_external_qemu/pull/57, I have "bt remote remove <bd_addr>" to allow removing one single remove device or all in the same scatternet if BDADDR_ALL is passed. So please also turn this into a more generic one: function removeEmulatorRemoteDevice(aAddress) { let cmd = "bt remote remove " + aAddress; return runEmulatorCmdSafe(cmd) .then(function(aResults) { // `bt remote remove <bd_addr>` returns a list of removed device one at a line. // The last item is "OK". return aResults.slice(0, -1); }); } @@ +145,5 @@ > + * A user friendly name of remote device. > + * > + * @return A deferred promise. > + */ > +function setRemoteDeviceName(aAddress, aName) { I'm not going to have local/remote device properties concept in bug 860696. You can just call `bt property <bd_addr>` to any available devices. The target will resolve the property name automatically and give an appropriate answer. So, there shouldn't be the term "remote" in the APIs here. Besides, I don't really prefer having one API for each property available. I'd like to turn them into a generic ones: function setEmulatorDeviceProperty(aAddress, aName, aValue) { let cmd = "bt property " + aAddress + " " + aName + " " + aValue; return runEmulatorCmdSafe(cmd); } function getEmulatorDeviceProperty(aAddress, aName) { let cmd = "bt property " + aAddress + " " + aName; return runEmulatorCmdSafe(cmd) .then(function(aResults) { return aResults[0]; }); } ::: dom/bluetooth/tests/marionette/test_dom_BluetoothAdapter_discovery.js @@ +25,5 @@ > + log("Testing the discovery process of BluetoothAdapter ..."); > + > + // The properies of remote device. > + let theProperies = { > + "address": REMOTE_DEVICE_ADDRESS, ditto @@ +44,5 @@ > + is(aResults[0].device.address, REMOTE_DEVICE_ADDRESS, > + "BluetoothDevice.address"); > + }) > + .then(() => stopDiscovery(aAdapter)) > + .then(() => removeAllRemoteDevice()); Then it becomes: return Promise.resolve() .then(() => removeEmulatorRemoteDevice(BDADDR_ALL)) .then(() => addEmulatorRemoteDevice(theProperies)) .then(function(aRemoteAddress) { let promises = []; promises.push(waitForAdapterEvent(aAdapter, "devicefound")); promises.push(startDiscovery(aAdapter)); return Promise.all(promises) .then(function(aResults) { is(aResults[0].device.address, aRemoteAddress, "BluetoothDevice.address"); }); }) .then(() => stopDiscovery(aAdapter)) .then(() => removeEmulatorRemoteDevice(BDADDR_ALL));

Comment on attachment 8398302 [details] [diff] [review] [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v5) Review of attachment 8398302 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/tests/marionette/head.js @@ +211,5 @@ > + > + let request = aAdapter.startDiscovery(); > + request.onsuccess = function () { > + log(" Start discovery - Success"); > + // TODO: Currently, discovering state wouldn't change immediately here. nit: RIL people add a bug id whenever we have a TODO/FIXME tag in the comment. It's up to you.

Thank you. I've put the related bug id to the comment. The patch is on the try server. (In reply to Vicamo Yang [:vicamo][:vyang] from comment #16) > Comment on attachment 8398302 [details] [diff] [review] > [Mnw] Add a test case for discover remote device through BluetoothAdapter. > (v5) > > Review of attachment 8398302 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/tests/marionette/head.js > @@ +211,5 @@ > > + > > + let request = aAdapter.startDiscovery(); > > + request.onsuccess = function () { > > + log(" Start discovery - Success"); > > + // TODO: Currently, discovering state wouldn't change immediately here. > > nit: RIL people add a bug id whenever we have a TODO/FIXME tag in the > comment. It's up to you.

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

Comment on attachment 8399208 [details] [diff] [review] [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v5), r=vyang Review of attachment 8399208 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit addressed. ::: dom/bluetooth/tests/marionette/head.js @@ +211,5 @@ > + > + let request = aAdapter.startDiscovery(); > + request.onsuccess = function () { > + log(" Start discovery - Success"); > + // TODO (bug 892207): Make Bluetooth APIs available for 3rd party apps. Bug 892207 seems not relevant to this function. Please remove this comment. @@ +244,5 @@ > + > + let request = aAdapter.stopDiscovery(); > + request.onsuccess = function () { > + log(" Stop discovery - Success"); > + // TODO (bug 892207): Make Bluetooth APIs available for 3rd party apps. ditto.

(In reply to Eric Chou [:ericchou] [:echou] from comment #19) > Comment on attachment 8399208 [details] [diff] [review] > [Mnw] Add a test case for discover remote device through BluetoothAdapter. > (v5), r=vyang > > Review of attachment 8399208 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with nit addressed. > > ::: dom/bluetooth/tests/marionette/head.js > @@ +211,5 @@ > > + > > + let request = aAdapter.startDiscovery(); > > + request.onsuccess = function () { > > + log(" Start discovery - Success"); > > + // TODO (bug 892207): Make Bluetooth APIs available for 3rd party apps. > > Bug 892207 seems not relevant to this function. Please remove this comment. > > @@ +244,5 @@ > > + > > + let request = aAdapter.stopDiscovery(); > > + request.onsuccess = function () { > > + log(" Stop discovery - Success"); > > + // TODO (bug 892207): Make Bluetooth APIs available for 3rd party apps. > > ditto. Per offline discussion with Jamin, let's make this version land.

> > Per offline discussion with Jamin, let's make this version land. I was convinced because we should always have a bug number when there is a "TODO" flag.