Last Comment Bug 758108 - Display test failures immediately
: Display test failures immediately
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Steve Fink [:sfink] [:s:]
:
:
Mentors:
: 745252 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 21:37 PDT by Steve Fink [:sfink] [:s:]
Modified: 2012-05-25 08:22 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Display test failures immediately (1.50 KB, patch)
2012-05-23 21:37 PDT, Steve Fink [:sfink] [:s:]
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2012-05-23 21:37:04 PDT
It would be nice to see test failures immediately rather than just a counter.
Comment 1 Steve Fink [:sfink] [:s:] 2012-05-23 21:37:13 PDT
Created attachment 626692 [details] [diff] [review]
Display test failures immediately

Ok, I'll admit I haven't actually tested this with any nondefault options
(--tinderbox etc.), nor thought too hard about whether this is really the right thing. But it's working for my basic use case, and I thought you might find it useful even if it's too low priority to review for a while.
Comment 2 Luke Wagner [:luke] 2012-05-24 01:20:00 PDT
Oh goodness, yes, please!  I've been meaning to ask for this for a year.
Comment 3 Terrence Cole [:terrence] 2012-05-24 11:13:14 PDT
Comment on attachment 626692 [details] [diff] [review]
Display test failures immediately

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

I no longer have hope that Bug 745252 is going to get done, so lets go ahead and do it here.

::: js/src/tests/lib/results.py
@@ +131,5 @@
>                  self.print_tinderbox_result(self.LABELS[
>                      (result.result, result.test.expect, result.test.random)][0],
>                      result.test.path, time=output.dt)
> +            else:
> +                if dev_label:

Tinderbox mode should always be with no progress bar, so just return at the end of the if tinderbox section, then put the if dev_lable in the main else block afterwards.  I had a patch to clean this all up, but missed an edge case, then got distracted.

@@ +133,5 @@
>                      result.test.path, time=output.dt)
> +            else:
> +                if dev_label:
> +                    if self.pb:
> +                        self.fp.write("\n")

Isn't this going to leave the existing progressbar state on the current line?  How about you make this something like:  self.fp.write("\033[1K\r")?
Comment 4 Steve Fink [:sfink] [:s:] 2012-05-24 12:05:13 PDT
(In reply to Terrence Cole [:terrence] from comment #3)
> Comment on attachment 626692 [details] [diff] [review]
> Display test failures immediately
> 
> Review of attachment 626692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I no longer have hope that Bug 745252 is going to get done, so lets go ahead
> and do it here.

Ah, sorry. I had a vague memory that I'd seen a bug for this, but didn't bother to search.

> ::: js/src/tests/lib/results.py
> @@ +131,5 @@
> >                  self.print_tinderbox_result(self.LABELS[
> >                      (result.result, result.test.expect, result.test.random)][0],
> >                      result.test.path, time=output.dt)
> > +            else:
> > +                if dev_label:
> 
> Tinderbox mode should always be with no progress bar, so just return at the
> end of the if tinderbox section, then put the if dev_lable in the main else
> block afterwards.  I had a patch to clean this all up, but missed an edge
> case, then got distracted.

Ok.

> @@ +133,5 @@
> >                      result.test.path, time=output.dt)
> > +            else:
> > +                if dev_label:
> > +                    if self.pb:
> > +                        self.fp.write("\n")
> 
> Isn't this going to leave the existing progressbar state on the current
> line?  How about you make this something like:  self.fp.write("\033[1K\r")?

Yes. But (1) as you mentioned in bug 745252, escape sequences are tricky, and I'd much rather have that sort of thing encapsulated in the ProgressBar module; and (2) I kind of like leaving it there. It tells me how far through the run the failure happened (both in test count and time). Admittedly, I could just add that info to the output line, but keeping it the same format as the progress bar is kind of nice.

On the other hand, it does look messy. I'll compare:

[   1|   0|   9]  13% =====>                                          |    0.1s
REGRESSION - js1_8_5/extensions/dataview.js
[   1|   1|   9]  14% ======>                                         |    0.1s
REGRESSION - js1_8_5/extensions/typedarray.js
[   3|   2|   9]  18% =======>                                        |    0.2s
REGRESSION - js1_8_5/extensions/clone-typed-array.js
[   7|   3|   9]  25% ===========>                                    |    0.4s
REGRESSION - js1_8_5/extensions/typedarray-subarray-of-subarray.js
[   8|   4|   9]  28% ============>                                   |    0.5s
REGRESSION - js1_8_5/extensions/worker-terminate.js
[  17|   5|   9]  41% ==================>                             |    0.8s
REGRESSION - js1_8_5/extensions/worker-fib.js
[  60|   6|   9] 100% ===============================================>|    9.0s
REGRESSIONS
    js1_8_5/extensions/dataview.js
    js1_8_5/extensions/typedarray.js
    js1_8_5/extensions/clone-typed-array.js
    js1_8_5/extensions/typedarray-subarray-of-subarray.js
    js1_8_5/extensions/worker-terminate.js
    js1_8_5/extensions/worker-fib.js
FAIL

---------------- vs -------------

REGRESSION - js1_8_5/extensions/dataview.js
REGRESSION - js1_8_5/extensions/typedarray.js
REGRESSION - js1_8_5/extensions/clone-typed-array.js
REGRESSION - js1_8_5/extensions/typedarray-subarray-of-subarray.js
REGRESSION - js1_8_5/extensions/worker-terminate.js
REGRESSION - js1_8_5/extensions/worker-fib.js
[  60|   6|   9] 100% ===============================================>|    9.0s
REGRESSIONS
    js1_8_5/extensions/dataview.js
    js1_8_5/extensions/typedarray.js
    js1_8_5/extensions/clone-typed-array.js
    js1_8_5/extensions/typedarray-subarray-of-subarray.js
    js1_8_5/extensions/worker-terminate.js
    js1_8_5/extensions/worker-fib.js
FAIL

Any votes?
Comment 5 Terrence Cole [:terrence] 2012-05-24 15:10:29 PDT
You are also right: my solution would print garbage when redirected, since we don't check if fp.isatty() first.

I vote that we should turn segments of the progressbar in which a test failure occurred red.  I'm fine with your version until we get such an ability.
Comment 6 Steve Fink [:sfink] [:s:] 2012-05-24 22:10:40 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/44921eaaa4e2
Comment 7 Steve Fink [:sfink] [:s:] 2012-05-24 22:11:18 PDT
*** Bug 745252 has been marked as a duplicate of this bug. ***
Comment 8 Ed Morley [:emorley] 2012-05-25 08:22:19 PDT
https://hg.mozilla.org/mozilla-central/rev/44921eaaa4e2

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