844292 - Add a mach target for GTest

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Comment 1

[Benoit Girard (:BenWa)]

The patches for GTest are going to land this weekend when inbound re-opens. All this needs to do is run firefox with '-unittest' flags. It doesn't need an empty profile or anything of that sort since it wont even load a profile for unit tests.

It would be nice to have a mach target to run firefox that worked on all platforms, I can easily do the rest based on top of that.

Currently I don't have any of GTest arguments hocked up but we can still control it using environment variables which is similar to what we do with mochitest/reftest. This doesn't need to be controled by mach initialized but it would be a nice to have. If we don't fix this here we can do a follow up. The most important env are found in this section which is worth reading:
http://code.google.com/p/googletest/wiki/AdvancedGuide#Running_Test_Programs:_Advanced_Options

GTEST_FILTER
GTEST_RANDOM_SEED

and also I'd like the test to run in parallel by default. This will force developers to write test that run in parallel (don't grab focus, specific sockets etc...)

GTEST_TOTAL_SHARDS / GTEST_SHARD_INDEX : http://code.google.com/p/googletest/wiki/AdvancedGuide#Distributing_Test_Functions_to_Multiple_Machines

We also have a custom 'MOZ_TBPL_PARSER' but it's for running on tinderbox and may not be needed with mach,

Comment 2

[Gregory Szorc [:gps]]

If it is literally |firefox -unittest| then the patch I just added to bug 648681 will pretty much make this an extremely simple mach command to implement!

Comment 3

[Benoit Girard (:BenWa)]

Yes it is :). Let's rush 648681 along.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch, depends on bug 648681 for path

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch, depends on bug 648681 for path

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 717580 [details] [diff] [review]
patch, depends on bug 648681 for path

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +227,5 @@
> +    @CommandArgument('gtest_filter', default='*', nargs='?', metavar='gtest_filter',
> +        help='test_filter is a \':\'-separated list of wildcard patterns (called the positive patterns),'
> +             'optionally followed by a \'-\' and another \':\'-separated pattern list (called the negative patterns).')
> +    @CommandArgument('--jobs', '-j', default='1', nargs='?', metavar='jobs',
> +        help='Run the tests in parallel using multiple processes.')

I think we should default to -j == # cores. See multiprocessing.cpu_count()

@@ +232,5 @@
> +    @CommandArgument('--tbpl-parser', '-t', action='store_true',
> +        help='Output test results in a format that can be parsed by TBPL.')
> +    @CommandArgument('--shuffle', '-s', action='store_true',
> +        help='Randomize the execution order of tests.')
> +    def gtest(self, **params):

Do we really need **params here? How about using named arguments like most Python methods.

@@ +256,5 @@
> +            gtest_env["GTEST_TOTAL_SHARDS"] = str(jobs)
> +            processes = {}
> +            for i in range(0, jobs):
> +                gtest_env["GTEST_SHARD_INDEX"] = str(i)
> +                processes[i] = subprocess.Popen([app_path, "-unittest"], env=gtest_env)

I'm not sure how I feel about using subprocess.Popen here. I believe this may result in interleaving of output from multiple processes on the same line (it depends on the platform).

If you use mozprocess.processhandler.ProcessHandlerMixin, you can register a callback that will be invoked with every line of output from the process. This way, all output is buffered through the mach command and thus no interleaving occurs.

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Gregory Szorc [:gps] from comment #6)

Thanks for the review. I'll be happy to finish this up once bug 648681 lands.

Comment 8

[Gregory Szorc [:gps]]

Prereqs have landed! Put this up for review and let's get it checked in!

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Gregory Szorc [:gps] from comment #6)
> @@ +232,5 @@
> > +    @CommandArgument('--tbpl-parser', '-t', action='store_true',
> > +        help='Output test results in a format that can be parsed by TBPL.')
> > +    @CommandArgument('--shuffle', '-s', action='store_true',
> > +        help='Randomize the execution order of tests.')
> > +    def gtest(self, **params):
> 
> Do we really need **params here? How about using named arguments like most
> Python methods.
> 

I just copied the example. I didn't know python could do that kind of reflection. Nice.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 730253 [details] [diff] [review]
patch

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

Very close to landing. I'd like to see the final revision before granting r+.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +8,4 @@
>  import logging
>  import operator
>  import os
> +import multiprocessing

