1181516 - Allow multiple test files to be specified for reftest harness

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[James Graham [:jgraham]]

The aim here is to be able to do something like |mach reftest layout/reftests/box layout/reftest/text/some-test-1.html| and have it run all the tests in the box directory and just the some-test-1.html test from the text directory. The implementation must be in the runreftest.py level or below so it can be used from mozharness where mach isn't used.

Comment 1

[James Graham [:jgraham]]

Attachment: MozReview Request: Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

This makes reftest command line arguments behave more like other test suites,
so we can use a simple unified syntax for e.g. |mach try|. The patch also
reworks the command line argument parsing to use argparse rather than optparse,
and causes mach to reuse the same parser as the suite.

Comment 2

[James Graham [:jgraham]]

Comment on attachment 8646439 [details]
MozReview Request: Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

Bug 1181516 - Better support for providing a directory name and discovering reftests under that directory

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8646439 [details]
MozReview Request: Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

https://reviewboard.mozilla.org/r/15749/#review14067

My review my appear to be scattered.  This is mostly due to being unfamiliar with the code and making a couple passes to fully comprehend the scope of the changes.  Even then, I imagine my comments which I made through each pass might be conflicting or confusing.  This IDE is not the most intuitive to figure out where previous comments exist.

::: layout/tools/reftest/mach_commands.py:126
(Diff revision 1)
> +        print kwargs["tests"]

I would prefer to have some context surrounding this print statement.

::: layout/tools/reftest/mach_commands.py:189
(Diff revision 1)
> -        parallel indicates whether tests should be run in parallel or not.
> +        if kwargs["suite"] not in ('reftest', 'crashtest', 'jstestbrowser'):

what about crashtest-ipc and reftest-ipc?  Previously we only had test_subdir for reftest/crashtest.

::: layout/tools/reftest/reftest.js:493
(Diff revision 1)
> +        gDumpLog(manifestURLs + "\n");

I would like this gDumpLog to have some context around it, I assume this is an array of manifest.list files from reading the code, so maybe something to indicate that.

::: layout/tools/reftest/reftest.js:497
(Diff revision 1)
> +            ReadTopManifest(manifestURL, [globalFilter, filters]);

I don't understand these filters.

::: layout/tools/reftest/reftest.js:797
(Diff revision 1)
> +         !aFilters[1].some(function(filter) {return filter.test(aTest.url1.spec)})))

does this imply that we have 1 or 2 filters?  it seems as though if we could write this to be smoother, to be honest I still don't understand the filters.

::: layout/tools/reftest/reftestcommandline.py:17
(Diff revision 1)
> +            self.build_obj = None

I assume this try/except is to determine we are in the mozbuild context (e.g. mach)?  I would prefer a comment if that is the case, and if it isn't the case, I would like to learn more about what mozbuild is.

::: layout/tools/reftest/reftestcommandline.py:23
(Diff revision 1)
> +                          # individual scripts will set a sane default

is this intended to be subclassed so individual scripts can set the default?

::: layout/tools/reftest/reftestcommandline.py:183
(Diff revision 1)
> +        self.add_argument("--suite",

do we not do ipc here in suite?

::: layout/tools/reftest/reftestcommandline.py:191
(Diff revision 1)
> +                          help="Path to test file, manifest file, or directory containing tests")

with the code below to check options.tests, I don't see how we can have a path to a test file.

::: layout/tools/reftest/reftestcommandline.py:217
(Diff revision 1)
> +            self.error("Must supply at least one path to a manifest file or test to run.")

or test directory.

