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

This reverts commit b35a9a9.

Reason for revert: Manual bisect shows that this is responsible for failures starting in https://ci.chromium.org/ui/p/chromium/builders/ci/linux-presubmit/19822/blamelist.  Local repro on Linux: `vpython3 chrome/android/webapk/PRESUBMIT_test.py --verbose`

Original change's description:
> 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}

Bug: None
Change-Id: Ibcc1931e5709d1a4209e56211595df8b94a35c9a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5913697
Reviewed-by: Andrew Grieve <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Auto-Submit: Łukasz Anforowicz <[email protected]>
Owners-Override: Łukasz Anforowicz <[email protected]>
Commit-Queue: Łukasz Anforowicz <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1364999}

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'):
-            # 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 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()))
 
     if results:
         return results
@@ -6764,8 +6764,8 @@ def CheckStrings(input_api, output_api):
         return []
 
     affected_png_paths = [
-        f.LocalPath() for f in input_api.AffectedFiles()
-        if f.LocalPath().endswith('.png')
+        f.AbsoluteLocalPath() for f in input_api.AffectedFiles()
+        if (f.LocalPath().endswith('.png'))
     ]
 
     # Check for screenshots. Developers can upload screenshots using

PRESUBMIT_test.py

@@ -365,15 +365,6 @@ 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(
@@ -470,6 +461,15 @@ 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,9 +485,11 @@ def mock(*args, **kwargs):
         self.input_api.ListSubmodules = mock
 
     def testApprovedAdditionalDep(self):
-        self.input_api.InitFiles([
-            MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']),
-        ])
+        old_deps = """include_rules = []""".splitlines()
+        new_deps = """include_rules = ["+v8/123"]""".splitlines()
+        self.input_api.files = [
+            MockAffectedFile("pdf/DEPS", new_deps, old_deps)
+        ]
 
         # mark the additional dep as approved.
         os_path = self.input_api.os_path
@@ -499,9 +501,11 @@ def testApprovedAdditionalDep(self):
         self.assertEqual([], results)
 
     def testUnapprovedAdditionalDep(self):
-        self.input_api.InitFiles([
-            MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']),
-        ])
+        old_deps = """include_rules = []""".splitlines()
+        new_deps = """include_rules = ["+v8/123"]""".splitlines()
+        self.input_api.files = [
+            MockAffectedFile('pdf/DEPS', new_deps, old_deps),
+        ]
 
         # pending.
         os_path = self.input_api.os_path
@@ -788,19 +792,23 @@ def testAddedPydep(self):
         if not self.mock_input_api.platform.startswith('linux'):
             return []
 
-        self.mock_input_api.InitFiles([
+        self.mock_input_api.files = [
             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.InitFiles([
+        self.mock_input_api.files = [
             MockAffectedFile('new.pydeps', [], action='A'),
-        ])
-        self.mock_input_api.os_path.exists = lambda x: False
+        ]
+        self.mock_input_api.CreateMockFileInPath([])
         results = self._RunCheck()
         self.assertEqual(0, len(results))
 
@@ -809,8 +817,12 @@ def testRemovedPydep(self):
         if not self.mock_input_api.platform.startswith('linux'):
             return []
 
-        self.mock_input_api.InitFiles([
+        self.mock_input_api.files = [
             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))
@@ -3735,7 +3747,13 @@ class StringTest(unittest.TestCase):
 
     def makeInputApi(self, files):
         input_api = MockInputApi()
-        input_api.InitFiles(files)
+        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)
+        ])
         return input_api
 
     """ CL modified and added messages, but didn't add any screenshots."""

PRESUBMIT_test_mocks.py

@@ -10,9 +10,6 @@
 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):
@@ -83,28 +80,16 @@ def __init__(self):
         self.files = []
         self.is_committing = False
         self.change = MockChange([])
-        self.presubmit_local_path = os.path.dirname(
-            os.path.abspath(sys.argv[0]))
+        self.presubmit_local_path = os.path.dirname(__file__)
         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 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 CreateMockFileInPath(self, f_list):
+        self.os_path.exists = lambda x: x in f_list
 
     def AffectedFiles(self, file_filter=None, include_deletes=True):
         for file in self.files:
@@ -161,7 +146,7 @@ def ReadFile(self, filename, mode='r'):
         if hasattr(filename, 'AbsoluteLocalPath'):
             filename = filename.AbsoluteLocalPath()
         for file_ in self.files:
-            if filename in (file_.LocalPath(), file_.AbsoluteLocalPath()):
+            if file_.LocalPath() == filename:
                 return '\n'.join(file_.NewContents())
         # Otherwise, file is not in our mock API.
         raise IOError("No such file or directory: '%s'" % filename)
@@ -262,7 +247,7 @@ def LocalPath(self):
         return self._local_path
 
     def AbsoluteLocalPath(self):
-        return os.path.join(_REPO_ROOT, self._local_path)
+        return self._local_path
 
     def GenerateScmDiff(self):
         return self._scm_diff
@@ -288,7 +273,9 @@ def replace(self, altsep, sep):
 
 
 class MockAffectedFile(MockFile):
-    pass
+
+    def AbsoluteLocalPath(self):
+        return self._local_path
 
 
 class MockChange(object):
@@ -314,6 +301,3 @@ def AffectedFiles(self,
 
     def GitFootersFromDescription(self):
         return self.footers
-
-    def RepositoryRoot(self):
-        return _REPO_ROOT

chrome/browser/PRESUBMIT_test.py

@@ -22,7 +22,11 @@
 class BlinkPublicWebUnwantedDependenciesTest(unittest.TestCase):
   def makeInputApi(self, files):
     input_api = MockInputApi()
-    input_api.InitFiles(files)
+    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)])
     return input_api
 
   INVALID_DEPS_MESSAGE = ('chrome/browser cannot depend on '

chrome/browser/ash/PRESUBMIT_test.py

@@ -20,6 +20,12 @@
 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())
@@ -49,23 +55,22 @@ 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.InitFiles([
-        MockAffectedFile(
-            os.path.join(_CHROME_BROWSER_ASH, 'foo', 'bar.h'), ''),
-        MockAffectedFile(
-            os.path.join(_CHROME_BROWSER_ASH, 'bar', 'baz.h'), ''),
-    ])
+    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'), ''),
+    ]
     self.assertDepsFilesWithErrors(
         input_api,
         [
-          os.path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
-          os.path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
+          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
+          input_api.os_path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
         ], [])
 
   # Create files in a new subdirectories with DEPS files not prohibiting
@@ -74,42 +79,42 @@ def testFileInDirectoryNotProhibitingChrome(self):
     DEPS_FILE_NOT_PROHIBITING_CHROME = ['include_rules = []']
 
     input_api = MockInputApi()
-    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),
-    ])
+    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),
+    ]
     self.assertDepsFilesWithErrors(
         input_api, [],
         [
-          os.path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
-          os.path.join(_CHROME_BROWSER_ASH, 'bar', 'DEPS'),
+          input_api.os_path.join(_CHROME_BROWSER_ASH, 'foo', 'DEPS'),
+          input_api.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.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),
-    ])
+    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),
+    ]
     self.assertDepsFilesWithErrors(input_api, [], [])
 
 if __name__ == '__main__':