service workers: Simplified tests with async/await - Part 1 #13103
Conversation
wanghongjuan
commented
Sep 20, 2018
wanghongjuan
commented
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in [testharness.js] Honor promises from cleanup fns #8748.
- The usage of unregister() method in some tests is not rigorous. As [spec](https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister):
No Taskcluster jobs started for this pull requestThe `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request. |
wpt-pr-bot
requested review from
asutherland,
mfalken,
mkruisselbrink and
wanderview
|
It's a common issue regarding to the usage of t. add_cleanup() and unregister(), which will produce a big PR that is not easy to review, so I intent to split this task into a piece of PRs. If this one is OK for you reviewers, I will continue the rest work. |
|
The failing Travis build in this PR was due to #13112, which has now been resolved. To recover, rebase the PR on master and push. (Simply restarting the Travis jobs may also work, not tested.) |
wanghongjuan
force-pushed
the
fix-cleanup-unregister-1
branch
from
8112f5a to
43fabbf
Compare
No Taskcluster jobs started for this pull requestThe `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request. |
|
@foolip Thanks for your reminder, this PR was rebased and passed on Travis. |
wanghongjuan
force-pushed
the
fix-cleanup-unregister-1
branch
from
43fabbf to
489bcb1
Compare
No Taskcluster jobs started for this pull requestThe `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request. |
wanghongjuan
force-pushed
the
fix-cleanup-unregister-1
branch
from
489bcb1 to
545cd4f
Compare
No Taskcluster jobs started for this pull requestThe `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request. |
|
@jugglinmike It seems that we are doing some same things for the method |
|
@wanghongjuan, see #13280 for why you're getting these comments about taskcluster. I've sent you an invite to the org which should fix this, please visit https://github.com/orgs/web-platform-tests if you can't find the invite in your email. |
|
@foolip Thanks for your reminder, I just find the invite and join the org. |
|
@mattto could you please take a look this pr? |
There was a problem hiding this comment.
Thanks, I like this change as long as it doesn't conflict with @jugglinmike's in-flight PRs.
| await wait_for_state(t, worker, 'activated'); | ||
| const frame = await with_iframe(SCOPE); | ||
|
|
||
| add_completion_callback(async() => { |
There was a problem hiding this comment.
add_cleanup() seems better (same comment throughout)
| if (registration) | ||
| await registration.unregister(); | ||
| if (frame) | ||
| frame.remove(); |
There was a problem hiding this comment.
It's preferable to remove the frame before unregistering, since unregister doesn't actually remove the registration until the frames are closed. (same comment throughout)
There was a problem hiding this comment.
Updated the code in latest commit.
| await wait_for_state(t, worker, 'activated'); | ||
| let data = await post_and_wait_for_reply(worker, 'getState'); | ||
| let states = await Promise.all([data, | ||
| registration.navigationPreload.getState()]); |
There was a problem hiding this comment.
I'd prefer registration to line up with data rather than the opening brace.
It's personal preference, but I tend to format like the Google style guide:
"Multiline array initializers and object initializers are indented 2 spaces, with the braces on their own line, just like blocks."
So this becomes:
let states = await Promise.all([
data,
registration.navigationPreload.getState()
]);
You might also just run all JavaScript through prettier.io and use that.
There was a problem hiding this comment.
Thanks for your recommendation, I have updated all of code in latest commit, PTAL.
| data = await post_and_wait_for_reply(worker, 'setHeaderValue'); | ||
| states = await Promise.all([ | ||
| post_and_wait_for_reply(worker, 'getState'), | ||
| registration.navigationPreload.getState()]); |
There was a problem hiding this comment.
This formatting is inconsistent with 121-123. To save time, I'd just run this through prettier.io.
| }); | ||
| if (result) | ||
| result.remove(); | ||
| }) |
| .concat(with_iframe(scope))); | ||
| const result = results[expected_urls.length]; | ||
|
|
||
| add_completion_callback(async() => { |
There was a problem hiding this comment.
Replaced with add_cleanup.
Thanks for watching out for that, @mattto! I can never resist an opportunity to answer a question with a shell script. The invocation $ compare-modified.sh 13042 13164 13165 13103shows 2 common files between the pull requests, but neither are touched by this |
wanghongjuan
force-pushed
the
fix-cleanup-unregister-1
branch
from
545cd4f to
98b186e
Compare
|
@mattto PTAL |
wanghongjuan
force-pushed
the
fix-cleanup-unregister-1
branch
from
98b186e to
e245813
Compare
- Fix a couple of tests with function t.add_cleanup() as it supports promise, see detail in web-platform-tests#8748. - The usage of unregister() method in some tests is not rigorous. As [spec]\(https://w3c.github.io/ServiceWorker/#dom-serviceworkerregistration-unregister): >> [NewObject] Promise<boolean> unregister(); unregister() method must run these steps: >> 1. Let promise be a promise. ... >> 4. Return promise.
