Closed Bug 615546 Opened 10 years ago Closed 3 years ago

SimpleTest.finish() should be asynchronous

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
mozilla2.0b8

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

Attachments

(2 files)

I fixed window.onerror case in bug 494397.

But asynchronous tests need the same fix,
as most (= all?) of them often call finish() from an observer or a setTimeout() or the like.
We should just make SimpleTest.finish() do the right thing internally, then. (Call a helper function via executeSoon()).
(In reply to comment #1)
> We should just make SimpleTest.finish() do the right thing internally, then.
> (Call a helper function via executeSoon()).

Yeah, that's exactly what I'm currently testing ;-)
Comment on attachment 494155 [details] [diff] [review]
(Av1) SimpleTest.js: Create SimpleTest._finishNow() out of SimpleTest.finish() then call it asynchronously, Fix some (unrelated) nits
[Checked in: See comment 5+10]

- */
+**/

I'd prefer to see comment terminations go the other way. Replace **/ with */.

-              .enablePrivilege("UniversalXPConnect");
-            var tm = Components.classes["@mozilla.org/thread-manager;1"]
-                       .getService(Components.interfaces.nsIThreadManager);
-
+                             .enablePrivilege("UniversalXPConnect");
+            var tm = Components.classes["@mozilla.org/thread-manager;1"].
+                     getService(Components.interfaces.nsIThreadManager);

I think it's more common to put move the . to the next line, as in,
.getService(Components.interfaces.nsIThreadManager);

the query:

http://mxr.mozilla.org/mozilla-central/search?string=executeSoon%28finish

returns 35 results that are currently running executeSoon(finish*) in some variation.

These (modulo the functions that do other tear-down in custom finish functions) will probably need to be updated to just call finish(). I think a follow-up bug to change those tests is in order.

Other than the endings, I like the changes to the comments in this file and calling finish() within an executeSoon() seems like the right thing to do.

r+ with some successful try results and the above nits addressed.

thanks!
Attachment #494155 - Flags: review?(rcampbell) → review+
Comment on attachment 494155 [details] [diff] [review]
(Av1) SimpleTest.js: Create SimpleTest._finishNow() out of SimpleTest.finish() then call it asynchronously, Fix some (unrelated) nits
[Checked in: See comment 5+10]

