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

RESOLVED FIXED in 2.1 S5 (26sep)

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

25 Branch
2.1 S5 (26sep)
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 affected, b2g-v2.2 affected)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 2 obsolete attachments)

[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.
Attachment #8491259 - Flags: review?(echou)
Ok, missed this in bluetooth2 also. Not that it runs yet, but I'm trying to cover all the bases here. :)
Attachment #8491259 - Attachment is obsolete: true
Attachment #8491259 - Flags: review?(echou)
Attachment #8491260 - Flags: review?(echou)
And it looks like I forgot to do this in mobileconnection too. Who knows how many intermittents this is going to fix. Wheee.
Attachment #8491262 - Flags: review?(vyang)
Generalizing title since I found more instances of this.
Summary: Bluetooth marionette webapi tests don't wait for settings transactions properly → Some marionette webapi tests don't wait for settings transactions properly
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.
Attachment #8491262 - Flags: review?(vyang) → review+
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?
}
Attachment #8491260 - Flags: review?(echou) → feedback?(jaliu)
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() ?
Attachment #8491260 - Flags: feedback?(jaliu) → feedback+
Not blocking on test only changes. Ask for approval and land.
blocking-b2g: 2.1? → ---
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.
Attachment #8491260 - Attachment is obsolete: true
Attachment #8491774 - Flags: review?(jaliu)
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
Attachment #8491774 - Flags: review?(jaliu)
Attachment #8491774 - Flags: review?(echou)
Attachment #8491774 - Flags: feedback+
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!
Attachment #8491774 - Flags: review?(echou) → review+
(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.
You need to log in before you can comment on or make changes to this bug.