Fixing the PRESUBMIT include check some more.

Fixing the PRESUBMIT include check some more.

The previous version didn't take into account that source files
can have a platform specific suffix.

BUG=NONE
TBR=ben@chromium.org

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

[marja, 2012-11-15 16:14:56]

Hi maruel, could your review a fix again?

As an attempt to restore my honor, I'm also adding a test. ;)

[M-A Ruel, 2012-11-15 16:49:31]

https://codereview.chromium.org/11413012/diff/3/OWNERS
File OWNERS (right):

https://codereview.chromium.org/11413012/diff/3/OWNERS#newcode11
OWNERS:11: per-file PRESUBMIT_test.py=maruel@chromium.org
Replace the lines above with:
per-file PRESUBMIT*.py=maruel@chromium.org
per-file PRESUBMIT*.py=joi@chromium.org

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT.py#newcode658
PRESUBMIT.py:658: return results
Would want to run the unit test when PRESUBMIT.py is changed;

if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
  input_api.canned_checks.RunUnitTestsInDirectory(
            input_api, output_api,
            input_api.PresubmitLocalPath(),
            whitelist=[r'.+_test\.py$']))

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT_test.py
File PRESUBMIT_test.py (right):

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT_test.py#newcode1
PRESUBMIT_test.py:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
#!/usr/bin/env python

and do a chmod +x PRESUBMIT_test.py; git commit -am. before the next upload.

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT_test.py#newcode156
PRESUBMIT_test.py:156: 
two lines

[marja, 2012-11-15 17:48:46]

Thanks for comments!

https://codereview.chromium.org/11413012/diff/3/OWNERS
File OWNERS (right):

https://codereview.chromium.org/11413012/diff/3/OWNERS#newcode11
OWNERS:11: per-file PRESUBMIT_test.py=maruel@chromium.org
On 2012/11/15 16:49:31, Marc-Antoine Ruel wrote:
> Replace the lines above with:
> per-file mailto:PRESUBMIT*.py=maruel@chromium.org
> per-file mailto:PRESUBMIT*.py=joi@chromium.org

Done.

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT.py#newcode658
PRESUBMIT.py:658: return results
On 2012/11/15 16:49:31, Marc-Antoine Ruel wrote:
> Would want to run the unit test when PRESUBMIT.py is changed;
> 
> if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
>   input_api.canned_checks.RunUnitTestsInDirectory(
>             input_api, output_api,
>             input_api.PresubmitLocalPath(),
>             whitelist=[r'.+_test\.py$']))

Done.

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT_test.py
File PRESUBMIT_test.py (right):

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT_test.py#newcode1
PRESUBMIT_test.py:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2012/11/15 16:49:31, Marc-Antoine Ruel wrote:
> #!/usr/bin/env python
> 
> and do a chmod +x PRESUBMIT_test.py; git commit -am. before the next upload.

Done.

https://codereview.chromium.org/11413012/diff/3/PRESUBMIT_test.py#newcode156
PRESUBMIT_test.py:156: 
On 2012/11/15 16:49:31, Marc-Antoine Ruel wrote:
> two lines

Done.

[M-A Ruel, 2012-11-15 17:57:33]

lgtm

Before setting the bit, try breaking PRESUBMIT_test.py and running:
 presubmit_support.py PRESUBMIT.py

just to confirm the test was correctly run.
You can use NOTRY=true to bypass the try jobs if you want.

[marja, 2012-11-15 17:59:56]

On 2012/11/15 17:57:33, Marc-Antoine Ruel wrote:
> lgtm
> 
> Before setting the bit, try breaking PRESUBMIT_test.py and running:
>  presubmit_support.py PRESUBMIT.py
> 
> just to confirm the test was correctly run.

This time I did this before uploading the patch :)

Thanks for the review!

[marja, 2012-11-15 18:01:01]

On 2012/11/15 17:59:56, marja wrote:
> On 2012/11/15 17:57:33, Marc-Antoine Ruel wrote:
> > lgtm
> > 
> > Before setting the bit, try breaking PRESUBMIT_test.py and running:
> >  presubmit_support.py PRESUBMIT.py
> > 
> > just to confirm the test was correctly run.
> 
> This time I did this before uploading the patch :)
> 
> Thanks for the review!

... To be accurate, I didn't do exactly that, but broke PRESUBMIT_test.py and
did git cl upload, verified that it showed the error.

[M-A Ruel, 2012-11-15 18:02:36]

