Make AffectedFile member variables private.

Cache SvnAffectedFile.IsTextFile() return value.

Improve the unit test by not/less relying on the local file system.

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

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

presubmit_support.py

@@ -316,12 +316,11 @@ class AffectedFile(object):
   """Representation of a file in a change."""
 
   def __init__(self, path, action, repository_root=''):
-    self.path = path
+    self._path = path
-    self.action = action
+    self._action = action
-    self.repository_root = repository_root
+    self._repository_root = repository_root
-    self.server_path = None
+    self._is_directory = None
-    self.is_directory = None
+    self._properties = {}
-    self.properties = {}
 
   def ServerPath(self):
     """Returns a path string that identifies the file in the SCM system.
@@ -333,32 +332,32 @@ class AffectedFile(object):
   def LocalPath(self):
     """Returns the path of this file on the local disk relative to client root.
     """
-    return normpath(self.path)
+    return normpath(self._path)
 
   def AbsoluteLocalPath(self):
     """Returns the absolute path of this file on the local disk.
     """
-    return normpath(os.path.join(self.repository_root, self.LocalPath()))
+    return normpath(os.path.join(self._repository_root, self.LocalPath()))
 
   def IsDirectory(self):
     """Returns true if this object is a directory."""
-    path = self.AbsoluteLocalPath()
+    if self._is_directory is None:
-    if self.is_directory is None:
+      path = self.AbsoluteLocalPath()
-      self.is_directory = (os.path.exists(path) and
+      self._is_directory = (os.path.exists(path) and
-                           os.path.isdir(path))
+                            os.path.isdir(path))
-    return self.is_directory
+    return self._is_directory
 
   def Action(self):
     """Returns the action on this opened file, e.g. A, M, D, etc."""
     # TODO(maruel): Somewhat crappy, Could be "A" or "A  +" for svn but
     # different for other SCM.
-    return self.action
+    return self._action
 
   def Property(self, property_name):
     """Returns the specified SCM property of this file, or None if no such
     property.
     """
-    return self.properties.get(property_name, None)
+    return self._properties.get(property_name, None)
 
   def IsTextFile(self):
     """Returns True if the file is a text file and not a binary file."""
@@ -397,38 +396,47 @@ class AffectedFile(object):
 class SvnAffectedFile(AffectedFile):
   """Representation of a file in a change out of a Subversion checkout."""
 
+  def __init__(self, *args, **kwargs):
+    AffectedFile.__init__(self, *args, **kwargs)
+    self._server_path = None
+    self._is_text_file = None
+
   def ServerPath(self):
-    if self.server_path is None:
+    if self._server_path is None:
-      self.server_path = gclient.CaptureSVNInfo(
+      self._server_path = gclient.CaptureSVNInfo(
           self.AbsoluteLocalPath()).get('URL', '')
-    return self.server_path
+    return self._server_path
 
   def IsDirectory(self):
-    path = self.AbsoluteLocalPath()
+    if self._is_directory is None:
-    if self.is_directory is None:
+      path = self.AbsoluteLocalPath()
       if os.path.exists(path):
         # Retrieve directly from the file system; it is much faster than
         # querying subversion, especially on Windows.
-        self.is_directory = os.path.isdir(path)
+        self._is_directory = os.path.isdir(path)
       else:
-        self.is_directory = gclient.CaptureSVNInfo(
+        self._is_directory = gclient.CaptureSVNInfo(
             path).get('Node Kind') in ('dir', 'directory')
-    return self.is_directory
+    return self._is_directory
 
   def Property(self, property_name):
-    if not property_name in self.properties:
+    if not property_name in self._properties:
-      self.properties[property_name] = gcl.GetSVNFileProperty(
+      self._properties[property_name] = gcl.GetSVNFileProperty(
           self.AbsoluteLocalPath(), property_name)
-    return self.properties[property_name]
+    return self._properties[property_name]
 
   def IsTextFile(self):
-    if self.Action() == 'D':
+    if self._is_text_file is None:
-      return False
+      if self.Action() == 'D':
-    mime_type = gcl.GetSVNFileProperty(self.AbsoluteLocalPath(),
+        # A deleted file is not a text file.
-                                       'svn:mime-type')
+        self._is_text_file = False
-    if not mime_type or mime_type.startswith('text/'):
+      elif self.IsDirectory():
-      return True
+        self._is_text_file = False
-    return False
+      else:
+        mime_type = gcl.GetSVNFileProperty(self.AbsoluteLocalPath(),
+                                           'svn:mime-type')
+        self._is_text_file = (not mime_type or mime_type.startswith('text/'))
+    return self._is_text_file
 
 
 class GclChange(object):

tests/presubmit_unittest.py

@@ -12,10 +12,12 @@ import unittest
 import warnings
 
 # Local imports
+import __init__
 import gcl
 import gclient
 import presubmit_support as presubmit
 import presubmit_canned_checks
+mox = __init__.mox
 
 
 class PresubmitTestsBase(unittest.TestCase):
@@ -25,8 +27,8 @@ class PresubmitTestsBase(unittest.TestCase):
       self._warnings_stack = warnings.catch_warnings()
     else:
       self._warnings_stack = None
-    
     warnings.simplefilter("ignore", DeprecationWarning)
+    self.mox = mox.Mox()
     self.original_IsFile = os.path.isfile
     def MockIsFile(f):
       dir = os.path.dirname(f)
@@ -91,6 +93,10 @@ def CheckChangeOnUpload(input_api, output_api):
     gcl.GetRepositoryRoot = MockGetRepositoryRoot
     self._sys_stdout = sys.stdout
     sys.stdout = StringIO.StringIO()
+    self._os_path_exists = os.path.exists
+    os.path.exists = self.mox.CreateMockAnything()
+    self._os_path_isdir = os.path.isdir
+    os.path.isdir = self.mox.CreateMockAnything()
 
   def tearDown(self):
     os.path.isfile = self.original_IsFile
@@ -99,6 +105,8 @@ def CheckChangeOnUpload(input_api, output_api):
     gcl.ReadFile = self.original_ReadFile
     gcl.GetRepositoryRoot = self.original_GetRepositoryRoot
     sys.stdout = self._sys_stdout
+    os.path.exists = self._os_path_exists
+    os.path.isdir = self._os_path_isdir
     self._warnings_stack = None
 
   @staticmethod
@@ -164,7 +172,15 @@ class PresubmitUnittest(PresubmitTestsBase):
       ['M', 'flop/notfound.txt'],  # not found in SVN, still exists locally
       ['D', 'boo/flap.h'],
     ]
-
+    os.path.exists(os.path.join('foo', 'blat.cc')).AndReturn(True)
+    os.path.isdir(os.path.join('foo', 'blat.cc')).AndReturn(False)
+    os.path.exists('binary.dll').AndReturn(True)
+    os.path.isdir('binary.dll').AndReturn(False)
+    os.path.exists('isdir').AndReturn(True)
+    os.path.isdir('isdir').AndReturn(True)
+    os.path.exists(os.path.join('flop', 'notfound.txt')).AndReturn(False)
+    os.path.exists(os.path.join('boo', 'flap.h')).AndReturn(False)
+    self.mox.ReplayAll()
     ci = gcl.ChangeInfo(name='mychange',
                         description='\n'.join(description_lines),
                         files=files)
@@ -222,6 +238,7 @@ class PresubmitUnittest(PresubmitTestsBase):
     self.failUnless(rhs_lines[3][0].LocalPath() == files[3][1])
     self.failUnless(rhs_lines[3][1] == 2)
     self.failUnless(rhs_lines[3][2] == 'two:%s' % files[3][1])
+    self.mox.VerifyAll()
 
   def testExecPresubmitScript(self):
     description_lines = ('Hello there',
@@ -391,6 +408,11 @@ def CheckChangeOnCommit(input_api, output_api):
       ['A', 'isdir'],
       ['A', 'isdir\\blat.cc'],
     ]
+    os.path.exists('isdir').AndReturn(True)
+    os.path.isdir('isdir').AndReturn(True)
+    os.path.exists(os.path.join('isdir', 'blat.cc')).AndReturn(True)
+    os.path.isdir(os.path.join('isdir', 'blat.cc')).AndReturn(False)
+    self.mox.ReplayAll()
     ci = gcl.ChangeInfo(name='mychange',
                         description='foo',
                         files=files)
@@ -399,7 +421,7 @@ def CheckChangeOnCommit(input_api, output_api):
     affected_files = change.AffectedFiles(include_dirs=False)
     self.failUnless(len(affected_files) == 1)
     self.failUnless(affected_files[0].LocalPath().endswith('blat.cc'))
-
+    self.mox.VerifyAll()
     affected_files_and_dirs = change.AffectedFiles(include_dirs=True)
     self.failUnless(len(affected_files_and_dirs) == 2)
 
@@ -514,6 +536,16 @@ class InputApiUnittest(PresubmitTestsBase):
       ['A', 'boo/flap.h'],
     ]
 
+    os.path.exists(os.path.join('foo', 'blat.cc')).AndReturn(True)
+    os.path.isdir(os.path.join('foo', 'blat.cc')).AndReturn(False)
+    os.path.exists(os.path.join('foo', 'blat', 'binary.dll')).AndReturn(True)
+    os.path.isdir(os.path.join('foo', 'blat', 'binary.dll')).AndReturn(False)
+    os.path.exists(os.path.join('foo', 'mat', 'beingdeleted.txt')).AndReturn(
+        False)
+    os.path.exists(os.path.join('flop', 'notfound.txt')).AndReturn(False)
+    os.path.exists(os.path.join('boo', 'flap.h')).AndReturn(True)
+    os.path.isdir(os.path.join('boo', 'flap.h')).AndReturn(False)
+    self.mox.ReplayAll()
     ci = gcl.ChangeInfo(name='mychange',
                         description='\n'.join(description_lines),
                         files=files)
@@ -536,6 +568,7 @@ class InputApiUnittest(PresubmitTestsBase):
     self.failUnless(len(rhs_lines) == 2)
     self.assertEqual(rhs_lines[0][0].LocalPath(),
                     presubmit.normpath('foo/blat.cc'))
+    self.mox.VerifyAll()
 
   def testGetAbsoluteLocalPath(self):
     # Regression test for bug of presubmit stuff that relies on invoking
@@ -633,10 +666,8 @@ class OuputApiUnittest(PresubmitTestsBase):
 class AffectedFileUnittest(PresubmitTestsBase):
   def testMembersChanged(self):
     members = [
-      'AbsoluteLocalPath', 'Action', 'IsDirectory', 'IsTextFile',
+      'AbsoluteLocalPath', 'Action', 'IsDirectory', 'IsTextFile', 'LocalPath',
-      'LocalPath', 'NewContents',
+      'NewContents', 'OldContents', 'OldFileTempPath', 'Property', 'ServerPath',
-      'OldContents', 'OldFileTempPath', 'Property', 'ServerPath', 'action',
-      'is_directory', 'path', 'properties', 'repository_root', 'server_path',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(presubmit.AffectedFile('a', 'b'), members)
@@ -644,11 +675,14 @@ class AffectedFileUnittest(PresubmitTestsBase):
 
   def testAffectedFile(self):
     af = presubmit.SvnAffectedFile('foo/blat.cc', 'M')
+    os.path.exists(os.path.join('foo', 'blat.cc')).AndReturn(False)
+    self.mox.ReplayAll()
     self.failUnless(af.ServerPath() == 'svn:/foo/foo/blat.cc')
     self.failUnless(af.LocalPath() == presubmit.normpath('foo/blat.cc'))
     self.failUnless(af.Action() == 'M')
     self.failUnless(af.NewContents() == ['one:%s' % af.LocalPath(),
                                          'two:%s' % af.LocalPath()])
+    self.mox.VerifyAll()
 
     af = presubmit.AffectedFile('notfound.cc', 'A')
     self.failUnless(af.ServerPath() == '')
@@ -658,6 +692,25 @@ class AffectedFileUnittest(PresubmitTestsBase):
     self.failUnless(affected_file.Property('svn:secret-property') ==
                     'secret-property-value')
 
+  def testIsDirectoryNotExists(self):
+    os.path.exists('foo.cc').AndReturn(False)
+    self.mox.ReplayAll()
+    affected_file = presubmit.SvnAffectedFile('foo.cc', 'A')
+    # Verify cache coherency.
+    self.failIf(affected_file.IsDirectory())
+    self.failIf(affected_file.IsDirectory())
+    self.mox.VerifyAll()
+
+  def testIsDirectory(self):
+    os.path.exists('foo.cc').AndReturn(True)
+    os.path.isdir('foo.cc').AndReturn(True)
+    self.mox.ReplayAll()
+    affected_file = presubmit.SvnAffectedFile('foo.cc', 'A')
+    # Verify cache coherency.
+    self.failUnless(affected_file.IsDirectory())
+    self.failUnless(affected_file.IsDirectory())
+    self.mox.VerifyAll()
+
 
 class CannedChecksUnittest(PresubmitTestsBase):
   """Tests presubmit_canned_checks.py."""