Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
394133002:
ServiceWorker: Add stress test for registration (Closed)
|
Created:
6 years, 5 months ago by nhiroki Modified:
6 years, 5 months ago Reviewers:
dominicc (has gone to gerrit) CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, falken, kinuko+serviceworker, horo+watch_chromium.org, alecflett+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionServiceWorker: Add stress test for registration
This test makes sure that many service workers can work simultaneously without
any crashes.
BUG=392735
TEST=run_webkit_tests.py --details --debug http/tests/serviceworker/
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178359
Patch Set 1 : #
Total comments: 9
Patch Set 2 : move to chromium/ #Patch Set 3 : fix test failure #Messages
Total messages: 14 (0 generated)
Ptal, thanks!
Some comments inline. https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-stress.html (right): https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:1: <!DOCTYPE html> Let's move this to chromium/ because stress is kind of an implementation-specific thing. https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:9: var step = t.step_func.bind(t); Let's indent two spaces, like this: async_test(function(t) { var ... }, '...'); Some examples are here: http://www.chromium.org/blink/serviceworker/testing https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:30: unregister_promises.push( I wonder if you should unregister them all first at the start of the test for reliability? https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:31: navigator.serviceWorker.unregister(workers[i].scope)); I wonder if you should unregister them all first at the start of the test for reliability?
Thank you for reviewing! Only comment reply. https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-stress.html (right): https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:9: var step = t.step_func.bind(t); On 2014/07/16 04:16:04, dominicc wrote: > Let's indent two spaces, like this: Hmmm... your example (and wiki's examples) has 4-space indents after "async_test"...? > async_test(function(t) { > var ... > }, '...'); > > Some examples are here: > > http://www.chromium.org/blink/serviceworker/testing
https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-stress.html (right): https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:9: var step = t.step_func.bind(t); On 2014/07/16 04:29:49, nhiroki wrote: > On 2014/07/16 04:16:04, dominicc wrote: > > Let's indent two spaces, like this: > > Hmmm... your example (and wiki's examples) has 4-space indents after > "async_test"...? > > > async_test(function(t) { > > var ... > > }, '...'); > > > > Some examples are here: > > > > http://www.chromium.org/blink/serviceworker/testing Ah, maybe I got what you mean. I should fix indents not here but somewhere else (for example, line 16-23), right?
Updated. https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-stress.html (right): https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:1: <!DOCTYPE html> On 2014/07/16 04:16:04, dominicc wrote: > Let's move this to chromium/ because stress is kind of an > implementation-specific thing. Done. https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:9: var step = t.step_func.bind(t); On 2014/07/16 04:42:28, nhiroki wrote: > On 2014/07/16 04:29:49, nhiroki wrote: > > On 2014/07/16 04:16:04, dominicc wrote: > > > Let's indent two spaces, like this: > > > > Hmmm... your example (and wiki's examples) has 4-space indents after > > "async_test"...? > > > > > async_test(function(t) { > > > var ... > > > }, '...'); > > > > > > Some examples are here: > > > > > > http://www.chromium.org/blink/serviceworker/testing > > Ah, maybe I got what you mean. I should fix indents not here but somewhere else > (for example, line 16-23), right? Done. https://codereview.chromium.org/394133002/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-stress.html:30: unregister_promises.push( On 2014/07/16 04:16:04, dominicc wrote: > I wonder if you should unregister them all first at the start of the test for > reliability? Done.
Sweet! LGTM
The CQ bit was checked by dominicc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/394133002/40002
The CQ bit was unchecked by nhiroki@chromium.org
Hmmm... the stress test failed on some bots due to assertion failure. I'll take a look into it...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15702) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17195)
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/394133002/70001
Message was sent while issue was closed.
Change committed as 178359 |
