Bug 1030536 (webbt-test-setprop)

Write marionette tests to set adapter properties

RESOLVED FIXED in 2.0 S5 (4july)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ben.tian, Assigned: jaliu)

Tracking

unspecified
2.0 S5 (4july)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Write marionette tests for BluetoothAdapter set* API.
(Reporter)

Updated

4 years ago
Alias: webbt-test-setprop
Whiteboard: [webbt-api]
(Reporter)

Comment 1

4 years ago
Jamin, please help on this bug since bug 1006310 is fixed.
Assignee: nobody → jaliu
Depends on: 1006310
(Assignee)

Updated

4 years ago
Whiteboard: [webbt-api] → [webbt-api][p=1]
(Assignee)

Updated

4 years ago
Target Milestone: --- → 2.0 S5 (4july)
(Assignee)

Comment 2

4 years ago
Created attachment 8447900 [details] [diff] [review]
Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v1)
Attachment #8447900 - Flags: review?(btian)
(Assignee)

Updated

4 years ago
Depends on: 1031252
No longer depends on: 1006310
(Assignee)

Updated

4 years ago
Depends on: 1006310
(Assignee)

Comment 3

4 years ago
Created attachment 8447923 [details] [diff] [review]
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.
Attachment #8447900 - Attachment is obsolete: true
Attachment #8447900 - Flags: review?(btian)
Attachment #8447923 - Flags: review?(btian)
(Reporter)

Comment 4

4 years ago
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|?
(Assignee)

Comment 5

4 years ago
Created attachment 8449232 [details] [diff] [review]
Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v3)
Attachment #8447923 - Attachment is obsolete: true
Attachment #8447923 - Flags: review?(btian)
Attachment #8449232 - Flags: review?(btian)
(Assignee)

Comment 6

4 years ago
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.
(Reporter)

Comment 7

4 years ago
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|?
(Assignee)

Comment 8

4 years ago
Created attachment 8450776 [details] [diff] [review]
Write a marionette test for verifying the setters of adapter based on bluetooth API v2. (v4)
Attachment #8449232 - Attachment is obsolete: true
Attachment #8449232 - Flags: review?(btian)
Attachment #8450776 - Flags: review?(btian)
(Assignee)

Comment 9

4 years ago
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.
(Reporter)

Comment 10

4 years ago
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.
Attachment #8450776 - Flags: review?(btian) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 8450856 [details] [diff] [review]
(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.
Attachment #8450776 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 1006317
(Assignee)

Comment 12

4 years ago
Created attachment 8452225 [details] [diff] [review]
(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].
Attachment #8450856 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e8e8e3d5ae5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.