Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521843002:
Fix flakyness in broadcastchannel/blobs.html test. (Closed)
|
Created:
4 years, 1 month ago by Marijn Kruisselbrink Modified:
4 years, 1 month ago Reviewers:
jsbell CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix flakyness in broadcastchannel/blobs.html test.
Due to lack of ordering guarantees in mojo, the "done" message send on the
BroadcastChannel could arrive after the worker signals that it was done
(the worker sending that message via a MessagePort).
Fortunately the spec doesn't give any guarantees of relative ordering of
messages on message ports and other events, so this is all perfectly spec
compliant (if not a little bit confusing), so this just changes the test to
wait for both "done" signals to arrive before verifying the results.
BUG=667552
Committed: https://crrev.com/55e6c2a1b1892268ca20d1c059638228d2a9c349
Cr-Commit-Position: refs/heads/master@{#433920}
Patch Set 1 #
Total comments: 5
Patch Set 2 : address comments #Messages
Total messages: 21 (15 generated)
The CQ bit was checked by mek@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mek@chromium.org changed reviewers: + jsbell@chromium.org
lgtm with nits https://codereview.chromium.org/2521843002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/broadcastchannel/blobs.html (right): https://codereview.chromium.org/2521843002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/broadcastchannel/blobs.html:36: let verifyEvents = function() { nit: could be const, but using let for consistency with above is fine. https://codereview.chromium.org/2521843002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/broadcastchannel/blobs.html:59: c2.addEventListener('message', e => { Wrap in t.step_func() https://codereview.chromium.org/2521843002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/broadcastchannel/blobs.html:68: let worker = new Worker('resources/worker.js'); could be const, but let is okay.
The CQ bit was checked by mek@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 checked by mek@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by mek@chromium.org
The CQ bit was checked by mek@chromium.org
https://codereview.chromium.org/2521843002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/broadcastchannel/blobs.html (right): https://codereview.chromium.org/2521843002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/broadcastchannel/blobs.html:36: let verifyEvents = function() { On 2016/11/22 at 17:13:22, jsbell wrote: > nit: could be const, but using let for consistency with above is fine. Ah yes, good point. Changed all the lets that could be consts to const. https://codereview.chromium.org/2521843002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/broadcastchannel/blobs.html:59: c2.addEventListener('message', e => { On 2016/11/22 at 17:13:22, jsbell wrote: > Wrap in t.step_func() Done
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/2521843002/#ps40001 (title: "address comments")
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": 40001, "attempt_start_ts": 1479837088229490,
"parent_rev": "a0aa006d1f7b728506f3faa952bdc0197090aa90", "commit_rev":
"33f9f49b8c82208fba0b0def7f2573c94b9c11e9"}
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix flakyness in broadcastchannel/blobs.html test. Due to lack of ordering guarantees in mojo, the "done" message send on the BroadcastChannel could arrive after the worker signals that it was done (the worker sending that message via a MessagePort). Fortunately the spec doesn't give any guarantees of relative ordering of messages on message ports and other events, so this is all perfectly spec compliant (if not a little bit confusing), so this just changes the test to wait for both "done" signals to arrive before verifying the results. BUG=667552 ========== to ========== Fix flakyness in broadcastchannel/blobs.html test. Due to lack of ordering guarantees in mojo, the "done" message send on the BroadcastChannel could arrive after the worker signals that it was done (the worker sending that message via a MessagePort). Fortunately the spec doesn't give any guarantees of relative ordering of messages on message ports and other events, so this is all perfectly spec compliant (if not a little bit confusing), so this just changes the test to wait for both "done" signals to arrive before verifying the results. BUG=667552 Committed: https://crrev.com/55e6c2a1b1892268ca20d1c059638228d2a9c349 Cr-Commit-Position: refs/heads/master@{#433920} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/55e6c2a1b1892268ca20d1c059638228d2a9c349 Cr-Commit-Position: refs/heads/master@{#433920} |
