Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
238993003:
ServiceWorker: "minimal" end-to-end sample as a W3C test (Closed)
|
Created:
6 years, 8 months ago by jsbell Modified:
6 years, 7 months ago CC:
blink-reviews, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, nhiroki, horo+watch_chromium.org, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionServiceWorker: "minimal" end-to-end sample as a W3C test
A few NYI things are commented out.
Also removes [NoInterfaceObject] from ServiceWorkerContainer since
script should be able to see the type.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173022
Patch Set 1 #Patch Set 2 : Clarify registration promise chain #
Total comments: 8
Patch Set 3 : Review feedback #Patch Set 4 : Sync with github #
Total comments: 14
Patch Set 5 : minify, rebase #
Total comments: 2
Patch Set 6 : Add 'parsed' to observed states #
Messages
Total messages: 25 (0 generated)
Please take a look. Could probably be tweaked further, feedback welcome.
Thanks for working on this. Probably we should give this test a better name.. registration-end-to-end etc? https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/minimal-end-to-end.html (right): https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/minimal-end-to-end.html:85: 'Out-of-scope document should not see pending state changes'); I think we can remove the latter two asserts for now https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js (right): https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js:24: message.source.postMessage(response, '*'); Let's remove this from our first end-to-end test for now https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/support.js (right): https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/support.js:30: function recordStateChanges(obj) { I think we can remove this for now, it's basically for testing in-scope docs events while we probably want to focus on end-to-end registration here https://codereview.chromium.org/238993003/diff/20001/LayoutTests/virtual/serv... File LayoutTests/virtual/serviceworker/http/tests/serviceworker/minimal-end-to-end-expected.txt (right): https://codereview.chromium.org/238993003/diff/20001/LayoutTests/virtual/serv... LayoutTests/virtual/serviceworker/http/tests/serviceworker/minimal-end-to-end-expected.txt:2: PASS Minimal end-to-end registration <noise> This is part of the reason why I didn't write the first minimal.html in w3c reftests, it basically outputs single PASS or FAIL for single test.. while most of the steps were not working it wasn't too useful to see how many % of steps were passing etc </noise>
Josh, btw dominicc and I have fixed & cleaned up the test code today, as it has had some timing-related bugs and lower-priority features we're not targeting in our current iteration. Can you reflect them as well? (Sorry for that) https://github.com/kinu/ServiceWorker/blob/end-to-end/examples/end-to-end/min... https://github.com/kinu/ServiceWorker/commit/4d025bc132f0b69a4dc95915e3476a1a...
Renamed / specific nits updated. Will sync with github next. https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/minimal-end-to-end.html (right): https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/minimal-end-to-end.html:85: 'Out-of-scope document should not see pending state changes'); On 2014/04/16 03:28:38, kinuko wrote: > I think we can remove the latter two asserts for now Done. https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js (right): https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js:24: message.source.postMessage(response, '*'); On 2014/04/16 03:28:38, kinuko wrote: > Let's remove this from our first end-to-end test for now Done. https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/support.js (right): https://codereview.chromium.org/238993003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/support.js:30: function recordStateChanges(obj) { On 2014/04/16 03:28:38, kinuko wrote: > I think we can remove this for now, it's basically for testing in-scope docs > events while we probably want to focus on end-to-end registration here Done. https://codereview.chromium.org/238993003/diff/20001/LayoutTests/virtual/serv... File LayoutTests/virtual/serviceworker/http/tests/serviceworker/minimal-end-to-end-expected.txt (right): https://codereview.chromium.org/238993003/diff/20001/LayoutTests/virtual/serv... LayoutTests/virtual/serviceworker/http/tests/serviceworker/minimal-end-to-end-expected.txt:2: PASS Minimal end-to-end registration On 2014/04/16 03:28:38, kinuko wrote: > <noise> > This is part of the reason why I didn't write the first minimal.html in w3c > reftests, it basically outputs single PASS or FAIL for single test.. while most > of the steps were not working it wasn't too useful to see how many % of steps > were passing etc > </noise> Yep - as part of the intro-to-Service Worker github, this test style is too succinct. I assume we'll upstream these w3c-style tests to the w3c web-platform-tests repo as soon as things settle down.
Sync'd with github's out-of-scope test update. A few notes below... https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right): https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:19: navigator.serviceWorker.oncurrentchange = function() { I reintroduced this as it seems like a valid assertion for this test. https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:28: t.step_func(doRegister), t.step_func(doRegister)); Having unregister() reject if there was no match seems like an API flaw to me. I filed a bug against the spec. For now, do the registration on both success/fail. https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:35: t.step_func(onRegister) On github, the registration.html calls onRegister in both success/fail here, which doesn't make sense. https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:48: onTestFinished(); Having two exit paths makes me sad. The test should not be racy. https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:57: assert_equals(lastServiceWorkerState, 'parsed'); This case currently never matches, but it's in github so I kept it. https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js (right): https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js:2: e.waitUntil(new Promise(function(r) { setTimeout(r, 5); })); I'm not happy with using setTimeout in this test. I'd prefer to go back to having a ping/pong protocol. Why was it removed? https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js:11: if (typeof message === 'object' && 'port' in message) { FWIW, `typeof null === 'object'`; the `Object(x) === x` I'd used handles that case, but is more subtle. I suppose `typeof x === 'object' && x !== null` would be the clearest.
Responses inline... btw please feel free to fix this test with your best judge, the reason I asked you to sync with the latest version was basically to avoid bothering you by initial errors/unnecessary code in earlier version of minimal.html. https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right): https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:28: t.step_func(doRegister), t.step_func(doRegister)); On 2014/04/16 17:43:38, jsbell wrote: > Having unregister() reject if there was no match seems like an API flaw to me. I > filed a bug against the spec. > > For now, do the registration on both success/fail. It's the behavior currently spec'ed, not the test's fault (crbug.com/363851) https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:35: t.step_func(onRegister) On 2014/04/16 17:43:38, jsbell wrote: > On github, the registration.html calls onRegister in both success/fail here, > which doesn't make sense. It was a bug, it's fixed now (by dominicc's pull request) https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:48: onTestFinished(); On 2014/04/16 17:43:38, jsbell wrote: > Having two exit paths makes me sad. The test should not be racy. Yup agreed, btw feel free to fix these flaws here then I can backport them (minimal.html was written in a very casual way, some of bits are probably not suitable for landing) https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:57: assert_equals(lastServiceWorkerState, 'parsed'); On 2014/04/16 17:43:38, jsbell wrote: > This case currently never matches, but it's in github so I kept it. I wasn't very sure which is expected behavior, whether we should be in 'parsed' state or 'installing' state after registration. (Actually from the code I was expecting this to start with 'parsed', and # of stateChangeCount is 4, but looks like it's not the current behavior) https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js (right): https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js:2: e.waitUntil(new Promise(function(r) { setTimeout(r, 5); })); On 2014/04/16 17:43:38, jsbell wrote: > I'm not happy with using setTimeout in this test. I'd prefer to go back to > having a ping/pong protocol. Why was it removed? I wanted to make this test dead simple / 'minimal', feel free to revert it https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/end-to-end-worker.js:11: if (typeof message === 'object' && 'port' in message) { On 2014/04/16 17:43:38, jsbell wrote: > FWIW, `typeof null === 'object'`; the `Object(x) === x` I'd used handles that > case, but is more subtle. I suppose `typeof x === 'object' && x !== null` would > be the clearest. Yup, sgtm, please feel free to fix it.
https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right): https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:74: onTestFinished(); FYI, I think the flaky timeouts are caused by activation finishing before onRegister is entered. I'm working on fixing that in crbug.com/365252. My tests in <https://github.com/mattto/ServiceWorker/tree/master/examples/layout-tests/ser...> have a workaround until then (not sure what to do for this test though... maybe just turn the flaky timeout into flaky failure?)
I replaced the onTestFinished logic with a Promise.all() join over the dependent states. If we get message queuing then I think this would be deterministic. I also removed the oninstall and onactivate handlers that called waitUntil. I couldn't decide between removing them or making them ping/pong with the document to unblock them, or even just capture the fact that they were called. After talking with michaeln@ we decided that if this was going to be "minimal" it should probably not have any extras. I'll add another test (different CL) which does something similar but logs the events seen in the SW. On 2014/04/22 14:03:08, falken wrote: > https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... > File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right): > > https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... > LayoutTests/http/tests/serviceworker/registration-end-to-end.html:74: > onTestFinished(); > FYI, I think the flaky timeouts are caused by activation finishing before > onRegister is entered. I'm working on fixing that in crbug.com/365252. My tests > in > <https://github.com/mattto/ServiceWorker/tree/master/examples/layout-tests/ser...> > have a workaround until then (not sure what to do for this test though... maybe > just turn the flaky timeout into flaky failure?) Yeah, I see this test flipping between only seeing two states and seeing all 4. I think we can hold off and land this once your change is in, since it's halfway through landing.
On 2014/04/25 23:21:47, jsbell wrote: > I replaced the onTestFinished logic with a Promise.all() join over the dependent > states. If we get message queuing then I think this would be deterministic. > > I also removed the oninstall and onactivate handlers that called waitUntil. I > couldn't decide between removing them or making them ping/pong with the document > to unblock them, or even just capture the fact that they were called. After > talking with michaeln@ we decided that if this was going to be "minimal" it > should probably not have any extras. > > I'll add another test (different CL) which does something similar but logs the > events seen in the SW. > > On 2014/04/22 14:03:08, falken wrote: > > > https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... > > File LayoutTests/http/tests/serviceworker/registration-end-to-end.html > (right): > > > > > https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... > > LayoutTests/http/tests/serviceworker/registration-end-to-end.html:74: > > onTestFinished(); > > FYI, I think the flaky timeouts are caused by activation finishing before > > onRegister is entered. I'm working on fixing that in crbug.com/365252. My > tests > > in > > > <https://github.com/mattto/ServiceWorker/tree/master/examples/layout-tests/ser...> > > have a workaround until then (not sure what to do for this test though... > maybe > > just turn the flaky timeout into flaky failure?) > > Yeah, I see this test flipping between only seeing two states and seeing all 4. > > I think we can hold off and land this once your change is in, since it's halfway > through landing. Sounds good, after the Blink roll I will land https://codereview.chromium.org/255813003/ which should fix this.
On 2014/04/25 23:21:47, jsbell wrote: > I replaced the onTestFinished logic with a Promise.all() join over the dependent > states. If we get message queuing then I think this would be deterministic. Sounds good! > I also removed the oninstall and onactivate handlers that called waitUntil. I > couldn't decide between removing them or making them ping/pong with the document > to unblock them, or even just capture the fact that they were called. After > talking with michaeln@ we decided that if this was going to be "minimal" it > should probably not have any extras. Ok, sgtm2. > I'll add another test (different CL) which does something similar but logs the > events seen in the SW. > > On 2014/04/22 14:03:08, falken wrote: > > > https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... > > File LayoutTests/http/tests/serviceworker/registration-end-to-end.html > (right): > > > > > https://codereview.chromium.org/238993003/diff/60001/LayoutTests/http/tests/s... > > LayoutTests/http/tests/serviceworker/registration-end-to-end.html:74: > > onTestFinished(); > > FYI, I think the flaky timeouts are caused by activation finishing before > > onRegister is entered. I'm working on fixing that in crbug.com/365252. My > tests > > in > > > <https://github.com/mattto/ServiceWorker/tree/master/examples/layout-tests/ser...> > > have a workaround until then (not sure what to do for this test though... > maybe > > just turn the flaky timeout into flaky failure?) > > Yeah, I see this test flipping between only seeing two states and seeing all 4. > > I think we can hold off and land this once your change is in, since it's halfway > through landing.
Can I get an lgtm, or is there anything else?
On 2014/04/29 21:16:33, jsbell wrote: > Can I get an lgtm, or is there anything else? Sure, lgtm
The CQ bit was checked by kinuko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jsbell@chromium.org/238993003/70001
The CQ bit was unchecked by kinuko@chromium.org
https://codereview.chromium.org/238993003/diff/70001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right): https://codereview.chromium.org/238993003/diff/70001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:68: // FIXME: Not currently seen. Is this issue fixed by Matt's change?
The CQ bit was checked by kinuko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jsbell@chromium.org/238993003/70001
https://codereview.chromium.org/238993003/diff/70001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right): https://codereview.chromium.org/238993003/diff/70001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-end-to-end.html:68: // FIXME: Not currently seen. On 2014/04/30 04:11:43, kinuko wrote: > Is this issue fixed by Matt's change? Yes, I expect this is fixed in ToT (since Chromium r266544)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink
The CQ bit was checked by jsbell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jsbell@chromium.org/238993003/90001
On 2014/04/30 04:21:31, falken wrote: > https://codereview.chromium.org/238993003/diff/70001/LayoutTests/http/tests/s... > File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right): > > https://codereview.chromium.org/238993003/diff/70001/LayoutTests/http/tests/s... > LayoutTests/http/tests/serviceworker/registration-end-to-end.html:68: // FIXME: > Not currently seen. > On 2014/04/30 04:11:43, kinuko wrote: > > Is this issue fixed by Matt's change? > > Yes, I expect this is fixed in ToT (since Chromium r266544) Yes! Added it to the list of expected states and removed the FIXME.
Message was sent while issue was closed.
Change committed as 173022 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