On 2012/11/15 18:01:01, marja wrote:
> On 2012/11/15 17:59:56, marja wrote:
> > On 2012/11/15 17:57:33, Marc-Antoine Ruel wrote:
> > > lgtm
> > > 
> > > Before setting the bit, try breaking PRESUBMIT_test.py and running:
> > >  presubmit_support.py PRESUBMIT.py
> > > 
> > > just to confirm the test was correctly run.
> > 
> > This time I did this before uploading the patch :)
> > 
> > Thanks for the review!
> 
> ... To be accurate, I didn't do exactly that, but broke PRESUBMIT_test.py and
> did git cl upload, verified that it showed the error.

It's the same result, thanks.

[commit-bot: I haz the power, 2012-11-15 18:15:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11413012/8002

[commit-bot: I haz the power, 2012-11-15 18:15:20]

Presubmit check for 11413012-8002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    

Presubmit checks took 1.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[marja, 2012-11-15 18:16:12]

Yup, getting this same error locally when trying to git cl dcommit... no idea :/

On 2012/11/15 18:15:20, I haz the power (commit-bot) wrote:
> Presubmit check for 11413012-8002 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     
> 
> Presubmit checks took 1.0s to calculate.
> 
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!

[M-A Ruel, 2012-11-15 18:22:05]

On 2012/11/15 18:16:12, marja wrote:
> Yup, getting this same error locally when trying to git cl dcommit... no idea
:/

+Dirk

Looking at the depot_tools/owners.py doc and from what I recall, per-file
supports glob?

[Dirk Pranke, 2012-11-15 19:13:41]

On 2012/11/15 18:22:05, Marc-Antoine Ruel wrote:
> On 2012/11/15 18:16:12, marja wrote:
> > Yup, getting this same error locally when trying to git cl dcommit... no
idea
> :/
> 
> +Dirk
> 
> Looking at the depot_tools/owners.py doc and from what I recall, per-file
> supports glob?

It's supposed to, and the syntax looks fine. Must be a bug ... feel free to tbr
this and I'll look into it.

[marja, 2012-11-15 19:48:55]

Ahh. Actually the OWNERS check is correct, I'm modifying OWNERS without LGTM
from anybody who is a OWNER of the OWNERS. Just the error message is a bit
confusing (missing an owner lgtm for "").

[Dirk Pranke, 2012-11-15 19:59:35]

On 2012/11/15 19:48:55, marja wrote:
> Ahh. Actually the OWNERS check is correct, I'm modifying OWNERS without LGTM
> from anybody who is a OWNER of the OWNERS. Just the error message is a bit
> confusing (missing an owner lgtm for "").

Oh. That's https://code.google.com/p/chromium/issues/detail?id=157191 .

[Isaac (away), 2013-01-06 10:35:48]

https://chromiumcodereview.appspot.com/11413012/diff/8002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/11413012/diff/8002/PRESUBMIT.py#newcod...
PRESUBMIT.py:663: whitelist=[r'.+_test\.py$']))
This seems overly broad.  Could we change this to run PRESUBMIT_test.py instead
of any python file that ends with test?

[M-A Ruel, 2013-01-07 15:27:04]

https://chromiumcodereview.appspot.com/11413012/diff/8002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/11413012/diff/8002/PRESUBMIT.py#newcod...
PRESUBMIT.py:663: whitelist=[r'.+_test\.py$']))
On 2013/01/06 10:35:48, Isaac wrote:
> This seems overly broad.  Could we change this to run PRESUBMIT_test.py
instead
> of any python file that ends with test?

Oh right, it should be r'[^\\/]+_test\.py$'.

[marja, 2013-01-07 16:34:14]

On 2013/01/07 15:27:04, Marc-Antoine Ruel wrote:
> https://chromiumcodereview.appspot.com/11413012/diff/8002/PRESUBMIT.py
> File PRESUBMIT.py (right):
> 
>
https://chromiumcodereview.appspot.com/11413012/diff/8002/PRESUBMIT.py#newcod...
> PRESUBMIT.py:663: whitelist=[r'.+_test\.py$']))
> On 2013/01/06 10:35:48, Isaac wrote:
> > This seems overly broad.  Could we change this to run PRESUBMIT_test.py
> instead
> > of any python file that ends with test?
> 
> Oh right, it should be r'[^\\/]+_test\.py$'.

Isn't the problem that this runs all "whatever_test.py" files in the current
directory, not that it runs any .py files in any subdirectories?

(RunUnitTestsInDirectory says: "Lists all files in a directory and runs them.
Doesn't recurse.".)

So, I'd just whitelist=[r'PRESUBMIT\.py'], is that OK?