Add presubmit check to lint test files.

Add presubmit check to lint test files.

BUG=5339
TEST=report error by "gcl presubmit" with bad test_expectations.txt.


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

[Mohamed Mansour, 2009-07-31 06:02:50]

looks good (wait for lgtm from maruel since he is a presubmit king)
I tested it and it works great! 

Should change description TEST to be valid, since you tested it :)

[M-A Ruel, 2009-07-31 09:51:56]

http://codereview.chromium.org/160442/diff/1/2
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/1/2#newcode11
Line 11: import sys
remove these

http://codereview.chromium.org/160442/diff/1/2#newcode19
Line 19: sys.path.append(curdir)
That should not be done this way. With this call, you modify the behavior of any
other PRESUBMIT script that would run later. This is important that a presubmit
script doesn't modify the current environment so it doesn't affect the following
PRESUBMIT.py that would be run (in a upper directory for instance). 

I think it'd be safer to shell out test_expectation instead. You can take a look
at input_api.canned_checks. RunPythonUnitTests(). Its implementation is at 
http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/presubmit_canne...

Note that this canned check has a bug pending on linux though since it doesn't
set PYTHONPATH correctly.

http://codereview.chromium.org/160442/diff/1/2#newcode55
Line 55: if TEST_EXPECTATIONS in [os.path.basename(path) for path in
input_api.LocalPaths()]:
80 cols

[ukai, 2009-07-31 10:39:45]

Thanks for review!

http://codereview.chromium.org/160442/diff/1/2
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/1/2#newcode11
Line 11: import sys
On 2009/07/31 09:51:57, Marc-Antoine Ruel wrote:
> remove these

OK, but I'd like to use os.pathsep.
Could you add it in input_api?

http://codereview.chromium.org/160442/diff/1/2#newcode19
Line 19: sys.path.append(curdir)
On 2009/07/31 09:51:57, Marc-Antoine Ruel wrote:
> That should not be done this way. With this call, you modify the behavior of
any
> other PRESUBMIT script that would run later. This is important that a
presubmit
> script doesn't modify the current environment so it doesn't affect the
following
> PRESUBMIT.py that would be run (in a upper directory for instance). 
> 
> I think it'd be safer to shell out test_expectation instead. You can take a
look
> at input_api.canned_checks. RunPythonUnitTests(). Its implementation is at 
>
http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/presubmit_canne...

Thanks!
 
> Note that this canned check has a bug pending on linux though since it doesn't
> set PYTHONPATH correctly.

I think we can fix it by using os.pathsep?

http://codereview.chromium.org/160442/diff/1/2#newcode55
Line 55: if TEST_EXPECTATIONS in [os.path.basename(path) for path in
input_api.LocalPaths()]:
On 2009/07/31 09:51:57, Marc-Antoine Ruel wrote:
> 80 cols

Done.

[M-A Ruel, 2009-07-31 10:53:01]

http://codereview.chromium.org/160442/diff/7/8
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/7/8#newcode1
Line 1: #!/usr/bin/python
Remove this line, this file is not meant to be run as-is.

