818819 - Wean "mach mochitest-*" off makefiles

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Comment 1

[Gregory Szorc [:gps]]

The mochitest runner depends on automation.py which is in the objdir. So, let's fix bug 841713. (Yes, this means I'm starting to code this up!)

Comment 2

[Gregory Szorc [:gps]]

Attachment: Overhaul mach mochitest integration, v1

While I was here I also added support for --shuffle, --rerun-failures, --no-autorun, and --keep-open. I'll be marking some bugs as dupes of this one...

Comment 5

[Gregory Szorc [:gps]]

Attachment: Overhaul mach mochitest integration, v2

Preserve log level from make targets.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 715376 [details] [diff] [review]
Overhaul mach mochitest integration, v2

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

::: testing/mochitest/mach_commands.py
@@ +84,4 @@
>  
> +        if rerun_failures and test_file:
> +            print('Cannot specify both --rerun-failures and a test path.')
> +            return 1

Why not an exception?

@@ +87,5 @@
> +            return 1
> +
> +        # Need to perform path test before os.chdir() below.
> +        if test_file and not os.path.exists(test_file):
> +            raise Exception('No manifest file was found at %s.' % test_file)

While you're here, there's no such thing as a mochitest manifest

@@ +170,5 @@
>      @Command('mochitest-plain', help='Run a plain mochitest.')
>      @CommandArgument('--debugger', '-d', metavar='DEBUGGER',
>          help=debugger_help)
> +    @CommandArgument('--shuffle', action='store_true', default=False,
> +        help='Shuffle execution order.')

It's kinda crappy that we have to repeat all that... Want to think about it a bit?

Comment 7

[Gregory Szorc [:gps]]

