Last Comment Bug 651902 - Make content/base/test/test_bug592366.html non flaky
: Make content/base/test/test_bug592366.html non flaky
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: FlakyTimeout
  Show dependency treegraph
 
Reported: 2011-04-21 10:18 PDT by Mounir Lamouri (:mounir)
Modified: 2011-05-31 13:35 PDT (History)
3 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (2.13 KB, patch)
2011-04-21 10:18 PDT, Mounir Lamouri (:mounir)
hsivonen: review-
Details | Diff | Splinter Review
Patch v2 (2.38 KB, patch)
2011-05-27 04:03 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.1 (2.01 KB, patch)
2011-05-27 05:42 PDT, Mounir Lamouri (:mounir)
hsivonen: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-04-21 10:18:17 PDT
Created attachment 527572 [details] [diff] [review]
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) 2011-04-25 23:17:14 PDT
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) 2011-05-27 04:03:48 PDT
Created attachment 535602 [details] [diff] [review]
Patch v2
Comment 3 Henri Sivonen (:hsivonen) 2011-05-27 04:41:53 PDT
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) 2011-05-27 05:42:26 PDT
Created attachment 535611 [details] [diff] [review]
Patch v2.1
Comment 5 Mounir Lamouri (:mounir) 2011-05-27 05:44:01 PDT
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) 2011-05-27 10:59:01 PDT
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) 2011-05-30 08:48:49 PDT
(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 9 :Ms2ger (⌚ UTC+1/+2) 2011-05-30 12:58:11 PDT
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) 2011-05-30 16:42:26 PDT
(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 :Ehsan Akhgari 2011-05-31 13:35:25 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.