Improve mocking of os_path.exists() in PRESUBMIT_test.py

I need this for an upcomping change, but it turns out it found 2 real
bugs:
 * A .pydeps check that would never be hit due to test mocks returning
   that deleted files exist
 * A translations check that would never be hit due to test mocks using
   local paths when absolute paths were requested.

Bug: None
Change-Id: I920e4f67b2ff1193b89c7abfd9664d14a94c44bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5910106
Reviewed-by: Ted Choc <[email protected]>
Commit-Queue: Andrew Grieve <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1364918}

PRESUBMIT.py

@@ -5174,23 +5174,23 @@ def CheckPydepsNeedsUpdating(input_api, output_api, checker_for_tests=None):
     # First, check for new / deleted .pydeps.
     for f in input_api.AffectedFiles(include_deletes=True):
         # Check whether we are running the presubmit check for a file in src.
-        # f.LocalPath is relative to repo (src, or internal repo).
-        # os_path.exists is relative to src repo.
-        # Therefore if os_path.exists is true, it means f.LocalPath is relative
-        # to src and we can conclude that the pydeps is in src.
         if f.LocalPath().endswith('.pydeps'):
-            if input_api.os_path.exists(f.LocalPath()):
-                if f.Action() == 'D' and f.LocalPath() in _ALL_PYDEPS_FILES:
-                    results.append(
-                        output_api.PresubmitError(
-                            'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
-                            'remove %s' % f.LocalPath()))
-                elif f.Action() != 'D' and f.LocalPath(
-                ) not in _ALL_PYDEPS_FILES:
-                    results.append(
-                        output_api.PresubmitError(
-                            'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
-                            'include %s' % f.LocalPath()))
+            # f.LocalPath is relative to repo (src, or internal repo).
+            # os_path.exists is relative to src repo.
+            # Therefore if os_path.exists is true, it means f.LocalPath is relative
+            # to src and we can conclude that the pydeps is in src.
+            exists = input_api.os_path.exists(f.LocalPath())
+            if f.Action() == 'D' and f.LocalPath() in _ALL_PYDEPS_FILES:
+                results.append(
+                    output_api.PresubmitError(
+                        'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
+                        'remove %s' % f.LocalPath()))
+            elif (f.Action() != 'D' and exists
+                  and f.LocalPath() not in _ALL_PYDEPS_FILES):
+                results.append(
+                    output_api.PresubmitError(
+                        'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
+                        'include %s' % f.LocalPath()))
 
     if results:
         return results
@@ -6764,8 +6764,8 @@ def CheckStrings(input_api, output_api):
         return []
 
     affected_png_paths = [
-        f.AbsoluteLocalPath() for f in input_api.AffectedFiles()
-        if (f.LocalPath().endswith('.png'))
+        f.LocalPath() for f in input_api.AffectedFiles()
+        if f.LocalPath().endswith('.png')
     ]
 
     # Check for screenshots. Developers can upload screenshots using

PRESUBMIT_test.py

@@ -365,6 +365,15 @@ def testDeleteSha256Success(self):
 
 class CheckAddedDepsHaveTestApprovalsTest(unittest.TestCase):
 
