Adds a 'webkit_lint' test step to the bots

Adds a 'webkit_lint' test step to the bots that will run before the actual
layout tests and check for any errors in the test_expectations.txt file.

Currently errors in the test_expectations.txt file get caught during the
webkit test run and prevent any actual tests from running. We want to
break this out separately so we can diagnose errors in the file
separately from actual layout_test failures.

R=ojan@chromium.org,nsylvain@chromium.org
BUG=110940
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119148

[ojan, 2012-01-25 03:06:01]

lint_**test_files_wrapper.py LGTM, except for the TODO. I don't see how you
intend to ever remove the "--chromium".

On Tue, Jan 24, 2012 at 5:17 PM, <dpranke@chromium.org> wrote:

> Reviewers: ojan, Nicolas Sylvain,
>
> Description:
> Adds a 'webkit_lint' test step to the bots that will run before the actual
> layout tests and check for any errors in
> the test_expectations.txt file.
>
> Currently errors in the test_expectations.txt file get caught during the
> webkit test run and prevent any actual tests from running. We want to
> break this out separately so we can diagnose errors in the file
> separately from actual layout_test failures.
>
> R=ojan@chromium.org,nsylvain@**chromium.org <nsylvain@chromium.org>
> BUG=110940
>
> Please review this at
https://chromiumcodereview.**appspot.com/9138042/<https://chromiumcodereview....
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/tools/build/<http://svn.chromium.org/chrome/trunk/tools/build/>
>
> Affected files:
>  M     masters/master.chromium.**webkit/master_linux_webkit_**
> latest_cfg.py
>  M     scripts/master/factory/**chromium_commands.py
>  M     scripts/master/factory/**chromium_factory.py
>  A     scripts/slave/chromium/lint_**test_files_wrapper.py
>  M     scripts/slave/chromium_**commands.py
>
>
> Index: masters/master.chromium.**webkit/master_linux_webkit_**
> latest_cfg.py
> ==============================**==============================**=======
> --- masters/master.chromium.**webkit/master_linux_webkit_**latest_cfg.py
>    (revision 118512)
> +++ masters/master.chromium.**webkit/master_linux_webkit_**latest_cfg.py
>    (working copy)
> @@ -32,7 +32,7 @@
>  B('Webkit Linux', 'f_webkit_linux_rel', scheduler='s6_webkit_rel',
>   auto_reboot=True)
>  F('f_webkit_linux_rel', linux().**ChromiumWebkitLatestFactory(
> -    tests=['test_shell', 'webkit', 'webkit_gpu', 'webkit_unit'],
> +    tests=['test_shell', 'webkit_lint', 'webkit', 'webkit_gpu',
> 'webkit_unit'],
>     options=['--compiler=goma', 'test_shell', 'test_shell_tests',
>              'webkit_unit_tests', 'DumpRenderTree'],
>     factory_properties={'archive_**webkit_results': True,
> @@ -57,7 +57,7 @@
>  B('Webkit Linux (dbg)', 'f_webkit_dbg_tests', scheduler='s6_webkit_dbg')
>  F('f_webkit_dbg_tests', linux().**ChromiumWebkitLatestFactory(
>     target='Debug',
> -    tests=['test_shell', 'webkit', 'webkit_gpu', 'webkit_unit'],
> +    tests=['test_shell', 'webkit_lint', 'webkit', 'webkit_gpu',
> 'webkit_unit'],
>     options=['--compiler=goma', 'test_shell', 'test_shell_tests',
>              'webkit_unit_tests', 'DumpRenderTree'],
>     factory_properties={'archive_**webkit_results': True,
> Index: scripts/master/factory/**chromium_commands.py
> ==============================**==============================**=======
> --- scripts/master/factory/**chromium_commands.py (revision 118512)
> +++ scripts/master/factory/**chromium_commands.py (working copy)
> @@ -745,6 +745,15 @@
>                      test_command=cmd,
>                      do_step_if=self.**TestStepFilter)
>
> +  def AddWebkitLint(self, factory_properties=None):
> +    """Adds a step to the factory to lint the WebKit
> test_expectations.txt file."""
> +    factory_properties = factory_properties or {}
> +    cmd = [self._python, self._lint_test_files_tool]
> +    self.AddTestStep(shell.**ShellCommand,
> +                     test_name='lint_test_files',
> +                     test_command=cmd,
> +                     do_step_if=self.**TestStepFilter)
> +
>   def AddWebkitTests(self, gpu, factory_properties=None):
>     """Adds a step to the factory to run the WebKit layout tests.
>
> Index: scripts/master/factory/**chromium_factory.py
> ==============================**==============================**=======
> --- scripts/master/factory/**chromium_factory.py  (revision 118512)
> +++ scripts/master/factory/**chromium_factory.py  (working copy)
> @@ -320,6 +320,7 @@
>     if R('devtools_perf'):  f.AddDevToolsTests(factory_**properties=fp)
>     if R('webkit_gpu'):     f.AddWebkitTests(gpu=True,
>                                              factory_properties=fp)
> +    if R('webkit_lint'):    f.AddWebkitLint(factory_**properties=fp)
>
>     # Benchmark tests:
>     if R('page_cycler_moz'):
> Index: scripts/slave/chromium/lint_**test_files_wrapper.py
> ==============================**==============================**=======
> --- scripts/slave/chromium/lint_**test_files_wrapper.py   (revision 0)
> +++ scripts/slave/chromium/lint_**test_files_wrapper.py   (revision 0)
> @@ -0,0 +1,32 @@
> +#!/usr/bin/env python
> +# Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +# Use of this source code is governed by a BSD-style license that can be
> +# found in the LICENSE file.
> +
> +"""A wrapper script to check the test_expectations.txt file for errors."""
> +
> +import optparse
> +import os
> +
> +from common import chromium_utils
> +from slave import slave_utils
> +
> +
> +def layout_test(options, args):
> +  """Parse options and call run_webkit_tests.py, using Python from the
> tree."""
> +  build_dir = os.path.abspath(options.build_**dir)
> +  webkit_tests_dir = chromium_utils.FindUpward(**build_dir,
> +                                              'webkit', 'tools',
> 'layout_tests')
> +  run_webkit_tests = os.path.join(webkit_tests_dir, 'run_webkit_tests.py')
> +  # TODO(dpranke): '--chromium' shouldn't be needed.
> +  command = [run_webkit_tests, '--lint-test-files', '--chromium']
> +  return slave_utils.**RunPythonCommandInBuildDir(**build_dir,
> options.target,
> +                                                command)
> +
> +def main():
> +  option_parser = optparse.OptionParser()
> +  options, args = option_parser.parse_args()
> +  return layout_test(options, args)
> +
> +if '__main__' == __name__:
> +  sys.exit(main())
>
> Property changes on: scripts/slave/chromium/lint_**test_files_wrapper.py
> ______________________________**______________________________**_______
> Added: svn:executable
>   + *
> Added: svn:eol-style
>   + LF
>
> Index: scripts/slave/chromium_**commands.py
> ==============================**==============================**=======
> --- scripts/slave/chromium_**commands.py  (revision 118512)
> +++ scripts/slave/chromium_**commands.py  (working copy)
> @@ -143,7 +143,6 @@
>   def __init__(self, *args, **kwargs):
>     # TODO(maruel): Mainly to keep pylint happy, remove once buildbot fixed
>     # their style.
> -    self.branch = None
>     self.srcdir = None
>     self.revision = None
>     self.env = None
> @@ -277,13 +276,7 @@
>     # GClient accepts --revision argument of two types 'module@rev' and
> 'rev'.
>     if self.revision:
>       command.append('--revision')
> -      if (not self.branch or
> -          self.no_gclient_branch or
> -          '@' in str(self.revision)):
> -        command.append(str(self.**revision))
> -      else:
> -        # Make the revision look like branch@revision.
> -        command.append('%s@%s' % (self.branch, self.revision))
> +      command.append(str(self.**revision))
>       # We only add the transitive flag if we have a revision, otherwise
> it is
>       # meaningless.
>       if self.gclient_transitive:
> @@ -444,7 +437,7 @@
>     return d
>
>   def doVCUpdateOnPatch(self, res):
> -    if self.revision and not self.branch and '@' not in
> str(self.revision):
> +    if self.revision:
>       self.branch = 'src'
>     return self.doVCUpdate()
>
>
>
>

[Dirk Pranke, 2012-01-25 03:08:56]

Whoops, you're right. That was left over from when I was thinking I
would lint the non-chromium ports as well (and when I might land this
before the lint-all-ports change).

I will remove the TODO.

-- Dirk

On Tue, Jan 24, 2012 at 7:05 PM, Ojan Vafai <ojan@chromium.org> wrote:
> lint_test_files_wrapper.py LGTM, except for the TODO. I don't see how you
> intend to ever remove the "--chromium".
>
>
> On Tue, Jan 24, 2012 at 5:17 PM, <dpranke@chromium.org> wrote:
>>
>> Reviewers: ojan, Nicolas Sylvain,
>>
>> Description:
>> Adds a 'webkit_lint' test step to the bots that will run before the actual
>> layout tests and check for any errors in
>> the test_expectations.txt file.
>>
>> Currently errors in the test_expectations.txt file get caught during the
>> webkit test run and prevent any actual tests from running. We want to
>> break this out separately so we can diagnose errors in the file
>> separately from actual layout_test failures.
>>
>> R=ojan@chromium.org,nsylvain@chromium.org
>> BUG=110940
>>
>> Please review this at https://chromiumcodereview.appspot.com/9138042/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/tools/build/
>>
>> Affected files:
>>  M     masters/master.chromium.webkit/master_linux_webkit_latest_cfg.py
>>  M     scripts/master/factory/chromium_commands.py
>>  M     scripts/master/factory/chromium_factory.py
>>  A     scripts/slave/chromium/lint_test_files_wrapper.py
>>  M     scripts/slave/chromium_commands.py
>>
>>
>> Index: masters/master.chromium.webkit/master_linux_webkit_latest_cfg.py
>> ===================================================================
>> --- masters/master.chromium.webkit/master_linux_webkit_latest_cfg.py
>>  (revision 118512)
>> +++ masters/master.chromium.webkit/master_linux_webkit_latest_cfg.py
>>  (working copy)
>> @@ -32,7 +32,7 @@
>>  B('Webkit Linux', 'f_webkit_linux_rel', scheduler='s6_webkit_rel',
>>   auto_reboot=True)
>>  F('f_webkit_linux_rel', linux().ChromiumWebkitLatestFactory(
>> -    tests=['test_shell', 'webkit', 'webkit_gpu', 'webkit_unit'],
>> +    tests=['test_shell', 'webkit_lint', 'webkit', 'webkit_gpu',
>> 'webkit_unit'],
>>     options=['--compiler=goma', 'test_shell', 'test_shell_tests',
>>              'webkit_unit_tests', 'DumpRenderTree'],
>>     factory_properties={'archive_webkit_results': True,
>> @@ -57,7 +57,7 @@
>>  B('Webkit Linux (dbg)', 'f_webkit_dbg_tests', scheduler='s6_webkit_dbg')
>>  F('f_webkit_dbg_tests', linux().ChromiumWebkitLatestFactory(
>>     target='Debug',
>> -    tests=['test_shell', 'webkit', 'webkit_gpu', 'webkit_unit'],
>> +    tests=['test_shell', 'webkit_lint', 'webkit', 'webkit_gpu',
>> 'webkit_unit'],
>>     options=['--compiler=goma', 'test_shell', 'test_shell_tests',
>>              'webkit_unit_tests', 'DumpRenderTree'],
>>     factory_properties={'archive_webkit_results': True,
>> Index: scripts/master/factory/chromium_commands.py
>> ===================================================================
>> --- scripts/master/factory/chromium_commands.py (revision 118512)
>> +++ scripts/master/factory/chromium_commands.py (working copy)
>> @@ -745,6 +745,15 @@
>>                      test_command=cmd,
>>                      do_step_if=self.TestStepFilter)
>>
>> +  def AddWebkitLint(self, factory_properties=None):
>> +    """Adds a step to the factory to lint the WebKit
>> test_expectations.txt file."""
>> +    factory_properties = factory_properties or {}
>> +    cmd = [self._python, self._lint_test_files_tool]
>> +    self.AddTestStep(shell.ShellCommand,
>> +                     test_name='lint_test_files',
>> +                     test_command=cmd,
>> +                     do_step_if=self.TestStepFilter)
>> +
>>   def AddWebkitTests(self, gpu, factory_properties=None):
>>     """Adds a step to the factory to run the WebKit layout tests.
>>
>> Index: scripts/master/factory/chromium_factory.py
>> ===================================================================
>> --- scripts/master/factory/chromium_factory.py  (revision 118512)
>> +++ scripts/master/factory/chromium_factory.py  (working copy)
>> @@ -320,6 +320,7 @@
>>     if R('devtools_perf'):  f.AddDevToolsTests(factory_properties=fp)
>>     if R('webkit_gpu'):     f.AddWebkitTests(gpu=True,
>>                                              factory_properties=fp)
>> +    if R('webkit_lint'):    f.AddWebkitLint(factory_properties=fp)
>>
>>     # Benchmark tests:
>>     if R('page_cycler_moz'):
>> Index: scripts/slave/chromium/lint_test_files_wrapper.py
>> ===================================================================
>> --- scripts/slave/chromium/lint_test_files_wrapper.py   (revision 0)
>> +++ scripts/slave/chromium/lint_test_files_wrapper.py   (revision 0)
>> @@ -0,0 +1,32 @@
>> +#!/usr/bin/env python
>> +# Copyright (c) 2012 The Chromium Authors. All rights reserved.
>> +# Use of this source code is governed by a BSD-style license that can be
>> +# found in the LICENSE file.
>> +
>> +"""A wrapper script to check the test_expectations.txt file for
>> errors."""
>> +
>> +import optparse
>> +import os
>> +
>> +from common import chromium_utils
>> +from slave import slave_utils
>> +
>> +
>> +def layout_test(options, args):
>> +  """Parse options and call run_webkit_tests.py, using Python from the
>> tree."""
>> +  build_dir = os.path.abspath(options.build_dir)
>> +  webkit_tests_dir = chromium_utils.FindUpward(build_dir,
>> +                                              'webkit', 'tools',
>> 'layout_tests')
>> +  run_webkit_tests = os.path.join(webkit_tests_dir,
>> 'run_webkit_tests.py')
>> +  # TODO(dpranke): '--chromium' shouldn't be needed.
>> +  command = [run_webkit_tests, '--lint-test-files', '--chromium']
>> +  return slave_utils.RunPythonCommandInBuildDir(build_dir,
>> options.target,
>> +                                                command)
>> +
>> +def main():
>> +  option_parser = optparse.OptionParser()
>> +  options, args = option_parser.parse_args()
>> +  return layout_test(options, args)
>> +
>> +if '__main__' == __name__:
>> +  sys.exit(main())
>>
>> Property changes on: scripts/slave/chromium/lint_test_files_wrapper.py
>> ___________________________________________________________________
>> Added: svn:executable
>>   + *
>> Added: svn:eol-style
>>   + LF
>>
>> Index: scripts/slave/chromium_commands.py
>> ===================================================================
>> --- scripts/slave/chromium_commands.py  (revision 118512)
>> +++ scripts/slave/chromium_commands.py  (working copy)
>> @@ -143,7 +143,6 @@
>>   def __init__(self, *args, **kwargs):
>>     # TODO(maruel): Mainly to keep pylint happy, remove once buildbot
>> fixed
>>     # their style.
>> -    self.branch = None
>>     self.srcdir = None
>>     self.revision = None
>>     self.env = None
>> @@ -277,13 +276,7 @@
>>     # GClient accepts --revision argument of two types 'module@rev' and
>> 'rev'.
>>     if self.revision:
>>       command.append('--revision')
>> -      if (not self.branch or
>> -          self.no_gclient_branch or
>> -          '@' in str(self.revision)):
>> -        command.append(str(self.revision))
>> -      else:
>> -        # Make the revision look like branch@revision.
>> -        command.append('%s@%s' % (self.branch, self.revision))
>> +      command.append(str(self.revision))
>>       # We only add the transitive flag if we have a revision, otherwise
>> it is
>>       # meaningless.
>>       if self.gclient_transitive:
>> @@ -444,7 +437,7 @@
>>     return d
>>
>>   def doVCUpdateOnPatch(self, res):
>> -    if self.revision and not self.branch and '@' not in
>> str(self.revision):
>> +    if self.revision:
>>       self.branch = 'src'
>>     return self.doVCUpdate()
>>
>>
>>
>

[nsylvain, 2012-01-25 20:19:54]

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
File scripts/master/factory/chromium_commands.py (right):

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
scripts/master/factory/chromium_commands.py:749: """Adds a step to the factory
to lint the WebKit test_expectations.txt file."""
80 chars

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
scripts/master/factory/chromium_commands.py:750: factory_properties =
factory_properties or {}
i dont think you need this line

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
File scripts/master/factory/chromium_factory.py (right):

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
scripts/master/factory/chromium_factory.py:323: if R('webkit_lint'):   
f.AddWebkitLint(factory_properties=fp)
if you want to run "before" the webkit tests, you probably want to move this
line before.  The test=[] order is not kept, it's the order in this function
that matters.

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/slave/chromi...
File scripts/slave/chromium_commands.py (right):

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/slave/chromi...
scripts/slave/chromium_commands.py:279: command.append(str(self.revision))
do you really want to touch this? or is this a rebase problem?

[Dirk Pranke, 2012-01-25 22:39:07]

Please take another look?

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
File scripts/master/factory/chromium_commands.py (right):

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
scripts/master/factory/chromium_commands.py:749: """Adds a step to the factory
to lint the WebKit test_expectations.txt file."""
On 2012/01/25 20:19:54, nsylvain wrote:
> 80 chars

Done.

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
scripts/master/factory/chromium_commands.py:750: factory_properties =
factory_properties or {}
On 2012/01/25 20:19:54, nsylvain wrote:
> i dont think you need this line

Done.

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
File scripts/master/factory/chromium_factory.py (right):

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/master/facto...
scripts/master/factory/chromium_factory.py:323: if R('webkit_lint'):   
f.AddWebkitLint(factory_properties=fp)
On 2012/01/25 20:19:54, nsylvain wrote:
> if you want to run "before" the webkit tests, you probably want to move this
> line before.  The test=[] order is not kept, it's the order in this function
> that matters.

Ah, good to know. Yes, I want it to run before. Fixed.

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/slave/chromi...
File scripts/slave/chromium_commands.py (right):

https://chromiumcodereview.appspot.com/9138042/diff/2001/scripts/slave/chromi...
scripts/slave/chromium_commands.py:279: command.append(str(self.revision))
On 2012/01/25 20:19:54, nsylvain wrote:
> do you really want to touch this? or is this a rebase problem?

Change contamination :( Reverted.

[nsylvain, 2012-01-25 23:07:54]

lgtm