1164597 - Make |mach mochitest| work with b2g/android and remove all mochitest-<flavor> commands

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew Halberstadt [:ahal]]

As part of an effort to make running tests as easy and intuitive as possible, I'd like to consolidate all of the mochitest-<flavor> mach commands into the single |mach mochitest| command.

Currently |mach mochitest| will automatically detect the flavor of mochitests you wish to run. If you specify a directory that contains multiple flavors, each flavor will be run in sequence. For example:

./mach mochitest dom/indexedDB

runs all the mochitest-chrome, mochitest-browser-chrome and mochitest-plain tests in that directory. The flavor can be limited by passing in -f (--flavor). So to only run the chrome tests in that directory you'd use:

./mach mochitest -f chrome dom/indexedDB

Commands of the form |mach mochitest-<flavor>| will be removed:

./mach mochitest-plain -> ./mach mochitest -f plain
./mach mochitest-browser -> ./mach mochitest -f browser
etc..

I believe this will make running tests easier and more intuitive. The fact that different flavors of mochitest exist will become a background implementation detail. Instead, an emphasis will be placed on running directories of tests, or tests related to certain features. The knowledge of which test belongs to which flavor will no longer be required, but of course the ability to run specific flavors will still be there for those who need it.

The described behaviour for |mach mochitest| above, already exists today. This bug is about making it the defacto way to run mochitests (maybe aside from |mach test|). This bug will also add b2g and android support to |mach mochitest|.

Exceptions to this will be robocop, webapprt-chrome and webapprt-content which for technical reasons will not be rolled into |mach mochitest| (for now).

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: bz://1164597/ahal

/r/8705 - Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

Pull down this commit:

hg pull -r bceb466e854c33f9a606a0c02c4495ce6d43977e https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Andrew Halberstadt [:ahal]]

Above is first attempt. Still needs more thorough testing.

Comment 3

[Andrew Halberstadt [:ahal]]

Just a bit of a progress update. Been testing as many edge cases as possible, and fixing all kinds of things along the way. So far desktop and b2g look very solid, still have to try out android. Should be ready for review shortly.

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8605447 [details]
MozReview Request: bz://1164597/ahal

/r/8705 - Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

Pull down this commit:

hg pull -r f9d0ae70544dd0ae4a14dc3af325cf0c14a5e242 https://reviewboard-hg.mozilla.org/gecko/

Comment 5

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/8703/#review8043

Sorry for dumping a massive review on you again. It might be best to try and understand the end result rather than the diff, as things were moved around quite a bit. I've tested on desktop, mulet, b2g_desktop, b2g and android. I think I caught most edge cases. Please let me know if you have any questions about it.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8605447 [details]
MozReview Request: bz://1164597/ahal

/r/8705 - Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

Pull down this commit:

hg pull -r f9d0ae70544dd0ae4a14dc3af325cf0c14a5e242 https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8703/#review8215

::: testing/mochitest/mach_commands.py:612
(Diff revision 2)
> +def DEPRECATED(cls):
> +    """Command no longer exists! Use |mach mochitest -f <flavor>| instead."""
> +    return False

These aren't really deprecated since they've already been disabled.

I think there's a use case this hinders -- when developing locally I might really need to run just bc or plain tests for a particular directory (depending on the feature I'm developing). For those users aren't we imposing an uglier command line without any real upside? I think it's good to get people out of the habit of running multiple commands when they could run just one, but this strikes me as overkill.

Anyway, you already posted to the mailing and didn't get a lot of pushback, so maybe it doesn't matter. What would you think of printing a deprecation or "did you know?" type message about the unified commands instead?

::: testing/mochitest/mach_commands.py:462
(Diff revision 2)
>      def run_mochitest_general(self, test_paths, flavor=None, test_objects=None,
>                                **kwargs):

Something looks wrong here. I don't think test_objects can ever be set. Unless I'm missing something just get rid of it and its proliferation to other methods.

