1164121 - Add |mach| command to run jsapi-tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Rahm [:erahm]]

It would be useful to be able to run the jsapi-tests under |js/src/jsapi-tests| via |mach|. Being able to run just one test or a subset would be helpful as well.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Came up in the context of a new contributor who wanted to run those tests; any chance you could find someone?

Comment 2

[Eric Rahm [:erahm]]

Attachment: Add |mach| command to run jsapi-tests

Provides a |mach jsapi-tests <optional_test_name>| command. :sfink would you
mind reviewing this or redirecting?

Comment 3

[Mike Hoye [:mhoye]]

Welp, sorry I didn't get back to this faster. 

Whiteboarding stuff as [good first bug] is a good way to get my attention, next time.

Comment 4

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

Comment on attachment 8606370 [details] [diff] [review]
Add |mach| command to run jsapi-tests

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

I don't know if my review is considered adequate here, but I'm fine with claiming that it is.

::: testing/mach_commands.py
@@ +337,5 @@
> +             'test suite is executed.')
> +
> +    def run_jsapitests(self, **params):
> +        import subprocess
> +        import sys

I know check_spidermonkey does this too, but I don't see the point of re-importing 'sys' when it's already imported at the top of the file and other stuff in this file uses it.

@@ +341,5 @@
> +        import sys
> +
> +        bin_suffix = ''
> +        if sys.platform.startswith('win'):
> +            bin_suffix = '.exe'

This is ugly enough if it occurs once. For twice, it really needs to be pulled out into a helper. For now, just

  def executable_name(name):
    return name + '.exe' if sys.platform.startswith('win') else name

seems good enough (or a more verbose expansion if you find it to be more clear.) Though it really feels like it ought to be in some helper lib, but it's not worth it for this change.

Comment 5

[Eric Rahm [:erahm]]

(In reply to Steve Fink [:sfink, :s:] from comment #4)
> Comment on attachment 8606370 [details] [diff] [review]
> Add |mach| command to run jsapi-tests
> 
> Review of attachment 8606370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know if my review is considered adequate here, but I'm fine with
> claiming that it is.
> 
> ::: testing/mach_commands.py
> @@ +337,5 @@
> > +             'test suite is executed.')
> > +
> > +    def run_jsapitests(self, **params):
> > +        import subprocess
> > +        import sys
> 
> I know check_spidermonkey does this too, but I don't see the point of
> re-importing 'sys' when it's already imported at the top of the file and
> other stuff in this file uses it.

Good point, I assumed check_spidermonkey was doing the right thing.

> @@ +341,5 @@
> > +        import sys
> > +
> > +        bin_suffix = ''
> > +        if sys.platform.startswith('win'):
> > +            bin_suffix = '.exe'
> 
> This is ugly enough if it occurs once. For twice, it really needs to be
> pulled out into a helper. For now, just
> 
>   def executable_name(name):
>     return name + '.exe' if sys.platform.startswith('win') else name
> 
> seems good enough (or a more verbose expansion if you find it to be more
> clear.) Though it really feels like it ought to be in some helper lib, but
> it's not worth it for this change.

I'll add this and update check_spidermonkey and jsapitests.

Comment 6

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75cae071bebc

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/75cae071bebc