-
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
Tests for https://github.com/whatwg/fetch/pull/435 #4518
Tests for https://github.com/whatwg/fetch/pull/435 #4518
Conversation
Notifying @ehsan and @mkruisselbrink. (Learn how reviewing works.) |
@wanderview Any chance you could review this please? |
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.
Overall looks reasonable. It would be really nice if we could avoid the wait(1000)
calls, though.
}); | ||
notification.close(); | ||
}), | ||
wait(1000).then(() => { throw Error(`Did not capture icon request for notification created from page`); }) |
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.
Any way we can avoid these arbitrary delays? They tend to play havoc with automated tests on slow virtualized machines.
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.
Could you set a cookie from the a server .py indicating the icon was loaded? Then the window could then poll for the completion cookie.
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 broadcast channel does the same thing the cookie would. The problem is, how do you detect the icon wasn't loaded. That's what the timeout is currently doing.
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 wait, the cookie would be detecting no-fetch-event. Sorry, I get it now.
addEventListener('fetch', event => { | ||
const url = new URL(event.request.url); | ||
|
||
if (url.origin == location.origin) { |
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: Can you use short-circuit style logic? Like:
if (url.origin !== location.origin) {
return;
}
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.
Doh! Of course. Schoolboy error.
@@ -52,6 +52,7 @@ function unreached_rejection(test, prefix) { | |||
function with_iframe(url) { | |||
return new Promise(function(resolve) { | |||
var frame = document.createElement('iframe'); | |||
frame.className = 'test-iframe'; |
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.
What does this do?
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.
See reset()
- it removes all elements with that class name.
|
||
iframe.contentWindow.fetch('show-notification'); | ||
}), | ||
wait(1000).then(() => { throw Error(`Did not capture icon request for notification created within 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.
For this wait(1000)
you could wait for the Done
text in the Response
no? It seems all the async delays should be handled by then.
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.
Interesting. Once the notification appears in the sequence returned by registration.getNotifications()
, it must have made the fetch for the icon (if the browser is following the spec). But that means there's a race between calling showNotification
and getNotification
, since showNotification
resolves before icon fetches.
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.
Filed whatwg/notifications#90
@wanderview great feedback - I've replaced the timeouts with cookies & polling. |
w3c-test:mirror |
@wanderview I don't suppose you could give this a second review? |
@mkruisselbrink could you give this a look-through? |
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 wonder if it might be a good idea to split out the manual test from the automated tests. Having them all in the same file kind of makes sense, but it would be unfortunate if that means that tests that could be run automated end up not being run as much because of the one manual test...
notification.close(); | ||
|
||
return race.then(winner => { | ||
assert_true(winner == 'broadcast', 'The service worker intercepted the from-window notification icon request'); |
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: why not assert_equals?
iframe.contentWindow.fetch(`show-notification?set-cookie-notification=${Math.random()}`); | ||
|
||
return race.then(winner => { | ||
assert_true(winner == 'broadcast', 'The service worker intercepted the from-service-worker notification icon request'); |
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: why not assert_equals?
return new Promise(r => setTimeout(r, ms)); | ||
} | ||
|
||
function reset() { |
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 it would be a good idea to not only call reset() at the beginning of your tests, but also have add_completion_callback(reset) somewhere to make sure things get cleaned up after running the tests
49d1eec
to
a6a7620
Compare
Thanks for the feedback @mkruisselbrink! I think I've addressed the issues - this should be good to merge. |
Chrome (unstable channel)Testing web-platform-tests at revision 885f165 Unstable results
All results/service-workers/service-worker/claim-using-registration.https.html
/service-workers/service-worker/claim-not-using-registration.https.html
/streams/readable-byte-streams/general.serviceworker.https.html
/streams/piping/multiple-propagation.serviceworker.https.html
/service-workers/service-worker/registration-service-worker-attributes.https.html
/service-workers/service-worker/register-closed-window.https.html
/streams/writable-streams/aborting.serviceworker.https.html
/streams/piping/flow-control.serviceworker.https.html
/service-workers/service-worker/register-default-scope.https.html
/service-workers/service-worker/service-worker-csp-default.https.html
/service-workers/cache-storage/serviceworker/cache-put.https.html
/service-workers/service-worker/skip-waiting-without-client.https.html
/service-workers/service-worker/xhr.https.html
/service-workers/service-worker/fetch-event.https.html
/service-workers/service-worker/fetch-request-redirect.https.html
/service-workers/service-worker/register-wait-forever-in-install-worker.https.html
/service-workers/service-worker/invalid-blobtype.https.html
/service-workers/service-worker/referer.https.html
/service-workers/service-worker/skip-waiting-installed.https.html
/streams/readable-streams/cancel.serviceworker.https.html
/service-workers/service-worker/registration.https.html
/service-workers/service-worker/activation-after-registration.https.html
/streams/piping/error-propagation-backward.serviceworker.https.html
/service-workers/service-worker/register-link-header.https.html
/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/clients-matchall-client-types.https.html
/service-workers/service-worker/getregistrations.https.html
/service-workers/service-worker/controller-on-reload.https.html
/service-workers/cache-storage/serviceworker/cache-delete.https.html
/service-workers/service-worker/skip-waiting-using-registration.https.html
/service-workers/service-worker/interfaces.https.html
/service-workers/service-worker/state.https.html
/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html
/streams/piping/close-propagation-forward.serviceworker.https.html
/service-workers/service-worker/fetch-request-xhr.https.html
/html/webappapis/scripting/events/messageevent-constructor.https.html
/streams/writable-streams/bad-strategies.serviceworker.https.html
/service-workers/service-worker/controller-on-load.https.html
/service-workers/service-worker/request-end-to-end.https.html
/streams/piping/transform-streams.serviceworker.https.html
/streams/writable-streams/write.serviceworker.https.html
/streams/readable-streams/count-queuing-strategy-integration.serviceworker.https.html
/service-workers/cache-storage/serviceworker/cache-storage.https.html
/service-workers/service-worker/register-same-scope-different-script-url.https.html
/service-workers/service-worker/fetch-cors-xhr.https.html
/service-workers/service-worker/postmessage.https.html
/streams/readable-streams/bad-strategies.serviceworker.https.html
/service-workers/service-worker/fetch-event-network-error.https.html
/service-workers/service-worker/navigation-redirect.https.html
/service-workers/service-worker/activate-event-after-install-state-change.https.html
/service-workers/service-worker/synced-state.https.html
/service-workers/service-worker/ready.https.html
/service-workers/service-worker/install-event-type.https.html
/service-workers/service-worker/service-worker-csp-script.https.html
/service-workers/service-worker/fetch-request-resources.https.html
/service-workers/service-worker/client-navigate.https.html
/service-workers/service-worker/fetch-request-fallback.https.html
/service-workers/service-worker/unregister-then-register.https.html
/service-workers/service-worker/clients-get.https.html
/streams/count-queuing-strategy.serviceworker.https.html
/streams/piping/pipe-through.serviceworker.https.html
/streams/byte-length-queuing-strategy.serviceworker.https.html
/service-workers/service-worker/extendable-event-waituntil.https.html
/service-workers/service-worker/registration-end-to-end.https.html
/streams/writable-streams/close.serviceworker.https.html
/service-workers/service-worker/activation.https.html
/service-workers/cache-storage/serviceworker/cache-matchAll.https.html
/service-workers/service-worker/fetch-canvas-tainting-cache.https.html
/service-workers/service-worker/onactivate-script-error.https.html
/service-workers/service-worker/fetch-mixed-content-to-inscope.https.html
/service-workers/service-worker/update-after-navigation-fetch-event.https.html
/service-workers/service-worker/skip-waiting-without-using-registration.https.html
/streams/piping/general.serviceworker.https.html
/service-workers/service-worker/ServiceWorkerGlobalScope/registration-attribute.https.html
/service-workers/service-worker/resource-timing.https.html
/service-workers/service-worker/fetch-header-visibility.https.html
/streams/readable-streams/templated.serviceworker.https.html
/streams/readable-streams/brand-checks.serviceworker.https.html
/streams/readable-streams/tee.serviceworker.https.html
/service-workers/service-worker/invalid-header.https.html
/service-workers/service-worker/registration-iframe.https.html
/service-workers/service-worker/unregister-then-register-new-script.https.html
/service-workers/service-worker/multiple-update.https.html
/service-workers/service-worker/registration-events.https.html
/service-workers/service-worker/fetch-request-css-images.https.html
/service-workers/service-worker/fetch-request-no-freshness-headers.https.html
/streams/readable-streams/pipe-through.serviceworker.https.html
/service-workers/service-worker/installing.https.html
/service-workers/cache-storage/serviceworker/cache-storage-keys.https.html
/service-workers/service-worker/multiple-register.https.html
/service-workers/service-worker/indexeddb.https.html
/service-workers/service-worker/ServiceWorkerGlobalScope/unregister.https.html
/service-workers/service-worker/uncontrolled-page.https.html
/service-workers/service-worker/update.https.html
/streams/piping/error-propagation-forward.serviceworker.https.html
/service-workers/service-worker/websocket.https.html
/service-workers/service-worker/register-link-element.https.html
/streams/writable-streams/count-queuing-strategy.serviceworker.https.html
/service-workers/service-worker/postmessage-msgport-to-client.https.html
/service-workers/service-worker/fetch-mixed-content-to-outscope.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/getregistration.https.html
/service-workers/service-worker/waiting.https.html
/service-workers/service-worker/registration-useCache.https.html
/streams/writable-streams/byte-length-queuing-strategy.serviceworker.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/cache-storage/serviceworker/credentials.https.html
/service-workers/service-worker/oninstall-script-error.https.html
/service-workers/service-worker/serviceworkerobject-scripturl.https.html
/service-workers/cache-storage/serviceworker/cache-storage-match.https.html
/streams/readable-streams/general.serviceworker.https.html
/streams/readable-streams/readable-stream-reader.serviceworker.https.html
/service-workers/service-worker/unregister.https.html
/service-workers/service-worker/multi-globals/url-parsing.https.html
/service-workers/service-worker/fetch-event-within-sw.html
/service-workers/service-worker/fetch-canvas-tainting.https.html
/streams/writable-streams/brand-checks.serviceworker.https.html
/service-workers/service-worker/fetch-waits-for-activate.https.html
/streams/writable-streams/bad-underlying-sinks.serviceworker.https.html
/service-workers/service-worker/clients-matchall-include-uncontrolled.https.html
/service-workers/service-worker/postmessage-to-client.https.html
/service-workers/service-worker/skip-waiting.https.html
/service-workers/service-worker/unregister-controller.https.html
/streams/readable-streams/garbage-collection.serviceworker.https.html
/service-workers/service-worker/appcache-ordering-main.https.html
/service-workers/service-worker/fetch-frame-resource.https.html
/service-workers/service-worker/performance-timeline.https.html
/streams/writable-streams/start.serviceworker.https.html
/service-workers/service-worker/fetch-event-after-navigation-within-page.https.html
/service-workers/service-worker/controller-on-disconnect.https.html
/service-workers/service-worker/update-after-oneday.https.html
/service-workers/service-worker/worker-interception.https.html
/streams/piping/close-propagation-backward.serviceworker.https.html
/service-workers/service-worker/clients-matchall.https.html
/streams/writable-streams/constructor.serviceworker.https.html
/service-workers/service-worker/fetch-csp.https.html
/service-workers/cache-storage/serviceworker/cache-match.https.html
/streams/writable-streams/general.serviceworker.https.html
/service-workers/service-worker/fetch-event-redirect.https.html
/service-workers/service-worker/shared-worker-controlled.https.html
/service-workers/service-worker/active.https.html
/service-workers/service-worker/extendable-event-async-waituntil.https.html
/streams/readable-streams/bad-underlying-sources.serviceworker.https.html
/service-workers/service-worker/update-recovery.https.html
/service-workers/service-worker/fetch-response-xhr.https.html
/service-workers/cache-storage/serviceworker/cache-add.https.html
/service-workers/service-worker/navigate-window.https.html
/service-workers/service-worker/serviceworker-message-event-historical.https.html
/preload/fetch-destination.https.html
/service-workers/service-worker/clients-get-cross-origin.https.html
/service-workers/service-worker/service-worker-csp-connect.https.html
|
Firefox (nightly channel)Testing web-platform-tests at revision 885f165 All results/service-workers/service-worker/claim-using-registration.https.html
/service-workers/service-worker/claim-not-using-registration.https.html
/streams/readable-byte-streams/general.serviceworker.https.html
/streams/piping/multiple-propagation.serviceworker.https.html
/service-workers/service-worker/registration-service-worker-attributes.https.html
/service-workers/service-worker/register-closed-window.https.html
/streams/writable-streams/aborting.serviceworker.https.html
/streams/piping/flow-control.serviceworker.https.html
/service-workers/service-worker/register-default-scope.https.html
/service-workers/service-worker/service-worker-csp-default.https.html
/service-workers/cache-storage/serviceworker/cache-put.https.html
/service-workers/service-worker/skip-waiting-without-client.https.html
/service-workers/service-worker/xhr.https.html
/service-workers/service-worker/fetch-event.https.html
/service-workers/service-worker/fetch-request-redirect.https.html
/service-workers/service-worker/register-wait-forever-in-install-worker.https.html
/service-workers/service-worker/invalid-blobtype.https.html
/service-workers/service-worker/referer.https.html
/service-workers/service-worker/skip-waiting-installed.https.html
/streams/readable-streams/cancel.serviceworker.https.html
/service-workers/service-worker/registration.https.html
/service-workers/service-worker/activation-after-registration.https.html
/streams/piping/error-propagation-backward.serviceworker.https.html
/service-workers/service-worker/register-link-header.https.html
/service-workers/service-worker/fetch-event-async-respond-with.https.html
/service-workers/service-worker/clients-matchall-client-types.https.html
/service-workers/service-worker/getregistrations.https.html
/service-workers/service-worker/controller-on-reload.https.html
/service-workers/cache-storage/serviceworker/cache-delete.https.html
/service-workers/service-worker/skip-waiting-using-registration.https.html
/service-workers/service-worker/interfaces.https.html
/service-workers/service-worker/state.https.html
/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html
/streams/piping/close-propagation-forward.serviceworker.https.html
/service-workers/service-worker/fetch-request-xhr.https.html
/html/webappapis/scripting/events/messageevent-constructor.https.html
/streams/writable-streams/bad-strategies.serviceworker.https.html
/service-workers/service-worker/controller-on-load.https.html
/service-workers/service-worker/request-end-to-end.https.html
/streams/piping/transform-streams.serviceworker.https.html
/streams/writable-streams/write.serviceworker.https.html
/streams/readable-streams/count-queuing-strategy-integration.serviceworker.https.html
/service-workers/cache-storage/serviceworker/cache-storage.https.html
/service-workers/service-worker/register-same-scope-different-script-url.https.html
/service-workers/service-worker/fetch-cors-xhr.https.html
/service-workers/service-worker/postmessage.https.html
/streams/readable-streams/bad-strategies.serviceworker.https.html
/service-workers/service-worker/fetch-event-network-error.https.html
/service-workers/service-worker/navigation-redirect.https.html
/service-workers/service-worker/activate-event-after-install-state-change.https.html
/service-workers/service-worker/synced-state.https.html
/service-workers/service-worker/ready.https.html
/service-workers/service-worker/install-event-type.https.html
/service-workers/service-worker/service-worker-csp-script.https.html
/service-workers/service-worker/fetch-request-resources.https.html
/service-workers/service-worker/client-navigate.https.html
/service-workers/service-worker/fetch-request-fallback.https.html
/service-workers/service-worker/unregister-then-register.https.html
/service-workers/service-worker/clients-get.https.html
/streams/count-queuing-strategy.serviceworker.https.html
/streams/piping/pipe-through.serviceworker.https.html
/streams/byte-length-queuing-strategy.serviceworker.https.html
/service-workers/service-worker/extendable-event-waituntil.https.html
/service-workers/service-worker/registration-end-to-end.https.html
/streams/writable-streams/close.serviceworker.https.html
/service-workers/service-worker/activation.https.html
/service-workers/cache-storage/serviceworker/cache-matchAll.https.html
/service-workers/service-worker/fetch-canvas-tainting-cache.https.html
/service-workers/service-worker/onactivate-script-error.https.html
/service-workers/service-worker/fetch-mixed-content-to-inscope.https.html
/service-workers/service-worker/update-after-navigation-fetch-event.https.html
/service-workers/service-worker/skip-waiting-without-using-registration.https.html
/streams/piping/general.serviceworker.https.html
/service-workers/service-worker/ServiceWorkerGlobalScope/registration-attribute.https.html
/service-workers/service-worker/resource-timing.https.html
/service-workers/service-worker/fetch-header-visibility.https.html
/streams/readable-streams/templated.serviceworker.https.html
/streams/readable-streams/brand-checks.serviceworker.https.html
/streams/readable-streams/tee.serviceworker.https.html
/service-workers/service-worker/invalid-header.https.html
/service-workers/service-worker/registration-iframe.https.html
/service-workers/service-worker/unregister-then-register-new-script.https.html
/service-workers/service-worker/multiple-update.https.html
/service-workers/service-worker/registration-events.https.html
/service-workers/service-worker/fetch-request-css-images.https.html
/service-workers/service-worker/fetch-request-no-freshness-headers.https.html
/streams/readable-streams/pipe-through.serviceworker.https.html
/service-workers/service-worker/installing.https.html
/service-workers/cache-storage/serviceworker/cache-storage-keys.https.html
/service-workers/service-worker/multiple-register.https.html
/service-workers/service-worker/indexeddb.https.html
/service-workers/service-worker/ServiceWorkerGlobalScope/unregister.https.html
/service-workers/service-worker/uncontrolled-page.https.html
/service-workers/service-worker/update.https.html
/streams/piping/error-propagation-forward.serviceworker.https.html
/service-workers/service-worker/websocket.https.html
/service-workers/service-worker/register-link-element.https.html
/streams/writable-streams/count-queuing-strategy.serviceworker.https.html
/service-workers/service-worker/postmessage-msgport-to-client.https.html
/service-workers/service-worker/fetch-mixed-content-to-outscope.https.html
/service-workers/service-worker/fetch-request-css-base-url.https.html
/service-workers/service-worker/getregistration.https.html
/service-workers/service-worker/waiting.https.html
/service-workers/service-worker/registration-useCache.https.html
/streams/writable-streams/byte-length-queuing-strategy.serviceworker.https.html
/service-workers/service-worker/fetch-event-respond-with-stops-propagation.https.html
/service-workers/cache-storage/serviceworker/credentials.https.html
/service-workers/service-worker/oninstall-script-error.https.html
/service-workers/service-worker/serviceworkerobject-scripturl.https.html
/service-workers/cache-storage/serviceworker/cache-storage-match.https.html
/streams/readable-streams/general.serviceworker.https.html
/streams/readable-streams/readable-stream-reader.serviceworker.https.html
/service-workers/service-worker/unregister.https.html
/service-workers/service-worker/multi-globals/url-parsing.https.html
/service-workers/service-worker/fetch-event-within-sw.html
/service-workers/service-worker/fetch-canvas-tainting.https.html
/streams/writable-streams/brand-checks.serviceworker.https.html
/service-workers/service-worker/fetch-waits-for-activate.https.html
/streams/writable-streams/bad-underlying-sinks.serviceworker.https.html
/service-workers/service-worker/clients-matchall-include-uncontrolled.https.html
/service-workers/service-worker/postmessage-to-client.https.html
/service-workers/service-worker/skip-waiting.https.html
/service-workers/service-worker/unregister-controller.https.html
/streams/readable-streams/garbage-collection.serviceworker.https.html
/service-workers/service-worker/appcache-ordering-main.https.html
/service-workers/service-worker/fetch-frame-resource.https.html
/service-workers/service-worker/performance-timeline.https.html
/streams/writable-streams/start.serviceworker.https.html
/service-workers/service-worker/fetch-event-after-navigation-within-page.https.html
/service-workers/service-worker/controller-on-disconnect.https.html
/service-workers/service-worker/update-after-oneday.https.html
/service-workers/service-worker/worker-interception.https.html
/streams/piping/close-propagation-backward.serviceworker.https.html
/service-workers/service-worker/clients-matchall.https.html
/streams/writable-streams/constructor.serviceworker.https.html
/service-workers/service-worker/fetch-csp.https.html
/service-workers/cache-storage/serviceworker/cache-match.https.html
/streams/writable-streams/general.serviceworker.https.html
/service-workers/service-worker/fetch-event-redirect.https.html
/service-workers/service-worker/shared-worker-controlled.https.html
/service-workers/service-worker/active.https.html
/service-workers/service-worker/extendable-event-async-waituntil.https.html
/streams/readable-streams/bad-underlying-sources.serviceworker.https.html
/service-workers/service-worker/update-recovery.https.html
/service-workers/service-worker/fetch-response-xhr.https.html
/service-workers/cache-storage/serviceworker/cache-add.https.html
/service-workers/service-worker/navigate-window.https.html
/service-workers/service-worker/serviceworker-message-event-historical.https.html
/preload/fetch-destination.https.html
/service-workers/service-worker/clients-get-cross-origin.https.html
/service-workers/service-worker/service-worker-csp-connect.https.html
|
The travis failure is unrelated to this PR. Due to a rebase it thinks I wrote all of the tests. |
This allows certain fetches within service workers to trigger fetch events. It also makes interception of redirects by foreign fetch possible. Tests: web-platform-tests/wpt#4518. Related service worker PR: w3c/ServiceWorker#1025. Fixes #303 and fixes #362.
Summary of spec changes: Previously any fetches started within a service worker would bypass the service worker fetch event. We're changing this so only particular methods bypass the fetch event.
This PR tests the
fetch
,cache.all
and notification APIs to ensure they use/bypass the fetch event as required.No browser passes all the tests - Chrome and Firefox fail on the notification tests.