Allow PRESUBMIT.py files to have old and new-style trybot specifications (as long as each file is c…

Allow PRESUBMIT.py files to have old and new-style trybot specifications (as long as each file is consistent).

BUG=278554
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=241590

[ghost stip (do not use), 2013-11-15 17:52:39]

ptal

This solves the issue where src/PRESUBMIT.py is in one format but
src/chrome/PRESUBMIT.py is in another.

[M-A Ruel, 2013-11-15 20:09:32]

https://codereview.chromium.org/74163002/diff/1/presubmit_support.py
File presubmit_support.py (left):

https://codereview.chromium.org/74163002/diff/1/presubmit_support.py#oldcode1084
presubmit_support.py:1084: results += executer.ExecPresubmitScript(
I dislike using += on list anyway.

https://codereview.chromium.org/74163002/diff/1/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/74163002/diff/1/presubmit_support.py#newcode1086
presubmit_support.py:1086: if (all(isinstance(i, tuple) for i in result) or
Add a comment:
# Ensure it's either all old-style or new-style.

https://codereview.chromium.org/74163002/diff/1/presubmit_support.py#newcode1091
presubmit_support.py:1091: for filename in presubmit_files:
is this missing an else: ?

https://codereview.chromium.org/74163002/diff/1/presubmit_support.py#newcode1101
presubmit_support.py:1101: results += result
results.extend(result)

[ghost stip (do not use), 2013-11-22 01:24:15]

ptal

https://codereview.chromium.org/74163002/diff/1/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/74163002/diff/1/presubmit_support.py#newcode1091
presubmit_support.py:1091: for filename in presubmit_files:
On 2013/11/15 20:09:33, M-A Ruel wrote:
> is this missing an else: ?

I don't think so? this is how the code was before

[ghost stip (do not use), 2013-12-03 22:03:29]

ping?

[M-A Ruel, 2013-12-04 01:02:30]

Did you want to write an unit test?

https://chromiumcodereview.appspot.com/74163002/diff/70001/presubmit_support.py
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/74163002/diff/70001/presubmit_support....
presubmit_support.py:1113: slaves = list(slave_dict.iteritems())
slaves = slave_dict.items()

https://chromiumcodereview.appspot.com/74163002/diff/70001/presubmit_support....
presubmit_support.py:1115: slaves += list(set(old_style))
slaves.extend(set(old_style))

https://chromiumcodereview.appspot.com/74163002/diff/70001/trychange.py
File trychange.py (right):

https://chromiumcodereview.appspot.com/74163002/diff/70001/trychange.py#newco...
trychange.py:391: bot_spec += _ApplyTestFilter(options.testfilter, new_style)
I really dislike += on list. It does silly things on str.

[Isaac (away), 2013-12-04 03:47:43]

drive by
   .--.
.-'    `--.    
'-()---()--'

https://chromiumcodereview.appspot.com/74163002/diff/70001/presubmit_support.py
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/74163002/diff/70001/presubmit_support....
presubmit_support.py:1105: 
if you make results a list of lists instead of flattening using extend, you
could mostly preserve the code below and keep the checks there.

https://chromiumcodereview.appspot.com/74163002/diff/70001/presubmit_support....
presubmit_support.py:1112: slave_dict.setdefault(result[0],
set()).update(result[1])
now that we're on 2.7, how about collections.defaultdict?  Also generators are
usually preferred over filter:

slave_dict = collections.defaultdict(set)
for r in results:
  if isinstance(x, tuple):  # preferred style
    slave_dict[results[0]].update[results[1]]

# list of slave -> set of tests
slaves = slave_dict.items() 
slaves.extend(set(r for r in results if isinstance(r, basestring)))


Are you sure this is what you want?  This won't condense duplication of bots
between old and new styles.

[ghost stip (do not use), 2013-12-18 00:43:08]

updated, ptal

https://codereview.chromium.org/74163002/diff/70001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/74163002/diff/70001/presubmit_support.py#newc...
presubmit_support.py:1112: slave_dict.setdefault(result[0],
set()).update(result[1])
On 2013/12/04 03:47:43, Isaac wrote:
> now that we're on 2.7, how about collections.defaultdict?  Also generators are
> usually preferred over filter:

we're not yet on 2.7, see crbug.com/328453.

filter is extremely concise here. generators are for nerds.

> 
> slave_dict = collections.defaultdict(set)
> for r in results:
>   if isinstance(x, tuple):  # preferred style
>     slave_dict[results[0]].update[results[1]]
> 
> # list of slave -> set of tests
> slaves = slave_dict.items() 
> slaves.extend(set(r for r in results if isinstance(r, basestring)))
> 
> 
> Are you sure this is what you want?  This won't condense duplication of bots
> between old and new styles.

That's outside the scope of this CL.

https://codereview.chromium.org/74163002/diff/70001/presubmit_support.py#newc...
presubmit_support.py:1113: slaves = list(slave_dict.iteritems())
On 2013/12/04 01:02:31, M-A Ruel wrote:
> slaves = slave_dict.items()

not sure if that's what we want, we want (bot, tests) tuple given by iteritems()

[M-A Ruel, 2013-12-18 00:52:42]

lgtm. I had hoped you were a nerd.

https://codereview.chromium.org/74163002/diff/70001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/74163002/diff/70001/presubmit_support.py#newc...
presubmit_support.py:1113: slaves = list(slave_dict.iteritems())
On 2013/12/18 00:43:08, stip wrote:
> On 2013/12/04 01:02:31, M-A Ruel wrote:
> > slaves = slave_dict.items()
> 
> not sure if that's what we want, we want (bot, tests) tuple given by
iteritems()

http://docs.python.org/2/library/stdtypes.html#dict.items

https://codereview.chromium.org/74163002/diff/110001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/74163002/diff/110001/presubmit_support.py#new...
presubmit_support.py:1070: if not (valid_oldstyle(result) or
valid_newstyle(result)):
optional nit: I think the following would be slightly more readable but I don't
mind:
if not valid_oldstyle(result) and not valid_newstyle(result):

https://codereview.chromium.org/74163002/diff/110001/presubmit_support.py#new...
presubmit_support.py:1130: output_stream.write(', '.join((str(x) for x in
slaves)))
sorted(slaves) would make a more deterministic output.

[commit-bot: I haz the power, 2013-12-18 17:42:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stip@chromium.org/74163002/130001

[commit-bot: I haz the power, 2013-12-18 17:44:59]

Change committed as 241590