(In reply to comment #4)
> r+ with some successful try results and the above nits addressed.

http://hg.mozilla.org/try/pushloghtml?changeset=53a04a8c9d81
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/sgautherie.bz@free.fr-53a04a8c9d81
didn't run exactly what I had asked for, but all it run went green :-)


http://hg.mozilla.org/mozilla-central/rev/ab44d15573e2
Av1, with comment 4 suggestion(s).
Attachment #494155 - Attachment description: (Av1) SimpleTest.js: Create SimpleTest._finishNow() out of SimpleTest.finish() then call it asynchronously, Fix some (unrelated) nits. → (Av1) SimpleTest.js: Create SimpleTest._finishNow() out of SimpleTest.finish() then call it asynchronously, Fix some (unrelated) nits [Checked in: See comment 5]
I've backed this out as as soon as it landed bug 592402 started failing far more frequently than normal. Another test (test_fallback.html) also started failing in the changeset after.
Depends on: 615923
Depends on: 592402
(In reply to comment #6)

Based on the builds which have run so far,

> I've backed this out as as soon as it landed bug 592402 started failing far

I'm not convinced about bug 592402 just yet, but could be,

> more frequently than normal. Another test (test_fallback.html) also started
> failing in the changeset after.

Bug 615923 seems obviously caused/revealed by my changeset.

NB: I also checked the Moth failures, which all seemed random/already_there.
Comment on attachment 494155 [details] [diff] [review]
(Av1) SimpleTest.js: Create SimpleTest._finishNow() out of SimpleTest.finish() then call it asynchronously, Fix some (unrelated) nits
[Checked in: See comment 5+10]

(In reply to comment #6)
> I've backed this out

http://hg.mozilla.org/mozilla-central/rev/01bdda1252f0
(In reply to comment #7)
> (In reply to comment #6)
> > I've backed this out as as soon as it landed bug 592402 started failing far
> 
> I'm not convinced about bug 592402 just yet, but could be,

I'm convinced now,
though I'm sure the issue is in that test, not in my harness patch.
Comment on attachment 494155 [details] [diff] [review]
(Av1) SimpleTest.js: Create SimpleTest._finishNow() out of SimpleTest.finish() then call it asynchronously, Fix some (unrelated) nits
[Checked in: See comment 5+10]

Bug 615923 has been fixed (at least the reported failure revealed by this bug).

I have Try-ed a set of patches twice and I got 3-4 oranges each time.
The oranges were all different and already filed.
One of them was bug 592402, but that one is "unrelated" to this bug.
{
Sat Dec 4 11:24:00 2010 PST
http://hg.mozilla.org/try/pushloghtml?changeset=7c12cad2c667
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/sgautherie.bz@free.fr-7c12cad2c667

Sat Dec 4 17:31:59 2010 PST
http://hg.mozilla.org/try/pushloghtml?changeset=54dc1c7b51d9
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/sgautherie.bz@free.fr-54dc1c7b51d9
}

Relanding:
http://hg.mozilla.org/mozilla-central/rev/d1a5599b49fa

If a bug (like bug 592402) is worsen but not obviously "broken" by my patch, please don't back the harness change out, disable the test.
If a bug (like bug 615923) is actually "broken" by my patch, add a blocking bug and I'll check it.
(I'll check the oranges from times to times during this sunday...)
Attachment #494155 - Attachment description: (Av1) SimpleTest.js: Create SimpleTest._finishNow() out of SimpleTest.finish() then call it asynchronously, Fix some (unrelated) nits [Checked in: See comment 5] → (Av1) SimpleTest.js: Create SimpleTest._finishNow() out of SimpleTest.finish() then call it asynchronously, Fix some (unrelated) nits [Checked in: See comment 5+10]
Decouple "(single) TestRunner <-> (all) SimpleTest",
just like patch A decouples "SimpleTest <-> (asynchronous) test".

I don't have an explicit/current case this fixes, but I think this should be an improvement.

This passed on Try per comment 10.
Attachment #495361 - Flags: review?(rcampbell)
(In reply to comment #10)
> (I'll check the oranges from times to times during this sunday...)

Various oranges, but nothing really new :-)
No longer depends on: 592402
Summary: Tests should always call SimpleTest.executeSoon(SimpleTest.finish) rather than SimpleTest.finish() → SimpleTest.finish() should be asynchronous
(In reply to comment #12)
> (In reply to comment #10)
> > (I'll check the oranges from times to times during this sunday...)
> 
> Various oranges, but nothing really new :-)

That doesn't instill a high level of confidence.

I don't like this for a couple of reasons:

1) You've added a patch to a bug after you checked in the fix. This bug should be considered RESO-FIXED.

2) You have no justification for why this is needed. We're trying to remove setTimeouts from the individual tests, not wrap them inside more.

and a bonus, third thing: You're fixing an unrelated nit. (OK, the nit fix is actually pretty good and probably the best part of this patch).
Comment on attachment 495361 [details] [diff] [review]
(Bv1) TestRunner.js: Create TestRunner._runNextTestNow() out of TestRunner.runNextTest() then call it asynchronously, Fix an (unrelated) nit

see above.
Attachment #495361 - Flags: review?(rcampbell) → review-
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #13)
> (In reply to comment #12)
> > Various oranges, but nothing really new :-)
> 
> That doesn't instill a high level of confidence.

Well, that's obviously true for any changeset :-|
Though, hopefully, improving the harness will help to prevent/find/fix some oranges.

*****

> 1) You've added a patch to a bug after you checked in the fix. This bug should
> be considered RESO-FIXED.

Why ? Patch A is only the first part of at least 3 patches: SimpleTest, TestRunner, tests.

> 2) You have no justification for why this is needed. We're trying to remove

I didn't look for an example but I have a justification: make the harness "asynchronous" so tests stop "silently" leaking from one to the next(s), or "stack one on top of the other" in other words.
(I wish each mochitest would run in a "sandbox", as reftests do (iirc) ... But that is not this bug.)

> setTimeouts from the individual tests, not wrap them inside more.

On the contrary, I'm trying to wrap (both asynchronous and synchronous) tests at the harness level, so individual "workarounds" are not needed/missing anymore.

> and a bonus, third thing: You're fixing an unrelated nit. (OK, the nit fix is
> actually pretty good and probably the best part of this patch).

The relation here is that I checked "setTimeout" calls, which is what this bug is about...

How do you plan to handle this then?
(In reply to comment #13)
> We're trying to remove
> setTimeouts from the individual tests, not wrap them inside more.

PS: This is different, iiuc.
a) These have a '0' delay, unlike a test which expect something to happen in the meantime of its delay.
b) I could use MochiKit callLater() instead (everywhere), which is used once in TestRunner, but I wasn't sure whether that would be better or not, so I didn't.
The summary "SimpleTest.finish() should be asynchronous" and the comments in #0 and #1 all refer to SimpleTest.finish() in an executeSoon() block and that's what the first patch did. If there was more to do afterwards, you should have included that before landing the first part.

If there's more work to do, open separate bugs. I don't like having to land units of work in multiple pieces as it's very difficult to ascertain the level of completion across a set of bugs.

Your comments,

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Various oranges, but nothing really new :-)
> > 
> > That doesn't instill a high level of confidence.
> 
> Well, that's obviously true for any changeset :-|
> Though, hopefully, improving the harness will help to prevent/find/fix some
> oranges.

unfortunately. It was a bit of a flippant comment but I don't like landing things that cause more work for people.

> > 1) You've added a patch to a bug after you checked in the fix. This bug should
> > be considered RESO-FIXED.
> 
> Why ? Patch A is only the first part of at least 3 patches: SimpleTest,
> TestRunner, tests.

See above.

> > 2) You have no justification for why this is needed. We're trying to remove
> 
> I didn't look for an example but I have a justification: make the harness
> "asynchronous" so tests stop "silently" leaking from one to the next(s), or
> "stack one on top of the other" in other words.
> (I wish each mochitest would run in a "sandbox", as reftests do (iirc) ... But
> that is not this bug.)