(In reply to :Ms2ger from comment #6)
> ::: testing/mochitest/mach_commands.py
> @@ +84,4 @@
> >  
> > +        if rerun_failures and test_file:
> > +            print('Cannot specify both --rerun-failures and a test path.')
> > +            return 1
> 
> Why not an exception?

That would cause mach to print out an exception stack!

> @@ +87,5 @@
> > +            return 1
> > +
> > +        # Need to perform path test before os.chdir() below.
> > +        if test_file and not os.path.exists(test_file):
> > +            raise Exception('No manifest file was found at %s.' % test_file)
> 
> While you're here, there's no such thing as a mochitest manifest

Sadly :/

> @@ +170,5 @@
> >      @Command('mochitest-plain', help='Run a plain mochitest.')
> >      @CommandArgument('--debugger', '-d', metavar='DEBUGGER',
> >          help=debugger_help)
> > +    @CommandArgument('--shuffle', action='store_true', default=False,
> > +        help='Shuffle execution order.')
> 
> It's kinda crappy that we have to repeat all that... Want to think about it
> a bit?

This horrible DRY violation was bothering me as well. Because of the use of decorators, I /may/ have to apply some Python light magic. I'll look into it.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Gregory Szorc [:gps] from comment #7)
> (In reply to :Ms2ger from comment #6)
> > ::: testing/mochitest/mach_commands.py
> > @@ +84,4 @@
> > >  
> > > +        if rerun_failures and test_file:
> > > +            print('Cannot specify both --rerun-failures and a test path.')
> > > +            return 1
> > 
> > Why not an exception?
> 
> That would cause mach to print out an exception stack!

So we print a stack when you mistype a test path, but not if you pass two mutually exclusive arguments? I guess I'm still finding it hard to figure out the logic behind which approach is used in which cases.

Comment 9

[Gregory Szorc [:gps]]

(In reply to :Ms2ger from comment #8)
> (In reply to Gregory Szorc [:gps] from comment #7)
> > (In reply to :Ms2ger from comment #6)
> > > ::: testing/mochitest/mach_commands.py
> > > @@ +84,4 @@
> > > >  
> > > > +        if rerun_failures and test_file:
> > > > +            print('Cannot specify both --rerun-failures and a test path.')
> > > > +            return 1
> > > 
> > > Why not an exception?
> > 
> > That would cause mach to print out an exception stack!
> 
> So we print a stack when you mistype a test path, but not if you pass two
> mutually exclusive arguments? I guess I'm still finding it hard to figure
> out the logic behind which approach is used in which cases.

It is inconsistent, yes. I'll fix this :)

Comment 10

[Gregory Szorc [:gps]]

Attachment: Overhaul mach mochitest integration, v3

Why not give the review to Ms2ger?

We now have a decorating decorator. Mind = blown.

Since adding arguments is much easier now, I added --repeat, --slow, and --fatal-assertions since the last patch.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 715564 [details] [diff] [review]
Overhaul mach mochitest integration, v3

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

Still hoping to do a careful review, but I'm going to dump these already:

::: testing/mochitest/mach_commands.py
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from __future__ import unicode_literals

print_function

@@ +25,5 @@
>  
>      This currently contains just the basics for running mochitests. We may want
>      to hook up result parsing, etc.
>      """
>      def run_plain_suite(self):

Do we actually use all those?

Comment 12

[Gregory Szorc [:gps]]

(In reply to :Ms2ger from comment #11)
> @@ +25,5 @@
> >  
> >      This currently contains just the basics for running mochitests. We may want
> >      to hook up result parsing, etc.
> >      """
> >      def run_plain_suite(self):
> 
> Do we actually use all those?

I believe they are all needed. But I'm no mochitest expert. Once the test lists are defined in moz.build files we can eliminate all these commands and just have mach look up the test type from moz.build files!

Comment 13

[Jeff Hammel]

I believe the longer term plan is to have mochitests live in manifestdestiny format, though this was long in the making.  That said, it shouldn't be too hard to consolidate them; while manifestdestiny is .ini, its inner structure is a list of dicts with str key, values.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 715564 [details] [diff] [review]
Overhaul mach mochitest integration, v3

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

Sorry for the delay.

::: testing/mochitest/mach_commands.py
@@ +25,5 @@
>  
>      This currently contains just the basics for running mochitests. We may want
>      to hook up result parsing, etc.
>      """
>      def run_plain_suite(self):

I think we got to the conclusion that these could be removed; could you do that?

@@ +124,5 @@
> +        options.autorun = not no_autorun
> +        options.closeWhenDone = not keep_open
> +        options.shuffle = shuffle
> +        options.consoleLevel = 'INFO'
> +        options.fatalAssertions = fatal_assertions

We dropped that

@@ +129,5 @@
> +        options.repeat = repeat
> +        options.runSlower = slow
> +        options.testingModulesDir = os.path.join(tests_dir, 'modules')
> +        options.extraProfileFiles.append(os.path.join(self.distdir, 'plugins'))
> +        options.symbolsPath = os.path.join(self.distdir, 'crashreporter-symbols')

Presumably this could be overridden before?

@@ +131,5 @@
> +        options.testingModulesDir = os.path.join(tests_dir, 'modules')
> +        options.extraProfileFiles.append(os.path.join(self.distdir, 'plugins'))
> +        options.symbolsPath = os.path.join(self.distdir, 'crashreporter-symbols')
> +
> +        options.failureFile = failure_file_path

_tests/testing/mochitest/makefailures.json, right?

@@ +140,3 @@
>          if suite == 'plain':
> +            # Don't need additional options for plain.
> +            pass

(I think mochitest should have something like options.type = 'plain' | 'chrome' | ...)

@@ +157,3 @@
>  
> +        if rerun_failures:
> +            options.testManifest = failure_file_path

runOnlyTests to preserve current behaviour?

@@ +170,5 @@
> +    # This employs light Python magic. Keep in mind a decorator is just a
> +    # function that takes a function, does something with it, then returns a
> +    # (modified) function. Here, we chain decorators onto the passed in
> +    # function.
> +    debugger = CommandArgument('--debugger', '-d', metavar='DEBUGGER',

Nit: maybe leave an empty line between the comment (that applies to the whole function) and the debugger = ... line.

@@ +205,5 @@
> +
> +    path = CommandArgument('test_file', default=None, nargs='?',
> +        metavar='TEST',
> +        help='Test to run. Can be specified as a single file, a ' +\
> +            'directory, or omitted. If omitted, the entire test suite is ' +\

Is the +\ required?

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I think we're still missing:

"--appname",
"--utility-path",
"--certificate-path",
"--log-file",
"--timeout",
"--total-chunks",
"--this-chunk",
"--chunk-by-dir",
"--file-level", 
"--test-path",
"--webapprt-content",
"--webapprt-chrome",
"--setenv",
"--exclude-extension",
"--browser-arg",
"--leak-threshold",
"--install-extension",
"--profile-path",
"--use-vmware-recording",
"--run-only-tests",
"--exclude-tests",
"--test-manifest",
"--failure-file",

"--setpref",

"--xre-path",
"--symbols-path",
"--debugger-args",
"--debugger-interactive",

Comment 16

[Gregory Szorc [:gps]]

(In reply to :Ms2ger from comment #14)
> ::: testing/mochitest/mach_commands.py
> @@ +25,5 @@
> >  
> >      This currently contains just the basics for running mochitests. We may want
> >      to hook up result parsing, etc.
> >      """
> >      def run_plain_suite(self):
> 
> I think we got to the conclusion that these could be removed; could you do
> that?

Done.

> @@ +124,5 @@
> > +        options.autorun = not no_autorun
> > +        options.closeWhenDone = not keep_open
> > +        options.shuffle = shuffle
> > +        options.consoleLevel = 'INFO'
> > +        options.fatalAssertions = fatal_assertions
> 
> We dropped that

What was dropped? I still see these in runtests.py!

> 
> @@ +129,5 @@
> > +        options.repeat = repeat
> > +        options.runSlower = slow
> > +        options.testingModulesDir = os.path.join(tests_dir, 'modules')
> > +        options.extraProfileFiles.append(os.path.join(self.distdir, 'plugins'))
> > +        options.symbolsPath = os.path.join(self.distdir, 'crashreporter-symbols')
> 
> Presumably this could be overridden before?

Follow-up.

> @@ +131,5 @@
> > +        options.testingModulesDir = os.path.join(tests_dir, 'modules')
> > +        options.extraProfileFiles.append(os.path.join(self.distdir, 'plugins'))
> > +        options.symbolsPath = os.path.join(self.distdir, 'crashreporter-symbols')
> > +
> > +        options.failureFile = failure_file_path
> 
> _tests/testing/mochitest/makefailures.json, right?

If I cared about compatibility between mach and the make targets. Honestly, I don't. I think it's somewhat silly for the make target to be adding state to a directory that gets archived. Just created more confusion later. mach has it's own state directory that is meant for this type of thing.

> @@ +140,3 @@
> >          if suite == 'plain':
> > +            # Don't need additional options for plain.
> > +            pass
> 
> (I think mochitest should have something like options.type = 'plain' |
> 'chrome' | ...)

Tell me about it :/

> @@ +157,3 @@
> >  
> > +        if rerun_failures:
> > +            options.testManifest = failure_file_path
> 
> runOnlyTests to preserve current behaviour?

I didn't do this because runtests.py says runOnlyTests is deprecated and to use testManifest instead.

> @@ +205,5 @@
> > +
> > +    path = CommandArgument('test_file', default=None, nargs='?',
> > +        metavar='TEST',
> > +        help='Test to run. Can be specified as a single file, a ' +\
> > +            'directory, or omitted. If omitted, the entire test suite is ' +\
> 
> Is the +\ required?

Nope. (I always forget the line continuation rules for Python. So crazy.)

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4edaad0bb454

Let's get the first iteration in and rev on improvements in follow-ups.

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Gregory Szorc [:gps] from comment #16)
> (In reply to :Ms2ger from comment #14)
> > @@ +124,5 @@
> > > +        options.autorun = not no_autorun
> > > +        options.closeWhenDone = not keep_open
> > > +        options.shuffle = shuffle
> > > +        options.consoleLevel = 'INFO'
> > > +        options.fatalAssertions = fatal_assertions
> > 
> > We dropped that
> 
> What was dropped? I still see these in runtests.py!

The mach argument. I don't think fatal_assertions can be anything else than False here.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4edaad0bb454

Comment 20

[Nicholas Nethercote [inactive]]

How do I use the new options?  I want to use --keep-open, but this doesn't work:

  mach mochitest-chrome --keep-open toolkit/components/aboutmemory/tests/test_aboutmemory.xul

And |mach help mochitest-chrome| doesn't mention --keep-open or any of the other options.

Comment 21

[Gregory Szorc [:gps]]

If |mach help mochitest-chrome| doesn't mention --keep-open then you need to update your tree. If testing/mochitest/mach_commands.py is up to date, then WTF.

Comment 22

[Nicholas Nethercote [inactive]]

Ah, my tree was out of date.  Sorry for the noise.