Last Comment Bug 741487 - Unify the progressbar output between jstest and jittests
: Unify the progressbar output between jstest and jittests
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on:
Blocks: 745237 776184
  Show dependency treegraph
 
Reported: 2012-04-02 11:39 PDT by Terrence Cole [:terrence]
Modified: 2012-07-26 05:10 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (3.64 KB, patch)
2012-07-16 18:02 PDT, Terrence Cole [:terrence]
sphink: review+
Details | Diff | Review

Description Terrence Cole [:terrence] 2012-04-02 11:39:18 PDT
Each test harness puts out a different set of numbers, parts of which appear to be basically useless.  The 3rd number in the jstests suite is skipped tests: a number that nobody cares about.  The last number in the jittests suite appears to be the total number of tests run: another number nobody cares about.

IMHO, the only numbers that should show up are "tests failed that should not have failed" and "tests passed that should not have passed".  This would make it very easy and intuitive to check the status of a test run: not zero == not good.

We should also print a legend for these numbers to the help text.
Comment 1 Bill McCloskey (:billm) 2012-04-02 11:41:26 PDT
jit-tests also includes information about timeouts. I find this useful when running with gczeal. Lots of tests time out, but I'm really only interested in whether actual failures have taken place. It would be nice if we could preserve that.
Comment 2 Terrence Cole [:terrence] 2012-04-02 14:50:42 PDT
Ah, I did not know that jit-tests did that.  I think we should definitely make jstests do that as well.
Comment 3 Josh Matthews [:jdm] 2012-04-14 07:55:27 PDT
Links to source files, please?
Comment 4 Terrence Cole [:terrence] 2012-04-16 10:13:48 PDT
Given that I've marked this as a [good first bug] I should also probably say what we need here :-).  What we would like to have is:
[ nun-unexpected-fail-or-pass | num-timeout ]

This is where we update the counters that get displayed:
http://mxr.mozilla.org/mozilla-central/source/js/src/tests/results.py#115

You can update the help text by calling methods on OptionParser:
http://mxr.mozilla.org/mozilla-central/source/js/src/tests/jstests.py#55
Comment 5 Terrence Cole [:terrence] 2012-07-16 18:02:17 PDT
Created attachment 642814 [details] [diff] [review]
v0

I think all we want here is to add a fourth field (in the 3rd position) for timeouts.  I'm going to leave the SKIP count in the last position, because this is more use than the total count that jittest shows.
Comment 6 Steve Fink [:sfink] [:s:] 2012-07-24 19:19:30 PDT
Comment on attachment 642814 [details] [diff] [review]
v0

Review of attachment 642814 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/lib/results.py
@@ +87,5 @@
>          self.n = 0
>  
>          self.pb = None
>          if not options.hide_progress:
> +            self.pb = ProgressBar('', testcount, 21)

I know this is picky, but any chance you could replace this magic number with something based on $COLUMNS (if set, else 80 or something)? It's fine if you just want to land it. (And no, I won't ask you to handle SIGWINCH or whatever.)

Wait! No! Never mind, this is the label width. The thing I'm talking about is currently a hardcoded constant 64 in progressbar.py. Ok, so let me change this request to use len('[%4d|%4d|%4d|%4d]' % (0,0,0,0)) instead. I just don't like the magic number.

@@ +123,2 @@
>              else:
> +                self.counts['SKIP'] += 1

Much, much nicer
Comment 7 Terrence Cole [:terrence] 2012-07-25 11:06:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b77354a5a40
Comment 8 Ed Morley [:emorley] 2012-07-26 05:10:59 PDT
https://hg.mozilla.org/mozilla-central/rev/0b77354a5a40

Note You need to log in before you can comment on or make changes to this bug.