Closed
Bug 483407
Opened 16 years ago
Closed 15 years ago
Improve the "mochitest*" harness
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: sgautherie, Assigned: sgautherie)
References
Details
(Keywords: meta, Whiteboard: [fixed1.9.1b99: Av1a, Bv1a, Cv1b2, Dv1a])
Attachments
(5 files, 5 obsolete files)
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rcampbell
:
review-
|
Details | Diff | Splinter Review |
A few things I noticed: see following patches.
Assignee | ||
Comment 1•16 years ago
|
||
[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 | ||
Comment 2•16 years ago
|
||
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)
Assignee | ||
Comment 3•16 years ago
|
||
[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)
Assignee | ||
Comment 4•16 years ago
|
||
(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
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
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)
Assignee | ||
Comment 8•16 years ago
|
||
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)
Assignee | ||
Comment 9•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Component: General → Mochitest
QA Contact: general → mochitest
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Comment 12•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #369956 -
Flags: review?(dbaron) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #369958 -
Flags: review?(dbaron)
Assignee | ||
Updated•16 years ago
|
Attachment #369959 -
Flags: review?(dbaron)
Assignee | ||
Updated•16 years ago
|
Attachment #369960 -
Flags: review?(dbaron)
Comment 13•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #369959 -
Flags: review?(dbaron) → review?
Updated•16 years ago
|
Attachment #369960 -
Flags: review?(dbaron) → review?
Assignee | ||
Updated•16 years ago
|
Attachment #369959 -
Flags: review? → review?(rcampbell)
Assignee | ||
Updated•16 years ago
|
Attachment #369960 -
Flags: review? → review?(jwalden+bmo)
Assignee | ||
Comment 14•16 years ago
|
||
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 15•16 years ago
|
||
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.
Assignee | ||
Comment 16•16 years ago
|
||
(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!]
Assignee | ||
Comment 18•16 years ago
|
||
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)
Assignee | ||
Comment 19•16 years ago
|
||
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]
Assignee | ||
Updated•16 years ago
|
Whiteboard: [Looking for any reviewer for each of the patches!] → [Looking for any reviewer for each of the patches!] [fixed1.9.1b5: Av1a]
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Comment 20•16 years ago
|
||
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 21•16 years ago
|
||
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+
Assignee | ||
Comment 22•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #369958 -
Flags: review? → review?(rcampbell)
Assignee | ||
Updated•16 years ago
|
Attachment #376830 -
Flags: review?(rcampbell)
Updated•16 years ago
|
Attachment #369958 -
Flags: review?(rcampbell) → review+
Comment 23•16 years ago
|
||
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+
Assignee | ||
Comment 24•16 years ago
|
||
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]
Assignee | ||
Updated•16 years ago
|
Whiteboard: [Looking for any reviewer for each of the patches!] [fixed1.9.1b5: Av1a] → [fixed1.9.1b5: Av1a, Cv1b2]
Assignee | ||
Updated•16 years ago
|
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)
Assignee | ||
Comment 25•16 years ago
|
||
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
Assignee | ||
Comment 26•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
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]
Assignee | ||
Comment 27•16 years ago
|
||
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]
Assignee | ||
Updated•16 years ago
|
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]
Assignee | ||
Comment 28•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
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]
Assignee | ||
Comment 29•16 years ago
|
||
[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)
Assignee | ||
Comment 30•16 years ago
|
||
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|.
Assignee | ||
Updated•15 years ago
|
Comment 32•15 years ago
|
||
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•15 years ago
|
||
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-
Comment 34•15 years ago
|
||
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: 15 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]
You need to log in
before you can comment on or make changes to this bug.
Description
•