::: layout/tools/reftest/reftestcommandline.py:267
(Diff revision 1)
> +        self.add_argument("--no-run-tests-in-parallel",

why do we have --no-run-tests-in-parallel and --run-tests-in-parallel, both options are false by default which would lead to us not running tests at all!?!

::: layout/tools/reftest/reftestcommandline.py:725
(Diff revision 1)
> +            f = open(options.pidFile, 'w')

maybe we could take the opportunity here to update this past python 2.3?
with open(options.pidFile, 'w') as f:
    f.write(str(os.getpid()))

::: layout/tools/reftest/reftestcommandline.py:742
(Diff revision 1)
> +                self.error("ERROR: Invalid screen resolution %sx%s, please adjust to 1366x1050 or higher" % (

I wonder if we actually use this resolution outside of panda boards now?  We might not run panda board reftests- so maybe --ignore-window-size is used all the time, which means we could remove this code!

::: layout/tools/reftest/remotereftest.py:228
(Diff revision 1)
>          prefs["toolkit.telemetry.notifiedOptOut"] = 999

I would love to have all the prefs in an external config file like we have for mochitest!  unrelated to the review, thought I would mention it.

::: layout/tools/reftest/runreftest.py:132
(Diff revision 1)
> +                "jstestbrowser": "jstests.list"}[suite]

is suite guaranteed t obe in the 3 possible options?

::: layout/tools/reftest/runreftest.py:143
(Diff revision 1)
> +            return os.path.join(test_file, self.defaultManifest(suite)), None

does this assume there is a manifest.list file in the directory, do we need to check for it?

::: layout/tools/reftest/runreftest.py:284
(Diff revision 1)
>              prefs['security.turn_off_all_security_so_that_viruses_can_take_over_this_computer'] = True

shouldn't we only do this if we have the specialpowers extension to install?

::: layout/tools/reftest/runreftestb2g.py:15
(Diff revision 1)
> +print here

please add context here as to what you are printing.

::: layout/tools/reftest/runreftestb2g.py:22
(Diff revision 1)
> -from runreftest import ReftestOptions
> +import reftestcommandline

seeing this with other imports I believe we should camelcase reftestcommandline:
ReftestCommandLine

Comment 4

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/15749/#review14067

> what about crashtest-ipc and reftest-ipc?  Previously we only had test_subdir for reftest/crashtest.

I made ipc into a seperate command line option that adds the ipc-related prefs to the suite if needed. See reftestcommandline.py around line 304 and note that the mach targets still exist.


I'm not sure what the test_subdir comment relates to, can you elaborate?

> I don't understand these filters.

So the deal is that there's a global filter which is just a regexp that the user can supply directly on the command line. This was pre-existing functionality that I didn't want to change.

There's a second filter which is designed to only match the test paths that have been passed in on the command line. This is the new functionality added here.

I actually refined this a little in a subsequent commit, but please let me know what you think would make the intent clearer. Is a comment enough? Where?

> is this intended to be subclassed so individual scripts can set the default?

It's not so much subclassed as set by the frontends for desktop, android, and b2g, according to what makes sense for each product in the typical case.

> do we not do ipc here in suite?

I moved that to the --ipc command line argument.

> with the code below to check options.tests, I don't see how we can have a path to a test file.

I'm not sure I understand. options.tests will be populated by positional arguments (because this is an option with no leading "-").

> why do we have --no-run-tests-in-parallel and --run-tests-in-parallel, both options are false by default which would lead to us not running tests at all!?!

This is essentially unchanged and not broken as such (note that one is store_true and one is store_false), but afaict --no-run-tests-in-parallel doesn't actually do anything that can't be done by not providing --run-tests-in-parallel because there isn't a case where we run tests in parallel by default. I might just remove it.

> I wonder if we actually use this resolution outside of panda boards now?  We might not run panda board reftests- so maybe --ignore-window-size is used all the time, which means we could remove this code!

I have no idea…

> I would love to have all the prefs in an external config file like we have for mochitest!  unrelated to the review, thought I would mention it.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1195443

> is suite guaranteed t obe in the 3 possible options?

I think so. At least mach will raise an error before this point if you haven't selected one of the allowed options.

In general I agree that the design around multiple suites in the same harness isn't wonderful. Do you have any particular suggestions for improvements that aren't complete refactors?

> does this assume there is a manifest.list file in the directory, do we need to check for it?

This is addressed in a later commit / different review where I explicitly look for any manifests under the supplied directory.

> shouldn't we only do this if we have the specialpowers extension to install?

Is that not what the options.specialPowersExtensionPath test does?

> seeing this with other imports I believe we should camelcase reftestcommandline:
> ReftestCommandLine

No, that would violate the style guide since reftestcommandline is a module not a class.

Comment 5

[James Graham [:jgraham]]

Comment on attachment 8646439 [details]
MozReview Request: Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

This makes reftest command line arguments behave more like other test suites,
so we can use a simple unified syntax for e.g. |mach try|. The patch also
reworks the command line argument parsing to use argparse rather than optparse,
and causes mach to reuse the same parser as the suite.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8646439 [details]
MozReview Request: Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

https://reviewboard.mozilla.org/r/15749/#review15049

a few small items here, thanks for addressing the previous feedback.  These are small enough to be nits and carry forward the r+ :)

::: layout/tools/reftest/reftestcommandline.py:252
(Diff revision 3)
> +            options.suite in ["crashtest", "jstestbrowser"]):

