Closed
Bug 844292
Opened 12 years ago
Closed 12 years ago
Add a mach target for GTest
Categories
(Firefox Build System :: Mach Core, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla22
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
(Whiteboard: [mach])
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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!
Depends on: 648681
Assignee | ||
Comment 3•12 years ago
|
||
Yes it is :). Let's rush 648681 along.
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #717579 -
Attachment is obsolete: true
Attachment #717579 -
Flags: review?(gps)
Attachment #717580 -
Flags: review?(gps)
Comment 6•12 years ago
|
||
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.
Attachment #717580 -
Flags: review?(gps) → feedback+
Assignee | ||
Comment 7•12 years ago
|
||
(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•12 years ago
|
||
Prereqs have landed! Put this up for review and let's get it checked in!
Status: ASSIGNED → NEW
Flags: needinfo?(bgirard)
Assignee | ||
Comment 9•12 years ago
|
||
(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.
Flags: needinfo?(bgirard)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #717580 -
Attachment is obsolete: true
Attachment #730253 -
Flags: review?(gps)
Comment 11•12 years ago
|
||
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.)
Attachment #730253 -
Flags: review?(gps) → feedback+
Assignee | ||
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
Feedback applied. Sadly this removes the color from GTest when running multiple jobs but I'm ok with that.
Attachment #730253 -
Attachment is obsolete: true
Attachment #730277 -
Flags: review?(gps)
Comment 14•12 years ago
|
||
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.
Attachment #730277 -
Flags: review?(gps) → review+
Assignee | ||
Comment 15•12 years ago
|
||
I forgot to address checking the error code for multijobs. It's trivial code really.
Attachment #730277 -
Attachment is obsolete: true
Attachment #730384 -
Flags: review?(gps)
Comment 16•12 years ago
|
||
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.
Attachment #730384 -
Flags: review?(gps) → review+
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #730384 -
Attachment is obsolete: true
Attachment #730397 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Attachment #730397 -
Attachment is obsolete: true
Assignee | ||
Comment 19•12 years ago
|
||
Comment 20•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Updated•11 years ago
|
Component: General → mach
Product: Testing → Core
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•