Closed
Bug 651902
Opened 13 years ago
Closed 13 years ago
Make content/base/test/test_bug592366.html non flaky
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(1 file, 2 obsolete files)
2.01 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
Henri, can you have a look at this patch and confirm it is still testing the correct behavior. Revert applying the original patch didn't work (merge issues).
Attachment #527572 -
Flags: review?(hsivonen)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs review]
Comment 1•13 years ago
|
||
Comment on attachment 527572 [details] [diff] [review] Patch v1 This no longer tests what the test is supposed to test. If the bug that this is testing regressed, the data: URL script would run soon after you've already finished the test. I *think* a real fix would be waiting for two executeSoon trips.
Attachment #527572 -
Flags: review?(hsivonen) → review-
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #527572 -
Attachment is obsolete: true
Attachment #535602 -
Flags: review?(hsivonen)
Comment 3•13 years ago
|
||
It seems to me that this might test the right thing. What's the purpose of the mutation event listeners? It seems to me it should be enough to use executeSoon enough times without mutation event listeners. (And indeed, the test as written was flaky not so much because of the timer but because the iframe onload handlers end up firing before the script below them has been parsed.)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #535602 -
Attachment is obsolete: true
Attachment #535602 -
Flags: review?(hsivonen)
Attachment #535611 -
Flags: review?(hsivonen)
Assignee | ||
Comment 5•13 years ago
|
||
AFAIUI, the test might not test anything if we don't hit the event loop enough. Henri, if we don't, the test is going to be always green, is that true? Do you have any idea how I could check if the test can actually fail?
Comment 6•13 years ago
|
||
Comment on attachment 535611 [details] [diff] [review] Patch v2.1 As far as I can tell, the way to make the test actually fail upon regression is to back out the fix this is the regression test for and see how many times the test needs to hit the event loop to show failure. 2 times is good enough a guess that I'm marking this r+.
Attachment #535611 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to comment #6) > Comment on attachment 535611 [details] [diff] [review] [review] > Patch v2.1 > > As far as I can tell, the way to make the test actually fail upon regression > is to back out the fix this is the regression test for and see how many > times the test needs to hit the event loop to show failure. 2 times is good > enough a guess that I'm marking this r+. Locally, it's working with 1 but I'm going to use 2 in case of.
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite+
Whiteboard: [needs review] → [fixed in cedar]
Comment 8•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/192ea0cfdcc9
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla7
Comment 9•13 years ago
|
||
Comment on attachment 535611 [details] [diff] [review] Patch v2.1 >+function hitEventLoop(times, next) >+{ >+} Can we put this on SimpleTest already?
Assignee | ||
Comment 10•13 years ago
|
||
(In reply to comment #9) > Comment on attachment 535611 [details] [diff] [review] [review] > Patch v2.1 > > >+function hitEventLoop(times, next) > >+{ > >+} > > Can we put this on SimpleTest already? Ehsan doesn't want to to prevent devs to misuse it and I believe he is right. Though, we could have a plan like always filing if you use it unless you specify a reason, like for the plain in the FlakyTest bug.
Comment 11•13 years ago
|
||
(In reply to comment #10) > (In reply to comment #9) > > Comment on attachment 535611 [details] [diff] [review] [review] [review] > > Patch v2.1 > > > > >+function hitEventLoop(times, next) > > >+{ > > >+} > > > > Can we put this on SimpleTest already? > > Ehsan doesn't want to to prevent devs to misuse it and I believe he is right. > Though, we could have a plan like always filing if you use it unless you > specify a reason, like for the plain in the FlakyTest bug. Nah, I don't think it's a good solution... I'm still kind of ashamed to have come up with this work-around, and I'm not convinced that it's a good solution. For now, we're trying to use the exact same function name in tests which need this so that if one day we decide to put it in SimpleTest, we'd have an easier time.
You need to log in
before you can comment on or make changes to this bug.
Description
•