::: testing/mochitest/mach_commands.py:369
(Diff revision 2)
> -        self.tests_dir = os.path.join(self.topobjdir, '_tests')
> -        self.mochitest_dir = os.path.join(self.tests_dir, 'testing', 'mochitest')
> +        # if first item is a dict, assume test objects.. quack
> +        if not tests or not isinstance(tests[0], dict):
> +            tests = self.resolve_tests(tests, cwd=context.cwd)

If you resolve tests in run_robocop it looks like this method will always get test objects as input, so this wouldn't be necessary.

::: testing/mochitest/mach_commands.py:272
(Diff revision 2)
> +        # if first item is a dict, assume test objects.. quack
> +        if not tests or not isinstance(tests[0], dict):
> +            tests = self.resolve_tests(tests, cwd=context.cwd)

I don't see any way this method wouldn't get test objects as input.

::: testing/mochitest/mach_commands.py:327
(Diff revision 2)
> +        # if first item is a dict, assume test objects.. quack
> +        if not tests or not isinstance(tests[0], dict):
> +            tests = self.resolve_tests(tests, cwd=context.cwd)

I don't see a way this method wouldn't get test objects as input.

::: testing/mochitest/mach_commands.py:313
(Diff revision 2)
>  
>          suite is the type of mochitest to run. It can be one of ('plain',
>          'chrome', 'browser', 'metro', 'a11y', 'jetpack-package', 'jetpack-addon').

This comment is a little out of date... at least metro is gone.

::: testing/mochitest/mach_commands.py:265
(Diff revision 2)
> +        elif not context.target_out:

The old condition in run_mochitest_remote checked for if self.target_out: ... is this condition inverted?

::: testing/mochitest/mach_commands.py:508
(Diff revision 2)
>          # Our current approach is to group the tests by suite and then perform
>          # an invocation for each suite. Ideally, this would be done
>          # automatically inside of core mochitest code. But it wasn't designed
>          # to do that.
>          #
>          # This does mean our output is less than ideal. When running tests from
>          # multiple suites, we see redundant summary lines. Hopefully once we
>          # have better machine readable output coming from mochitest land we can
>          # aggregate that here and improve the output formatting.
>  

This comment isn't adding much. You could delete or update it.

::: python/mach/mach/dispatcher.py:121
(Diff revision 2)
> -                suggested_commands = set(difflib.get_close_matches(command, self._mach_registrar.command_handlers.keys(), cutoff=0.5))
> -                suggested_commands |= {cmd for cmd in self._mach_registrar.command_handlers if cmd.startswith(command)}
> +                suggested_commands = set(difflib.get_close_matches(command, names, cutoff=0.5))
> +                suggested_commands |= {cmd for cmd in names if cmd.startswith(command)}

This looks like this was wrong before and should have checked self.\_mach_registrar.command_handlers.keys() when doing the "startswith" check, right?

::: python/mach/mach/dispatcher.py:113
(Diff revision 2)
> +            names = [h.name for h in self._mach_registrar.command_handlers.values()
> +                        if h.cls.__name__ not in ('DeprecatedCommands',)]

Why not == 'DeprecatedCommands'?

::: testing/mochitest/mach_commands.py:242
(Diff revision 2)
> -        # Need to call relpath before os.chdir() below.
> -        test_path = ''
> -        if test_paths:
> -            if len(test_paths) > 1:
> -                print('Warning: Only the first test path will be used.')
> +            if mozpath.abspath(tp).startswith(self.topobjdir):
> +                tp = mozpath.relpath(mozpath.abspath(tp), self.topobjdir)
> +            elif mozpath.abspath(tp).startswith(self.topsrcdir):
> +                tp = mozpath.relpath(mozpath.abspath(tp), self.topsrcdir)
> +            rel_paths.append(tp)

This is just wrap_path_argument(<path>).relpath(), right? Any particular reason to change it?

Comment 8

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8703/#review8269

::: testing/mochitest/mach_commands.py:631
(Diff revision 2)
> +    @Command('mochitest-devtools', category='testing', conditions=[DEPRECATED])
> +    def mochitest_devtools(self):

It's not immediately obvious how to run devtools based on the message.

