1286711 - jstests.py with only slow tests to run throws TypeError

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Leo Gaspard [:ekleog]]

Attachment: warn-no-tests.patch

When running js/src/tests/jstests.py with an explicit list of tests and the only ones are slow tests, it has no result object and throws a TypeError:

Traceback (most recent call last):
  File "../tests/jstests.py", line 380, in <module>
    sys.exit(main())
  File "../tests/jstests.py", line 371, in main
    results.finish(True)
  File ".../js/src/tests/lib/results.py", line 197, in finish
    self.pb.finish(completed)
  File ".../js/src/tests/lib/progressbar.py", line 86, in finish
    self.update(final_count, self.prior[1])
TypeError: 'NoneType' object has no attribute '__getitem__'


There are two ways to fix this, either by forcing the run of explicitly mentioned test cases, or by outputting an explicit and helpful error message. The attached patch tries to implement the second option.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8770756 [details] [diff] [review]
warn-no-tests.patch

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

Awesome! Thanks for the patch.

::: js/src/tests/lib/progressbar.py
@@ +87,5 @@
> +                             '--run-slow-tests or --run-skipped to run more tests\n')
> +        else:
> +            final_count = self.limit if complete else self.prior[0]
> +            self.update(final_count, self.prior[1])
> +            sys.stdout.write('\n')

Else with nothing after it is a bit of an anti-pattern in SpiderMonkey. Instead of lifting the whole body into an if/else, SpiderMonkey prefers to use the pattern of early-return to avoid the extra indentation.

if condition:
    ... handle condition ...
    return
... rest of body ...

Comment 2

[Leo Gaspard [:ekleog]]

Attachment: warn-no-tests-v2.patch

Thanks! Here is an updated version of the patch.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8771149 [details] [diff] [review]
warn-no-tests-v2.patch

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

Yup!

Note that it's fine to land a patch with an r+ if all that's requested are a few minor syntactical fixes. Generally if a reviewer wants to see a patch again they'll cancel review instead of giving r+. (At least this is true on our team. Other teams use r- for this, whereas in SpiderMonkey r- usually means vigorous disagreement with the approach). Sometimes a patch requires a substantial rewrite after an r+ because of issues only discovered on mozilla-inbound (intermittent failures, for example). In this case it is sometimes desirable to get a re-review, although it depends on the intricacy and importance of the patch in question. For the test suite, this bar is especially low.

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 8771149 [details] [diff] [review]
warn-no-tests-v2.patch

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

Heh, I've never actually seen the checkin flag! I don't know what it's intended to do here. The patch looks good. I'll set checkin-needed on the bug, which will get the patch landed later today semi-automatically by a helpful soul.

Comment 5

[Sean Stangl [:sstangl]]

Note that it's not idiomatic python to construct multiline strings as 'foo' + 'bar': instead, you can just do:

> ('foo'
> ' bar')

which produces, careful of the spacing:

> ('foo bar')

But it doesn't really matter.

Comment 6

[Leo Gaspard [:ekleog]]

Attachment: warn-no-tests-v3.patch

Here is the style fix, thanks for the pointer!

Comment 7

[Leo Gaspard [:ekleog]]

Attachment: warn-no-tests-v4.patch

Updated patch with the metadata

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e9af18d314
Warn the user when no tests have run, before the TypeError. r=sstangl, a=KWierso

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54e9af18d314