860698 - QEMU Bluetooth tests: discover remote devices

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[John Lin [:jhlin][:jolin]]

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

Comment 1

[John Lin [:jhlin][:jolin]]

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

Comment 2

[John Lin [:jhlin][:jolin]]

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

Comment 3

[u459114]

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

Comment 4

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

Attachment: [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v1)

Comment 5

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

Attachment: [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v2)

Comment 6

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

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 7

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

Attachment: [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v2)

Comment 8

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

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.

Comment 9

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

Attachment: [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v3)

Comment 10

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

Depending on bug 860697 for utility functions.

Comment 11

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

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 12

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

Attachment: [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v4)

Comment 13

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

Attachment: [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v4)

Comment 14

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

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 15

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

Attachment: [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v5)

Comment 16

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

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.

Comment 17

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

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.

Comment 18

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

Attachment: [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v5), r=vyang

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

Comment 19

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

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.

Comment 20

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

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

Comment 21

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

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

Comment 22

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

Attachment: (final) [Mnw] Add a test case for discover remote device through BluetoothAdapter. (v5), r=vyang. r=echou

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e951b27626b8

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e951b27626b8