758108 - Display test failures immediately

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

It would be nice to see test failures immediately rather than just a counter.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: 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]]

Oh goodness, yes, please!  I've been meaning to ask for this for a year.

Comment 3

[Terrence Cole [:terrence]]

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:]]

(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]]

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:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44921eaaa4e2

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/44921eaaa4e2