818744 - mach command to run Python unit tests

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Gregory Szorc [:gps]]

Python unit tests can be difficult to run manually. I'd like to make it easier to run them by providing a mach command.

Bug 780448 is related.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: mach commands to run Python and Python unit tests. r=gps

I started by trying to build a unittest.TestSuite with all the given
files and directories, but it is surprisingly difficult and prone to
order-of-import errors that I couldn't work around.  By contrast, this
is inefficient but easy to understand and maintain.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 768024 [details] [diff] [review]
mach commands to run Python and Python unit tests. r=gps

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

|mach python| is a nice touch. You wouldn't believe how often people ask how to run a Python script in the context of the build system.

I wish we didn't have to reinvent test discovery. The code isn't too bad, so I'm not complaining too loudly.

Apply feedback. Get r+.

::: build/mach_bootstrap.py
@@ +63,5 @@
>      'testing/marionette/mach_commands.py',
>      'testing/mochitest/mach_commands.py',
>      'testing/xpcshell/mach_commands.py',
>      'tools/mach_commands.py',
> +    'python/mach_commands.py',

Alphabetize, please.

::: python/mach_commands.py
@@ +27,5 @@
> +    Easily run Python and Python unit tests.
> +    '''
> +    @CommandArgument('args', nargs=argparse.REMAINDER)
> +    @Command('python', category='devenv',
> +        description='Run Python.')

Nit: Please put @Command before @CommandArgument to aid readability.

@@ +29,5 @@
> +    @CommandArgument('args', nargs=argparse.REMAINDER)
> +    @Command('python', category='devenv',
> +        description='Run Python.')
> +    def python(self, args, pass_thru=True):
> +        executable = mozpack.path.join(self.topobjdir, '_virtualenv/bin/python')

On Windows, the Python executable is _virtualenv/Scripts/python.exe.

We should also test if this file exists before executing. If it doesn't we should prompt the user to run |mach configure| or |mach build|. (The virtualenv is created at the top of configure.)

@@ +53,5 @@
> +        # Python's unittest, and in particular discover, has problems with
> +        # clashing namespaces when importing multiple test modules. What follows
> +        # is a simple way to keep environments separate, at the price of
> +        # launching Python multiple times. This also runs tests via mozunit,
> +        # which produces output in the format Mozilla infrastructure expects.

Hmmm. I wonder if the 'nose' package has solved this problem. If so, I'd be tempted to just import it into the tree. It also facilitates better readability of test output, which is a lifesaver. I frequently manually install nose into my objdir's virtualenv and use it to run tests for this reason.

Don't let that hold up getting this landed, however.

@@ +70,5 @@
> +                         'Invalid test: {test}')
> +                if stop:
> +                    return 2
> +        for file in files:
> +            inner_return_code = self.python([file])

Nice trick! Although, this does rely on the target file having a __main__. I /think/ we have that currently since we unit mozunit everywhere, so it shouldn't be an issue. Might want to document it though, since typical Python test files tend to rely on external discovery rather than self invocation.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: mach commands to run Python and Python unit tests. r=gps

Addressed nits and restored a missing allow_all_args (I had this, then
removed it because the actual effect on argparse is non-obvious).

This version filters the log output, a cheap way to get highlighting
for TEST-*.  I didn't see an easy way to customize highlighting, so I
reuse TEST-UNEXPECTED-FAIL for status messages.  It's much easier to
see bad test inputs now.

Finally, I use the log filter to verify that each test file produces
log output.  No sense documenting what you can test for.

Finally, I'll leave nose et al. to others.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 769211 [details] [diff] [review]
mach commands to run Python and Python unit tests. r=gps

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

I'd prefer a lot of this code live outside a mach_commands.py file so it can be used elsewhere. But, I don't care enough to hold up review. There's always follow-up bugs.

::: python/mach_commands.py
@@ +31,5 @@
> +        MachCommandBase.__init__(self, context)
> +        self._python_executable = None
> +
> +    @property
> +    def python_executable(self):

This seems like something we should add to MachCommandBase or an ancestor! Followup?

@@ +46,5 @@
> +        path = mozpack.path.join(self.topobjdir, executable)
> +        if not os.path.exists(path):
> +            print("Could not find Python executable at %s." % path,
> +                  "Run |mach configure| or |mach build| to install it.")
> +            sys.exit(2)

Why 2? Why not 1?

@@ +99,5 @@
> +                if stop:
> +                    return 2
> +
> +        for file in files:
> +            file_displayed_test = {} # Used as a boolean.

This is what happens when your programming language doesn't declare variables :(

It's also common to use an empty list and .append(True)

Comment 5

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76b0a7f72e35

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Comment on attachment 769211 [details] [diff] [review]
> mach commands to run Python and Python unit tests. r=gps
> 
> Review of attachment 769211 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd prefer a lot of this code live outside a mach_commands.py file so it can
> be used elsewhere. But, I don't care enough to hold up review. There's
> always follow-up bugs.

Commented in Bug 858197.

> ::: python/mach_commands.py
> @@ +31,5 @@
> > +        MachCommandBase.__init__(self, context)
> > +        self._python_executable = None
> > +
> > +    @property
> > +    def python_executable(self):
> 
> This seems like something we should add to MachCommandBase or an ancestor!
> Followup?

Filed Bug 889684.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76b0a7f72e35