If you could show me a good example of test contamination from one to the next, then I'd consider this. Unfortunately, that's not what you said in the summary of *this* bug!

opening bugs is cheap. The cost of doing indefinite work in individual bugs is much higher.

> > setTimeouts from the individual tests, not wrap them inside more.
> 
> On the contrary, I'm trying to wrap (both asynchronous and synchronous) tests
> at the harness level, so individual "workarounds" are not needed/missing
> anymore.

A noble goal, but what are we going to do about the workarounds that are already in place? (e.g., the 35 executeSoon(finish()) cases I found in mxr for starters)

> > and a bonus, third thing: You're fixing an unrelated nit. (OK, the nit fix is
> > actually pretty good and probably the best part of this patch).
> 
> The relation here is that I checked "setTimeout" calls, which is what this bug
> is about...
> 
> How do you plan to handle this then?

Please open a new bug explaining what you're doing in it with some example tests that illustrate the problem. Having some actual testcases for the harness would be a good thing and limit the amount of guesswork involved.

thanks.
(In reply to comment #13)
> and a bonus, third thing: You're fixing an unrelated nit. (OK, the nit fix is
> actually pretty good and probably the best part of this patch).

Moved that part to bug 617788.
Backed out in http://hg.mozilla.org/mozilla-central/rev/a256e543da3f with a=shaver.

There's absolutely no question but what this turned bug 592402 from something which hadn't happened for nearly three months to something which is a top-10 failure, around #5 last time I looked.

Feel free to reland after you or someone else has figured out why that is and fixed it, but you can't land this workaround for improperly written tests at the cost of turning something that was not failing at all into one of our worst.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 628890
(In reply to comment #10)
> If a bug (like bug 592402) is worsen but not obviously "broken" by my patch,
> please don't back the harness change out, disable the test.

(In reply to comment #19)
> Feel free to reland after you or someone else has figured out why that is and
> fixed it, but you can't land this workaround for improperly written tests at
> the cost of turning something that was not failing at all into one of our
> worst.

We disagree on how to proceed (because I care for this bug whereas noone seems to care for bug 592402), you win:
let's add yet more workarounds (like bug 628890) for now...
Depends on: 592402
(In reply to comment #20)
> (In reply to comment #19)
> > Feel free to reland after you or someone else has figured out why that is and
> > fixed it, but you can't land this workaround for improperly written tests at
> > the cost of turning something that was not failing at all into one of our
> > worst.
> 
> We disagree on how to proceed (because I care for this bug whereas noone seems
> to care for bug 592402), you win:
> let's add yet more workarounds (like bug 628890) for now...

alternative: stop churning on test harness code with no clear direction. None of these are "simple" fixes. All of them change the test harness in subtle ways.

We care a lot more about not breaking what we have during critical release phases (and really, mozilla-central is always in some form of "critical release phase"). I won't be reviewing any more  of these harness fixes until after fx4 and then they will likely be a low-priority.

Alternatively work with someone in release engineering to land these types of fixes in a staging environment. Try server isn't adequate.

feel free to email me if you have any questions.
Blocks: 556687
Blocks: 629230
No longer blocks: 556687
Blocks: 629990
Comment on attachment 495361 [details] [diff] [review]
(Bv1) TestRunner.js: Create TestRunner._runNextTestNow() out of TestRunner.runNextTest() then call it asynchronously, Fix an (unrelated) nit

The nit is still there:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#184

***

The asynchronous part has been superseded by bug 642175 + bug 666636, hasn't it?

Ftr, it seems the later missed to update SimpleTest.executeSoon() per the comment added by the former.
(In reply to Serge Gautherie (:sgautherie) from comment #18)
> Moved that part to bug 617788.

(In reply to Serge Gautherie (:sgautherie) from comment #22)
> The nit is still there:

I had forgotten comment 18.
Mass closing mochitest bugs that haven't had activity in the past 5 years. Please re-open or file a new bug with modern context if this is still relevant.
Status: REOPENED → RESOLVED
Closed: 10 years ago3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.