Upgrade from Task.jsm and Promise.jsm to async/await in Sync tests

RESOLVED FIXED in Firefox 53

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

Inspired by bug 1313956, it seems that changing the sync test suite to use async functions might get us some experience and should be largely immune from unexpected bad things happening for our users. Driven by a little procrastination, I did this and also removed all uses of Promise.jsm (using PromiseUtils.defer() for the few places where the change didn't seem worthwhile) and also started using other promise-based helpers to make the tests easier to read.
Comment on attachment 8809270 [details]
Bug 1316500 - remove use of tasks and promise.jsm promises from Sync tests.

https://reviewboard.mozilla.org/r/91862/#review91978

I've just asked in #fx-team, and we're not supposed to land code (even test code) using it for another cycle due to concerns that async/await may be turned off for web compat reasons (... that I don't fully understand, but apparently it's a request from the spidermonkey team?).

Other than that it looks fine, I didn't look *that* closely, but it seems mechanical and as long as the tests pass I'm content with landing this once we get the go-ahead on using async/await in firefox.
Attachment #8809270 - Flags: review?(tchiovoloni)
Flags: needinfo?(markh)
Comment on attachment 8809270 [details]
Bug 1316500 - remove use of tasks and promise.jsm promises from Sync tests.

I just asked in #jsapi and doing test only changes for 53 is fine and probably helpful. Even production code in 53 is probably OK with some caveats (there's still a tiny risk of backout, there are some questions about performance, but even then it's almost certainly not worse than Task.jsm) - but for now, let's get these tests in.
Flags: needinfo?(markh)
Attachment #8809270 - Flags: review?(tchiovoloni)
Comment on attachment 8809270 [details]
Bug 1316500 - remove use of tasks and promise.jsm promises from Sync tests.

https://reviewboard.mozilla.org/r/91862/#review93130

In that case, ship it!
Attachment #8809270 - Flags: review?(tchiovoloni) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/ca02f30bc3a0
remove use of tasks and promise.jsm promises from Sync tests. r=tcsc
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6239169c1eff
Backed out changeset ca02f30bc3a0 for eslint failures
If we add a services/sync/.eslintrc.js file (like http://searchfox.org/mozilla-central/source/.eslintrc.js), and set the `"parserOptions": { "ecmaVersion": 8 }` option, I think that'll squelch the failures.
Depends on: 1318493
Priority: -- → P1
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/843b7449f803
remove use of tasks and promise.jsm promises from Sync tests. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/843b7449f803
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.