1293295 - Replace all the mochitest flavor arguments (i.e --chrome) with a single --flavor argument

Status: RESOLVED FIXED

(Testing :: Mochitest, defect, P1)

Description

[Andrew Halberstadt [:ahal]]

Mochitest has a bunch of (non-enforced) mutually exclusive arguments to specify which flavor to run. These are --a11y, --browser-chrome, --chrome, --jetpack-addon and --jetpack-package. Instead, these should be specified as a 'choices' type under a single '--flavor' argument. There are three motivations for doing this:

1) When running from a test package (interactive loaner), this makes the CLI more similar to the mach environment developers are used to, creating a better UX.

2) This guarantees these arguments will be mutually exclusive, so we won't get undefined behaviour if someone accidentally specifies more than one.

3) This simplifies a lot of logic in the harness.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1293295 - Replace all mochitest 'flavor' options with a single --flavor argument,

This accomplishes three things:

1) Easier to use CLI when running without the benefit of testing/mochitest/mach_commands.py
2) Guarantees these arguments are mutually exclusive
3) Simplifies a bunch of logic in the test harness

The primary motivation for this change is to slightly improve the UX when running mochitest
from a taskcluster interactive loaner. However, this is more of a bandaid solution that was
easy to implement before the proper fix in bug 1293259 can be landed.

Review commit: https://reviewboard.mozilla.org/r/70066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70066/

Comment 2

[Andrew Halberstadt [:ahal]]

Here's the above patch on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fef631b84d2667e979bd2610d78cea4fa4326c8b&group_state=expanded

Looking good so far.

Comment 3

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

Comment on attachment 8779007 [details]
Bug 1293295 - Replace all mochitest 'flavor' options with a single --flavor argument,

https://reviewboard.mozilla.org/r/70066/#review67284

<p>this leaves a good flavor in my mind :)  Please answer the question or address it about devtools and devtools-chrome.</p>

::: testing/mochitest/mochitest_options.py:89
(Diff revision 1)
>  
>  
>  class MochitestArguments(ArgumentContainer):
>      """General mochitest arguments."""
>  
> +    FLAVORS = ('a11y', 'browser', 'chrome', 'jetpack-addon', 'jetpack-package', 'plain')

what about devtools?

::: testing/mochitest/runtests.py:2099
(Diff revision 1)
>          self.urlOpts = []
>  
>      def resolve_runtime_file(self, options):
>          data_dir = os.path.join(SCRIPT_DIR, 'runtimes')
>  
> -        flavor = self.getTestFlavor(options)
> +        flavor = self.normflavor(options.flavor)

won't normflavor return the wrong thing?  what if flavor == 'browser-chrome' and there is no subsuite, why are we not setting flavor=browser?

::: testing/mochitest/runtests.py:2101
(Diff revision 1)
>      def resolve_runtime_file(self, options):
>          data_dir = os.path.join(SCRIPT_DIR, 'runtimes')
>  
> -        flavor = self.getTestFlavor(options)
> +        flavor = self.normflavor(options.flavor)
>          if flavor == 'browser-chrome' and options.subsuite == 'devtools':
>              flavor = 'devtools-chrome'

here we have devtools-chrome, but this flavor isn't indicated anywhere else.

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8779007 [details]
Bug 1293295 - Replace all mochitest 'flavor' options with a single --flavor argument,

https://reviewboard.mozilla.org/r/70066/#review67284

> what about devtools?

Devtools isn't actually a flavor, it's a subsuite. So devtools jobs should all have flavor == 'browser'. It's kind of confusing because in a lot of places we kind of treat devtools like a flavor.

> won't normflavor return the wrong thing?  what if flavor == 'browser-chrome' and there is no subsuite, why are we not setting flavor=browser?

This logic is only to find the runtimes files under testing/runtimes. Because that file is called 'mochitest-browser-chrome.runtimes.json', we need to use 'browser-chrome' here. There are a couple other places that expect 'browser-chrome' instead of 'browser', so I used the normflavors function for those instances, but keep it as 'browser' everywhere else because it's easier to read and type.

> here we have devtools-chrome, but this flavor isn't indicated anywhere else.

Yeah, this is also just to find the runtimes file. Maybe I could call that local variable 'suite_id' or 'slug' or something to reduce confusion.

Comment 5

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

I would think a few comments in the source to outline some of these differences would save some confusion in the future.  I agree devtools is confusing.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8779007 [details]
Bug 1293295 - Replace all mochitest 'flavor' options with a single --flavor argument,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70066/diff/1-2/

Comment 7

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

Comment on attachment 8779007 [details]
Bug 1293295 - Replace all mochitest 'flavor' options with a single --flavor argument,

https://reviewboard.mozilla.org/r/70066/#review67360

thanks for the update, this should prevent a face palm 16 months from now

Comment 8

[Andrew Halberstadt [:ahal]]

Here's a new try run with the latest update: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]

But I realized I forgot to update the mach command (I missed it because it uses e.g 'chrome': True instead of --chrome). So I'll have to upload another patch.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8779007 [details]
Bug 1293295 - Replace all mochitest 'flavor' options with a single --flavor argument,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70066/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43ebf330ad94
Replace all mochitest 'flavor' options with a single --flavor argument, r=jmaher

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43ebf330ad94