1069115 - Some marionette webapi tests don't wait for settings transactions properly

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Make bluetooth webapi marionette tests wait for transaction success when changing settings

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

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 ( v2) - Make bluetooth webapi marionette tests wait for transaction success when changing settings

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

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Make mobileconnection webapi marionette tests wait for transaction success when changing setting

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

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Generalizing title since I found more instances of this.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

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 5

[Eric Chou [:ericchou] [:echou]]

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 6

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

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

Comment 7

[Gregor Wagner [:gwagner]]

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

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Make bluetooth webapi marionette tests wait for transaction success when changing settings

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

Comment 10

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

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 11

[Eric Chou [:ericchou] [:echou]]

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!

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 15

[Ed Morley [:emorley]]

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