http://codereview.chromium.org/160442/diff/7/8#newcode25
Line 25: subproc = input_api.subprocess.Popen(
can you extract the command argument out? That would help the readability of the
function call.

http://codereview.chromium.org/160442/diff/7/8#newcode38
Line 38: # TODO(ukai): consolidate run_webkit_tests --lint-test-files rerpots.
rerpots?

http://codereview.chromium.org/160442/diff/7/8#newcode40
Line 40: for line in stdoutdata.split('\n'):
stdoutdata.splitlines()

http://codereview.chromium.org/160442/diff/7/8#newcode47
Line 47: results += [output_api.PresubmitError('Lint error %s' % stdoutdata)]
Maybe you should just output the error lines? Wouldn't it be too much data
otherwise? I don't know, just asking.
Also, it would maybe be a good idea to put it in long_text=stdoutdata

[ukai, 2009-08-03 05:02:15]

Thanks for review!

http://codereview.chromium.org/160442/diff/7/8
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/7/8#newcode1
Line 1: #!/usr/bin/python
On 2009/07/31 10:53:01, Marc-Antoine Ruel wrote:
> Remove this line, this file is not meant to be run as-is.

Done.

http://codereview.chromium.org/160442/diff/7/8#newcode25
Line 25: subproc = input_api.subprocess.Popen(
On 2009/07/31 10:53:01, Marc-Antoine Ruel wrote:
> can you extract the command argument out? That would help the readability of
the
> function call.

Done.

http://codereview.chromium.org/160442/diff/7/8#newcode38
Line 38: # TODO(ukai): consolidate run_webkit_tests --lint-test-files rerpots.
On 2009/07/31 10:53:01, Marc-Antoine Ruel wrote:
> rerpots?

Done.

http://codereview.chromium.org/160442/diff/7/8#newcode40
Line 40: for line in stdoutdata.split('\n'):
On 2009/07/31 10:53:01, Marc-Antoine Ruel wrote:
> stdoutdata.splitlines()

Done.

[M-A Ruel, 2009-08-03 13:05:11]

lgtm, you can commit with the folliwing changes.

http://codereview.chromium.org/160442/diff/12/1002
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/12/1002#newcode16
Line 16: curdir = input_api.PresubmitLocalPath()
Use current_dir instead.

http://codereview.chromium.org/160442/diff/12/1002#newcode19
Line 19: pythonpaths += [input_api.os_path.join(curdir,
'../../../tools/python')]
Simpler instead:
python_paths = [
  current_dir,
  input_api.os_path.join(current_dir, '..', '..', '..', 'tools', 'python'),
]

http://codereview.chromium.org/160442/diff/12/1002#newcode28
Line 28: ]
align -4 spaces

http://codereview.chromium.org/160442/diff/12/1002#newcode36
Line 36: stdoutdata, stderrdata = subproc.communicate()
Since you don't use stderrdata, maybe it'd be better to not pipe it.

http://codereview.chromium.org/160442/diff/12/1002#newcode46
Line 46: results += [output_api.PresubmitError('Lint error %s' % error,
nit: Output will be illformated since the first error will be on the same line
and the rest will be aligned at column 0. Not a big deal.

[M-A Ruel, 2009-08-03 14:02:50]

2 additional changes (I just found out about os.path.pathsep).

http://codereview.chromium.org/160442/diff/12/1002
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/12/1002#newcode11
Line 11: import os  # for os.pathsep
Remove this.

http://codereview.chromium.org/160442/diff/12/1002#newcode23
Line 23: env['PYTHONPATH'] = os.pathsep.join(pythonpaths)
Use os.path.pathsep instead.

[ukai, 2009-08-04 00:59:25]

Thanks for review

http://codereview.chromium.org/160442/diff/12/1002
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/12/1002#newcode11
Line 11: import os  # for os.pathsep
On 2009/08/03 14:02:50, Marc-Antoine Ruel wrote:
> Remove this.

Done.

http://codereview.chromium.org/160442/diff/12/1002#newcode16
Line 16: curdir = input_api.PresubmitLocalPath()
On 2009/08/03 13:05:11, Marc-Antoine Ruel wrote:
> Use current_dir instead.

Done.

http://codereview.chromium.org/160442/diff/12/1002#newcode19
Line 19: pythonpaths += [input_api.os_path.join(curdir,
'../../../tools/python')]
On 2009/08/03 13:05:11, Marc-Antoine Ruel wrote:
> Simpler instead:
> python_paths = [
>   current_dir,
>   input_api.os_path.join(current_dir, '..', '..', '..', 'tools', 'python'),
> ]

Done.

http://codereview.chromium.org/160442/diff/12/1002#newcode23
Line 23: env['PYTHONPATH'] = os.pathsep.join(pythonpaths)
On 2009/08/03 14:02:50, Marc-Antoine Ruel wrote:
> Use os.path.pathsep instead.

Done.

http://codereview.chromium.org/160442/diff/12/1002#newcode28
Line 28: ]
On 2009/08/03 13:05:11, Marc-Antoine Ruel wrote:
> align -4 spaces

Done.

http://codereview.chromium.org/160442/diff/12/1002#newcode36
Line 36: stdoutdata, stderrdata = subproc.communicate()
On 2009/08/03 13:05:11, Marc-Antoine Ruel wrote:
> Since you don't use stderrdata, maybe it'd be better to not pipe it.

Hmm, what do you mean?
It seems run_webkit_tests --lint-test-files reports "Path does not exist" on
stderr and other errors on stdout, so I set stderr merge into stdout.

http://codereview.chromium.org/160442/diff/12/1002#newcode46
Line 46: results += [output_api.PresubmitError('Lint error %s' % error,
On 2009/08/03 13:05:11, Marc-Antoine Ruel wrote:
> nit: Output will be illformated since the first error will be on the same line
> and the rest will be aligned at column 0. Not a big deal.

Done.

[M-A Ruel, 2009-08-04 12:46:25]

lgtm with nits

http://codereview.chromium.org/160442/diff/17/1004
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/17/1004#newcode16
Line 16: pythonpaths = [
in fact per style guide it should be python_paths.

http://codereview.chromium.org/160442/diff/17/1004#newcode43
Line 43: if input_api.re.match('^.*ERROR Line:', line):
Wouldn't 'ERROR Line:' work? I don't understand the need for both '^' and '.*'.
Also, you duplicate a line. So rewrite this as:
if (input_api.re.match('^Line:', line) or
    input_api.re.match('ERROR Line:', line)):
  error += line + '\n'

http://codereview.chromium.org/160442/diff/17/1004#newcode46
Line 46: results += [output_api.PresubmitError('Lint error\n%s' % error,
We really prefer .append() and .extend() since the python list  += operator is
easy to mess up. Please change the 4 uses of it.
But you don't really need results at all. You should just remove this variable.

[ukai, 2009-08-05 01:12:26]

http://codereview.chromium.org/160442/diff/17/1004
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/17/1004#newcode16
Line 16: pythonpaths = [
On 2009/08/04 12:46:25, Marc-Antoine Ruel wrote:
> in fact per style guide it should be python_paths.

Done.

http://codereview.chromium.org/160442/diff/17/1004#newcode43
Line 43: if input_api.re.match('^.*ERROR Line:', line):
On 2009/08/04 12:46:25, Marc-Antoine Ruel wrote:
> Wouldn't 'ERROR Line:' work? I don't understand the need for both '^' and
'.*'.

re.match seems to imply '^', so I change to use re.search for it.

> Also, you duplicate a line. So rewrite this as:
> if (input_api.re.match('^Line:', line) or
>     input_api.re.match('ERROR Line:', line)):
>   error += line + '\n'
> 

Done.

http://codereview.chromium.org/160442/diff/17/1004#newcode46
Line 46: results += [output_api.PresubmitError('Lint error\n%s' % error,
On 2009/08/04 12:46:25, Marc-Antoine Ruel wrote:
> We really prefer .append() and .extend() since the python list  += operator is
> easy to mess up. Please change the 4 uses of it.
> But you don't really need results at all. You should just remove this
variable.

Done.

[M-A Ruel, 2009-08-05 01:22:31]

You know what that means? Please check-in ASAP before I invent yet another way
to rewrite this. :)

http://codereview.chromium.org/160442/diff/1007/1008
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/1007/1008#newcode39
Line 39: for line in stdoutdata.splitlines():
stdout_data = subproc.communicate()[0]
# TODO(ukai): consolidate run_webkit_tests --lint-test-files reports.
is_error = lambda line: (input_api.re.match('Line:', line) or
                         input_api.re.search('ERROR Line:', line))
error = filter(is_error, stdout_data.splitlines())
if error:
  return [output_api.PresubmitError('Lint error\n%s' % '\n'.join(error),
                                    long_text=stdout_data)]

[ukai, 2009-08-05 01:31:18]

Thanks for review.
I'll check it in :)

http://codereview.chromium.org/160442/diff/1007/1008
File webkit/tools/layout_tests/PRESUBMIT.py (right):

http://codereview.chromium.org/160442/diff/1007/1008#newcode39
Line 39: for line in stdoutdata.splitlines():
On 2009/08/05 01:22:31, Marc-Antoine Ruel wrote:
> stdout_data = subproc.communicate()[0]
> # TODO(ukai): consolidate run_webkit_tests --lint-test-files reports.
> is_error = lambda line: (input_api.re.match('Line:', line) or
>                          input_api.re.search('ERROR Line:', line))
> error = filter(is_error, stdout_data.splitlines())
> if error:
>   return [output_api.PresubmitError('Lint error\n%s' % '\n'.join(error),
>                                     long_text=stdout_data)]

Done.

[M-A Ruel, 2009-08-05 01:33:52]

On 2009/08/05 01:31:18, ukai wrote:
> Done.

This code is now perfect.