Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Service worker: self.serviceWorker #17285

Merged
merged 5 commits into from
Jun 21, 2019
Merged

Service worker: self.serviceWorker #17285

merged 5 commits into from
Jun 21, 2019

Conversation

jakearchibald
Copy link
Contributor

@jakearchibald
Copy link
Contributor Author

@mattto this test feels small, but I can't think of anything I'm missing

This was referenced Jun 12, 2019
Merged
Closed
@gsnedders
Copy link
Member

(Needs a rebase to pass CI, due to #17286. Sorry!)

wanderview
wanderview approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

It took me a bit to wrap my head around how the tests are setup at the 'parsed' eval state and then the later async tests wait for the service worker installation process to progress. Maybe a comment would be worth it?

@jakearchibald
Copy link
Contributor Author

@wanderview yeah, setting the tests up early means it doesn't pass just because some of the tests weren't defined in time. I'll add some comments.

@jakearchibald
Copy link
Contributor Author

@gsnedders thanks for letting me know. Done!

mfalken
mfalken approved these changes Jun 13, 2019
View reviewed changes
Copy link
Member

@mfalken mfalken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I guess it's ok to assume the SW stays alive until activate is fired... if it causes problems someday we can split the test up.

service-workers/service-worker/global-serviceworker.https.any.js
@@ -0,0 +1,53 @@
// META: title=serviceWorker on service worker global
// META: global=!default,serviceworker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there are some tests today with names like "can-make-payment-event-constructor.https.serviceworker.js". Does it work to make this a "serviceworker.js" test, then you don't need this META?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this would be better, but it doesn't seem to work.

https://web-platform-tests.org/writing-tests/testharness.html#auto-generated-test-boilerplate suggests only window and worker work in the URL.

The global=!default,serviceworker line is also directly from their documentation.

Maybe I'm holding it wrong though. I agree it's a little weird having an 'any' test that's limited to just service workers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok.

@jakearchibald jakearchibald merged commit 3725067 into web-platform-tests:master Jun 21, 2019
@jakearchibald jakearchibald deleted the sw-global-serviceworker branch June 21, 2019 07:42
marcoscaceres pushed a commit that referenced this pull request Jul 23, 2019
@jakearchibald
Service worker: self.serviceWorker (#17285)
ee21871 
* Tests for self.serviceWorker

* Don't need `self`

* Add read only test

* Adding comments & test the object doesn't change

* Swap order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfalken mfalken mfalken approved these changes

@wanderview wanderview wanderview approved these changes

@asutherland asutherland Awaiting requested review from asutherland

@mkruisselbrink mkruisselbrink Awaiting requested review from mkruisselbrink

Assignees

@wanderview wanderview

Labels
service-workers wg-sw
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jakearchibald @gsnedders @wanderview @mfalken @wpt-pr-bot