Unused.

@@ +12,3 @@
>  import sys
>  import time
> +import subprocess

Unused.

@@ +21,5 @@
>  
>  from mozbuild.base import MachCommandBase
>  
> +# GTest parallel execution
> +from mozprocess import ProcessHandlerMixin

Please import this inside the command handler. (We try to minimize global imports to stdlib modules and to mach core modules because global imports are processed for every invocation of mach even if the command is not executed and excessive imports has a perf hit.)

@@ +279,5 @@
> +class GTestCommands(MachCommandBase):
> +    @Command('gtest', help='Run GTest unit tests.')
> +    @CommandArgument('gtest_filter', default='*', nargs='?', metavar='gtest_filter',
> +        help='test_filter is a \':\'-separated list of wildcard patterns (called the positive patterns),'
> +             'optionally followed by a \'-\' and another \':\'-separated pattern list (called the negative patterns).')

You can use double quotes for the string so you don't have to escape single quotes!

@@ +281,5 @@
> +    @CommandArgument('gtest_filter', default='*', nargs='?', metavar='gtest_filter',
> +        help='test_filter is a \':\'-separated list of wildcard patterns (called the positive patterns),'
> +             'optionally followed by a \'-\' and another \':\'-separated pattern list (called the negative patterns).')
> +    @CommandArgument('--jobs', '-j', default='1', nargs='?', metavar='jobs',
> +        help='Run the tests in parallel using multiple processes.')

You can pass "type=int" so the value is automagically validated as and converted to an int!

http://docs.python.org/2/library/argparse.html#type

@@ +292,5 @@
> +
> +        # Use GTest environment variable to control test execution
> +        # For details see:
> +        # https://code.google.com/p/googletest/wiki/AdvancedGuide#Running_Test_Programs:_Advanced_Options
> +        gtest_env = {"GTEST_FILTER": gtest_filter}

Because of a bug in Python < 2.7.3, passing a unicode type as an environment variable will fail. Normally this doesn't bite you in Python 2 because string literals are str (not unicode). But, since this file has unicode literals enabled (for Python 3 compat), this will bite you.

Do the following instead:

  gtest_env = {b'GTEST_FILTER': gtest_filter}

The b'' syntax is a raw, byte string. u'' is always unicode. '' is whatever the file defaults to. Crazy, I know.

@@ +295,5 @@
> +        # https://code.google.com/p/googletest/wiki/AdvancedGuide#Running_Test_Programs:_Advanced_Options
> +        gtest_env = {"GTEST_FILTER": gtest_filter}
> +
> +        if shuffle == True:
> +            gtest_env["GTEST_SHUFFLE"] = "True"

if shuffle:

@@ +300,5 @@
> +
> +        if tbpl_parser == True:
> +            gtest_env["MOZ_TBPL_PARSER"] = "True"
> +        
> +        if jobs == '1':

Fix the command argument and this will be an int.

@@ +301,5 @@
> +        if tbpl_parser == True:
> +            gtest_env["MOZ_TBPL_PARSER"] = "True"
> +        
> +        if jobs == '1':
> +            self.run_process([app_path, "-unittest"], append_env=gtest_env, pass_thru=True)

return self.run_process(...). Remove the else and unindent.

@@ +306,5 @@
> +        else:
> +            # The logging could be formatted to include the pid. It currently interleaves all outputs.
> +            jobs = int(jobs)
> +
> +            def handleLine(jobId, line):

Nit: No camelCase for function names: use underscores. handle_line

@@ +308,5 @@
> +            jobs = int(jobs)
> +
> +            def handleLine(jobId, line):
> +                # Prepend the jobId
> +                line = "[" + str(jobId+1) + "] " + line.strip()

Most Python programmers would write this as:

line = '[%d] %s]' % (job_id + 1, line.strip())

Strings are immutable in Python and building strings through builders is more efficient than concat. (I don't believe CPython has SpiderMonkey's "ropes" to make string concat more efficient.)

@@ +315,5 @@
> +            gtest_env["GTEST_TOTAL_SHARDS"] = str(jobs)
> +            processes = {}
> +            for i in range(0, jobs):
> +                gtest_env["GTEST_SHARD_INDEX"] = str(i)
> +                #processes[i] = subprocess.Popen([app_path, "-unittest"], env=gtest_env)