::: testing/mochitest/mach_commands.py:169
(Diff revision 2)
> +SUPPORTED_FLAVORS = chain.from_iterable([f['aliases'] for f in FLAVORS.values()])

I think you need to cast this to a list for it to actually get printed out in the optparse error message.

Comment 9

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/8703/#review8323

> These aren't really deprecated since they've already been disabled.
> 
> I think there's a use case this hinders -- when developing locally I might really need to run just bc or plain tests for a particular directory (depending on the feature I'm developing). For those users aren't we imposing an uglier command line without any real upside? I think it's good to get people out of the habit of running multiple commands when they could run just one, but this strikes me as overkill.
> 
> Anyway, you already posted to the mailing and didn't get a lot of pushback, so maybe it doesn't matter. What would you think of printing a deprecation or "did you know?" type message about the unified commands instead?

In my mind, one of the goals here is to move away from a world where developers need to care about flavors and subsuites. I think --flavor and --subsuite are going to fall into the "power user" category, while 95% of the time just running |mach mochitest path/to/test/or/dir| will do what the developer wants. So yes, we are imposing a (slightly) uglier command line for that use case.. but it's a use case that I think should slowly disappear. One could even argue that if two flavors of tests exist in the same directory, then developers *should* be running them both together, as presumably they are both affected by the same code. Making it harder to select a specific flavor might actually mean better test coverage.

The problem with not removing the mochitest-<foo> commands is that then they'll still show up in |mach help| and having 10+ different ways of running mochitest is very confusing.

> This looks like this was wrong before and should have checked self.\_mach_registrar.command_handlers.keys() when doing the "startswith" check, right?

Iterating over a dict iterates over the keys anyway, so I think it worked before. This change also works because the keys were handler.name

> This is just wrap_path_argument(<path>).relpath(), right? Any particular reason to change it?

Oh heh, oops. Not only did I reinvent the wheel, but I destroyed it first :p.

> I don't see any way this method wouldn't get test objects as input.

Yeah, at some point in my refactoring there was a way, but not anymore. The only thing that still passes in test_paths is the robocop command to run_android_test. But I should just modify it to call resolve_tests as well. I'll remove these.

> Something looks wrong here. I don't think test_objects can ever be set. Unless I'm missing something just get rid of it and its proliferation to other methods.

It's kind of hacky, but it's used by |mach test| which has already resolved the tests:
https://dxr.mozilla.org/mozilla-central/source/testing/mach_commands.py#247

Comment 10

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8703/#review8355

> In my mind, one of the goals here is to move away from a world where developers need to care about flavors and subsuites. I think --flavor and --subsuite are going to fall into the "power user" category, while 95% of the time just running |mach mochitest path/to/test/or/dir| will do what the developer wants. So yes, we are imposing a (slightly) uglier command line for that use case.. but it's a use case that I think should slowly disappear. One could even argue that if two flavors of tests exist in the same directory, then developers *should* be running them both together, as presumably they are both affected by the same code. Making it harder to select a specific flavor might actually mean better test coverage.
> 
> The problem with not removing the mochitest-<foo> commands is that then they'll still show up in |mach help| and having 10+ different ways of running mochitest is very confusing.

I'm not convinced, but I'm sure we'll get more feedback (or lack of feedback) that will settle this once is lands. If I were developing the firefox ui, I might go through a lengthy process of writing and debugging code and tests for a particular feature that would never benefit from running mochitest-plain, and if I were developing a platform api I might go through a similar process that would never benefit from running browser-chrome. It's a power use, but I don't see why we shouldn't optimize for power users.

I don't know about protecting developers from the complexity of various test flavors if we aren't going to address that complexity directly. I think I'd rather have an interface that's straightforward and stable.

Comment 11

[Andrew Halberstadt [:ahal]]

