Last Comment Bug 741487 - Unify the progressbar output between jstest and jittests
: Unify the progressbar output between jstest and jittests
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla17
Assigned To: Terrence Cole [:terrence]
: Jason Orendorff [:jorendorff]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image 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 User image 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 User image 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 User image Josh Matthews [:jdm] 2012-04-14 07:55:27 PDT
Links to source files, please?
Comment 4 User image 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:

You can update the help text by calling methods on OptionParser:
Comment 5 User image Terrence Cole [:terrence] 2012-07-16 18:02:17 PDT
Created attachment 642814 [details] [diff] [review]

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 User image Steve Fink [:sfink] [:s:] 2012-07-24 19:19:30 PDT
Comment on attachment 642814 [details] [diff] [review]

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

::: js/src/tests/lib/
@@ +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 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 User image Terrence Cole [:terrence] 2012-07-25 11:06:02 PDT
Comment 8 User image Ed Morley [:emorley] 2012-07-26 05:10:59 PDT

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