Add support for parallel presubmit unit testing.

Add support for parallel presubmit unit testing.

Enable parallel tests on depot_tools.
On Z620 presubmit times: 3m -> 35s.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195377

[Isaac (away), 2013-04-19 03:43:07]

Request for initial review.  I still need to fix tests/presubmit_unittest.py,
which is failing due to new members / different output text.  It appears the
other tests work even when run in parallel -- do you know of any conflicts that
might only occasionally occur?

The other main use case for this code is tools/build, where I use it to reduce
presubmit from 30s -> 8s, crrev.com/14246025.

[M-A Ruel, 2013-04-19 16:49:38]

I like the idea. I'm sad we didn't start with generators right at the start, it
would have made making the whole thing parallelizable from the start.

On 2013/04/19 03:43:07, Isaac wrote:
> Request for initial review.  I still need to fix tests/presubmit_unittest.py,
> which is failing due to new members / different output text.  It appears the
> other tests work even when run in parallel -- do you know of any conflicts
that
> might only occasionally occur?

Unit test expectation of in-order result. Maybe you could include the index in
the inputs so the output would include the index too, making it possible to
reconstruct the list exactly in order. This would make testing easier at least.

> The other main use case for this code is tools/build, where I use it to reduce
> presubmit from 30s -> 8s, crrev.com/14246025.

Yeah, that's awesome, thanks for doing this.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.py
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:146: 
two lines between file level symbols

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:150: super(OutputApi.PresubmitAddReviewers,
self).__init__('')
Stale reference to OutputApi (and below)

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:454: def RunTests(self, tests_mix, parallel=True):
Will parallel=False ever make sense?

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:467: msgs.extend(pool.map_async(CallCommand,
tests).get(99999))
Can you note why this is used? (e.g. the Ctrl-C handling)

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:472: return [m for m in msgs if m is not None]
return [m for m in msgs if m]
or
return filter(None, msgs)

I don't think '' messages are valuable (?)

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:1275: # multiprocessing requires a top level fun with a
single argument
fun -> function
I'd prefer the code to be inside the function.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:1282: #import pdb; pdb.set_trace()
Remove before committing

[Isaac (away), 2013-04-19 19:52:04]

Some comments -- I'll respond to the rest when I upload next patchset.

> Unit test expectation of in-order result. Maybe you could include the index in
> the inputs so the output would include the index too, making it possible to
> reconstruct the list exactly in order. This would make testing easier at
least.

AFAIK, my implementation preserves result order.  We get a list of presubmit
commands, map these to PresubmitResult objects, and then return the list.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.py
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:454: def RunTests(self, tests_mix, parallel=True):
On 2013/04/19 16:49:39, Marc-Antoine Ruel wrote:
> Will parallel=False ever make sense?

Yes, it is used by the legacy commands in presubmit_canned_checks.  I'll
probably switch those to parallell execution later and PRESUBMITs needing serial
execution can call input_api.RunUnitTests() multiple times.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:472: return [m for m in msgs if m is not None]
On 2013/04/19 16:49:39, Marc-Antoine Ruel wrote:
> return [m for m in msgs if m]
> or
> return filter(None, msgs)
> 
> I don't think '' messages are valuable (?)

The intention is that each item in msg list is either an instance of
PresubmitResult or None, so your suggestion should work fine.  I will add an
assertion somewhere to verify my precondition.

[Dirk Pranke, 2013-04-19 19:58:02]

On 2013/04/19 16:49:38, Marc-Antoine Ruel wrote:
> I like the idea. I'm sad we didn't start with generators right at the start,
it
> would have made making the whole thing parallelizable from the start.
> 

How would generators help here?

On a related note, in blink-land I have several classes and a driver that run
our python tests in parallel that are a bit fancier than what you have here. The
downside to using multiprocessing.pool() is in the join()  - you don't get any
feedback until everything completes, which can make things kinda frustrating,
and make it hard to debug problems. 

