Closed Bug 1164121 Opened 5 years ago Closed 5 years ago

Add |mach| command to run jsapi-tests

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: erahm)

Details

Attachments

(1 file)

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.
Came up in the context of a new contributor who wanted to run those tests; any chance you could find someone?
Flags: needinfo?(mhoye)
Provides a |mach jsapi-tests <optional_test_name>| command. :sfink would you
mind reviewing this or redirecting?
Attachment #8606370 - Flags: review?(sphink)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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.
Flags: needinfo?(mhoye)
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.
Attachment #8606370 - Flags: review?(sphink) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/75cae071bebc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.