+    def setUp(self):
+        self.input_api = input_api = MockInputApi()
+        input_api.environ = {}
+        input_api.owners_client = self.FakeOwnersClient()
+        input_api.gerrit = self.fakeGerrit()
+        input_api.change.issue = 123
+        self.mockOwnersAndReviewers("owner", set(["reviewer"]))
+        self.mockListSubmodules([])
+
     def calculate(self, old_include_rules, old_specific_include_rules,
                   new_include_rules, new_specific_include_rules):
         return PRESUBMIT._CalculateAddedDeps(
@@ -461,15 +470,6 @@ class fakeGerrit(object):
         def IsOwnersOverrideApproved(self, issue):
             return False
 
-    def setUp(self):
-        self.input_api = input_api = MockInputApi()
-        input_api.environ = {}
-        input_api.owners_client = self.FakeOwnersClient()
-        input_api.gerrit = self.fakeGerrit()
-        input_api.change.issue = 123
-        self.mockOwnersAndReviewers("owner", set(["reviewer"]))
-        self.mockListSubmodules([])
-
     def mockOwnersAndReviewers(self, owner, reviewers):
 
         def mock(*args, **kwargs):
@@ -485,11 +485,9 @@ def mock(*args, **kwargs):
         self.input_api.ListSubmodules = mock
 
     def testApprovedAdditionalDep(self):
-        old_deps = """include_rules = []""".splitlines()
-        new_deps = """include_rules = ["+v8/123"]""".splitlines()
-        self.input_api.files = [
-            MockAffectedFile("pdf/DEPS", new_deps, old_deps)
-        ]
+        self.input_api.InitFiles([
+            MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']),
+        ])
 
         # mark the additional dep as approved.
         os_path = self.input_api.os_path
@@ -501,11 +499,9 @@ def testApprovedAdditionalDep(self):
         self.assertEqual([], results)
 
     def testUnapprovedAdditionalDep(self):
-        old_deps = """include_rules = []""".splitlines()
-        new_deps = """include_rules = ["+v8/123"]""".splitlines()
-        self.input_api.files = [
-            MockAffectedFile('pdf/DEPS', new_deps, old_deps),
-        ]
+        self.input_api.InitFiles([
+            MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']),
+        ])
 
         # pending.
         os_path = self.input_api.os_path
@@ -792,23 +788,19 @@ def testAddedPydep(self):
         if not self.mock_input_api.platform.startswith('linux'):
             return []
 
-        self.mock_input_api.files = [
+        self.mock_input_api.InitFiles([
             MockAffectedFile('new.pydeps', [], action='A'),
-        ]
-
-        self.mock_input_api.CreateMockFileInPath([
-            x.LocalPath()
-            for x in self.mock_input_api.AffectedFiles(include_deletes=True)
         ])
+
         results = self._RunCheck()
         self.assertEqual(1, len(results))
         self.assertIn('PYDEPS_FILES', str(results[0]))
 
     def testPydepNotInSrc(self):
-        self.mock_input_api.files = [
+        self.mock_input_api.InitFiles([
             MockAffectedFile('new.pydeps', [], action='A'),
-        ]
-        self.mock_input_api.CreateMockFileInPath([])
+        ])
+        self.mock_input_api.os_path.exists = lambda x: False
         results = self._RunCheck()
         self.assertEqual(0, len(results))
 
@@ -817,12 +809,8 @@ def testRemovedPydep(self):
         if not self.mock_input_api.platform.startswith('linux'):
             return []
 
-        self.mock_input_api.files = [
+        self.mock_input_api.InitFiles([
             MockAffectedFile(PRESUBMIT._ALL_PYDEPS_FILES[0], [], action='D'),
-        ]
-        self.mock_input_api.CreateMockFileInPath([
-            x.LocalPath()
-            for x in self.mock_input_api.AffectedFiles(include_deletes=True)
         ])
         results = self._RunCheck()
         self.assertEqual(1, len(results))
@@ -3747,13 +3735,7 @@ class StringTest(unittest.TestCase):
 
     def makeInputApi(self, files):
         input_api = MockInputApi()
-        input_api.files = files
-        # Override os_path.exists because the presubmit uses the actual
-        # os.path.exists.
-        input_api.CreateMockFileInPath([
-            x.LocalPath()
-            for x in input_api.AffectedFiles(include_deletes=True)
-        ])
+        input_api.InitFiles(files)
         return input_api
 
     """ CL modified and added messages, but didn't add any screenshots."""

PRESUBMIT_test_mocks.py

@@ -10,6 +10,9 @@
 import subprocess
 import sys
 
+
+_REPO_ROOT = os.path.abspath(os.path.dirname(__file__))
+
 # TODO(dcheng): It's kind of horrible that this is copy and pasted from
 # presubmit_canned_checks.py, but it's far easier than any of the alternatives.
 def _ReportErrorFileAndLine(filename, line_num, dummy_line):
@@ -80,16 +83,28 @@ def __init__(self):
         self.files = []
         self.is_committing = False
         self.change = MockChange([])
-        self.presubmit_local_path = os.path.dirname(__file__)
+        self.presubmit_local_path = os.path.dirname(
+            os.path.abspath(sys.argv[0]))
         self.is_windows = sys.platform == 'win32'
         self.no_diffs = False
         # Although this makes assumptions about command line arguments used by
         # test scripts that create mocks, it is a convenient way to set up the
         # verbosity via the input api.
         self.verbose = '--verbose' in sys.argv
 
-    def CreateMockFileInPath(self, f_list):
-        self.os_path.exists = lambda x: x in f_list
+    def InitFiles(self, files):
+        self.files = files
+        files_that_exist = {
+            p.AbsoluteLocalPath()
+            for p in files if p.Action() != 'D'
+        }
+
+        def mock_exists(path):
+            if not os.path.isabs(path):
+                path = os.path.join(self.presubmit_local_path, path)
+            return path in files_that_exist
+
+        self.os_path.exists = mock_exists
 
     def AffectedFiles(self, file_filter=None, include_deletes=True):
         for file in self.files:
@@ -146,7 +161,7 @@ def ReadFile(self, filename, mode='r'):
         if hasattr(filename, 'AbsoluteLocalPath'):
             filename = filename.AbsoluteLocalPath()
         for file_ in self.files:
-            if file_.LocalPath() == filename:
+            if filename in (file_.LocalPath(), file_.AbsoluteLocalPath()):
                 return '\n'.join(file_.NewContents())
         # Otherwise, file is not in our mock API.
         raise IOError("No such file or directory: '%s'" % filename)
@@ -247,7 +262,7 @@ def LocalPath(self):
         return self._local_path
 
     def AbsoluteLocalPath(self):
-        return self._local_path
+        return os.path.join(_REPO_ROOT, self._local_path)
 
     def GenerateScmDiff(self):
         return self._scm_diff
@@ -273,9 +288,7 @@ def replace(self, altsep, sep):
 
 
 class MockAffectedFile(MockFile):
-
-    def AbsoluteLocalPath(self):
-        return self._local_path
+    pass
 
 
 class MockChange(object):
@@ -301,3 +314,6 @@ def AffectedFiles(self,
 
     def GitFootersFromDescription(self):
         return self.footers
+
+    def RepositoryRoot(self):
+        return _REPO_ROOT

chrome/browser/PRESUBMIT_test.py

@@ -22,11 +22,7 @@
 class BlinkPublicWebUnwantedDependenciesTest(unittest.TestCase):
   def makeInputApi(self, files):
     input_api = MockInputApi()
-    input_api.files = files
-    # Override os_path.exists because the presubmit uses the actual
-    # os.path.exists.
-    input_api.CreateMockFileInPath(
-        [x.LocalPath() for x in input_api.AffectedFiles(include_deletes=True)])
+    input_api.InitFiles(files)
     return input_api
 
   INVALID_DEPS_MESSAGE = ('chrome/browser cannot depend on '

chrome/browser/ash/PRESUBMIT_test.py

@@ -20,12 +20,6 @@
 class DepsFileProhibitingChromeExistsFileTest(unittest.TestCase):
   def assertDepsFilesWithErrors(self, input_api, expected_missing_deps_files,
       expected_deps_files_missing_chrome):
-    # Restore path that was changed to import test mocks above.
-    input_api.presubmit_local_path = _CHROME_BROWSER_ASH
-
-    # Create mock files for all affected files so that os_path.exists() works.
-    input_api.CreateMockFileInPath(
-        [x.LocalPath() for x in input_api.AffectedFiles(include_deletes=True)])
 
     results = PRESUBMIT._CheckDepsFileProhibitingChromeExists(
         input_api, MockOutputApi())
@@ -55,22 +49,23 @@ def assertDepsFilesWithErrors(self, input_api, expected_missing_deps_files,
   # No files created; no errors expected.
   def testNoFiles(self):
     input_api = MockInputApi()
+    input_api.InitFiles([])
     self.assertDepsFilesWithErrors(input_api, [], [])
 
   # Create files in new subdirectories without DEPS files.
   def testFileInDirectoryWithNoDeps(self):
     input_api = MockInputApi()
-    input_api.files = [
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'bar.h'), ''),
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'bar', 'baz.h'), ''),
-    ]
+    input_api.InitFiles([
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'foo', 'bar.h'), ''),
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'bar', 'baz.h'), ''),
+    ])
     self.assertDepsFilesWithErrors(
         input_api,
         [
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
+          os.path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
+          os.path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
         ], [])
 
   # Create files in a new subdirectories with DEPS files not prohibiting
