Improve the "mochitest*" harness

RESOLVED FIXED in mozilla1.9.2a1

Status

defect
--
minor
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Depends on 1 bug, {meta})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1b99: Av1a, Bv1a, Cv1b2, Dv1a])

Attachments

(5 attachments, 5 obsolete attachments)

3.60 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.24 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
2.92 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
6.82 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
6.61 KB, patch
rcampbell
: review-
Details | Diff | Splinter Review
A few things I noticed: see following patches.
[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 ?
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #367391 - Flags: review?(sayrer)
Not sure if (near duplicates)
/dom/tests/mochitest/ajax/lib/test.css
/dom/tests/mochitest/ajax/mochikit/tests/SimpleTest/test.css
want this too ?
Attachment #367397 - Flags: review?(sayrer)
[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.
Attachment #367406 - Flags: review?(sayrer)
(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
Depends on: 483391
Depends on: 451949
Depends on: 483624
Depends on: 483633
Depends on: 483635
Depends on: 483967
Depends on: 483975
Depends on: 483978
Depends on: 483981
Depends on: 483985
Depends on: 483989
Depends on: 483992
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.
Depends on: 443763
Depends on: 461710
Depends on: 476406
Depends on: 462106
Depends on: 431503
Depends on: 485380
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.
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'.
Attachment #367391 - Attachment is obsolete: true
Attachment #369956 - Flags: review?(sayrer)
Attachment #367391 - Flags: review?(sayrer)
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.
Attachment #367397 - Attachment is obsolete: true
Attachment #369958 - Flags: review?(sayrer)
Attachment #367397 - Flags: review?(sayrer)
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|.
Attachment #367406 - Attachment is obsolete: true
Attachment #369959 - Flags: review?(sayrer)
Attachment #367406 - Flags: review?(sayrer)
No longer depends on: 485380
Component: General → Mochitest
QA Contact: general → mochitest
Depends on: 486254
Depends on: 485175
Depends on: 479352, 424974
sayrer, or anyone, ping for review?
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?
Attachment #369956 - Flags: review?(rcampbell)
Attachment #369956 - Flags: review?(jwalden+bmo)
Attachment #369956 - Flags: review?(dbaron)
Attachment #369956 - Flags: review?(dbaron) → review+
Attachment #369958 - Flags: review?(dbaron)
Attachment #369959 - Flags: review?(dbaron)
Attachment #369960 - Flags: review?(dbaron)
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.
Attachment #369958 - Flags: review?(dbaron) → review?
Attachment #369959 - Flags: review?(dbaron) → review?
Attachment #369960 - Flags: review?(dbaron) → review?
Attachment #369959 - Flags: review? → review?(rcampbell)
Attachment #369960 - Flags: review? → review?(jwalden+bmo)
Dv1, plus:
*Make 'No tests logged' be an error, instead of a success.
*Synchronize 'Running <test>...' format, like the other mochitests.
Attachment #369960 - Attachment is obsolete: true
Attachment #376830 - Flags: review?(sayrer)
Attachment #376830 - Flags: review?(jwalden+bmo)
Attachment #369960 - Flags: review?(sayrer)
Attachment #369960 - Flags: review?(jwalden+bmo)
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.
(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?
Whiteboard: [Looking for any reviewer for each of the patches!]
Depends on: 492467
Depends on: 492476
Depends on: 490384
Depends on: 492477
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
Attachment #369956 - Attachment description: (Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support → (Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support [Checkin: Comment 18]
Attachment #369956 - Flags: review?(sayrer)
Attachment #369956 - Flags: review?(rcampbell)
Attachment #369956 - Flags: review?(jwalden+bmo)
Depends on: 426546
Depends on: 492481
Depends on: 492482
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
Attachment #369956 - Attachment description: (Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support [Checkin: Comment 18] → (Av1a) SimpleTest.js: Report tests which did not actually check anything, Improve ToDo support [Checkin: Comment 18 & 19]
Whiteboard: [Looking for any reviewer for each of the patches!] → [Looking for any reviewer for each of the patches!] [fixed1.9.1b5: Av1a]
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
}
Attachment #369959 - Attachment is obsolete: true
Attachment #377322 - Flags: review?(sayrer)
Attachment #377322 - Flags: review?(rcampbell)
Attachment #369959 - Flags: review?(sayrer)
Attachment #369959 - Flags: review?(rcampbell)
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.
Attachment #377322 - Flags: review?(rcampbell) → review+
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)".
Attachment #377322 - Attachment description: (Cv1b) TestRunner.js: Improved timeout handling, Check for no checks case, More consistent updateUI() output → (Cv1b) TestRunner.js: Improved timeout handling, Check for no checks case, More consistent updateUI() output [Checkin: See comment 22]
Attachment #377322 - Flags: review?(sayrer)
Attachment #369958 - Flags: review? → review?(rcampbell)
Attachment #376830 - Flags: review?(rcampbell)
Attachment #369958 - Flags: review?(rcampbell) → review+
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.
Attachment #376830 - Flags: review?(rcampbell) → review+
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
Attachment #377322 - Attachment description: (Cv1b) TestRunner.js: Improved timeout handling, Check for no checks case, More consistent updateUI() output [Checkin: See comment 22] → (Cv1b) TestRunner.js: Improved timeout handling, Check for no checks case, More consistent updateUI() output [Checkin: See comment 22 & 24]
Whiteboard: [Looking for any reviewer for each of the patches!] [fixed1.9.1b5: Av1a] → [fixed1.9.1b5: Av1a, Cv1b2]
Attachment #369958 - Attachment description: (Bv1a) test.css: Whitespace cleanup, Sync' '#0d0' and ToDo supports → (Bv1a) test.css: Whitespace cleanup, Sync' '#0d0' and ToDo supports [Checkin: Comment 25]
Attachment #369958 - Flags: review?(sayrer)
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 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
Attachment #376830 - Attachment description: (Dv1) browser-harness.xul: Sync' '#0d0' and improved ToDo supports → (Dv1a) browser-harness.xul: Sync' '#0d0', Improve ToDo support, Make 'No tests' case be a failure, Sync' 'Running' output [Checkin: Comment 26]
Attachment #376830 - Flags: review?(sayrer)
Attachment #376830 - Flags: review?(jwalden+bmo)
Flags: in-testsuite-
Keywords: meta
Whiteboard: [fixed1.9.1b5: Av1a, Cv1b2] → [Leave open until I may fix some more] [fixed1.9.1b5: Av1a, Bv1a, Cv1b2, Dv1a]
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
Attachment #369958 - Attachment description: (Bv1a) test.css: Whitespace cleanup, Sync' '#0d0' and ToDo supports [Checkin: Comment 25] → (Bv1a) test.css: Whitespace cleanup, Sync' '#0d0' and ToDo supports [Checkin: Comment 25 & 27]
Attachment #376830 - Attachment description: (Dv1a) browser-harness.xul: Sync' '#0d0', Improve ToDo support, Make 'No tests' case be a failure, Sync' 'Running' output [Checkin: Comment 26] → (Dv1a) browser-harness.xul: Sync' '#0d0', Improve ToDo support, Make 'No tests' case be a failure, Sync' 'Running' output [Checkin: Comment 26 & 28]
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
Depends on: 494397
No longer depends on: 494397
Depends on: 483555
Depends on: 451287
Whiteboard: [Leave open until I may fix some more] [fixed1.9.1b5: Av1a, Bv1a, Cv1b2, Dv1a] → [Leave open until I may fix some more] [fixed1.9.1b99: Av1a, Bv1a, Cv1b2, Dv1a]
Blocks: 499435
[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.
Attachment #384293 - Flags: review?(rcampbell)
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|.
Depends on: 502603
Depends on: 481437
Blocks: 481437
No longer depends on: 481437
Depends on: 502562
rcampbell, ping for review ?
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 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.
Attachment #384293 - Flags: review?(rcampbell) → review-
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Leave open until I may fix some more] [fixed1.9.1b99: Av1a, Bv1a, Cv1b2, Dv1a] → [fixed1.9.1b99: Av1a, Bv1a, Cv1b2, Dv1a]
No longer blocks: 483992
You need to log in before you can comment on or make changes to this bug.