-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
Service worker: Tests for updateViaCache (previously useCache) #5515
Service worker: Tests for updateViaCache (previously useCache) #5515
Conversation
Notifying @ehsan, @mattto, and @mkruisselbrink. (Learn how reviewing works.) |
Generating the tests in a loop seemed like the simplest way forward here. Meant I could write the tests fairly linearly without abstracting a lot. But I can look at changing it if it's too confusing. Also, yay async functions! |
Firefox (nightly channel)Testing web-platform-tests at revision d5a1e32 All results1 test ran/service-workers/service-worker/registration-updateviacache.https.html
|
Chrome (unstable channel)Testing web-platform-tests at revision d5a1e32 All results1 test ran/service-workers/service-worker/registration-updateviacache.https.html
|
These tests are now available on w3c-test.org |
The failing tests (in Chrome & Firefox) are as-expected. |
We actually have a wpt for the cache changes written, but not uplifted yet. Sorry if there was duplicate work making this test. We can either keep both or just this one. I'll talk to Ho-Pang about it Monday. |
Ah, sorry! Anyway, I'll let you pick whichever & I promise not to be offended. 😆 |
No problem! The test here has the benefit of being run against chrome to check for flakiness. Our wpt uplift process is lacking that step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jake,
The code is arranged elegantly! I left some questions in the comments. In addition to those, I think it might be a good idea to check whether updatefound
is dispatched when there should be a new service worker, but I have no strong preference on this.
await reg.update(); | ||
|
||
if (updateViaCache == 'all') { | ||
assert_equals(sw.installing, undefined, "No new service worker"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By sw.installing
, are we referring to reg.installing
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! Good catch.
@hopanghsu what's the benefit of checking |
Just because the spec requires firing one? Maybe that's covered by a different test. |
@wanderview yeah, my feeling is it's unrelated to this test. |
eb3f302
to
cab9d6c
Compare
I've resolved the merge conflicts. Are we good to merge this? |
*This report has been truncated because the total content is 1004931 characters in length, which is in excess of github.com's limit for comments (65536 characters). Firefox (nightly)Testing web-platform-tests at revision d58ea82 All results2 tests ran/html/dom/reflection-metadata.html
|
*This report has been truncated because the total content is 1004862 characters in length, which is in excess of github.com's limit for comments (65536 characters). Chrome (unstable)Testing web-platform-tests at revision d58ea82 All results2 tests ran/html/dom/reflection-metadata.html
|
cab9d6c
to
ad8943a
Compare
*This report has been truncated because the total content is 999898 characters in length, which is in excess of github.com's limit for comments (65536 characters). Sauce (safari)Testing web-platform-tests at revision d58ea82 All results2 tests ran/html/dom/reflection-metadata.html
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather an implementer (like @mkruisselbrink?) review the actual processing model parts of the tests, but only one minor comment on the reflection part.
html/dom/elements-metadata.js
Outdated
scope: "string", | ||
updateViaCache: { | ||
type: "enum", | ||
keywords: ["imports", "all", "none"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll want to add defaultVal
here too.
This is used by http://w3c-test.org/html/dom/reflection-metadata.html
Changing useCache boolean to updateViaCache enum Issue: #1104. Tests: web-platform-tests/wpt#5515. HTML change: whatwg/html@b5fcec0.
Reflection tests LGTM but would like LGTM from someone else on the actual semantics tests. |
Thanks for the tests! The discussion here seems relevant: https://groups.google.com/a/chromium.org/d/msg/net-dev/wbhbSn9ICfc/XDmCkpboAAAJ The conclusion there was that browser caching behaviors cannot be tested in WPTs, because caching is a SHOULD. |
Is there an actual compat issue that we expect to prevent these tests passing in currently implementing browsers? I would prefer to keep the tests and if a browser wants to deviate from the SHOULD language they can internally mark the test as disabled/expected-fail/etc. |
OK. It makes sense to me that WPT tests can test SHOULDs. Sounds good. |
return new Promise(r => setTimeout(r, ms)); | ||
} | ||
|
||
function waitForActivated(sw) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use wait_for_state(t, sw, 'activated')
<script> | ||
const updateViaCacheValues = [undefined, 'imports', 'all', 'none']; | ||
const scriptUrl = 'resources/update-max-aged-worker.py'; | ||
const scope = 'resources/blank.html'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make clear they are global constants, could we name these globals like kUpdateViaCacheValues, kScope or else UPDATE_VIA_CACHE_VALUES, SCOPE, etc?
}); | ||
} | ||
|
||
function registerViaApi(scriptUrl, opts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little confusing that this param is the same name as the global |scriptUrl|.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixed by renaming the global
); | ||
|
||
const sw = reg.installing || reg.waiting || reg.active; | ||
if (!sw) console.log(reg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this console.log()? It tends to pollute test results. Should it just be an assert() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops!
assert_not_equals(values.mainTime, newValues.mainTime, "Main script should have updated"); | ||
assert_equals(values.importTime, newValues.importTime, "Imported script should be the same"); | ||
} | ||
else { // 'none' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more clear: // updateViaCache == 'none'
assert_not_equals(values.mainTime, newValues.mainTime, "Main script should have updated"); | ||
assert_equals(values.importTime, newValues.importTime, "Imported script should be the same"); | ||
} | ||
else { // 'none' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as line 167
return reg; | ||
} | ||
|
||
await wait(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polling with timeout is a bit unfortunate, it puts a bound on how fast the test can run (100ms per test adds up for hundreds of tests). A similar test, register-link-element.https.html, just waits for link.onload. Could we just do that here?
return reg; | ||
} | ||
|
||
await wait(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
register-link-header.https.html uses an iframe and navigator.serviceWorker.ready. Could we do that here instead?
|
||
await navigator.serviceWorker.register(fullScriptUrl, opts); | ||
|
||
// If there's no change, register should be a no-op. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bit subtle for me. Recommend saying "If there's no change to the updateViaCache value"
} | ||
} | ||
|
||
// Testing changing registration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Test changing the updateViaCache value of an existing registration.
[registerViaLinkHeader, 'via-link-header'] | ||
]; | ||
|
||
// Testing creating registrations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service worker WPT tests tend to follow Chromium Style Guide for comments, so this would take the form:
// Test creating registrations.
(imperative form + full stop)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, thanks!
Jake do you want to continue with this PR? Is it ok if we take what's here now and work on it in a Chromium codereview as we're trying to implement? |
@mattto I'll address your comments now. Apologies for the (usual) slowness, was out conferencing last week. |
@mattto Done! Thanks for the feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test! lgtm, thanks!
const fullScope = new URL(opts.scope, window.location).href; | ||
scriptUrl = new URL(scriptUrl, location).href; | ||
|
||
// Assuming that if this isn't supported, the header isn't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Sort of looks like the sentence got cut-off mid-sentence. "Assume that if the link element doesn't support serviceworker, the header doesn't either." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, now you point it out, that comment was really badly written. Fixed with your suggestion.
Thanks! Can we get this reviewed and committed so our implementation can use it (@mkruisselbrink)? There is a linter error above: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi all, and sorry for jumping in here. I am currently working on the related implementation, and I found something we might want to change.
|
||
// If there's no change to the updateViaCache value, register should be a no-op. | ||
// The default value should behave as 'imports'. | ||
if ((updateViaCache1 || 'imports') == (updateViaCache2 || 'imports')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to do this when (updateViaCache2 == 'all'), since it cannot pass the byte-check and thus there won't be a new service worker in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, good catch. Because we ignore the byte-check when the script URL changes, I thought we'd do the same here. But it doesn't make sense to, and we don't 😄
assert_equals(values.mainTime, newValues.mainTime, "Main script should be the same"); | ||
assert_equals(values.importTime, newValues.importTime, "Imported script should be the same"); | ||
} | ||
else if (updateViaCache2 == 'imports') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should explicitly handle the default case, updateViaCache2 == undefined
. Otherwise, it goes to the else
branch.
assert_true(!!newWorker, "New worker installing"); | ||
const newValues = await getScriptTimes(newWorker, testName); | ||
|
||
if (updateViaCache2 == 'all') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there shouldn't be a newly created service worker, we don't have to check anything here.
@hopanghsu Don't apologise, thanks for the great feedback! Should be addressed now. @mattto I've addressed that linting issue (the I've also added tests for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rs+ based on previous review comments being addressed.
No description provided.