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

RESOLVED FIXED in mozilla7

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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).
Attachment #527572 - Flags: review?(hsivonen)
(Assignee)

Updated

6 years ago
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-
(Assignee)

Comment 2

6 years ago
Created attachment 535602 [details] [diff] [review]
Patch v2
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.)
(Assignee)

Comment 4

6 years ago
Created attachment 535611 [details] [diff] [review]
Patch v2.1
Attachment #535602 - Attachment is obsolete: true
Attachment #535602 - Flags: review?(hsivonen)
Attachment #535611 - Flags: review?(hsivonen)
(Assignee)

Comment 5

6 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 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

6 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

6 years ago
Flags: in-testsuite+
Whiteboard: [needs review] → [fixed in cedar]

Comment 8

6 years ago
http://hg.mozilla.org/mozilla-central/rev/192ea0cfdcc9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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?
(Assignee)

Comment 10

6 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

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