Perhaps we should consider moving the stuff I had into depot_tools?

[Dirk Pranke, 2013-04-19 19:58:38]

On 2013/04/19 19:52:04, Isaac wrote:
> Some comments -- I'll respond to the rest when I upload next patchset.
> 
> > Unit test expectation of in-order result. Maybe you could include the index
in
> > the inputs so the output would include the index too, making it possible to
> > reconstruct the list exactly in order. This would make testing easier at
> least.
> 
> AFAIK, my implementation preserves result order.  We get a list of presubmit
> commands, map these to PresubmitResult objects, and then return the list.
>

The results are returned in order, but the tests are not guaranteed to be
executed in order.

[Isaac (away), 2013-04-20 00:41:36]

PTAL

Unittests now pass.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.py
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:146: 
On 2013/04/19 16:49:39, Marc-Antoine Ruel wrote:
> two lines between file level symbols

Done.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:150: super(OutputApi.PresubmitAddReviewers,
self).__init__('')
On 2013/04/19 16:49:39, Marc-Antoine Ruel wrote:
> Stale reference to OutputApi (and below)

Done.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:467: msgs.extend(pool.map_async(CallCommand,
tests).get(99999))
On 2013/04/19 16:49:39, Marc-Antoine Ruel wrote:
> Can you note why this is used? (e.g. the Ctrl-C handling)

Done.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:1275: # multiprocessing requires a top level fun with a
single argument
On 2013/04/19 16:49:39, Marc-Antoine Ruel wrote:
> fun -> function
> I'd prefer the code to be inside the function.

Done.

https://chromiumcodereview.appspot.com/14247012/diff/5001/presubmit_support.p...
presubmit_support.py:1282: #import pdb; pdb.set_trace()
On 2013/04/19 16:49:39, Marc-Antoine Ruel wrote:
> Remove before committing

Done.

[M-A Ruel, 2013-04-20 02:24:55]

lgtm

https://chromiumcodereview.appspot.com/14247012/diff/9001/presubmit_canned_ch...
File presubmit_canned_checks.py (right):

https://chromiumcodereview.appspot.com/14247012/diff/9001/presubmit_canned_ch...
presubmit_canned_checks.py:707: if files == 1:
you forgot len()

https://chromiumcodereview.appspot.com/14247012/diff/9001/presubmit_support.py
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/14247012/diff/9001/presubmit_support.p...
presubmit_support.py:126: self._items = []
self._items = items or []

[Isaac (away), 2013-04-20 03:51:24]

Done.  Committing.

https://chromiumcodereview.appspot.com/14247012/diff/9001/presubmit_canned_ch...
File presubmit_canned_checks.py (right):

https://chromiumcodereview.appspot.com/14247012/diff/9001/presubmit_canned_ch...
presubmit_canned_checks.py:707: if files == 1:
On 2013/04/20 02:24:55, Marc-Antoine Ruel wrote:
> you forgot len()

Done.

https://chromiumcodereview.appspot.com/14247012/diff/9001/presubmit_support.py
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/14247012/diff/9001/presubmit_support.p...
presubmit_support.py:126: self._items = []
On 2013/04/20 02:24:55, Marc-Antoine Ruel wrote:
> self._items = items or []

Done.

[commit-bot: I haz the power, 2013-04-20 03:51:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ilevy@chromium.org/14247012/16001

[commit-bot: I haz the power, 2013-04-20 03:51:34]

Presubmit check for 14247012-16001 failed and returned exit status 1.

INFO:root:Found 4 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/tools/depot_tools/PRESUBMIT.py
Traceback (most recent call last):
  File "/b/commit-queue/verification/presubmit_shim.py", line 33, in <module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1305, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1129, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1046, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 106, in CheckChangeOnCommit
  File "<string>", line 26, in CommonChecks
AttributeError: 'InputApi' object has no attribute 'RunTests'

[Isaac (away), 2013-04-20 03:57:59]

Committed patchset #3 manually as r195377 (presubmit successful).