1006317 - (webbt-test-onoff) Write marionette tests for BT on/off

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Write marionette tests for API in bug 1006308 comment 0.

Comment 1

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

Attachment: [dev] Add two marionette tests for verifying Bluetooth on/off in development stage. DO NOT LAND IT.

Add two experimental marionette tests for verifying Bluetooth on/off in development stage. DO NOT LAND IT.

Comment 2

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

Attachment: [dev] Add two marionette tests for verifying Bluetooth on/off in development stage for device. DO NOT LAND IT.

Comment 3

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

Attachment: [dev] Add a marionette test for verifying Bluetooth on/off in development stage for device. (v0)

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8441931 [details] [diff] [review]
[dev] Add a marionette test for verifying Bluetooth on/off in development stage for device. (v0)

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

f=me with comment addressed. Thanks.

::: dom/bluetooth2/tests/marionette/head.js
@@ +56,5 @@
> + * Push required permissions and test if |navigator.mozBluetooth| exists.
> + * Resolve if it does, reject otherwise.
> + *
> + * Fulfill params:
> + *   bluetoothManager -- an reference to navigator.mozBluetooth.

nit: 'a' reference

::: dom/bluetooth2/tests/marionette/test_BluetoothAdapter_enable_API2.js
@@ +10,5 @@
> +
> +  let statesArray = [];
> +  aAdapter.addEventListener("onattributechanged", function onevent(aEvent) {
> +    let stateIndex = 0;
> +    log("Attribute: " + aEvent.attr + " has changed.");

Comment out implementation about onattributechanged since we are revising webidl of attribute event for multiple changed attributes. Rewrite here once bug 1019544 is landed.

Comment 5

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

Attachment: [dev] Add a marionette test for verifying Bluetooth on/off in development stage for device. (v1)

Update the marionette test to support 'onattributechanged' since Bug 1019544 has landed.

Comment 6

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

The 'bluetooth2' API should pass the marionette test in #attachment 8442702 [details] [diff] [review] when Bug 1027552 land.

Please switch BT API to 'bluetooth2' by applying API configuration patch #attachment 8426731 [details] [diff] [review] which was attached in Bug 1005848 before you run this test.

Comment 7

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

Attachment: Write a marionette test for BT on/off based on bluetooth API v2.

Comment 8

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

Attachment: Write a marionette test for BT on/off based on bluetooth API v2. (v2)

Move a utility function from test file to head.js.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8447921 [details] [diff] [review]
Write a marionette test for BT on/off based on bluetooth API v2. (v2)

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

Please see my following questions.

::: dom/bluetooth2/tests/marionette/head.js
@@ +396,5 @@
> + * with specified order.
> + *
> + * Resolve if that expected events occur in order.  Never reject.
> + *
> + * Fulfill params: a arrary which contains every changed attributes during

nit: 'an array'

@@ +402,5 @@
> + *
> + * @param aAdapter
> + *        The BluetoothAdapter you want to use.
> + * @param aStateChangesInOrder
> + *        A array which contains a expected order of BluetoothAdapterState.

nit: 'An' array ... 'an' expected

@@ +404,5 @@
> + *        The BluetoothAdapter you want to use.
> + * @param aStateChangesInOrder
> + *        A array which contains a expected order of BluetoothAdapterState.
> + *        example 1: [enabling, enabled]
> + *        example 1: [disabling, disabled]

Should [disabling, disabled] be example 2?

@@ +424,5 @@
> +          log("  'state' changed to " + aAdapter.state);
> +          if (aStateChangesInOrder.indexOf(aAdapter.state) >= stateIndex) {
> +            statesArray.push(aAdapter.state);
> +            stateIndex = aStateChangesInOrder.indexOf(aAdapter.state);
> +            if (stateIndex > statesArray.length) {

Why '>' instead of '==='? Can we check whether |aAdapter.state| equals to |aStateChangesInOrder[stateIndex]| directly?

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_enable_API2.js
@@ +21,5 @@
> +startBluetoothTest(true, function testCaseMain(aAdapter) {
> +  log("Checking adapter attributes ...");
> +
> +  is(aAdapter.state, "enabled", "adapter.state");
> +  is(aAdapter.discovering, false, "adapter.discovering");

Why should |adapter.discovering| be false here?

@@ +22,5 @@
> +  log("Checking adapter attributes ...");
> +
> +  is(aAdapter.state, "enabled", "adapter.state");
> +  is(aAdapter.discovering, false, "adapter.discovering");
> +  isnot(aAdapter.address, "", "adapter.address");

Should |adapter.name| be checked here?

@@ +43,5 @@
> +      return Promise.all(promises);
> +    })
> +    .then(function(aResults) {
> +      isnot(aResults[0].indexOf("address"), -1, "Indicator of 'address' changed event");
> +      is(aAdapter.address, "", "adapter.address");

Why check |adapter.address| only? How about |adapter.name|?

@@ +54,5 @@
> +      return Promise.all(promises);
> +    })
> +    .then(function(aResults) {
> +      isnot(aResults[0].indexOf("address"), -1, "Indicator of 'address' changed event");
> +      is(aAdapter.address, originalAddr, "adapter.address");

Ditto.

Comment 10

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

Attachment: Write a marionette test for BT on/off based on bluetooth API v2. (v3)

Comment 11

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

Thank you for your comments.
I've uploaded a new patch based on your comments.

(In reply to Ben Tian [:btian] from comment #9)
> @@ +424,5 @@
> > +          log("  'state' changed to " + aAdapter.state);
> > +          if (aStateChangesInOrder.indexOf(aAdapter.state) >= stateIndex) {
> > +            statesArray.push(aAdapter.state);
> > +            stateIndex = aStateChangesInOrder.indexOf(aAdapter.state);
> > +            if (stateIndex > statesArray.length) {
> 
> Why '>' instead of '==='? Can we check whether |aAdapter.state| equals to
> |aStateChangesInOrder[stateIndex]| directly?
I corrected this statement and add few comments for it.
Thanks.

I didn't use "==" as the condition here since the value of state could be changed before we receive corresponded 'onattributechanged' event.
If state was changed by this sequence [enabling, enabled], the value we get from aAdapter.state could be [enabling, enabled] or [enabled, enabled].

> ::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_enable_API2.js
> @@ +21,5 @@
> > +startBluetoothTest(true, function testCaseMain(aAdapter) {
> > +  log("Checking adapter attributes ...");
> > +
> > +  is(aAdapter.state, "enabled", "adapter.state");
> > +  is(aAdapter.discovering, false, "adapter.discovering");
> 
> Why should |adapter.discovering| be false here?
Since adapter has just been re-enabled here, the expected value of aAdapter.discovering is 'false'.

> @@ +22,5 @@
> > +  log("Checking adapter attributes ...");
> > +
> > +  is(aAdapter.state, "enabled", "adapter.state");
> > +  is(aAdapter.discovering, false, "adapter.discovering");
> > +  isnot(aAdapter.address, "", "adapter.address");
> 
> Should |adapter.name| be checked here?
Yes, thank you for reminding me.

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8449231 [details] [diff] [review]
Write a marionette test for BT on/off based on bluetooth API v2. (v3)

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

Please see my comment below.

::: dom/bluetooth2/tests/marionette/head.js
@@ +394,5 @@
>  /**
> + * Wait for 'onattributechanged' events for state changes of BluetoothAdapter
> + * with specified order.
> + *
> + * Resolve if that expected events occur in order.  Never reject.

nit: 'the'/'those' expected events, since 'events' is plural.

@@ +396,5 @@
> + * with specified order.
> + *
> + * Resolve if that expected events occur in order.  Never reject.
> + *
> + * Fulfill params: an arrary which contains every changed attributes during

nit: 'array'

@@ +414,5 @@
> +
> +  let stateIndex = 0;
> +  let statesArray = [];
> +  let changedAttrs = [];
> +  aAdapter.onattributechanged = null;

Does the event handler have to be cleared before being overridden?

@@ +421,5 @@
> +      changedAttrs.push(aEvent.attrs[i]);
> +      switch (aEvent.attrs[i]) {
> +        case "state":
> +          log("  'state' changed to " + aAdapter.state);
> +          if (aStateChangesInOrder.indexOf(aAdapter.state) >= stateIndex) {

I suggest to simplify this section as following:

 let prevStateIndex = 0;
 ...
 switch (aEvent.attrs[i]) {
   case "state":
     log(" 'state' changed to " + aAdapter.state);

     /**
      * Received state change order may differ from expected one even though
      * state changes in expected order, because the value of state may change
      * again before we receive prior 'onattributechanged' event.
      *
      * For example, expected state change order [A,B,C] may result in
      * received ones:
      * - [A,C,C] if state becomes C before we receive 2nd 'onattributechanged'
      * - [B,B,C] if state becomes B before we receive 1st 'onattributechanged'
      * - [C,C,C] if state becomes C before we receive 1st 'onattributechanged'
      * - [A,B,C] if all 'onattributechanged' are received in perfect timing
      * 
      * As a result, we ensure only following conditions instead of exactly
      * matching received and expected state change order.
      * - Received state change order never reverse expected one. For example,
      *   [B,A,C] should never occur with expected state change order [A,B,C].
      * - The changed value of state in received state change order never
      *   appears later than that in expected one. For example, [A,A,C] should
      *   never occur with expected state change order [A,B,C].
      */
     let stateIndex = aStateChangesInOrder.indexOf(aAdapter.state);
     if (stateIndex >= prevStateIndex && stateIndex + 1 > statesArray.length) {
       statesArray.push(aAdapter.state);
       prevStateIndex = stateIndex;

       if (statesArray.length == aStateChangesInOrder.length) {
         aAdapter.onattributechanged = null;
         ok(true, "BluetoothAdapter event 'onattributechanged' got.");
         deferred.resolve(changedAttrs);
       }
     } else {
       // Test fails
     }

@@ +513,5 @@
>      });
>  }
>  
>  function startBluetoothTest(aReenable, aTestCaseMain) {
> +  startBluetoothTestBase([], function() {

Can we remove parameter |aPermissions| from |startBluetoothTestBase|?

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_enable_API2.js
@@ +22,5 @@
> +  log("Checking adapter attributes ...");
> +
> +  is(aAdapter.state, "enabled", "adapter.state");
> +  // Since adapter has just been re-enabled, aAdapter.discovering should be 'false'.
> +  is(aAdapter.discovering, false, "adapter.discovering");

Why should only |adapter.discovering| be false here but not |adapter.discoverable|?

Comment 13

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

Attachment: Write a marionette test for BT on/off based on bluetooth API v2. (v4)

Comment 14

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

Thank you for your comments.
I've uploaded a new patch based on your comments.

(In reply to Ben Tian [:btian] from comment #12)
> @@ +414,5 @@
> > +
> > +  let stateIndex = 0;
> > +  let statesArray = [];
> > +  let changedAttrs = [];
> > +  aAdapter.onattributechanged = null;
> 
> Does the event handler have to be cleared before being overridden?
I think it's redundant.
I've removed  it.
Thanks.

> @@ +513,5 @@
> >      });
> >  }
> >  
> >  function startBluetoothTest(aReenable, aTestCaseMain) {
> > +  startBluetoothTestBase([], function() {
> 
> Can we remove parameter |aPermissions| from |startBluetoothTestBase|?
I'd prefer to keep the parameter and let it be a empty array if you don't need additional permissions.
I would be useful if we want to design a test which across different components. i.e. BT + NFC, BT + RIL

> ::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_enable_API2.js
> @@ +22,5 @@
> > +  log("Checking adapter attributes ...");
> > +
> > +  is(aAdapter.state, "enabled", "adapter.state");
> > +  // Since adapter has just been re-enabled, aAdapter.discovering should be 'false'.
> > +  is(aAdapter.discovering, false, "adapter.discovering");
> 
> Why should only |adapter.discovering| be false here but not
> |adapter.discoverable|?
After discussion with Ben and Jocelyn, we decide to make 'adapter.discoverable' invalid to be 'false' when adapter is just re-enabled.

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8450775 [details] [diff] [review]
Write a marionette test for BT on/off based on bluetooth API v2. (v4)

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

r=me with nits addressed. Thanks.

::: dom/bluetooth2/tests/marionette/head.js
@@ +394,5 @@
>  /**
> + * Wait for 'onattributechanged' events for state changes of BluetoothAdapter
> + * with specified order.
> + *
> + * Resolve if those expected events occur in order.  Never reject.

nit: remove extra space before 'Never'.

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_enable_API2.js
@@ +5,5 @@
> +///////////////////////////////////////////////////////////////////////////////
> +// Test Purpose:
> +//   To verify that enable/disable process of BluetoothAdapter is correct.
> +//
> +// Test Coverage:

Add Test Procedure in comment as setter test.

@@ +46,5 @@
> +      return Promise.all(promises);
> +    })
> +    .then(function(aResults) {
> +      isnot(aResults[0].indexOf("address"), -1, "Indicator of 'address' changed event");
> +      if(originalName != "") {

nit: space after if.

@@ +61,5 @@
> +      return Promise.all(promises);
> +    })
> +    .then(function(aResults) {
> +      isnot(aResults[0].indexOf("address"), -1, "Indicator of 'address' changed event");
> +      if(originalName != "") {

Ditto.

Comment 16

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

Attachment: (final) Write a marionette test for BT on/off based on bluetooth API v2. (v5), r=btian

Thank Ben for the great reviews.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Have we confirmed that this runs green on Try? :)

Comment 18

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

Hi Ryan,

Thank you for the reminder.

Bluetooth team is working on the refined BT APIs which are expected to expose to privilege app in future.
The refined BT APIs was placed in a new folder gecko/dom/bluetooth2/ which wouldn't be compiled in current stage. Therefore, the marionette test wouldn't be added to unit-tests.ini until the API refinement complete.

Currently, B2G device and B2G emulator use 'BlueZ' as a Bluetooth protocol stack. (It means the 'BlueZ' stack is supported by Gonk.)
Bluetooth team plans to replace BlueZ by BlueDroid in v2.2, therefore, the new BT APIs will only support 'Bluedroid' stack.

Since the new APIs wouldn't be activated until v2.2, thus, it can't run Marionette-WebAPI-Test in Try server. Nevertheless, the test was verified in a specialized B2G device which has installed the prebuilt 'Bluedroid' library.

P.S. For the development stage, Gecko and Gaia guys use marionette tests to verify the correctness of API implementation.

Sorry for the lack of explanation.
Thank you.

Comment 19

[Ben Tian (inactive)]

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

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/87b7fee443c8