809312 - add ability to run a subset of reftests from a manifest

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Cameron McCormack (:heycam)]

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?

Comment 1

[Cameron McCormack (:heycam)]

Attachment: 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).

Comment 2

[Cameron McCormack (:heycam)]

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.)

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch (v1.1)

Fix a couple of white space issues in runreftests.py help text.

Comment 4

[Gregory Szorc [:gps]]

Moving to testing component because this patch is primarily related to the test runner, not mach.

Comment 5

[Gregory Szorc [:gps]]

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.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: patch (v1.2)

I'll see how easy it is to call directly into runreftest.py.

Comment 7

[Gregory Szorc [:gps]]

This patch has been bit rotted by bug 809742, right?

Comment 8

[Cameron McCormack (:heycam)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[Gregory Szorc [:gps]]

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')

Comment 12

[Cameron McCormack (:heycam)]

Attachment: patch (v1.3)

I changed the option to match only against the test URL as dbaron suggested, and made the other changes from gps.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 685494 [details] [diff] [review]
patch (v1.3)

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

LGTM

Comment 14

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9efd4d0da893

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9efd4d0da893