1207377 - Create a mach command for invoking testsuites in mozharness

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[James Graham [:jgraham]]

I have to run this occasionally and it's always pretty annoying to work out which set of command line options are required to make things work. Just having a mach command that does the right thing would be a big improvement.

Comment 1

[James Graham [:jgraham]]

Attachment: mach-mozharness.patch

Not done, a bit hacky, and (at least) mochitest-plain seems to be broken. Not tested at all on non-linux. But it's a start.

Comment 2

[Armen [:armenzg]]

This will be useful to me as I was planning on adding the metadata needed to determine how to run each job (config files et al).

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8664531 [details] [diff] [review]
mach-mozharness.patch

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

This looks pretty useful but runs the risk of falling out of step with things in production. As an intermediate step along the way to better convergence of the mach and mozharness code-paths, we could have buildbot/taskcluster invoke these mach commands directly.

Comment 4

[James Graham [:jgraham]]

Yeah that might be good, although there are still dev/test differences in the command line arguments and so on.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8664531 [details] [diff] [review]
mach-mozharness.patch

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

Lgtm. This could get out of date with buildbot-configs though. I think this is fine for now as we're trying to move off buildbot anyway, so changes there should be minimal.

Moving forward though, it would be nice if we could set up the taskcluster configs such that taskcluster and this mach command can share the same configuration.. I don't know if that means that this reads the taskcluster yaml files, or if it means we pull mozharness related configuration out of the yaml files.

Comment 6

[James Graham [:jgraham]]

FWIW the problem with mochitests turned out to be that the default mochitest setup doesn't work on my machine. I don't really have a good solution for that, so if this works on !linux I think it should land and get iterative improvements. Anyone able to try on other platforms?

Comment 7

[Julian Seward [:jseward]]

Attachment: bug1207377-1.cset

Rebased to current m-c, and fixes the "mochitest-devtools-chrome" target.

Comment 8

[Julian Seward [:jseward]]

From my point of view (re bug 1229348), this patch is a good thing,
hence +1 for landing it.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8692566 [details] [diff] [review]
bug1207377-1.cset

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

Looks good! Just some minor stuff. Please either address or comment before landing.

::: testing/mozharness/mach_commands.py
@@ +71,5 @@
> +            "__defaults__": {
> +                "config": ["--no-read-buildbot-config",
> +                           "--download-symbols", "ondemand",
> +                           "--installer-url", self.installer_url,
> +                           "--test-packages-url", self.test_packages_url]

Doesn't seem like making these lazy load is saving much computing. I'd prefer just making them all properties in __init__ (or @property if needed) for simplicity.

@@ +179,5 @@
> +        suite_config = config.get(suite)
> +
> +        if suite_config is None:
> +            print("Unknown suite %s" % suite)
> +            sys.exit(1)

It's better to return 1. Makes it a bit easier for other commands that might want to dispatch to this one.

@@ +188,5 @@
> +                   for item in default_config["config"] + suite_config["config"]]
> +
> +        cmd = [script] + options
> +
> +        rv = subprocess.call(cmd, cwd=os.path.dirname(script))

I'd mildly prefer if the script was imported with imp and run as if it where __main__:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/desktop_unittest.py#680

That way it would be easier to do things like integrate mozharness' argument parser. But that can be a follow up for when the time comes. This is good enough for now.

@@ +189,5 @@
> +
> +        cmd = [script] + options
> +
> +        rv = subprocess.call(cmd, cwd=os.path.dirname(script))
> +        print(cmd)

Is this a left over debug statement? If not, wouldn't it be better to print the command before hand? Or at least "Finished running:\n" + cmd

Comment 10

[James Graham [:jgraham]]

Fixed all the comments apart from the import one.

Comment 11

[Pulsebot]

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

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cb1a697de90d