Closed Bug 818819 Opened 9 years ago Closed 9 years ago

Wean "mach mochitest-*" off makefiles

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: Ms2ger, Assigned: gps)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
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!)
Depends on: 841713
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...
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #715341 - Flags: review?(ted)
Duplicate of this bug: 797096
Duplicate of this bug: 812583
Preserve log level from make targets.
Attachment #715341 - Attachment is obsolete: true
Attachment #715341 - Flags: review?(ted)
Attachment #715376 - Flags: review?(ted)
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?
(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.
(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.
(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 :)
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.
Attachment #715376 - Attachment is obsolete: true
Attachment #715376 - Flags: review?(ted)
Attachment #715564 - Flags: review?(Ms2ger)
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?
(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!
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 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?
Attachment #715564 - Flags: review?(Ms2ger) → review+
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",
(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.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4edaad0bb454

Let's get the first iteration in and rev on improvements in follow-ups.
Target Milestone: --- → mozilla22
(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.
https://hg.mozilla.org/mozilla-central/rev/4edaad0bb454
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 855562
Blocks: 855563
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.
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.
Ah, my tree was out of date.  Sorry for the noise.
Blocks: 857984
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.