Enhance RunPylint to use white_list and black_list arguments.

Change RunPylint to parse all .py files when one is modified.

Make all depot_tools/tests pass on pylint. That mostly meant fixing some
builtins aliasing, wrong alignment and turning off most remaining warnings.

BUG=none
TEST=unit tests

Review URL: http://codereview.chromium.org/5695007

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@69159 0039d316-1c4b-4281-b951-d872f2087c98

PRESUBMIT.py

@@ -27,17 +27,14 @@ def CommonChecks(input_api, output_api):
       UNIT_TESTS))
   output.extend(WasGitClUploadHookModified(input_api, output_api))
 
-  def filter_python_sources(affected_file):
-    filepath = affected_file.LocalPath()
-    return ((filepath.endswith('.py') and
-             filepath != 'cpplint.py' and
-             not filepath.startswith('tests')) or
-            filepath == 'git-try')
-
+  white_list = [r'.*\.py$', r'.*git-try$']
+  black_list = list(input_api.DEFAULT_BLACK_LIST) + [
+      r'.*cpplint\.py$', r'.*git_cl_repo.*']
   output.extend(input_api.canned_checks.RunPylint(
       input_api,
       output_api,
-      source_file_filter=filter_python_sources))
+      white_list=white_list,
+      black_list=black_list))
   return output
 
 

presubmit_canned_checks.py

@@ -450,17 +450,48 @@ def RunPythonUnitTests(input_api, output_api, unit_tests):
   return []
 
 
-def RunPylint(input_api, output_api, source_file_filter=None):
-  """Run pylint on python files."""
-  import warnings
+def RunPylint(input_api, output_api, white_list=None, black_list=None):
+  """Run pylint on python files.
+
+  The default white_list enforces looking only a *.py files.
+  """
+  white_list = white_list or ['.*\.py$']
+  black_list = black_list or input_api.DEFAULT_BLACK_LIST
+
+  # Only trigger if there is at least one python file affected.
+  src_filter = lambda x: input_api.FilterSourceFile(x, white_list, black_list)
+  if not input_api.AffectedSourceFiles(src_filter):
+    return []
+
   # On certain pylint/python version combination, running pylint throws a lot of
   # warning messages.
+  import warnings
   warnings.filterwarnings('ignore', category=DeprecationWarning)
   try:
-    if not source_file_filter:
-      source_file_filter = lambda f: f.LocalPath().endswith('.py')
-    files = [f.LocalPath()
-            for f in input_api.AffectedSourceFiles(source_file_filter)]
+    # We cannot use AffectedFiles here because we want to test every python
+    # file on each single python change. It's because a change in a python file
+    # can break another unmodified file.
+    # Use code similar to InputApi.FilterSourceFile()
+    def Find(filepath, filters):
+      for item in filters:
+        if input_api.re.match(item, filepath):
+          return True
+      return False
+
+    import os
+    files = []
+    for dirpath, dirnames, filenames in os.walk(input_api.PresubmitLocalPath()):
+      # Passes dirnames in black list to speed up search.
+      for item in dirnames[:]:
+        if Find(input_api.os_path.join(dirpath, item), black_list):
+          dirnames.remove(item)
+      for item in filenames:
+        filepath = input_api.os_path.join(dirpath, item)
+        if Find(filepath, white_list) and not Find(filepath, black_list):
+          files.append(filepath)
+
+    # Now that at least one python file was modified and all the python files
+    # were listed, try to run pylint.
     try:
       from pylint import lint
       if lint.Run(sorted(files)):

pylintrc

@@ -44,6 +44,7 @@ load-plugins=
 # R0901: Too many ancestors (8/7)
 # R0902: Too many instance attributes (N/7)
 # R0903: Too few public methods (N/2)
+# R0904: Too many public methods (N/20)
 # R0911: Too many return statements (N/6)
 # R0912: Too many branches (N/12)
 # R0913: Too many arguments (N/5)
@@ -58,7 +59,7 @@ load-plugins=
 # W0613: Unused argument ''
 # W0703: Catch "Exception"
 # W6501: Specify string format arguments as logging function parameters
