SimpleTest.finish() should be asynchronous

REOPENED
Assigned to

Status

Testing
Mochitest
REOPENED
7 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 2

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

Comment 3

7 years ago
Created 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]

http://hg.mozilla.org/try/pushloghtml?changeset=4dae54e5169c
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/sgautherie.bz@free.fr-4dae54e5169c

There seems to be some "infrastructure" issues on (linux*) Try atm,
then I'll try it again later.

Yet, there doesn't seem to be any test issue, other than the usual random oranges.
Attachment #494155 - Flags: review?(rcampbell)
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+
(Assignee)

Comment 5

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

Updated

7 years ago
Depends on: 615923
(Assignee)

Updated

7 years ago
Depends on: 592402
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

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

Comment 10

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

Comment 11

7 years ago
Created attachment 495361 [details] [diff] [review]
(Bv1) TestRunner.js: Create TestRunner._runNextTestNow() out of TestRunner.runNextTest() then call it asynchronously, Fix an (unrelated) nit

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)
(Assignee)

Comment 12

7 years ago
(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
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

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

Comment 16

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

Comment 18

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

Updated

7 years ago
Blocks: 628890
(Assignee)

Comment 20

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

Updated

7 years ago
Blocks: 556687
(Assignee)

Updated

7 years ago
Blocks: 629230
No longer blocks: 556687
(Assignee)

Updated

7 years ago
Blocks: 629990
(Assignee)

Comment 22

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

Comment 23

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.