483407 - Improve the "mochitest*" harness

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Serge Gautherie (:sgautherie)]

A few things I noticed: see following patches.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) SimpleTest.js: Report tests which did not actually check anything

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090313 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/6f9cdb6932a2
 +http://hg.mozilla.org/comm-central/rev/bd9dc914d935)

See bug 483391 as an example.

***

Not sure if (near duplicates)
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/ajax/lib/SimpleTest.js
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/SimpleTest.js
want this too ?

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) test.css: whitespace cleanup

Not sure if (near duplicates)
/dom/tests/mochitest/ajax/lib/test.css
/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/test.css
want this too ?

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) TestRunner.js: more consistent updateUI() output (+ minor whitespace+code cleanup)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090313 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/6f9cdb6932a2
 +http://hg.mozilla.org/comm-central/rev/bd9dc914d935)

*Use same partial-green color in header as in details.
*Add a 'ToDo' header case, just in case.
*Failures don't turn Success column red anymore !
 *(That's what made me file this bug.)
*Add some missing spaces in the code...

While there:
*Replace a tab character in the code.
*Remove useless code.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> *Use same partial-green color in header as in details.

In addition,
the initial state of the header is |background-color: green;|.
I think this should be changed to something that more clearly "says" that nothing has run yet.
What would you suggest ?

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/static/harness.css?mark=87,89#86

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 367391 [details] [diff] [review]
(Av1) SimpleTest.js: Report tests which did not actually check anything


See
https://bugzilla.mozilla.org/show_bug.cgi?id=483633#c3
for a short rational.

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 367391 [details] [diff] [review]
(Av1) SimpleTest.js: Report tests which did not actually check anything

>+    if (SimpleTest._tests.length == 0) {
>+      SimpleTest.ok(false, "[SimpleTest.report()] No checks actually run.");
>+    }

I'd like to check this in with 'true' initially:
I would then switch it to 'false' when all tests are fixed.

Nit: I'll remove the superfluous brackets.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support [Checkin: Comment 18 & 19]

Av1, plus:
*s/ok/todo/ until all the tests are fixed.
*Handle a new 'todo_only' css class.
*Toggles:
 *s/tests/checks/g to better differentiate
 *Add 'todo'.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) test.css: Whitespace cleanup, Sync' '#0d0' and ToDo supports [Checkin: Comment 25 & 27]

Bv1, plus:
*s/green/#0d0/
*New 'test_todo' class which was kind of missing.
*s/lime/#0d0/
*New 'todo_only' class for SimpleTest.js use.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a) TestRunner.js: More consistent updateUI() output, Improved timeout handling

Cv1, plus:
*Optimize TestRunner.timeout handling and make its value more explicit.
*In case of too many timeouts,
 *Improve the error message(s).
 *Don't call |TestRunner._checkForHangs|.
*Fix documentation.
*Get rid of superfluous |var finishedURL|.

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) browser-harness.xul: Sync' '#0d0' and improved ToDo supports

Comment 11

[Serge Gautherie (:sgautherie)]

sayrer, or anyone, ping for review?

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 369956 [details] [diff] [review]
(Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support
[Checkin: Comment 18 & 19]

Ping for reviews, anyone?

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 369958 [details] [diff] [review]
(Bv1a) test.css: Whitespace cleanup, Sync' '#0d0' and ToDo supports
[Checkin: Comment 25 & 27]

Please find another reviewer for these other than me.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a) browser-harness.xul: Sync' '#0d0', Improve ToDo support, Make 'No tests' case be a failure, Sync' 'Running' output [Checkin: Comment 26 & 28]