(In reply to Chris Manchester [:chmanchester] from comment #10)
> if I were developing a platform api I might go
> through a similar process that would never benefit from running
> browser-chrome.

Sure, but if you're touching code that doesn't have any impact on browser-chrome, then chances are there won't be any browser chrome tests in the subdir that you're touching. Obviously there will be exceptions to this, but I think the principle holds to some degree.

> I don't know about protecting developers from the complexity of various test
> flavors if we aren't going to address that complexity directly. I think I'd
> rather have an interface that's straightforward and stable.

I agree with this statement, but I believe that this patch *is* addressing the complexity directly, and that the new interface is much more straightforward and stable than the old. Could you be more specific with you believe this patch is failing to address? So far the only downside I can see is that you need to type "-f".

Comment 12

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andrew Halberstadt [:ahal] from comment #11)
> (In reply to Chris Manchester [:chmanchester] from comment #10)
> > if I were developing a platform api I might go
> > through a similar process that would never benefit from running
> > browser-chrome.
> 
> Sure, but if you're touching code that doesn't have any impact on
> browser-chrome, then chances are there won't be any browser chrome tests in
> the subdir that you're touching. Obviously there will be exceptions to this,
> but I think the principle holds to some degree.
Right, I guess that may be.
> 
> > I don't know about protecting developers from the complexity of various test
> > flavors if we aren't going to address that complexity directly. I think I'd
> > rather have an interface that's straightforward and stable.
> 
> I agree with this statement, but I believe that this patch *is* addressing
> the complexity directly, and that the new interface is much more
> straightforward and stable than the old. Could you be more specific with you
> believe this patch is failing to address? So far the only downside I can see
> is that you need to type "-f".
I just mean that removing |mach mochitest-plain| would very easy to justify if we magically made the difference between mochitest-plain and other flavors go away, I'm not suggesting it's feasible.

Comment 13

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8612504 [details]
MozReview Request: Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

https://reviewboard.mozilla.org/r/8705/#review8387

Just cosmetic nits at this point.

::: testing/mochitest/mach_commands.py:104
(Diff revision 3)
>  FLAVORS = {

Maybe call this ALL_FLAVORS? It's confusing below where this appears in a method that also has a variable "flavors"

::: testing/mochitest/mach_commands.py:509
(Diff revision 3)
> -            if flavor and test['flavor'] != flavor:
> +            if test['flavor'] not in flavors:
> +                unsupported_flavors.add(test['flavor'])
>                  continue

Additions here mean a test in a directory specified has a flavor that wasn't specified, but if no tests are found and this is the case the SUPPORTED_FLAVORS_NOT_FOUND error message is printed. The error message isn't correct if the relevant tests were excluded due to a condition other than flavor (such as subsuite).

As an example, if I try |./mach mochitest testing/mochitest/ --subsuite devtools -f browser| I get a message that's misleading.

::: testing/mochitest/mach_commands.py:513
(Diff revision 3)
> -            suite = FLAVORS[test['flavor']]
> -            suites.setdefault(suite, []).append(test)
> -
> -        mochitest = self._spawn(MochitestRunner)
> -        overall = None
> -        for suite, tests in sorted(suites.items()):
> +            if subsuite == 'default':
> +                # "--subsuite default" means only run tests that don't have a subsuite
> +                if test['subsuite']:
> +                    continue
> +            elif subsuite and test['subsuite'] != subsuite:
> +                continue

Don't we do the appropriate subsuite filtering in the harness by forwarding the option? Any reason to do it here?

::: testing/mochitest/mach_commands.py:551
(Diff revision 3)
> +            harness_args['subsuite'] = subsuite

I think this is already in kwargs.

Comment 15

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8705/#review8389

Just cosmetic nits at this point.

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8612504 [details]
MozReview Request: Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

Comment 17

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/8705/#review8457

Thanks, the error message change ended up being a little less trivial than I thought, so one last review on the 3-4 diff would be much appreciated!

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8612504 [details]
MozReview Request: Bug 1164597 - Consolidate all mochitest mach commands into single |mach mochitest|, r=chmanchester

https://reviewboard.mozilla.org/r/8705/#review8469

Ship It!

Comment 19

[Andrew Halberstadt [:ahal]]

This shouldn't really have an effect on automation, but here's a try run anyway:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e9b674def3

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/213dcfe0e21d

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/213dcfe0e21d