[Blocking Requested - why for this release]: Somehow managed to miss this in my test fixes for bug 900551. Bluetooth still uses the return of lock.set() to confirm settings changes, instead of using the transaction finish event. This can cause tests to go intermittent depending on whether or not the settings system finishes the transaction before the check returns.

Ok, missed this in bluetooth2 also. Not that it runs yet, but I'm trying to cover all the bases here. :)

And it looks like I forgot to do this in mobileconnection too. Who knows how many intermittents this is going to fix. Wheee.

Generalizing title since I found more instances of this.

Comment on attachment 8491262 [details] [diff] [review] Patch 2 (v1) - Make mobileconnection webapi marionette tests wait for transaction success when changing setting Review of attachment 8491262 [details] [diff] [review]: ----------------------------------------------------------------- Had a check in dom/mobileconnection/ dom/cellbroadcast/ dom/icc/ dom/mobilemessage/ dom/telephony/ dom/voicemail/ dom/wappush/ dom/system/gonk/, this is the only one remains. Thank you.

Comment on attachment 8491260 [details] [diff] [review] Patch 1 ( v2) - Make bluetooth webapi marionette tests wait for transaction success when changing settings Review of attachment 8491260 [details] [diff] [review]: ----------------------------------------------------------------- Hey Kyle, Please see my question below. I would also like to add Jamin, the original author of Bluetooth test cases, to give his feedback for the patch. :) ::: dom/bluetooth/tests/marionette/head.js @@ +485,4 @@ > * > * @return A deferred promise. > */ > +function setSettings(aKey, aValue, aAllowError) { Question: the only place which uses this function is function setBluetoothEnabled(), and the way it gets called is function setBluetoothEnabled(aEnabled) { let obj = {}; obj["bluetooth.enabled"] = aEnabled; return setSettings(obj); // Should we revise this line as well? }

Comment on attachment 8491260 [details] [diff] [review] Patch 1 ( v2) - Make bluetooth webapi marionette tests wait for transaction success when changing settings Review of attachment 8491260 [details] [diff] [review]: ----------------------------------------------------------------- Kyle, It looks fine to me. Thank you. I have a question for you. Is this change only needed by marionette tests? or should it be apply to Gaia apps which have same usage of mozSettings.createLock().set() ?

Not blocking on test only changes. Ask for approval and land.

Actually patch 1 is wildly wrong. I moved the wrong function over, so the arguments are off and all bluetooth tests fail. That caused some nasty Mnw breakage on try and locally. Fixing that and resubmitting for review.

Copy/pasted wrong version of setSettings using transactions that had different arguments. Unsurprisingly, that broke everything. New patch uses arguments expected by tests.

Comment on attachment 8491774 [details] [diff] [review] Patch 1 (v3) - Make bluetooth webapi marionette tests wait for transaction success when changing settings Review of attachment 8491774 [details] [diff] [review]: ----------------------------------------------------------------- f+. I'd like to ask Eric to review this patch since I'm not a peer reviewer. Thanks. ::: dom/bluetooth/tests/marionette/head.js @@ +494,5 @@ > + deferred.resolve(); > + }; > + lock.onsettingstransactionfailure = function (aEvent) { > + ok(false, "setSettings(" + JSON.stringify(aSettings) + ")"); > + deferred.reject(); Should we throw an error at here like you did at wifi/test/marionette/head.js [1] ? [1] http://dxr.mozilla.org/mozilla-central/source/dom/wifi/test/marionette/head.js#595

Comment on attachment 8491774 [details] [diff] [review] Patch 1 (v3) - Make bluetooth webapi marionette tests wait for transaction success when changing settings Review of attachment 8491774 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

(In reply to Jamin Liu [:jaliu] from comment #10) > Comment on attachment 8491774 [details] [diff] [review] > Patch 1 (v3) - Make bluetooth webapi marionette tests wait for transaction > success when changing settings > > Review of attachment 8491774 [details] [diff] [review]: > ----------------------------------------------------------------- > > f+. > I'd like to ask Eric to review this patch since I'm not a peer reviewer. > Thanks. > > ::: dom/bluetooth/tests/marionette/head.js > @@ +494,5 @@ > > + deferred.resolve(); > > + }; > > + lock.onsettingstransactionfailure = function (aEvent) { > > + ok(false, "setSettings(" + JSON.stringify(aSettings) + ")"); > > + deferred.reject(); > > Should we throw an error at here like you did at > wifi/test/marionette/head.js [1] ? > [1] > http://dxr.mozilla.org/mozilla-central/source/dom/wifi/test/marionette/head. > js#595 I was just following the method used in each particular test (wifi had a throw, bluetooth didn't), since head.js differs between all of the different webapi tests. We should really make a shared library for this stuff, but that's a task for another bug.

Mnw try for b2g-i https://tbpl.mozilla.org/?tree=Try&rev=f76b56f7ba55 Mnw try for m-a https://tbpl.mozilla.org/?tree=Try&rev=585495b28a85

m-c: https://hg.mozilla.org/integration/b2g-inbound/rev/1d4c174496a4 https://hg.mozilla.org/integration/b2g-inbound/rev/88fe3f04f8f7 m-a: https://hg.mozilla.org/releases/mozilla-aurora/rev/fdeade9b70af https://hg.mozilla.org/releases/mozilla-aurora/rev/432389a51daa

https://hg.mozilla.org/mozilla-central/rev/1d4c174496a4 https://hg.mozilla.org/mozilla-central/rev/88fe3f04f8f7