@@ -79,42 +74,42 @@ def testFileInDirectoryNotProhibitingChrome(self):
     DEPS_FILE_NOT_PROHIBITING_CHROME = ['include_rules = []']
 
     input_api = MockInputApi()
-    input_api.files = [
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'bar.h'), ''),
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
-          DEPS_FILE_NOT_PROHIBITING_CHROME),
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'bar', 'baz.h'), ''),
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
-          DEPS_FILE_NOT_PROHIBITING_CHROME),
-    ]
+    input_api.InitFiles([
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'foo', 'bar.h'), ''),
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
+            DEPS_FILE_NOT_PROHIBITING_CHROME),
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'bar', 'baz.h'), ''),
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
+            DEPS_FILE_NOT_PROHIBITING_CHROME),
+    ])
     self.assertDepsFilesWithErrors(
         input_api, [],
         [
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
+          os.path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
+          os.path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
         ])
 
   # Create files in a new subdirectories with DEPS files prohibiting //chrome.
   def testFilesWithDepsFilesProhibitingChrome(self):
     DEPS_FILE_PROHIBITING_CHROME = ['include_rules = [ "-chrome", ]']
 
     input_api = MockInputApi()
-    input_api.files = [
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'bar.h'), ''),
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
-          DEPS_FILE_PROHIBITING_CHROME),
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'bar', 'baz.h'), ''),
-      MockAffectedFile(
-          input_api.os_path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
-          DEPS_FILE_PROHIBITING_CHROME),
-    ]
+    input_api.InitFiles([
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'foo', 'bar.h'), ''),
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
+            DEPS_FILE_PROHIBITING_CHROME),
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'bar', 'baz.h'), ''),
+        MockAffectedFile(
+            os.path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
+            DEPS_FILE_PROHIBITING_CHROME),
+    ])
     self.assertDepsFilesWithErrors(input_api, [], [])
 
 if __name__ == '__main__':