-disable=C0103,C0111,C0302,I0011,R0401,R0801,R0901,R0902,R0903,R0911,R0912,R0913,R0914,R0915,W0122,W0141,W0142,W0402,W0511,W0603,W0613,W0703,W6501
+disable=C0103,C0111,C0302,I0011,R0401,R0801,R0901,R0902,R0903,R0904,R0911,R0912,R0913,R0914,R0915,W0122,W0141,W0142,W0402,W0511,W0603,W0613,W0703,W6501
 
 
 [REPORTS]

tests/fake_repos.py

@@ -28,9 +28,13 @@ def addKill():
   """Add kill() method to subprocess.Popen for python <2.6"""
   if getattr(subprocess.Popen, 'kill', None):
     return
+  # Unable to import 'module'
+  # pylint: disable=F0401
   if sys.platform == 'win32':
     def kill_win(process):
       import win32process
+      # Access to a protected member _handle of a client class
+      # pylint: disable=W0212
       return win32process.TerminateProcess(process._handle, -1)
     subprocess.Popen.kill = kill_win
   else:
@@ -169,7 +173,6 @@ def commit_svn(repo):
                 '--no-auth-cache', '--username', 'user1', '--password', 'foo'],
                cwd=repo)
   out, err = proc.communicate()
-  last_line = out.splitlines()[-1]
   match = re.search(r'(\d+)', out)
   if not match:
     raise Exception('Commit failed', out, err, proc.returncode)
@@ -267,7 +270,8 @@ class FakeRepos(object):
       logging.debug('Removing %s' % self.trial_dir())
       rmtree(self.trial_dir())
 
-  def _genTree(self, root, tree_dict):
+  @staticmethod
+  def _genTree(root, tree_dict):
     """For a dictionary of file contents, generate a filesystem."""
     if not os.path.isdir(root):
       os.makedirs(root)
@@ -292,7 +296,6 @@ class FakeRepos(object):
     try:
       check_call(['svnadmin', 'create', root])
     except OSError:
-      self.svn_enabled = False
       return False
     write(join(root, 'conf', 'svnserve.conf'),
         '[general]\n'
@@ -548,13 +551,13 @@ hooks = [
   def _commit_git(self, repo, tree):
     repo_root = join(self.git_root, repo)
     self._genTree(repo_root, tree)
-    hash = commit_git(repo_root)
+    commit_hash = commit_git(repo_root)
     if self.git_hashes[repo][-1]:
       new_tree = self.git_hashes[repo][-1][1].copy()
       new_tree.update(tree)
     else:
       new_tree = tree.copy()
-    self.git_hashes[repo].append((hash, new_tree))
+    self.git_hashes[repo].append((commit_hash, new_tree))
 
 
 class FakeReposTestBase(unittest.TestCase):

tests/gcl_unittest.py

@@ -5,6 +5,9 @@
 
 """Unit tests for gcl.py."""
 
+# pylint is too confused.
+# pylint: disable=E1101,E1103,E1120,W0212,W0403
+
 # Fixes include path.
 from super_mox import mox, SuperMoxTestBase
 

tests/gclient_scm_test.py

@@ -5,10 +5,11 @@
 
 """Unit tests for gclient_scm.py."""
 
+# pylint: disable=E1101,E1103,W0403
+
 # Import before super_mox to keep valid references.
 from os import rename
 from shutil import rmtree
-import StringIO
 from subprocess import Popen, PIPE, STDOUT
 import tempfile
 import unittest
@@ -62,7 +63,7 @@ class BaseTestCase(GCBaseTestCase, SuperMoxTestBase):
 
 class SVNWrapperTestCase(BaseTestCase):
   class OptionsObject(object):
-     def __init__(self, verbose=False, revision=None):
+    def __init__(self, verbose=False, revision=None):
       self.verbose = verbose
       self.revision = revision
       self.manually_grab_svn_rev = True
@@ -208,7 +209,6 @@ class SVNWrapperTestCase(BaseTestCase):
     gclient_scm.os.path.islink(file_path).AndReturn(False)
     gclient_scm.os.path.isdir(file_path).AndReturn(True)
     gclient_scm.gclient_utils.RemoveDirectory(file_path)
-    file_list1 = []
     gclient_scm.scm.SVN.RunAndGetFileList(options.verbose,
                                           ['update', '--revision', 'BASE'],
                                           cwd=self.base_path,
@@ -340,10 +340,6 @@ class SVNWrapperTestCase(BaseTestCase):
 
   def testUpdateSingleCheckoutSVN14(self):
     options = self.Options(verbose=True)
-    file_info = {
-      'URL': self.url,
-      'Revision': 42,
-    }
 
     # Checks to make sure that we support svn co --depth.
     gclient_scm.scm.SVN.current_version = None
@@ -466,7 +462,7 @@ class GitWrapperTestCase(GCBaseTestCase, StdoutCheck, TestCaseUtils,
                          unittest.TestCase):
   """This class doesn't use pymox."""
   class OptionsObject(object):
-     def __init__(self, verbose=False, revision=None):
+    def __init__(self, verbose=False, revision=None):
       self.verbose = verbose
       self.revision = revision
       self.manually_grab_svn_rev = True
@@ -523,7 +519,8 @@ from :3
   def Options(self, *args, **kwargs):
     return self.OptionsObject(*args, **kwargs)
 
-  def CreateGitRepo(self, git_import, path):
+  @staticmethod
+  def CreateGitRepo(git_import, path):
     """Do it for real."""
     try:
       Popen(['git', 'init', '-q'], stdout=PIPE, stderr=STDOUT,
@@ -694,9 +691,9 @@ from :3
     options = self.Options()
     expected_file_list = []
     for f in ['a', 'b']:
-        file_path = join(self.base_path, f)
-        open(file_path, 'a').writelines('touched\n')
-        expected_file_list.extend([file_path])
+      file_path = join(self.base_path, f)
+      open(file_path, 'a').writelines('touched\n')
+      expected_file_list.extend([file_path])
     scm = gclient_scm.CreateSCM(url=self.url, root_dir=self.root_dir,
                                 relpath=self.relpath)
     file_list = []
@@ -758,7 +755,7 @@ from :3
     scm = gclient_scm.CreateSCM(url=self.url, root_dir=self.root_dir,
                                 relpath=self.relpath)
     file_path = join(self.base_path, 'b')
-    f = open(file_path, 'w').writelines('conflict\n')
+    open(file_path, 'w').writelines('conflict\n')
     exception = (
         "error: Your local changes to 'b' would be overwritten by merge.  "
         "Aborting.\n"
@@ -773,7 +770,8 @@ from :3
     scm = gclient_scm.CreateSCM(url=self.url, root_dir=self.root_dir,
                                 relpath=self.relpath)
     file_path = join(self.base_path, 'b')
-    f = open(file_path, 'w').writelines('conflict\n')
+    open(file_path, 'w').writelines('conflict\n')
+    # pylint: disable=W0212
     scm._Run(['commit', '-am', 'test'], options)
     __builtin__.raw_input = lambda x: 'y'
     exception = ('Conflict while rebasing this branch.\n'

tests/gclient_smoketest.py

@@ -10,6 +10,8 @@ Shell out 'gclient' and run basic conformance tests.
 This test assumes GClientSmokeBase.URL_BASE is valid.
 """
 
+# pylint: disable=E1103,W0403
+
 import logging
 import os
 import re
@@ -127,7 +129,8 @@ class GClientSmokeBase(FakeReposTestBase):
     self.assertEquals(len(results), len(items), (stdout, items, len(results)))
     return results
 
-  def svnBlockCleanup(self, out):
+  @staticmethod
+  def svnBlockCleanup(out):
     """Work around svn status difference between svn 1.5 and svn 1.6
     I don't know why but on Windows they are reversed. So sorts the items."""
     for i in xrange(len(out)):
@@ -660,12 +663,13 @@ class GClientSmokeSVN(GClientSmokeBase):
     if not self.enabled:
       return
     self.gclient(['config', self.svn_base + 'trunk/src/'])
-    self.parseGclient(['sync', '--jobs', '1'],
-                      ['running', 'running',
-                       # This is due to the way svn update is called for a
-                       # single file when File() is used in a DEPS file.
-                       ('running', os.path.join(self.root_dir, 'src', 'file', 'other')),
-                       'running', 'running', 'running', 'running'])
+    self.parseGclient(
+        ['sync', '--jobs', '1'],
+        ['running', 'running',
+         # This is due to the way svn update is called for a
+         # single file when File() is used in a DEPS file.
+         ('running', os.path.join(self.root_dir, 'src', 'file', 'other')),
+         'running', 'running', 'running', 'running'])
 
   def testInitialCheckoutFailed(self):
     # Check that gclient can be executed from an arbitrary sub directory if the
@@ -883,9 +887,7 @@ class GClientSmokeGIT(GClientSmokeBase):
            'src/repo2/repo_renamed: %(base)srepo_3\n' %
           {
             'base': self.git_base,
-            'hash1': self.githash('repo_1', 2)[:7],
             'hash2': self.githash('repo_2', 1)[:7],
-            'hash3': self.githash('repo_3', 2)[:7],
           })
     self.check((out, '', 0), results)
     results = self.gclient(['revinfo', '--deps', 'mac', '--actual'])
@@ -1012,9 +1014,7 @@ class GClientSmokeBoth(GClientSmokeBase):
            'src/third_party/foo: %(svn_base)s/third_party/foo@1\n') % {
                'svn_base': self.svn_base + 'trunk',
                'git_base': self.git_base,
-               'hash1': self.githash('repo_1', 2)[:7],
                'hash2': self.githash('repo_2', 1)[:7],
-               'hash3': self.githash('repo_3', 2)[:7],
           }
     self.check((out, '', 0), results)
     results = self.gclient(['revinfo', '--deps', 'mac', '--actual'])
@@ -1149,10 +1149,10 @@ class GClientSmokeFromCheckout(GClientSmokeBase):
       return
     self.gclient(['sync'])
     # TODO(maruel): This is incorrect, it should run on ./ too.
-    out = self.parseGclient(
+    self.parseGclient(
         ['cleanup', '--deps', 'mac', '--verbose', '--jobs', '1'],
         [('running', join(self.root_dir, 'foo', 'bar'))])
-    out = self.parseGclient(
+    self.parseGclient(
         ['diff', '--deps', 'mac', '--verbose', '--jobs', '1'],
         [('running', join(self.root_dir, 'foo', 'bar'))])
 

tests/gclient_utils_test.py

@@ -3,6 +3,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+# pylint: disable=E1101,W0403
+
 import StringIO
 
 # Fixes include path.
@@ -76,7 +78,7 @@ class CheckCallAndFilterTestCase(GclientUtilBase):
     def __init__(self, test_string):
       self.stdout = StringIO.StringIO(test_string)
     def wait(self):
-        pass
+      pass
 
   def _inner(self, args, test_string):
     cwd = 'bleh'
@@ -112,7 +114,8 @@ class CheckCallAndFilterTestCase(GclientUtilBase):
     self._inner(args, test_string)
     self.checkstdout('\n________ running \'boo foo bar\' in \'bleh\'\n'
         'ahah\naccb\nallo\naddb\n\n'
-        '________ running \'boo foo bar\' in \'bleh\'\nahah\naccb\nallo\naddb\n')
+        '________ running \'boo foo bar\' in \'bleh\'\nahah\naccb\nallo\naddb'
+        '\n')
 
   def testNoLF(self):
     # Exactly as testCheckCallAndFilterAndHeader without trailing \n

tests/scm_unittest.py

@@ -5,11 +5,10 @@
 
 """Unit tests for scm.py."""
 
-from shutil import rmtree
-import tempfile
+# pylint: disable=E1101,W0403
 
 # Fixes include path.
-from super_mox import mox, TestCaseUtils, SuperMoxTestBase
+from super_mox import SuperMoxTestBase
 
 import scm
 
@@ -171,7 +170,7 @@ class SVNTestCase(BaseSCMTestCase):
     self.assertEqual(file_info, expected)
 
   def testCaptureStatus(self):
-    text =r"""<?xml version="1.0"?>
+    text = r"""<?xml version="1.0"?>
 <status>
 <target path=".">
 <entry path="unversionned_file.txt">

tests/super_mox.py

@@ -5,7 +5,6 @@
 
 """Simplify unit tests based on pymox."""
 
-import __builtin__
 import os
 import random
 import shutil
@@ -41,10 +40,10 @@ class TestCaseUtils(object):
   ## Some utilities for generating arbitrary arguments.
   def String(self, max_length):
     return ''.join([self._RANDOM_CHOICE(self._STRING_LETTERS)
-                    for x in xrange(self._RANDOM_RANDINT(1, max_length))])
+                    for _ in xrange(self._RANDOM_RANDINT(1, max_length))])
 
   def Strings(self, max_arg_count, max_arg_length):
-    return [self.String(max_arg_length) for x in xrange(max_arg_count)]
+    return [self.String(max_arg_length) for _ in xrange(max_arg_count)]
 
   def Args(self, max_arg_count=8, max_arg_length=16):
     return self.Strings(max_arg_count,
@@ -75,7 +74,8 @@ class TestCaseUtils(object):
     if actual_members != expected_members:
       diff = ([i for i in actual_members if i not in expected_members] +
               [i for i in expected_members if i not in actual_members])
-      print>>sys.stderr, diff
+      print >> sys.stderr, diff
+    # pylint: disable=E1101
     self.assertEqual(actual_members, expected_members)
 
   def setUp(self):
@@ -97,6 +97,7 @@ class StdoutCheck(object):
   def tearDown(self):
     try:
       # If sys.stdout was used, self.checkstdout() must be called.
+      # pylint: disable=E1101
       self.assertEquals('', sys.stdout.getvalue())
     except AttributeError:
       pass
@@ -105,6 +106,7 @@ class StdoutCheck(object):
   def checkstdout(self, expected):
     value = sys.stdout.getvalue()
     sys.stdout.close()
+    # pylint: disable=E1101
     self.assertEquals(expected, value)
 
 
@@ -113,7 +115,6 @@ class SuperMoxTestBase(TestCaseUtils, StdoutCheck, mox.MoxTestBase):
     """Patch a few functions with know side-effects."""
     TestCaseUtils.setUp(self)
     mox.MoxTestBase.setUp(self)
-    #self.mox.StubOutWithMock(__builtin__, 'open')
     os_to_mock = ('chdir', 'chown', 'close', 'closerange', 'dup', 'dup2',
       'fchdir', 'fchmod', 'fchown', 'fdopen', 'getcwd', 'getpid', 'lseek',
       'makedirs', 'mkdir', 'open', 'popen', 'popen2', 'popen3', 'popen4',
@@ -144,9 +145,9 @@ class SuperMoxTestBase(TestCaseUtils, StdoutCheck, mox.MoxTestBase):
         except TypeError:
           raise TypeError('Couldn\'t mock %s in %s' % (item, parent.__name__))
 
-  def UnMock(self, object, name):
+  def UnMock(self, obj, name):
     """Restore an object inside a test."""
     for (parent, old_child, child_name) in self.mox.stubs.cache:
-      if parent == object and child_name == name:
+      if parent == obj and child_name == name:
         setattr(parent, child_name, old_child)
         break

tests/trychange_unittest.py

@@ -5,8 +5,10 @@
 
 """Unit tests for trychange.py."""
 
+# pylint: disable=E1103,W0403
+
 # Fixes include path.
-from super_mox import mox, SuperMoxTestBase
+from super_mox import SuperMoxTestBase
 
 import trychange
 

tests/watchlists_unittest.py

@@ -5,6 +5,9 @@
 
 """Unit tests for watchlists.py."""
 
+# pylint is too confused.
+# pylint: disable=E1103,E1120,W0212,W0403
+
 import super_mox
 import watchlists