add ability to run a subset of reftests from a manifest

RESOLVED FIXED in mozilla20

Status

Testing
Reftest
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

Trunk
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
mach currently takes a reftest manifest file, just like the reftest make target does.  I often want to run a subset of tests from a manifest file, and I would tend to temporarily comment out the irrelevant tests from the file.  Given that URLs in a manifest are not necessarily unique, I'm thinking it might be good to be able to specify a pattern to match the test item URLs against.

  $ ./mach reftest layout/reftest/svg/reftest.list 'use-0[1-4]'
  [ runs each test in layout/reftest/svg/reftest.list, or included from there,
    that has a test or reference URL that matches /use-0[1-4]/ ]

I say just use a JS regular expression string (as you could pass to the RegExp constructor).  The question then is whether to match against just what's listed in the manifest, or whether the url-prefix should be added in before matching, or whether (if it is a relative reference) it should match against the pathname underneath layout/reftests/.

gps, dbaron, WDYT?
(Assignee)

Comment 1

5 years ago
Created attachment 679039 [details] [diff] [review]
patch

Here's a patch to do it.  The regular expression matches against the resolved URL (e.g. http://mochi.test/blah/blah/reftests/svg/whatever.svg).
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #679039 - Flags: review?(gps)
Attachment #679039 - Flags: review?(dbaron)
(Assignee)

Comment 2

5 years ago
Note that for some reason to get the command arguments in the right order, I had to make the @Command decorator add them in reverse order.  (I guess the decorations are handled last to first.)
(Assignee)

Comment 3

5 years ago
Created attachment 679051 [details] [diff] [review]
patch (v1.1)

Fix a couple of white space issues in runreftests.py help text.
Attachment #679039 - Attachment is obsolete: true
Attachment #679039 - Flags: review?(gps)
Attachment #679039 - Flags: review?(dbaron)
Attachment #679051 - Flags: review?(gps)
Attachment #679051 - Flags: review?(dbaron)

Updated

5 years ago
Attachment #679051 - Attachment is patch: true

Comment 4

5 years ago
Moving to testing component because this patch is primarily related to the test runner, not mach.
Component: mach → Reftest
Product: Core → Testing

Comment 5

5 years ago
Comment on attachment 679051 [details] [diff] [review]
patch (v1.1)

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

::: layout/tools/reftest/mach_commands.py
@@ +75,5 @@
> +                env['EXTRA_TEST_ARGS'] = os.environ['EXTRA_TEST_ARGS'] + ' '
> +            else:
> +                env['EXTRA_TEST_ARGS'] = ' '
> +            env['EXTRA_TEST_ARGS'] += ("--filter %s" %
> +                                       self._make_shell_string(filter))

I'm pretty sure all these environment assignments will fail in Python < 2.7.3 because those older versions of Python don't like unicode types as keys in the environment dictionary (the file has unicode literals enabled for easier Python 3 compatibility). The workaround is to force the str type:

  env[b'TEST_PATH'] = path

@@ +80,3 @@
>  
>          # TODO hook up harness via native Python
>          self._run_make(directory='.', target=suite, append_env=env)

I *really* want this TODO done because all new features we add just create engineering debt for this transition. I'm not asking for it as part of landing this, but if you have the cycles to hook up the test runner properly, I'd love to see a patch!

@@ +89,5 @@
> +        help='Reftest manifest file, or a directory in which to select '
> +             'reftest.list. If omitted, the entire test suite is executed.')
> +    @CommandArgument('filter', default=None, nargs='?', metavar='FILTER',
> +        help='A JS regular expression to match test URLs against, to select '
> +             'a subset of tests to run.')

What you did here was add a second optional positional argument. This seems a bit weird to me. I think it makes more sense for the filter to be a named argument. e.g. --filter='/foo/'. You can make it a named argument by:

  @CommandArgument('--filter', default=None, help='...')

::: python/mach/mach/base.py
@@ +86,5 @@
>  
>      def __call__(self, func):
>          command_args = getattr(func, '_mach_command_args', [])
>  
> +        command_args.insert(0, self._command_args)

Good catch. I wouldn't mind this as a part 0 or a separate bug.
Attachment #679051 - Flags: review?(gps) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 679398 [details] [diff] [review]
patch (v1.2)

I'll see how easy it is to call directly into runreftest.py.
Attachment #679051 - Attachment is obsolete: true
Attachment #679051 - Flags: review?(dbaron)
Attachment #679398 - Flags: review?(gps)
Attachment #679398 - Flags: review?(dbaron)

Comment 7

5 years ago
This patch has been bit rotted by bug 809742, right?
(Assignee)

Comment 8

5 years ago
I have the bug 809742 based on top of this patch.  I think this one can land first.  Also it has the reftest.js changes to implement --filter, which bug 809742 doesn't have.
(In reply to Cameron McCormack (:heycam) from comment #0)
> I say just use a JS regular expression string (as you could pass to the
> RegExp constructor).  The question then is whether to match against just
> what's listed in the manifest, or whether the url-prefix should be added in
> before matching, or whether (if it is a relative reference) it should match
> against the pathname underneath layout/reftests/.
> 
> gps, dbaron, WDYT?

I'm inclined to say to match against only the test name (not the ref name) (I'm not sure what you did here), and I'm find with matching against against what's listed in the manifest or against the resolved URL (which is what you did).
Comment on attachment 679398 [details] [diff] [review]
patch (v1.2)

I'm inclined to prefer testing against only url1, though I'd be ok with testing against both, I suppose.

r=dbaron on the reftest.js parts.
Attachment #679398 - Flags: review?(dbaron) → review+
Comment on attachment 679398 [details] [diff] [review]
patch (v1.2)

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

A few more nits on top of my previous review. I recuse myself from the reftest runner review.

::: layout/tools/reftest/mach_commands.py
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from __future__ import unicode_literals
>  
> +import re, os

Nit: One import per line, typically alphabetical order.

@@ +91,5 @@
> +    @CommandArgument('--filter', default=None, metavar='REGEX',
> +        help='A JS regular expression to match test URLs against, to select '
> +             'a subset of tests to run.')
> +    def run_reftest(self, test_file, filter):
> +        self._run_reftest(test_file, filter, 'reftest')

For named arguments, it's probably best to call the functions with the names. e.g.

  self._run_reftest(test_file, filter=filter, suite='reftest')

@@ +101,2 @@
>      def run_crashtest(self, test_file):
> +        self._run_reftest(test_file, None, 'crashtest')

And here you don't need to add a "None". But, you should add the argument name.

  self._run_reftest(test_file, suite='crashtest')
Attachment #679398 - Flags: review?(gps) → feedback+
(Assignee)

Comment 12

5 years ago
Created attachment 685494 [details] [diff] [review]
patch (v1.3)

I changed the option to match only against the test URL as dbaron suggested, and made the other changes from gps.
Attachment #679398 - Attachment is obsolete: true
Attachment #685494 - Flags: review?(gps)
Comment on attachment 685494 [details] [diff] [review]
patch (v1.3)

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

LGTM
Attachment #685494 - Flags: review?(gps) → review+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9efd4d0da893
https://hg.mozilla.org/mozilla-central/rev/9efd4d0da893
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Updated

5 years ago
Depends on: 820060
You need to log in before you can comment on or make changes to this bug.