Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775613004:
Fix mouse-event-buttons-attribute flakiness (Closed)
|
Created:
4 years, 9 months ago by Navid Zolghadr Modified:
4 years, 9 months ago CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRunning the test after the image is loaded.
This test was still flaky because sometimes the tests
were running before the image is loaded. Moving
the runtests to onload so that we are sure the
image is loaded.
BUG=469657
Committed: https://crrev.com/70653316bb99c4053eb5a1720db44af84c65db92
Cr-Commit-Position: refs/heads/master@{#380470}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Remove async #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 23 (7 generated)
Description was changed from ========== Running the test after the image is loaded This test was flaky because sometimes the tests were running before the image is loaded. Moving the runtests to onload so that we are sure the image is loaded. BUG=469657 ========== to ========== Running the test after the image is loaded. This test was still flaky because sometimes the tests were running before the image is loaded. Moving the runtests to onload so that we are sure the image is loaded. BUG=469657 ==========
nzolghadr@chromium.org changed reviewers: + mustaq@chromium.org, rbyers@chromium.org
https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:17: <body> nit: style says to omit body when it's not necessary: https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:161: window.jsTestIsAsync = true; does this actually need to be an async test? IIRC non-async tests are considered complete after all onload handlers have finished running. So moving the runtests call to the img onload is perhaps all you need here.
https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:17: <body> On 2016/03/09 20:16:16, Rick Byers wrote: > nit: style says to omit body when it's not necessary: > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... Sure. https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:161: window.jsTestIsAsync = true; On 2016/03/09 20:16:16, Rick Byers wrote: > does this actually need to be an async test? IIRC non-async tests are > considered complete after all onload handlers have finished running. So moving > the runtests call to the img onload is perhaps all you need here. The problem was that I would get one "PASS successfullyParsed is true TEST COMPLETE" at the beginning of output file. Is that okay to add that to the -expected file or is there another way of getting rid of that beside making the test async?
On 2016/03/09 20:21:07, Navid Zolghadr wrote: > https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html > (right): > > https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:17: > <body> > On 2016/03/09 20:16:16, Rick Byers wrote: > > nit: style says to omit body when it's not necessary: > > > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... > > Sure. > > https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:161: > window.jsTestIsAsync = true; > On 2016/03/09 20:16:16, Rick Byers wrote: > > does this actually need to be an async test? IIRC non-async tests are > > considered complete after all onload handlers have finished running. So > moving > > the runtests call to the img onload is perhaps all you need here. > > The problem was that I would get one > "PASS successfullyParsed is true > > TEST COMPLETE" > > at the beginning of output file. Is that okay to add that to the -expected file > or is there another way of getting rid of that beside making the test async? I thought maybe showing it is better. I removed the async and updated the -expected file. Is this one okay?
lgtm. https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:161: window.jsTestIsAsync = true; On 2016/03/09 20:21:07, Navid Zolghadr wrote: > On 2016/03/09 20:16:16, Rick Byers wrote: > > does this actually need to be an async test? IIRC non-async tests are > > considered complete after all onload handlers have finished running. So > moving > > the runtests call to the img onload is perhaps all you need here. > > The problem was that I would get one > "PASS successfullyParsed is true > > TEST COMPLETE" > > at the beginning of output file. Is that okay to add that to the -expected file > or is there another way of getting rid of that beside making the test async? I thought too-early TEST COMPLETE message is a sign of "asyncness". I remember encountering it before, and marked the test as async (w/o waitUntilDone which seems redundant here: js-test.js:102). I didn't try not-marking-async, worth a try though.
https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:161: window.jsTestIsAsync = true; On 2016/03/09 21:38:01, mustaq wrote: > On 2016/03/09 20:21:07, Navid Zolghadr wrote: > > On 2016/03/09 20:16:16, Rick Byers wrote: > > > does this actually need to be an async test? IIRC non-async tests are > > > considered complete after all onload handlers have finished running. So > > moving > > > the runtests call to the img onload is perhaps all you need here. > > > > The problem was that I would get one > > "PASS successfullyParsed is true > > > > TEST COMPLETE" > > > > at the beginning of output file. Is that okay to add that to the -expected > file > > or is there another way of getting rid of that beside making the test async? > > I thought too-early TEST COMPLETE message is a sign of "asyncness". I remember > encountering it before, and marked the test as async (w/o waitUntilDone which > seems redundant here: js-test.js:102). > > I didn't try not-marking-async, worth a try though. I'm a bit confused of what you asked me to do. I think this comment is on patchset 1. Can you look at latest patchset and see whether that is what you meant?
https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:161: window.jsTestIsAsync = true; On 2016/03/10 13:57:50, Navid Zolghadr wrote: > On 2016/03/09 21:38:01, mustaq wrote: > > On 2016/03/09 20:21:07, Navid Zolghadr wrote: > > > On 2016/03/09 20:16:16, Rick Byers wrote: > > > > does this actually need to be an async test? IIRC non-async tests are > > > > considered complete after all onload handlers have finished running. So > > > moving > > > > the runtests call to the img onload is perhaps all you need here. > > > > > > The problem was that I would get one > > > "PASS successfullyParsed is true > > > > > > TEST COMPLETE" > > > > > > at the beginning of output file. Is that okay to add that to the -expected > > file > > > or is there another way of getting rid of that beside making the test async? > > > > I thought too-early TEST COMPLETE message is a sign of "asyncness". I remember > > encountering it before, and marked the test as async (w/o waitUntilDone which > > seems redundant here: js-test.js:102). > > > > I didn't try not-marking-async, worth a try though. > > I'm a bit confused of what you asked me to do. I think this comment is on > patchset 1. Can you look at latest patchset and see whether that is what you > meant? Yes, I meant patch#2 or even #3 worth a try, to confirm if early TEST COMPLETE message is really not an issue. If the test turns out to be flaky again, please mark it as async before anything else. https://codereview.chromium.org/1775613004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/1775613004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:143: eventSender.mouseMoveTo(500+i, 500+i); Why do you need this loop? Nit: there's a tab char at the beginning of line.
https://codereview.chromium.org/1775613004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/1775613004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:143: eventSender.mouseMoveTo(500+i, 500+i); On 2016/03/10 14:51:13, mustaq wrote: > Why do you need this loop? > > Nit: there's a tab char at the beginning of line. Oops. This whole for loop was just a test thing that slipped into the commit. I'll remove it.
ptal
Still lgtm.
The CQ bit was checked by nzolghadr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775613004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775613004/60001
On 2016/03/10 14:51:13, mustaq wrote: > https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html > (right): > > https://codereview.chromium.org/1775613004/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:161: > window.jsTestIsAsync = true; > On 2016/03/10 13:57:50, Navid Zolghadr wrote: > > On 2016/03/09 21:38:01, mustaq wrote: > > > On 2016/03/09 20:21:07, Navid Zolghadr wrote: > > > > On 2016/03/09 20:16:16, Rick Byers wrote: > > > > > does this actually need to be an async test? IIRC non-async tests are > > > > > considered complete after all onload handlers have finished running. So > > > > moving > > > > > the runtests call to the img onload is perhaps all you need here. > > > > > > > > The problem was that I would get one > > > > "PASS successfullyParsed is true > > > > > > > > TEST COMPLETE" > > > > > > > > at the beginning of output file. Is that okay to add that to the -expected > > > file > > > > or is there another way of getting rid of that beside making the test > async? > > > > > > I thought too-early TEST COMPLETE message is a sign of "asyncness". I > remember > > > encountering it before, and marked the test as async (w/o waitUntilDone > which > > > seems redundant here: js-test.js:102). > > > > > > I didn't try not-marking-async, worth a try though. > > > > I'm a bit confused of what you asked me to do. I think this comment is on > > patchset 1. Can you look at latest patchset and see whether that is what you > > meant? > > Yes, I meant patch#2 or even #3 worth a try, to confirm if early TEST COMPLETE > message is really not an issue. If the test turns out to be flaky again, please > mark it as async before anything else. This is a known limitation of js-test, I think people have tried to fix it before but had some issues changing the timing of some tests and getting all tests fixed. Latest bug I see for this is http://crbug.com/486486. But anyway I think it's common, so repeating it here should be fine. LGTM too > > https://codereview.chromium.org/1775613004/diff/40001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html > (right): > > https://codereview.chromium.org/1775613004/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/events/mouse-event-buttons-attribute.html:143: > eventSender.mouseMoveTo(500+i, 500+i); > Why do you need this loop? > > Nit: there's a tab char at the beginning of line.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nzolghadr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775613004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775613004/60001
Message was sent while issue was closed.
Description was changed from ========== Running the test after the image is loaded. This test was still flaky because sometimes the tests were running before the image is loaded. Moving the runtests to onload so that we are sure the image is loaded. BUG=469657 ========== to ========== Running the test after the image is loaded. This test was still flaky because sometimes the tests were running before the image is loaded. Moving the runtests to onload so that we are sure the image is loaded. BUG=469657 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Running the test after the image is loaded. This test was still flaky because sometimes the tests were running before the image is loaded. Moving the runtests to onload so that we are sure the image is loaded. BUG=469657 ========== to ========== Running the test after the image is loaded. This test was still flaky because sometimes the tests were running before the image is loaded. Moving the runtests to onload so that we are sure the image is loaded. BUG=469657 Committed: https://crrev.com/70653316bb99c4053eb5a1720db44af84c65db92 Cr-Commit-Position: refs/heads/master@{#380470} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/70653316bb99c4053eb5a1720db44af84c65db92 Cr-Commit-Position: refs/heads/master@{#380470} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
