Closed Bug 651902 Opened 9 years ago Closed 9 years ago

Make content/base/test/test_bug592366.html non flaky

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v1 (obsolete) — 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)
Whiteboard: [needs review]
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-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #527572 - Attachment is obsolete: true
Attachment #535602 - Flags: review?(hsivonen)
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.)
Attached patch Patch v2.1Splinter Review
Attachment #535602 - Attachment is obsolete: true
Attachment #535602 - Flags: review?(hsivonen)
Attachment #535611 - Flags: review?(hsivonen)
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 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+
(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.
Flags: in-testsuite+
Whiteboard: [needs review] → [fixed in cedar]
http://hg.mozilla.org/mozilla-central/rev/192ea0cfdcc9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → mozilla7
Comment on attachment 535611 [details] [diff] [review]
Patch v2.1

>+function hitEventLoop(times, next)
>+{
>+}

Can we put this on SimpleTest already?
(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.
(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.