Add tests for service worker inheritance#4610
Add tests for service worker inheritance#4610jungkees wants to merge 10 commits intoweb-platform-tests:masterfrom
Conversation
|
Notifying @ehsan and @mkruisselbrink. (Learn how reviewing works.) |
|
The spec work for this is in progress here: whatwg/html#2080. It'd be appreciated if anyone could review it. |
| return service_worker_unregister(t, scope1); | ||
| }) | ||
| .then(function() { | ||
| service_worker_unregister_and_done(t, scope2); |
There was a problem hiding this comment.
you don't need the and_done() variant in a promise_test. When the overall promise resolves the test will be marked as done anyway.
| .then(function() { | ||
| service_worker_unregister_and_done(t, scope2); | ||
| }) | ||
| .catch(unreached_rejection(t)); |
There was a problem hiding this comment.
You also don't need this .catch(unreached_rejection(t)) in a promise_test. If the promise passed to promise_test rejects the test will be considered a failure.
| 'about:blank Document should inherit parent\'s SW.'); | ||
| }) | ||
| .then(function() { | ||
| return with_srcdoc_iframe('<p>Some HTML</p>'); |
There was a problem hiding this comment.
It would be nice if the various tests (about:blank, srcdoc, etc) would be actually separate promise_tests, rather than just assertions all in the same test. That way the test results would much clearer show which features work/don't work, rather than only reporting the first failing result. Not sure how tricky that is going to be with the various service worker registrations, but it shouldn't be too bad I think.
One way might be to do something like (but then with proper syntax etc):
let registered_workers = service_worker_unregister_and_register(...).then(...).then(...);
promise_test(t => {
registered_workers.then(() => with_iframe(''))
.then(f => {
assert(...)
});
}, 'about:blank document');
promise_test(t => {....}, 'srcdoc iframe');
// etcThere was a problem hiding this comment.
This is a good point indeed. I addressed them as suggested basically but put the registration and activation chain in the first promise_test as it needs |t|.
| var script1 = script + '?for_scope1'; | ||
| var script2 = script + '?for_scope2'; | ||
| var registration1, registration2; | ||
| var parent_controller, child_controller; |
There was a problem hiding this comment.
Why did you declare child_controller at this scope? I think just declaring it in all the closures where you actually use it would be cleaner (I don't think you ever use it across closure boundaries?)
There was a problem hiding this comment.
Addressed by declaring them in their closures.
| var scope2 = 'resources/blank.html'; | ||
| var script1 = script + '?for_scope1'; | ||
| var script2 = script + '?for_scope2'; | ||
| var registration1, registration2; |
There was a problem hiding this comment.
What are these variables used for? You assign to them, but I don't see them being read anywhere?
There was a problem hiding this comment.
Right. They're unused. Deleted.
| 'matched SW should set the controller to null.'); | ||
| }) | ||
| .then(function() { | ||
| return service_worker_unregister(t, scope1); |
There was a problem hiding this comment.
Rather than calling service_worker_unregister at the end of your test here, it is probably cleaner to call add_completion_callback(() => {r.unregister();}); as soon as you have registered a service worker, to make sure they get cleaned up even if the test fails.
There was a problem hiding this comment.
Right. Addressed them for registration.unregister() and frame.remove().
|
I think the travis check failure is because an unrelated test (that just includes the test-helpers.sub.js file you modified) has non-unique test names (see #4404) |
|
@mkruisselbrink, thanks for reviewing! I uploaded a patch addressing those comments. |
What would be the expected resolution for this for now? It seems
has non-unique test names at least. |
That should be fixed now #4626 landed (hopefully that was the only one). Not sure how to trigger checks again on this CL though... |
| // This client's active service worker is set to scope1's registration's | ||
| // active worker by clients.claim() during the activation. | ||
| parent_controller = navigator.serviceWorker.controller; | ||
| return with_iframe('') |
There was a problem hiding this comment.
Maybe adding an "options" parameter to with_iframe with an auto_remove option (like in the chromium version of with_iframe) would make sense? Having a boolean that defaults to true like in the chromium version seems wrong, but having an option to pass to with_iframe to get auto remove behavior does seem reasonable.
There was a problem hiding this comment.
Yes, that makes sense. I think there are quite a few tests already that manually remove the iframes. So I made it opt-in to auto remove the iframe by passing options.auto_remove set to true.
| }); | ||
| } | ||
|
|
||
| function with_sandboxed_iframe(url, sandbox) { |
There was a problem hiding this comment.
If you add an "options" dictionary to with_iframe to support auto_remove, you could also add a sandbox option to that same dictionary rather than having a separate function for this.
There was a problem hiding this comment.
Yes, I added sandbox property and srcdoc property to the options as suggested. Looks cleaner.
|
Does anyone have any idea what causes this travis-ci error?: https://travis-ci.org/w3c/web-platform-tests/jobs/195441119. |
|
It seems the test takes too long to run from "The job exceeded the maximum time limit for jobs, and has been terminated." |
|
|
Chrome (unstable channel)Testing web-platform-tests at revision f403d50 |
Firefox (nightly channel)Testing web-platform-tests at revision f403d50 |
|
The test added in this PR seems okay now. Need to look at |
|
These tests are now available on w3c-test.org |
|
Try the Travis CI again after checking |
|
Recently added |
Loading an iframe without involving a navigate fetch inherits its parent's active service worker, if it has one. By this rule: - Initial about:blank Document on iframe inherits its parent's SW - iframe srcdoc Document inherits its parent's SW However, the navigate fetch through http fetch still wins. So, if a client obtains a matched SW during the navigation, the matched SW replaces the inherited SW. An exception: - Navigating an iframe to a non-initial about:blank Document inherits the parent's SW Related work: whatwg/html#2080.
Add options param to with_iframe to set the iframe's sandbox attribute and srcdoc attribute as well as add a callback to remove the iframe when the test is done.
e57580e to
24f6162
Compare
|
@jungkees, can you rebase this to resolve the conflicts and retrigger Travis? Whatever the trouble with Travis was over a year ago, it's probably gone now. |
|
Some of this may have changed in the intervening time see eg whatwg/html#3725 and existing tests mentioned at whatwg/html#3725 (comment) |
Loading an iframe without involving a navigate fetch inherits its
parent's active service worker, if it has one. By this rule:
However, the navigate fetch through http fetch still wins. So, if a
client obtains a matched SW during the navigation, the matched SW
replaces the inherited SW.
An exception:
the parent's SW
Related work: whatwg/html#2080.