1043180 - (webbt-test-pairing) Write marionette tests for Bluetooth pairing.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Write marionette tests for Bluetooth pairing, including related methods, event handlers, BluetoothPairingRequestListeningHandle and BluetoothPairingEvent.

Comment 1

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

Attachment: Write a marionette test for Bluetooth pairing based on Bluetooth API v2. (v1)

Comment 2

[Ben Tian (inactive)]

Comment on attachment 8464558 [details] [diff] [review]
Write a marionette test for Bluetooth pairing based on Bluetooth API v2. (v1)

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

Please see my comment below. Also should we verify |getPairedDevices| method correctness?

::: dom/bluetooth2/tests/marionette/head.js
@@ +600,5 @@
>  
>  /**
> + * Wait for 'devicefound' events of specified devices.
> + *
> + * Resolve if that every specified devices has been found.  Never reject.

nit: 1) redundant 'that', 2) every specified 'device', and 3) extra space before 'Never'.

@@ +638,5 @@
> +  return deferred.promise;
> +}
> +
> +/**
> + * Verifies the correctness of 'devicepaired' or 'deviceunpaired' events.

nit: Verify

@@ +642,5 @@
> + * Verifies the correctness of 'devicepaired' or 'deviceunpaired' events.
> + *
> + * Use BluetoothAdapter.getPairedDevices() to get current device array.
> + * Resolve if the address from 'devicepaired' event exists in device array or
> + * the address from 'deviceunpaired' event has removed from device array.

nit: has 'been' removed

@@ +645,5 @@
> + * Resolve if the address from 'devicepaired' event exists in device array or
> + * the address from 'deviceunpaired' event has removed from device array.
> + * Otherwise, reject the promise.
> + *
> + * Fulfill params: the device event from the second parameter.

'from the second parameter' is unclear. I suggest just 'the device event from "devicepaired" or "deviceunpaired"', the same as below.

@@ +671,5 @@
> +    if (isPromisePending) {
> +      deferred.reject(aDeviceEvent);
> +      isPromisePending = false;
> +    }
> +  } else if (aDeviceEvent.address){

nit: space before {

@@ +676,5 @@
> +    log("  - Verify 'deviceunpaired' event");
> +    for (let i in devices) {
> +      if (devices[i].address == aDeviceEvent.address) {
> +        isPromisePending = false;
> +        deferred.reject(aDeviceEvent);

Why here set |isPromisePending| first but 'devicepaired' verification resolve promise first?

@@ +684,5 @@
> +      deferred.resolve(aDeviceEvent);
> +      isPromisePending = false;
> +    }
> +  } else {
> +    log("  - Exception occurs. The properties of aDeviceEvent are unexpected");

nit: 'Unexpected aDeviceEvent properties' is simpler.

@@ +688,5 @@
> +    log("  - Exception occurs. The properties of aDeviceEvent are unexpected");
> +    deferred.reject(aDeviceEvent);
> +    isPromisePending = false;
> +  }
> +  return deferred.promise;

nit: newline before.

@@ +692,5 @@
> +  return deferred.promise;
> +}
> +
> +/**
> + * Add evet handlers for pairing request listeners.

typo: event.

'listener' since only one listener with 4 event handlers.

@@ +696,5 @@
> + * Add evet handlers for pairing request listeners.
> + *
> + * @param aAdapter
> + *        The BluetoothAdapter you want to use.
> + * @param aSpecifiedBtAddress (nullable)

Use conventional |aSpecifiedB'd'Address|. Also why make it null instead of empty string to accept all pairing requests?

@@ +697,5 @@
> + *
> + * @param aAdapter
> + *        The BluetoothAdapter you want to use.
> + * @param aSpecifiedBtAddress (nullable)
> + *        The Bluetooth address for a specified remote device which adapter wants

nit: Bluetooth address of the specified remote device ...

@@ +698,5 @@
> + * @param aAdapter
> + *        The BluetoothAdapter you want to use.
> + * @param aSpecifiedBtAddress (nullable)
> + *        The Bluetooth address for a specified remote device which adapter wants
> + *        to pair with. This parameter can be null if user want to accept

nit: The parameter is null if user want's' to accept all pairing requests.

@@ +705,5 @@
> +function addEventHandlerForPairingRequest(aAdapter, aSpecifiedBtAddress) {
> +  log("  - Add event handlers for pairing requests.");
> +
> +  aAdapter.pairingReqs.ondisplaypasskeyreq = function onDisplayPasskeyReq(evt) {
> +    log("  - 'displaypasskeyreq' event got.");

"Received 'displaypasskeyreq' event" and revise all the following. 'Receive' is preciser than 'get'.

@@ +710,5 @@
> +
> +    var device = evt.device;
> +    var passkey = evt.handle.passkey; // passkey to display
> +    if (aSpecifiedBtAddress == null || device.address == aSpecifiedBtAddress) {
> +      cleanupPairingReqListeners(aAdapter.pairingReqs);

Rename function to |cleanupPairingListener| since |BluetoothPairingRequestListeningHandle| is renamed.

@@ +723,5 @@
> +      // TODO: Allow user to enter passkey.
> +      var UserEnteredPasskey = "0000";
> +      var passkey = UserEnteredPasskey;
> +
> +      evt.handle.setPasskey(passkey).then(

setPinCode

@@ +735,5 @@
> +        });
> +    }
> +  };
> +
> +  aAdapter.pairingReqs.onpairingconfirmationreq = function onPairingConfirmationReq(evt) {

nit: this line is too long.

@@ +766,5 @@
> +  };
> +}
> +
> +/**
> + * Clean up evet handlers for pairing request listeners.

typo: event

@@ +769,5 @@
> +/**
> + * Clean up evet handlers for pairing request listeners.
> + *
> + * @param aPairingReqs
> + *        A BluetoothPairingRequestListeningHandle with event handlers.

Rename to |BluetoothPairingListener|.

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_pair_API2.js
@@ +5,5 @@
> +///////////////////////////////////////////////////////////////////////////////
> +// Test Purpose:
> +//   To verify entire paring process of BluetoothAdapter, except for
> +//   in-line pairing.
> +//   Testers have to put a discoverable remote device near to the testing device

near the testing device. redundant 'to'.

@@ +6,5 @@
> +// Test Purpose:
> +//   To verify entire paring process of BluetoothAdapter, except for
> +//   in-line pairing.
> +//   Testers have to put a discoverable remote device near to the testing device
> +//   and click the 'confirm' button on remote device when it receive a pairing

receive's'

@@ +8,5 @@
> +//   in-line pairing.
> +//   Testers have to put a discoverable remote device near to the testing device
> +//   and click the 'confirm' button on remote device when it receive a pairing
> +//   request from testing device.
> +//   To pass this test, Bluetooth address of the remote device should equals to

should equal

@@ +13,5 @@
> +//   ADDRESS_OF_TARGETED_REMOTE_DEVICE.
> +//
> +// Test Procedure:
> +//   [0] Set Bluetooth permission and enable default adapter.
> +//   [1] Unpair device if it's already beenpaired.

redundant 'been'

@@ +15,5 @@
> +// Test Procedure:
> +//   [0] Set Bluetooth permission and enable default adapter.
> +//   [1] Unpair device if it's already beenpaired.
> +//   [2] Start discovery.
> +//   [3] Wait for a specified 'devicefound' event.

'the' specified event since it's specified. Also revise all the following.

@@ +21,5 @@
> +//   [5] Pair and wait for confirmation.
> +//   [6] Get paired devices and verify 'devicepaired' event.
> +//   [7] Pair again.
> +//   [8] Unpair.
> +//   [9] Get paired devices and and verify 'deviceunpaired' event.

redundant 'and'

@@ +36,5 @@
> +//
> +//   - BluetoothPairingRequestListeningHandle.ondisplaypassykeyreq()
> +//   - BluetoothPairingRequestListeningHandle.onenterpasskeyreq()
> +//   - BluetoothPairingRequestListeningHandle.onpairingconfirmationreq()
> +//   - BluetoothPairingRequestListeningHandle.onpairingconsentreq()

Rename |BluetoothPairingRequestListeningHandle|.

@@ +41,5 @@
> +//
> +//   - BluetoothPairingEvent.device
> +//   - BluetoothPairingEvent.handle
> +//
> +//   - BluetoothPairingHandle.setPasskey()

setPinCode

@@ +66,5 @@
> +  log("adapter.name: " + aAdapter.name);
> +
> +  return Promise.resolve()
> +    .then(function() {
> +      log("[1] Unpair device if it's already been paired ... ");

redundant 'been'

Comment 3

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

Attachment: Write a marionette test for Bluetooth pairing based on Bluetooth API v2. (v2)

Comment 4

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

(In reply to Ben Tian [:btian] from comment #2)
> Comment on attachment 8464558 [details] [diff] [review]
Thank you for your comments. :)

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8468357 [details] [diff] [review]
Write a marionette test for Bluetooth pairing based on Bluetooth API v2. (v2)

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

Almost there. Please rename all passkey to pincode in onenterpincodereq event handler. Also any reason to use |aSpecifiedBtAddress| rather than |aSpecifiedBdAddress|?

::: dom/bluetooth2/tests/marionette/head.js
@@ +617,5 @@
> +function waitForSpecifiedDevicesFound(aDiscoveryHandle, aRemoteAddresses) {
> +  let deferred = Promise.defer();
> +
> +  ok(aDiscoveryHandle instanceof BluetoothDiscoveryHandle,
> +    "discoveryHandle should be a BluetoothDiscoveryHandle");

nit: I think |discoveryHandle| should be |aDiscoveryHandle| as |aEvent| check below.

@@ +641,5 @@
> +/**
> + * Verify the correctness of 'devicepaired' or 'deviceunpaired' events.
> + *
> + * Use BluetoothAdapter.getPairedDevices() to get current device array.
> + * Resolve if the address from 'devicepaired' event exists in device array or

Make the comment preciser:

  Resolve if the device from 'devicepaired' ... or device of the address from 'deviceunpaired' ...

@@ +697,5 @@
> + * Add event handlers for pairing request listener.
> + *
> + * @param aAdapter
> + *        The BluetoothAdapter you want to use.
> + * @param aSpecifiedBtAddress (optional)

Any reason to keep |BtAddress| rather than |BdAddress|? Since |BdAddress| is more conventional.

@@ +699,5 @@
> + * @param aAdapter
> + *        The BluetoothAdapter you want to use.
> + * @param aSpecifiedBtAddress (optional)
> + *        Bluetooth address of the specified remote device which adapter wants
> + *        to pair with. If aSpecifiedBtAddress is a empty string, 'null' or

nit: 'an' empty string

@@ +700,5 @@
> + *        The BluetoothAdapter you want to use.
> + * @param aSpecifiedBtAddress (optional)
> + *        Bluetooth address of the specified remote device which adapter wants
> + *        to pair with. If aSpecifiedBtAddress is a empty string, 'null' or
> + *        'undefined', it represents this function accepts every pairing request.

nit: Remove 'it represents'.

@@ +715,5 @@
> +      cleanupPairingListener(aAdapter.pairingReqs);
> +    }
> +  };
> +
> +  aAdapter.pairingReqs.onenterpincodereq = function onEnterPasskeyReq(evt) {

onEnterPinCodeReq

@@ +716,5 @@
> +    }
> +  };
> +
> +  aAdapter.pairingReqs.onenterpincodereq = function onEnterPasskeyReq(evt) {
> +    log("  - Received'onenterpincodereq' event.");

nit: space after Received.

@@ +720,5 @@
> +    log("  - Received'onenterpincodereq' event.");
> +
> +    var device = evt.device;
> +    if (!aSpecifiedBtAddress || device.address == aSpecifiedBtAddress) {
> +      // TODO: Allow user to enter passkey.

enter pincode.

@@ +722,5 @@
> +    var device = evt.device;
> +    if (!aSpecifiedBtAddress || device.address == aSpecifiedBtAddress) {
> +      // TODO: Allow user to enter passkey.
> +      var UserEnteredPinCode = "0000";
> +      var passkey = UserEnteredPasskey;

var pinCode = UserEnteredPinCode;

@@ +724,5 @@
> +      // TODO: Allow user to enter passkey.
> +      var UserEnteredPinCode = "0000";
> +      var passkey = UserEnteredPasskey;
> +
> +      evt.handle.setPinCode(passkey).then(

setPinCode(pinCode)

@@ +726,5 @@
> +      var passkey = UserEnteredPasskey;
> +
> +      evt.handle.setPinCode(passkey).then(
> +        function onResolve() {
> +          log("  - 'setPasskey' resolve.");

setPinCode

@@ +730,5 @@
> +          log("  - 'setPasskey' resolve.");
> +          cleanupPairingListener(aAdapter.pairingReqs);
> +        },
> +        function onReject() {
> +          log("  - 'setPasskey' reject.");

setPinCode

Comment 6

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

Attachment: Write a marionette test for Bluetooth pairing based on Bluetooth API v2. (v3)

Comment 7

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

(In reply to Ben Tian [:btian] from comment #5)
> Comment on attachment 8468357 [details] [diff] [review]
> @@ +697,5 @@
> > + * Add event handlers for pairing request listener.
> > + *
> > + * @param aAdapter
> > + *        The BluetoothAdapter you want to use.
> > + * @param aSpecifiedBtAddress (optional)
> 
> Any reason to keep |BtAddress| rather than |BdAddress|? Since |BdAddress| is
> more conventional.
Thank you for your suggestion.
I replaced 'BtAddress' by 'BdAddress' in #attachment 8468981 [details] [diff] [review].

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8468981 [details] [diff] [review]
Write a marionette test for Bluetooth pairing based on Bluetooth API v2. (v3)

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

r=me with nits addressed. Thanks.

::: dom/bluetooth2/tests/marionette/head.js
@@ +642,5 @@
> + * Verify the correctness of 'devicepaired' or 'deviceunpaired' events.
> + *
> + * Use BluetoothAdapter.getPairedDevices() to get current device array.
> + * Resolve if the device from 'devicepaired' event exists in device array or
> + * the device from 'deviceunpaired' event has been removed from device array.

or "device of the address" from 'deviceunpaired' event.

@@ +708,5 @@
> +
> +  aAdapter.pairingReqs.ondisplaypasskeyreq = function onDisplayPasskeyReq(evt) {
> +    let passkey = evt.handle.passkey; // passkey to display
> +    ok(typeof passkey === 'string', "type checking for passkey.");
> +    log("  - Received 'displaypasskeyreq' event. with passkey: " + passkey);

nit: remove extra period and add 'on' to 'displaypasskeyreq':

  Received 'ondisplaypasskeyreq' event with passkey:

Comment 9

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

Attachment: Write a marionette test for Bluetooth pairing based on Bluetooth API v2. (v4), r=btian

Thank Ben for the great reviews.

Comment 10

[Ben Tian (inactive)]

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

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/230e14dfa5a0