Closed Bug 1006317 (webbt-test-onoff) Opened 7 years ago Closed 7 years ago

Write marionette tests for BT on/off

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S6 (18july)

People

(Reporter: ben.tian, Assigned: jaliu)

References

Details

(Whiteboard: [webbt-api][p=1])

Attachments

(1 file, 8 obsolete files)

10.65 KB, patch
Details | Diff | Splinter Review
Write marionette tests for API in bug 1006308 comment 0.
Whiteboard: [webbt-api]
Assignee: nobody → jaliu
Add two experimental marionette tests for verifying Bluetooth on/off in development stage. DO NOT LAND IT.
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.
Attachment #8441931 - Flags: feedback?(btian) → feedback+
Update the marionette test to support 'onattributechanged' since Bug 1019544 has landed.
Attachment #8441931 - Attachment is obsolete: true
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.
Depends on: 1027552
No longer depends on: 1006316
Blocks: webbt-api-meta
No longer blocks: webbt-api-onoff
Alias: webbt-api-test-onoff
Alias: webbt-api-test-onoff → webbt-test-onoff
Whiteboard: [webbt-api] → [webbt-api][p=1]
Attachment #8442702 - Attachment is obsolete: true
Attachment #8447899 - Flags: review?(btian)
Move a utility function from test file to head.js.
Attachment #8447921 - Flags: review?(btian)
Attachment #8447899 - Attachment is obsolete: true
Attachment #8447899 - Flags: review?(btian)
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.
Attachment #8447921 - Attachment is obsolete: true
Attachment #8447921 - Flags: review?(btian)
Attachment #8449231 - Flags: review?(btian)
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 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|?
Attachment #8449231 - Attachment is obsolete: true
Attachment #8449231 - Flags: review?(btian)
Attachment #8450775 - Flags: review?(btian)
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 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.
Attachment #8450775 - Flags: review?(btian) → review+
Thank Ben for the great reviews.
Attachment #8450775 - Attachment is obsolete: true
Attachment #8450850 - Attachment description: (final) Write a marionette test for BT on/off based on bluetooth API v2. (v5) → (final) Write a marionette test for BT on/off based on bluetooth API v2. (v5), r=btian
Keywords: checkin-needed
Have we confirmed that this runs green on Try? :)
Keywords: checkin-needed
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87b7fee443c8
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
You need to log in before you can comment on or make changes to this bug.