Closed Bug 1101856 Opened 5 years ago Closed 5 years ago

jit_test.py and jstests.py swallow assertions by default

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ehoogeveen, Assigned: ehoogeveen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

jit_test.py and jstests.py don't show any output from their tests by default, even failed ones, and unfortunately this also swallows any assertion failure that a test may have reported before crashing. Although test failures are reported, this isn't very helpful if you can't reproduce them locally.

The following patches 1) extend jit_test.py so that it is able to display the output of only failed tests (like jstests.py with -o -F), 2) switch jit_test.py and jstests.py to display the output of failed tests by default, and add a '--no-show-failed' switch to disable this behavior.
This adds '--failed-only' and '--no-show-failed' to js/src/jit-test/jit_test.py and its backend js/src/tests/lib/jittests.py and changes the default behavior to display the output of failed tests. Since its check_output() reports a binary result, this doesn't differentiate between regressions and timeouts. I don't know if that's a problem or if showing the output for timeouts is desirable as well (I figure we mostly care about assertion failures, but maybe knowing how far a test got before being killed could be useful too).
Attachment #8525599 - Flags: review?(sphink)
This changes the behavior of jstests.py in the same way, except that it doesn't display output for timeouts by default (--show-output --failed-only still does).

The conditionals got pretty hairy; I wanted to make sure you could still get the previous behavior. In particular, --show-cmd --failed-only --no-show-failed is the new --show-cmd --failed-only.
Attachment #8525602 - Flags: review?(sphink)
Oh, part 1 also adds some missing pb.beginline() so that the output doesn't start right after the progress bar.
\o/
Blocks: 1101662
Blocks: 745230
jstests in the browser (jsreftest) use a different harness, so won't be affected by this bug. If and when we start running them in the shell (bug 1101662), that will be affected.
Comment on attachment 8525599 [details] [diff] [review]
Part 1: Show the output of failed jit-tests unless '--no-show-failed' is passed.

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

So I only looked at the stdout/stderr logic. I'm assuming that the command line output for --failed-only --show-cmd is correct? (As in, it should only display command lines for failures?)

Even if not, r=me for this patch.

::: js/src/tests/lib/jittests.py
@@ +647,5 @@
> +            show_output = False
> +            if options.show_output and (not ok or not options.failed_only):
> +                show_output = True
> +            if not ok and not options.no_show_failed:
> +                show_output = True

The one weird case is if you pass both --show-output and --no-show-failed. Does that mean you want to see failures' output or not? If not, then I think this would be simpler as:

  if ok:
    show_output = options.show_output and not options.failed_only
  else:
    show_output = not options.no_show_failed

but I think it's probably better to show in that case (it doesn't seem useful to see only successful output), so matching your logic I'd probably say:

  if ok:
    show_output = options.show_output and not options.failed_only
  else:
    show_output = options.show_output or not options.no_show_failed

Then again, I could also see:

  if options.show_output:
    show_output = not (ok and options.fail_only)
  elif not ok:
    show_output = not options.no_show_failed

Hopefully I got all that right. :-)
Attachment #8525599 - Flags: review?(sphink) → review+
Comment on attachment 8525602 [details] [diff] [review]
Part 2: Show the output of failed jstests unless '--no-show-failed' is passed.

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

::: js/src/tests/lib/results.py
@@ +130,5 @@
>              self.groups.setdefault(dev_label, []).append(result.test.path)
>  
> +            show_output = False
> +            if self.options.show_output and (dev_label in ('REGRESSIONS', 'TIMEOUTS') or not self.options.failed_only):
> +                show_output = True

This reads better to me the other way around:

  if self.options.show_output and (not self.options.failed_only or dev_label in ('REGRESSIONS', 'TIMEOUTS')):

mostly because "failed_only" is understandable immediately and so it's easier to figure out that the dev_label part means "if it failed".

I guess you could do that explicitly:

  if self.options.show_output:
    if not self.options.failed_only or dev_label in...:

but I don't like that as well.

@@ +136,5 @@
> +                show_output = True
> +
> +            show_cmd = False
> +            if self.options.show_cmd and (dev_label in ('REGRESSIONS', 'TIMEOUTS') or not self.options.failed_only):
> +                show_cmd = True

Flip this one around too.
Attachment #8525602 - Flags: review?(sphink) → review+
Carrying forward r=sfink.

(In reply to Steve Fink [:sfink, :s:] from comment #7)
> So I only looked at the stdout/stderr logic. I'm assuming that the command
> line output for --failed-only --show-cmd is correct? (As in, it should only
> display command lines for failures?)

As discussed on IRC, --show-cmd is handled up front for jit_test.py, so it doesn't factor into this logic. I adjusted the comment for --failed-only to reflect this. jit_test.py does have --show-failed-cmd, which should be unaffected by this change as well.

> but I think it's probably better to show in that case (it doesn't seem
> useful to see only successful output), so matching your logic I'd probably
> say:
> 
>   if ok:
>     show_output = options.show_output and not options.failed_only
>   else:
>     show_output = options.show_output or not options.no_show_failed

Done, I like that much better (especially how the conditionals line up).
Attachment #8525599 - Attachment is obsolete: true
Attachment #8526968 - Flags: review+
Carrying forward r=sfink.

I flipped the conditionals around to depend on the failure state, as in part 1 v2, which cleaned them up very nicely.

(*) The default is to show the output only for 'regressions' (not timeouts), without showing the commandline.
(*) To also show the commandline for both regressions and timeouts, run with --show-cmd --failed-only. 
(*) To show output for both regressions and timeouts, run with --show-output --failed-only.
(*) To suppress all output, run with --no-show-failed.
(*) To show only commandlines for regressions and timeouts, run with --show-cmd --failed-only --no-show-failed.
(*) If --show-output is passed, --no-show-failed does nothing.
Attachment #8525602 - Attachment is obsolete: true
Attachment #8526995 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d4d9503e64da
https://hg.mozilla.org/mozilla-central/rev/74b738dc7837
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1105204
You need to log in before you can comment on or make changes to this bug.