Write marionette tests for BluetoothAdapter set* API.

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

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

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|?

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 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|?

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 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.

Thank Ben for the great reviews.

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