1030536 - (webbt-test-setprop) Write marionette tests to set adapter properties

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Write marionette tests for BluetoothAdapter set* API.

Comment 1

[Ben Tian (inactive)]

Jamin, please help on this bug since bug 1006310 is fixed.

Comment 2

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

Attachment: Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v1)

Comment 3

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

Attachment: Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v2)

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

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8447923 [details] [diff] [review]
Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v2)

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

Please see my comment below. Also we may wrap common testing steps into functions (e.g., enable/disable bluetooth) to reuse them in different tests.

::: dom/bluetooth2/tests/marionette/head.js
@@ +482,5 @@
>    return deferred.promise;
>  }
>  
>  /**
> + * Wait for a 'onattributechanged' event for a specified attribute and compare

nit: 'an' onattributechanged event

@@ +494,5 @@
> + * @param aAdapter
> + *        The BluetoothAdapter you want to use.
> + * @param aAttrName
> + *        The name of the attribute of adapter.
> +* @param aExpectedValue

nit: add extra space.

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_setters_API2.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +///////////////////////////////////////////////////////////////////////////////
> +// Test Purpose:

Please add comment to brief test procedure for understanding.

@@ +39,5 @@
> +      let promises = [];
> +      if (aAdapter.discoverable == originalDiscoverable) {
> +        promises.push(waitForAdapterAttributeChanged(aAdapter, "discoverable", !originalDiscoverable));
> +      }
> +      promises.push(aAdapter.setDiscoverable(!originalDiscoverable));

Why not |setDiscoverable(!adapter.discoverable)| and wait for its attribute change as |setName|?

Comment 5

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

Attachment: Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v3)

Comment 6

[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 #4)
> ::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_setters_API2.js
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +///////////////////////////////////////////////////////////////////////////////
> > +// Test Purpose:
> 
> Please add comment to brief test procedure for understanding.
Great idea.

> @@ +39,5 @@
> > +      let promises = [];
> > +      if (aAdapter.discoverable == originalDiscoverable) {
> > +        promises.push(waitForAdapterAttributeChanged(aAdapter, "discoverable", !originalDiscoverable));
> > +      }
> > +      promises.push(aAdapter.setDiscoverable(!originalDiscoverable));
> 
> Why not |setDiscoverable(!adapter.discoverable)| and wait for its attribute
> change as |setName|?
Sounds like a good idea.
Thanks.

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8449232 [details] [diff] [review]
Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v3)

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

Please see my comment below.

::: dom/bluetooth2/tests/marionette/head.js
@@ +491,5 @@
>  /**
> + * Wait for an 'onattributechanged' event for a specified attribute and compare
> + * the new value with the expected value.
> + *
> + * Resolve if that the specified event occurs.  Never reject.

nit: remove redundant 'that'

@@ +509,5 @@
> +function waitForAdapterAttributeChanged(aAdapter, aAttrName, aExpectedValue) {
> +  let deferred = Promise.defer();
> +
> +  let statesArray = [];
> +  let stateIndex = 0;

Remove these two unused variables.

@@ +510,5 @@
> +  let deferred = Promise.defer();
> +
> +  let statesArray = [];
> +  let stateIndex = 0;
> +  aAdapter.onattributechanged = null;

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

@@ +513,5 @@
> +  let stateIndex = 0;
> +  aAdapter.onattributechanged = null;
> +  aAdapter.onattributechanged = function(aEvent) {
> +    let i = aEvent.attrs.indexOf(aAttrName);
> +    if (i >= 0) {

Tests would fail if |i < 0|, right?

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_setters_API2.js
@@ +12,5 @@
> +//   [1] Verify the functionality of 'setDiscoverable'.
> +//   [2] Verify the functionality of 'setName'.
> +//   [3] Disable Bluetooth and collect changed attributes.
> +//   [4] Verify the changes of attributes when BT is disabled.
> +//   [5] Set properties when Bluetooth is disabled ...

nit: replace '...' with period only.

@@ +36,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|?

@@ +70,5 @@
> +    })
> +    .then(function(aResults) {
> +      log("[4] Verify the changes of attributes ...");
> +      isnot(aResults[0].indexOf("name"), -1, "Indicator of 'name' changed event");
> +      isnot(aResults[0].indexOf("discoverable"), -1, "Indicator of 'discoverable' changed event");

Adapter.discoverable may not change if it's false before bluetooth disabling.

@@ +91,5 @@
> +    .then(function(aResults) {
> +      log("[7] Verify the changes of attributes ...");
> +      isnot(aResults[0].indexOf("name"), -1, "Indicator of 'name' changed event");
> +      is(aAdapter.name, originalName + "_modified", "adapter.name");
> +      // Don't check 'discoverable' since BT stack may set it to false anytime.

Why would |adapter.discoverable| become true before? It should be false right after adapter is enabled, before any |adapter.setDiscoverable| call.

@@ +99,5 @@
> +      let promises = [];
> +      if (aAdapter.discoverable != originalDiscoverable) {
> +        promises.push(waitForAdapterAttributeChanged(aAdapter, "discoverable", originalDiscoverable));
> +      }
> +      promises.push(aAdapter.setDiscoverable(originalDiscoverable));

Should we do |adapter.setDiscoverable| even though |adapter.discoverable == originalDiscoverable|?

Comment 8

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

Attachment: Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v4)

Comment 9

[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 #7)
> @@ +510,5 @@
> > +  let deferred = Promise.defer();
> > +
> > +  let statesArray = [];
> > +  let stateIndex = 0;
> > +  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 @@
> > +  let stateIndex = 0;
> > +  aAdapter.onattributechanged = null;
> > +  aAdapter.onattributechanged = function(aEvent) {
> > +    let i = aEvent.attrs.indexOf(aAttrName);
> > +    if (i >= 0) {
> 
> Tests would fail if |i < 0|, right?
I would prefer not to make the test fail directly when |i < 0|.
If we do so, the test would be invalid when we add new attribute in future.

> @@ +36,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|?
The test would check aAdapter.discoverable with Attachment #8450776 [details] [diff].
Thanks.

> @@ +99,5 @@
> > +      let promises = [];
> > +      if (aAdapter.discoverable != originalDiscoverable) {
> > +        promises.push(waitForAdapterAttributeChanged(aAdapter, "discoverable", originalDiscoverable));
> > +      }
> > +      promises.push(aAdapter.setDiscoverable(originalDiscoverable));
> 
> Should we do |adapter.setDiscoverable| even though |adapter.discoverable ==
> originalDiscoverable|?
I'd like to check the validity of setting properties to their present value.
I modified this part and add a comment.

Thank you.

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8450776 [details] [diff] [review]
Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v4)

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

r=me with comment addressed. Thanks.

::: dom/bluetooth2/tests/marionette/head.js
@@ +506,5 @@
>  /**
> + * Wait for an 'onattributechanged' event for a specified attribute and compare
> + * the new value with the expected value.
> + *
> + * Resolve if the specified event occurs.  Never reject.

nit: remove extra space before 'Never'.

::: dom/bluetooth2/tests/marionette/test_dom_BluetoothAdapter_setters_API2.js
@@ +16,5 @@
> +//   [5] Set properties when Bluetooth is disabled.
> +//   [6] Enable Bluetooth and collect changed attributes.
> +//   [7] Verify the changes of attributes when BT is enabled.
> +//   [8] Restore the original 'adapter.name'.
> +//   [9] Check the validity of setting properties to their present value...

nit: '.' instead of '...'

@@ +56,5 @@
> +    })
> +    .then(function() {
> +      log("[2] Set 'discoverable' ... ");
> +      let promises = [];
> +      promises.push(waitForAdapterAttributeChanged(aAdapter, "discoverable", !aAdapter.discoverable));

Replace |!aAdapter.discoverable| with true.

Comment 11

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

Attachment: (final) Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v5), r=btian

Thank Ben for the great reviews.

Comment 12

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

Attachment: (final) Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v6), r=btian

Uploaded a new patch.
1. Modified the description of 'is(aAdapter.discoverable, ...)'
2. Correct the empty function for the rejected promise at step [5].

Comment 13

[Ben Tian (inactive)]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e8e8e3d5ae5