Closed Bug 1286711 Opened 3 years ago Closed 3 years ago

jstests.py with only slow tests to run throws TypeError

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ekleog, Assigned: ekleog, Mentored)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch warn-no-tests.patch (obsolete) — Splinter Review
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.
Attachment #8770756 - Flags: review?(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 ...
Attachment #8770756 - Flags: review?(terrence) → review+
Attached patch warn-no-tests-v2.patch (obsolete) — Splinter Review
Thanks! Here is an updated version of the patch.
Attachment #8771149 - Flags: review?(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.
Attachment #8771149 - Flags: review?(terrence) → review+
Attachment #8771149 - Flags: checkin?(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.
Attachment #8771149 - Flags: checkin?(sstangl) → checkin+
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.
Attached patch warn-no-tests-v3.patch (obsolete) — Splinter Review
Here is the style fix, thanks for the pointer!
Attachment #8770756 - Attachment is obsolete: true
Attachment #8771149 - Attachment is obsolete: true
Updated patch with the metadata
Attachment #8771548 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e9af18d314
Warn the user when no tests have run, before the TypeError. r=sstangl, a=KWierso
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54e9af18d314
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.