-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Changes from 5 commits
fd2bd88
512ae2a
60db228
ad8943a
572762c
11d016c
52c3bd9
9786ea9
3c2289d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,230 @@ | ||
<!DOCTYPE html> | ||
<title>Service Worker: Registration-updateViaCache</title> | ||
<script src="/resources/testharness.js"></script> | ||
<script src="resources/testharness-helpers.js"></script> | ||
<script src="/resources/testharnessreport.js"></script> | ||
<script src="resources/test-helpers.sub.js"></script> | ||
<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 commentThe 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? |
||
|
||
async function cleanup() { | ||
const reg = await navigator.serviceWorker.getRegistration(scope); | ||
if (!reg) return; | ||
if (reg.scope == new URL(scope, location).href) { | ||
return reg.unregister() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add a semi-colon |
||
}; | ||
} | ||
|
||
function wait(ms) { | ||
return new Promise(r => setTimeout(r, ms)); | ||
} | ||
|
||
function waitForActivated(sw) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could use wait_for_state(t, sw, 'activated') |
||
return new Promise((resolve, reject) => { | ||
function check() { | ||
if (sw.state == 'activated') { | ||
sw.removeEventListener('statechange', check); | ||
resolve(); | ||
return; | ||
} | ||
if (sw.state == 'redundant') { | ||
sw.removeEventListener('statechange', check); | ||
reject(Error('Service worker rejected')); | ||
return; | ||
} | ||
} | ||
|
||
sw.addEventListener('statechange', check); | ||
check(); | ||
}); | ||
} | ||
|
||
function getScriptTimes(sw, testName) { | ||
return new Promise(resolve => { | ||
navigator.serviceWorker.addEventListener('message', function listener(event) { | ||
if (event.data.test !== testName) return; | ||
navigator.serviceWorker.removeEventListener('message', listener); | ||
resolve({ | ||
mainTime: event.data.mainTime, | ||
importTime: event.data.importTime | ||
}); | ||
}); | ||
|
||
sw.postMessage(''); | ||
}); | ||
} | ||
|
||
function registerViaApi(scriptUrl, opts) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. This was fixed by renaming the global |
||
return navigator.serviceWorker.register(scriptUrl, opts); | ||
} | ||
|
||
async function registerViaLinkElement(scriptUrl, opts) { | ||
const link = document.createElement('link'); | ||
|
||
if (link.relList.supports('serviceworker') == false) throw Error("link rel=serviceworker not supported"); | ||
|
||
link.rel = 'serviceworker'; | ||
link.href = scriptUrl; | ||
link.scope = opts.scope; | ||
|
||
if (opts.updateViaCache) { | ||
link.updateViaCache = opts.updateViaCache; | ||
} | ||
|
||
document.head.appendChild(link); | ||
|
||
const fullScope = new URL(opts.scope, window.location).href; | ||
|
||
while (true) { | ||
const regs = await navigator.serviceWorker.getRegistrations(); | ||
const reg = regs.find(r => r.scope == fullScope && (r.installing || r.waiting || r.active)); | ||
|
||
if (reg) { | ||
document.head.removeChild(link); | ||
return reg; | ||
} | ||
|
||
await wait(100); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
} | ||
} | ||
|
||
async function registerViaLinkHeader(scriptUrl, opts) { | ||
const link = document.createElement('link'); | ||
|
||
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 commentThe 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 commentThe 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. |
||
if (link.relList.supports('serviceworker') == false) throw Error("link rel=serviceworker not supported"); | ||
|
||
let linkHeader = `<${scriptUrl}>; rel=serviceworker; scope="${fullScope}"`; | ||
|
||
if (opts.updateViaCache) { | ||
linkHeader += `; updateviacache="${opts.updateViaCache}"`; | ||
} | ||
|
||
const url = new URL('resources/link-header.py', location); | ||
url.searchParams.set('Link', linkHeader); | ||
|
||
const frame = await with_iframe(url); | ||
|
||
while (true) { | ||
const regs = await navigator.serviceWorker.getRegistrations(); | ||
const reg = regs.find(r => r.scope == fullScope && (r.installing || r.waiting || r.active)); | ||
|
||
if (reg) { | ||
frame.parentNode.removeChild(frame); | ||
return reg; | ||
} | ||
|
||
await wait(100); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
} | ||
} | ||
|
||
const registrationMethods = [ | ||
[registerViaApi, 'via-api'], | ||
[registerViaLinkElement, 'via-link-element'], | ||
[registerViaLinkHeader, 'via-link-header'] | ||
]; | ||
|
||
// Testing creating registrations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL, thanks! |
||
for (const [registrationMethod, registrationMethodName] of registrationMethods) { | ||
for (const updateViaCache of updateViaCacheValues) { | ||
const testName = `register-${registrationMethodName}-updateViaCache-${updateViaCache}`; | ||
|
||
promise_test(async () => { | ||
await cleanup(); | ||
|
||
const opts = {scope}; | ||
|
||
if (updateViaCache) opts.updateViaCache = updateViaCache; | ||
|
||
const reg = await registrationMethod( | ||
`${scriptUrl}?test=${testName}`, | ||
opts | ||
); | ||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Whoops! |
||
await waitForActivated(sw); | ||
const values = await getScriptTimes(sw, testName); | ||
await reg.update(); | ||
|
||
if (updateViaCache == 'all') { | ||
assert_equals(reg.installing, null, "No new service worker"); | ||
} | ||
else { | ||
const newWorker = reg.installing; | ||
assert_true(!!newWorker, "New worker installing"); | ||
const newValues = await getScriptTimes(newWorker, testName); | ||
|
||
if (!updateViaCache || updateViaCache == 'imports') { | ||
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 commentThe 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_not_equals(values.importTime, newValues.importTime, "Imported script should have updated"); | ||
} | ||
} | ||
|
||
await cleanup(); | ||
}, testName); | ||
} | ||
} | ||
|
||
// Testing changing registration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. // Test changing the updateViaCache value of an existing registration. |
||
for (const updateViaCache1 of updateViaCacheValues) { | ||
for (const updateViaCache2 of updateViaCacheValues) { | ||
const testName = `register-with-updateViaCache-${updateViaCache1}-then-${updateViaCache2}`; | ||
|
||
promise_test(async () => { | ||
await cleanup(); | ||
|
||
const fullScriptUrl = `${scriptUrl}?test=${testName}`; | ||
let opts = {scope}; | ||
if (updateViaCache1) opts.updateViaCache = updateViaCache1; | ||
|
||
const reg = await navigator.serviceWorker.register(fullScriptUrl, opts); | ||
|
||
const sw = reg.installing; | ||
await waitForActivated(sw); | ||
const values = await getScriptTimes(sw, testName); | ||
|
||
opts = {scope}; | ||
if (updateViaCache2) opts.updateViaCache = updateViaCache2; | ||
|
||
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 commentThe 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" |
||
// 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 commentThe 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 commentThe 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(reg.installing, null, "No new service worker"); | ||
} | ||
else { | ||
const newWorker = reg.installing; | ||
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 commentThe 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. |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I think we should explicitly handle the default case, |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. same as line 167 |
||
assert_not_equals(values.mainTime, newValues.mainTime, "Main script should have updated"); | ||
assert_not_equals(values.importTime, newValues.importTime, "Imported script should have updated"); | ||
} | ||
} | ||
|
||
await cleanup(); | ||
}, testName); | ||
} | ||
} | ||
|
||
</script> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import time | ||
|
||
def main(request, response): | ||
headers = [('Content-Type', 'application/javascript'), | ||
('Cache-Control', 'max-age=86400'), | ||
('Last-Modified', time.strftime("%a, %d %b %Y %H:%M:%S GMT", time.gmtime()))] | ||
|
||
body = ''' | ||
const importTime = {time}; | ||
'''.format(time=time.time()) | ||
|
||
return headers, body |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import time | ||
import json | ||
|
||
def main(request, response): | ||
headers = [('Content-Type', 'application/javascript'), | ||
('Cache-Control', 'max-age=86400'), | ||
('Last-Modified', time.strftime("%a, %d %b %Y %H:%M:%S GMT", time.gmtime()))] | ||
|
||
test = request.GET['test']; | ||
|
||
body = ''' | ||
const mainTime = {time}; | ||
const testName = {test}; | ||
importScripts('update-max-aged-worker-imported-script.py'); | ||
|
||
addEventListener('message', event => {{ | ||
event.source.postMessage({{ | ||
mainTime, | ||
importTime, | ||
test: {test} | ||
}}); | ||
}}); | ||
'''.format( | ||
time=time.time(), | ||
test=json.dumps(test) | ||
) | ||
|
||
return headers, body |
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