938712 - Mach mochitest commands should be smart enough to pick the suite for you

Status: RESOLVED FIXED

Description

[Andrew Halberstadt [:ahal]]

There are a ton of mochitest-* mach commands. In some cases these commands are actually different suites (e.g mochitest-plain and mochitest-chrome), but many of these are actually just the same suite running on different platforms (e.g mochitest-plain, mochitest-remote and mochitest-b2g-desktop).

In the latter case, the mochitest command could easily detect what build is currently active and automatically choose which entry path to pick based off of this. This would mean less clutter in the help, and no need to remember which command is meant for which platform.

I don't know if this makes a good first bug as it could be a bit tricky, but if a contributor wants to tackle it, I'd be happy to mentor.

Comment 1

[Gregory Szorc [:gps]]

Somewhat related to bug 920193. There is also code in part 2 of bug 920849 that will make this much, much easier. Feel free to steal review from ted.

Comment 2

[Gregory Szorc [:gps]]

Now that mochitests are almost 100% in .ini manifests, the mach commands should be able to load up all-tests.json and glean the mochitest flavor from that metadata. There's even a Python API for that. See testing/xpcshell/mach_commands.py.

Comment 3

[Andrew Halberstadt [:ahal]]

Hi Anshu, I noticed you are interested in this bug. Let me know if you have any questions or need a pointer in the right direction.

(In reply to Gregory Szorc [:gps] from comment #2)
> Now that mochitests are almost 100% in .ini manifests, the mach commands
> should be able to load up all-tests.json and glean the mochitest flavor from
> that metadata.

That would only be possible if a test path is passed in right? So we'd still need separate targets for all the flavours (or at least an argument) in the event that they want to run all tests.

Comment 4

[Gregory Szorc [:gps]]

The end goal I've been working towards is to make the end-user's life as easy as possible. We do this by optimizing for common scenarios:

1) Run all tests relevant for a given feature
2) Run a specific test(s) with customizations

For #1, we assume "run all tests in a directory [tree]" is a sufficient proxy for "feature." The solution here is `mach test path/to/directory` which will discover every relevant test and sequentially run each test suite for the subset of tests in that directory. This will enable new contributors to run all relevant tests without requiring them to know details about what the different test suites do. This is huge. This is the goal of bug 920193.

For #2, I don't think we'll be able to shoehorn every test suite customization into `mach test`. So, we'll still need suite-specific mach commands (mach xpcshell-test, mach mochitest-plain, etc).

Given that the mach mochitest-* commands all share similar flags, we should be able to consolidate down to |mach mochitest|. That's the purpose of this bug. We were previously blocked on not having mochitest flavor metadata available to mach nor the build config. (Yay for moz.build data leading to better UX!) I think it makes sense to provide a --flavor (bikeshed welcome) argument to limit tests to a certain mochitest flavor. If a path is not given, the entire suite is executed. |mach mochitest| without arguments would be ambiguous.

Does that all make sense? Does that sound like a reasonable proposal?

Comment 5

[Andrew Halberstadt [:ahal]]

Yeah, I was thinking the same thing. I was just asking how we specify the flavor. I think --flavor would be a good solution, though I think it would be nice if it had a default so |mach mochitest| without arguments ran mochitest-plain. So:

mach mochitest                             # run all mochitest plain
mach mochitest --flavor foo                # run all mochitest foo
mach mochitest path/to/tests               # run all mochitests in path; autodetect flavor
mach mochitest --flavor foo path/to/tests  # error out, --flavor not necessary

FTR, the original intent of this bug was to get rid of the platform specific commands, e.g mochitest-remote or mochitest-b2g-desktop. Though I agree we should continue from there into flavors.

Comment 6

[Andrew Halberstadt [:ahal]]

Haven't seen anything from Anshu, resetting assignee.

Comment 7

[Gregory Szorc [:gps]]

Created attachment 8441717 [details] [diff] [review]
Allow multiple instantiations of MochitestOptions

The MochitestOptions class has a class-local definition of the options
going into the optparse instance. The default values for these options
would be reused by subsequent consumers. In the case of the profile
path, the same temporary directory would be used. In the case of list
arguments, subsequent runs would inherit members added by earlier runs.

This patch should make subsequent runs free from the baggage of the
first.

Comment 8

[Gregory Szorc [:gps]]

Created attachment 8441718 [details] [diff] [review]
Implement mach mochitest

The `mach mochitest` command is now implemented. Given test path
arguments, it will identify mochitests of any flavor and run the
appropriate mochitest suite.

If tests from multiple suites are present, it will invoke each suite
separately. Although, the output in this mode isn't very friendly.

There are a number of enhancements that could be made to this command,
including the abilities to filter by flavor and sub-suite. These will
come in another patch.

Comment 9

[Gregory Szorc [:gps]]

Created attachment 8441722 [details] [diff] [review]
Add --flavor to mach mochitest

`mach mochitest` now accepts a --flavor argument to limit execution to
tests of a certain flavor. Executing `mach mochitest --flavor=X` should
be equivalent to executing `mach mochitest-X`.

This paves the road to deprecating the various `mach mochitest-X`
commands.

Comment 10

[Gregory Szorc [:gps]]

Created attachment 8441728 [details] [diff] [review]
Rewrote mach test mochitest suites via mach mochitest

`mach test` now dispatches through `mach mochitest --flavor` where
supported. As part of testing this patch, it was discovered that `mach
test` may have been broken for quite some time, as it was still
referring to "test_file" arguments instead of "test_paths." This has
been corrected.

Comment 11

[Andrew Halberstadt [:ahal]]

This is awesome.. though it doesn't get rid of the 'mochitest-remote' and 'mochitest-b2g-desktop' commands (which was my original motivation for filing this bug). If you don't feel like tackling that as well, I don't mind filing a new bug for it.

Also, did you mean to flag :AutomatedTester for review?

Comment 12

[David Burns :automatedtester]

Comment on attachment 8441717 [details] [diff] [review]
Allow multiple instantiations of MochitestOptions

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

r+ after updating the comment

::: testing/mochitest/mochitest_options.py
@@ +427,5 @@
>      def __init__(self, **kwargs):
>  
>          optparse.OptionParser.__init__(self, **kwargs)
>          for option, value in self.mochitest_options:
> +            # Copy lists to avoid updating original, thus allowing multiple

the comment here doesnt seem to agree with the code. It is overwriting value['default'].

Comment 13

[David Burns :automatedtester]

Comment on attachment 8441728 [details] [diff] [review]
Rewrote mach test mochitest suites via mach mochitest

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

lgtm

Follow up needed for devtools to have a flavour when doing --subsuite as discussed.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/cb5244b95c40
https://hg.mozilla.org/integration/fx-team/rev/9f6baa671811
https://hg.mozilla.org/integration/fx-team/rev/3dedef93c16d
https://hg.mozilla.org/integration/fx-team/rev/5301f1aaf305

Comment 15

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/cb5244b95c40
https://hg.mozilla.org/mozilla-central/rev/9f6baa671811
https://hg.mozilla.org/mozilla-central/rev/3dedef93c16d
https://hg.mozilla.org/mozilla-central/rev/5301f1aaf305

Comment 16

[Andrew Halberstadt [:ahal]]

This got resolved without merging the different mochitest platform commands. I filed bug 1046992 to fix that.