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

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

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).

Comment 1

[Henri Sivonen (:hsivonen)]

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.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 3

[Henri Sivonen (: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.)

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

Comment 5

[Mounir Lamouri (:mounir)]

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

[Henri Sivonen (:hsivonen)]

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+.

Comment 7

[Mounir Lamouri (:mounir)]

(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.

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/192ea0cfdcc9

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 535611 [details] [diff] [review]
Patch v2.1

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

Can we put this on SimpleTest already?

Comment 10

[Mounir Lamouri (:mounir)]

(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

[(no longer active)]

(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.