Dv1, plus:
*Make 'No tests logged' be an error, instead of a success.
*Synchronize 'Running <test>...' format, like the other mochitests.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 369956 [details] [diff] [review]
(Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support
[Checkin: Comment 18 & 19]

Sorry, when I marked this as review+, I meant "it's ok with me as long as it's also ok with the other requestees".  If you were intending me to be the only reviewer, you should have said so.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> If you were intending me to be the only reviewer, you should have said so.

Well, that was the (implicit) idea of my (desperate) comment 11 :-|

Do you want that I back it out, or simply wait for some other after-the-fact review?

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I guess it's ok.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 369956 [details] [diff] [review]
(Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support
[Checkin: Comment 18 & 19]


http://hg.mozilla.org/mozilla-central/rev/9cbd47d7b025

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 369956 [details] [diff] [review]
(Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support
[Checkin: Comment 18 & 19]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1891aec77c3e

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1b) TestRunner.js: Improved timeout handling, Check for no checks case, More consistent updateUI() output [Checkin: See comment 22 & 24]

Cva1, plus:
(eventually) replace
http://mxr.mozilla.org/build/source/buildbotcustom/steps/unittest.py
{
437         # Support browser-chrome result summary format which differs from MozillaMochitest's.
438         if self.name != 'mochitest-browser-chrome':
439             if not re.search('TEST-PASS', cmd.logs['stdio'].getText()):
440                 return WARNINGS
}

Comment 21

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 377322 [details] [diff] [review]
(Cv1b) TestRunner.js: Improved timeout handling, Check for no checks case, More consistent updateUI() output
[Checkin: See comment 22 & 24]

this looks pretty good. I don't really hang out in this code very often, but I like what you've done with it.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 377322 [details] [diff] [review]
(Cv1b) TestRunner.js: Improved timeout handling, Check for no checks case, More consistent updateUI() output
[Checkin: See comment 22 & 24]


http://hg.mozilla.org/mozilla-central/rev/c0e396f01848
with 1 "(TestRunner.js)" -> "(SimpleTest/TestRunner.js)".

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 376830 [details] [diff] [review]
(Dv1a) browser-harness.xul: Sync' '#0d0', Improve ToDo support, Make 'No tests' case be a failure, Sync' 'Running' output
[Checkin: Comment 26 & 28]

I tried to come up with some negative comments or nits for this one, but really couldn't. This looks good.

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 377322 [details] [diff] [review]
(Cv1b) TestRunner.js: Improved timeout handling, Check for no checks case, More consistent updateUI() output
[Checkin: See comment 22 & 24]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/81cb8f660fdc

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 369958 [details] [diff] [review]
(Bv1a) test.css: Whitespace cleanup, Sync' '#0d0' and ToDo supports
[Checkin: Comment 25 & 27]


http://hg.mozilla.org/mozilla-central/rev/a61f865efe3a

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 376830 [details] [diff] [review]
(Dv1a) browser-harness.xul: Sync' '#0d0', Improve ToDo support, Make 'No tests' case be a failure, Sync' 'Running' output
[Checkin: Comment 26 & 28]


http://hg.mozilla.org/mozilla-central/rev/b25c21bcde1e

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 369958 [details] [diff] [review]
(Bv1a) test.css: Whitespace cleanup, Sync' '#0d0' and ToDo supports
[Checkin: Comment 25 & 27]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/879ed1c2927c

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 376830 [details] [diff] [review]
(Dv1a) browser-harness.xul: Sync' '#0d0', Improve ToDo support, Make 'No tests' case be a failure, Sync' 'Running' output
[Checkin: Comment 26 & 28]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/62c257ecebe3

Comment 29

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) browser-test.js: Improve Timeout+NoTests+Exception reports, Add NoChecks report

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090613 Minefield/3.6a1pre] (mozilla-central-win32-unittest/...) (W2Ksp4)

This reports 3 "no checks" tests.

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 384293 [details] [diff] [review]
(Ev1) browser-test.js: Improve Timeout+NoTests+Exception reports, Add NoChecks report

>diff --git a/testing/mochitest/browser-test.js b/testing/mochitest/browser-test.js
>+    if (this.currentTestIndex >= 0 && this.currentTest.results.length == 0)

Nit: I'll use |... && !this.currentTest.results.length|.

Comment 31

[Serge Gautherie (:sgautherie)]

rcampbell, ping for review ?

Comment 32

[Rob Campbell [:rc] (:robcee)]

For the future, a few helpful suggestions:

"Improve mochitest* harness" doesn't tell me what you're improving, exactly. Your initial comment doesn't tell me what you intend to do and "see attached patches" requires me to read through a bunch of diffs and comments to figure out your intent.

In the future, please break these up into individual chunks of work. If you need to track a number of patches through to completion, create a dependent "tracking bug" to monitor their progress.

This allows reviewers to not have to refamiliarize themselves with an increasingly large and nebulous bug everytime a new patch arises.

Comment 33

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 384293 [details] [diff] [review]
(Ev1) browser-test.js: Improve Timeout+NoTests+Exception reports, Add NoChecks report

this is fixing problems that don't exist. It's unnecessary and a waste of review time.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Yeah, this bug has long outlived its usefulness - it's just a mess. Please file specifically targetted bugs and attach specifically targetted patches to them instead of keeping this bug rolling for anything vaguely related to test infrastructure.