do we not need specialPowersExtensionPath for 'reftest'?

::: layout/tools/reftest/reftestcommandline.py:306
(Diff revision 3)
> +            if options.totalChunks is not None and options.thisChunk is None:

won't ReftestArgumentsParser catch this?

::: layout/tools/reftest/reftestcommandline.py:318
(Diff revision 3)
> +            bin_dir = (self.build_obj.get_binary_path() if

while I like the simplicity of the build_obj, this seems like logic that should be in mach or mozharness.  Not sure I would make this a requirement, but it seems worth considering.

::: layout/tools/reftest/reftestcommandline.py:614
(Diff revision 3)
> +                          default=moznetwork.get_ip(),

above in the nonremote class we only do moznetwork.get_ip() if os.name != 'nt'.  We should do a consistent method for the webserver calls.

::: layout/tools/reftest/runreftest.py:22
(Diff revision 3)
>  sys.path.insert(0, SCRIPT_DIRECTORY)

can we check if SCRIPT_DIRECTORY is already in the path, if not then insert it?

Comment 7

[James Graham [:jgraham]]

Comment on attachment 8646439 [details]
MozReview Request: Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

This makes reftest command line arguments behave more like other test suites,
so we can use a simple unified syntax for e.g. |mach try|. The patch also
reworks the command line argument parsing to use argparse rather than optparse,
and causes mach to reuse the same parser as the suite.

Comment 8

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/15749/#review15049

> do we not need specialPowersExtensionPath for 'reftest'?

I was duplicating the logic that before looked like:

        # I would prefer to use "--install-extension reftest/specialpowers", but that requires tight coordination with
        # release engineering and landing on multiple branches at once.
        if special_powers and (manifest.endswith('crashtests.list') or manifest.endswith('jstests.list')):
            addons.append(os.path.join(SCRIPT_DIRECTORY, 'specialpowers'))

> won't ReftestArgumentsParser catch this?

Ah, the logic here was just wrong.

> while I like the simplicity of the build_obj, this seems like logic that should be in mach or mozharness.  Not sure I would make this a requirement, but it seems worth considering.

I agree, but this was already implemented like this and I didn't want to change even more than I already had (believe it or not!). Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1198219

Comment 9

[Benoit Girard (:BenWa)]

I blocked landing bug 1196831 on this to minimize conflict. Will this be landing soon?

Comment 10

[James Graham [:jgraham]]

I'm going full steam ahead on landing this; I had a few issues with a rebase onto some mozharness config changes that interfered with my patch, but I think we're nearly there now.

Comment 11

[James Graham [:jgraham]]

Comment on attachment 8646439 [details]
MozReview Request: Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line.

This makes reftest command line arguments behave more like other test suites,
so we can use a simple unified syntax for e.g. |mach try|. The patch also
reworks the command line argument parsing to use argparse rather than optparse,
and causes mach to reuse the same parser as the suite.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da3dd2f650d0

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8d86582839 since one of this changes caused https://treeherder.mozilla.org/logviewer.html#?job_id=13953993&repo=mozilla-inbound

Comment 14

[James Graham [:jgraham]]

I fixed this now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=164862935682 and will repush at some convenient moment.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6137013d4933

Comment 16

[Nigel Babu [:nigelb]]

Had to back it out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0a54cadfd0 for failures on Android 4.0 API11 reftests that look like this https://treeherder.mozilla.org/logviewer.html#?job_id=14037560&repo=mozilla-inbound

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81a5cf4c8f19

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for breaking Android 4.0 debug reftests
https://hg.mozilla.org/integration/mozilla-inbound/rev/d90bfbc8688a

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/967ad761ec8c

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/967ad761ec8c