Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2520093002:
Make Cache Storage tests resilient to execution order (Closed)
DescriptionMake Cache Storage tests resilient to execution order
BUG=663871
R=jkarlin@chromium.org
Committed: https://crrev.com/cfe8a0730ee7c02fa8ec082c821b738244b4dd74
Cr-Commit-Position: refs/heads/master@{#433949}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rename string, add comment and helper #
Total comments: 2
Messages
Total messages: 17 (7 generated)
The CQ bit was checked by jsbell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2520093002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js (right): https://codereview.chromium.org/2520093002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js:206: var bad_name = 'unpaired\uD800'; bad_name is very confusing. It makes me think that it's a name that shouldn't exist. Perhaps call it unpaired_name instead? https://codereview.chromium.org/2520093002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js:210: self.caches.delete(converted_name) How was this issue (not deleting the caches) not causing flakiness before?
In https://github.com/w3c/web-platform-tests/pull/4233 it is suggested to delete all caches. I'll add a helper to do that and use it here. (the plan is to delete this local copy of the test ASAP, but I want to fix the flakiness) https://codereview.chromium.org/2520093002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js (right): https://codereview.chromium.org/2520093002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js:206: var bad_name = 'unpaired\uD800'; On 2016/11/22 15:08:34, jkarlin wrote: > bad_name is very confusing. It makes me think that it's a name that shouldn't > exist. Perhaps call it unpaired_name instead? Agreed, will do. https://codereview.chromium.org/2520093002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js:210: self.caches.delete(converted_name) On 2016/11/22 15:08:34, jkarlin wrote: > How was this issue (not deleting the caches) not causing flakiness before? This test runs 3 times (as window, worker, and serviceworker). Normally these are sharded into 3 different processes, but if the order is randomized they can end up in the same process; or if you turn off sharding you can make it fail: run-webkit-tests --no-retry-failures --child-processes=1 http/tests/cachestorage/*/cache-storage.html
https://codereview.chromium.org/2520093002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js (right): https://codereview.chromium.org/2520093002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js:210: self.caches.delete(converted_name) On 2016/11/22 18:02:39, jsbell wrote: > On 2016/11/22 15:08:34, jkarlin wrote: > > How was this issue (not deleting the caches) not causing flakiness before? > > This test runs 3 times (as window, worker, and serviceworker). Normally these > are sharded into 3 different processes, but if the order is randomized they can > end up in the same process; or if you turn off sharding you can make it fail: > > run-webkit-tests --no-retry-failures --child-processes=1 > http/tests/cachestorage/*/cache-storage.html > Acknowledged.
Renamed the string, added a helper/comment (per WPT PR feedback). jkarlin@ - another more look?
https://codereview.chromium.org/2520093002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js (right): https://codereview.chromium.org/2520093002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js:213: return delete_all_caches() Why not have promise_test run delete_all_caches at the end instead of having to insert it into tests?
https://codereview.chromium.org/2520093002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js (right): https://codereview.chromium.org/2520093002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cachestorage/script-tests/cache-storage.js:213: return delete_all_caches() On 2016/11/22 18:20:40, jkarlin wrote: > Why not have promise_test run delete_all_caches at the end instead of having to > insert it into tests? promise_test is generic (part of testharness.js); we could rework all of these test to go through a common wrapper, but I'd rather do that upstream.
lgtm
The CQ bit was checked by jsbell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479840141525790,
"parent_rev": "b5a8cdc3afda5c71cdd9cb475d801ad875983432", "commit_rev":
"d3b30a6b53daa8dd44aa64b1452f8f5aed237150"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make Cache Storage tests resilient to execution order BUG=663871 R=jkarlin@chromium.org ========== to ========== Make Cache Storage tests resilient to execution order BUG=663871 R=jkarlin@chromium.org Committed: https://crrev.com/cfe8a0730ee7c02fa8ec082c821b738244b4dd74 Cr-Commit-Position: refs/heads/master@{#433949} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cfe8a0730ee7c02fa8ec082c821b738244b4dd74 Cr-Commit-Position: refs/heads/master@{#433949} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
