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.

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

Merged
merged 9 commits into from
Aug 2, 2017
7 changes: 7 additions & 0 deletions html/dom/elements-metadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ var metadataElements = {
defaultVal: "",
invalidVal: ""
},
scope: "string",
updateViaCache: {
type: "enum",
keywords: ["imports", "all", "none"],
defaultVal: "imports",
invalidVal: "imports"
},
media: "string",
nonce: "string",
integrity: "string",
Expand Down
230 changes: 230 additions & 0 deletions service-workers/service-worker/registration-updateviacache.https.html
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';
Copy link
Member

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?


async function cleanup() {
const reg = await navigator.serviceWorker.getRegistration(scope);
if (!reg) return;
if (reg.scope == new URL(scope, location).href) {
return reg.unregister()
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Member

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')

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) {
Copy link
Member

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|.

Copy link
Contributor Author

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

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);
Copy link
Member

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?

}
}

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
Copy link
Member

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." ?

Copy link
Contributor Author

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.

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);
Copy link
Member

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?

}
}

const registrationMethods = [
[registerViaApi, 'via-api'],
[registerViaLinkElement, 'via-link-element'],
[registerViaLinkHeader, 'via-link-header']
];

// Testing creating registrations
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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'
Copy link
Member

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_not_equals(values.importTime, newValues.importTime, "Imported script should have updated");
}
}

await cleanup();
}, testName);
}
}

// Testing changing registration
Copy link
Member

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.

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.
Copy link
Member

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"

// The default value should behave as 'imports'.
if ((updateViaCache1 || 'imports') == (updateViaCache2 || 'imports')) {
Copy link
Contributor

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.

Copy link
Contributor Author

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(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') {
Copy link
Contributor

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.

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') {
Copy link
Contributor

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_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'
Copy link
Member

Choose a reason for hiding this comment

The 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
28 changes: 28 additions & 0 deletions service-workers/service-worker/resources/update-max-aged-worker.py
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