Kill commented line.

@@ +322,5 @@
> +                processes[i].run()
> +
> +            for i in range(0, jobs):
> +                processes[i].wait()
> +                processes[i].processOutput()

I /think/ you only need .wait() here.

We should capture exit code and return 0 if all were 0 or pick a random exit code if not all 0. (mach will use the return value for mach's exit code.)

Comment 12

[Benoit Girard (:BenWa)]

Ohh wow not even close :)

(In reply to Gregory Szorc [:gps] from comment #11)
> @@ +308,5 @@
> > +            jobs = int(jobs)
> > +
> > +            def handleLine(jobId, line):
> > +                # Prepend the jobId
> > +                line = "[" + str(jobId+1) + "] " + line.strip()
> 
> Most Python programmers would write this as:
> 
> line = '[%d] %s]' % (job_id + 1, line.strip())
> 

I assumed the extra ']' is a typo. Removed.

Comment 13

[Benoit Girard (:BenWa)]

Attachment: patch

Feedback applied. Sadly this removes the color from GTest when running multiple jobs but I'm ok with that.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 730277 [details] [diff] [review]
patch

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

This looks good enough to r+. I trust you'll fix the nits.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +290,5 @@
> +        # https://code.google.com/p/googletest/wiki/AdvancedGuide#Running_Test_Programs:_Advanced_Options
> +        gtest_env = {b'GTEST_FILTER': gtest_filter}
> +
> +        if shuffle:
> +            gtest_env["GTEST_SHUFFLE"] = "True"

You need to b'' all gtest_env instances.

@@ +294,5 @@
> +            gtest_env["GTEST_SHUFFLE"] = "True"
> +
> +        if tbpl_parser:
> +            gtest_env["MOZ_TBPL_PARSER"] = "True"
> +        

Nit: trailing whitespace.

@@ +296,5 @@
> +        if tbpl_parser:
> +            gtest_env["MOZ_TBPL_PARSER"] = "True"
> +        
> +        if jobs == 1:
> +            return self.run_process([app_path, "-unittest"], append_env=gtest_env, pass_thru=True)

I forgot - you need to add ensure_exit_code=False or else an exception will be raised if the exit code is non-0.

@@ +303,5 @@
> +        import functools
> +        def handle_line(job_id, line):
> +            # Prepend the jobId
> +            line = '[%d] %s' % (job_id + 1, line.strip())
> +            self.log(logging.INFO, "GTest", {'line': line}, '{line}')

if print() preserves color, go with that.

@@ +310,5 @@
> +        processes = {}
> +        for i in range(0, jobs):
> +            gtest_env["GTEST_SHARD_INDEX"] = str(i)
> +            processes[i] = ProcessHandlerMixin([app_path, "-unittest"], env=gtest_env,
> +                             processOutputLine=[functools.partial(handle_line, i)], universal_newlines=True)

Nit: please wrap long line.

Comment 15

[Benoit Girard (:BenWa)]

Attachment: patch

I forgot to address checking the error code for multijobs. It's trivial code really.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 730384 [details] [diff] [review]
patch

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

r+ without lower.py changes.

::: ipc/ipdl/ipdl/lower.py
@@ +3854,5 @@
> +        ##ifdef DEBUG
> +        #   if (!ok) {
> +        #     NS_RUNTIMEABORT("bad Shmem");
> +        #   }
> +        ##endif // DEBUG

Oops.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +298,1 @@
>          

Nit: trailing whitespace

@@ +325,5 @@
> +            if status != 0:
> +              failed_job = True
> +        
> +        if failed_job:
> +            return 1

How about:

exit_code = 0

for process in processes.values():
    status = process.wait()
    if status:
        exit_code = status

return status

(I assume we don't care about wait ordering.)

@@ +326,5 @@
> +              failed_job = True
> +        
> +        if failed_job:
> +            return 1
> +        

Nite: 2 lines with trailing whitespace.

Comment 17

[Benoit Girard (:BenWa)]

Attachment: patch r=gps

Comment 18

[Benoit Girard (:BenWa)]

Attachment: patch r=gps

Stupid mq

Comment 19

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30bea74a353e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30bea74a353e