Display test failures immediately

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

It would be nice to see test failures immediately rather than just a counter.
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.
Attachment #626692 - Flags: review?(terrence)

Comment 2

5 years ago
Oh goodness, yes, please!  I've been meaning to ask for this for a year.
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")?
Attachment #626692 - Flags: review?(terrence) → review+
(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?
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/44921eaaa4e2
Target Milestone: --- → mozilla15
Duplicate of this bug: 745252

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/44921eaaa4e2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.