diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 009e1e1a247b99..396f41cdccda46 100644 --- a/PRESUBMIT.py +++ b/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 diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index 0e7ae32f392534..4fd8663b1426be 100755 --- a/PRESUBMIT_test.py +++ b/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.""" diff --git a/PRESUBMIT_test_mocks.py b/PRESUBMIT_test_mocks.py index f11fae2ede8113..c8c4cb8e9c006c 100644 --- a/PRESUBMIT_test_mocks.py +++ b/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,7 +83,8 @@ 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 @@ -88,8 +92,19 @@ def __init__(self): # 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 diff --git a/chrome/browser/PRESUBMIT_test.py b/chrome/browser/PRESUBMIT_test.py index 17a0d16ce5cf4c..c3a195aad44a09 100755 --- a/chrome/browser/PRESUBMIT_test.py +++ b/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 ' diff --git a/chrome/browser/ash/PRESUBMIT_test.py b/chrome/browser/ash/PRESUBMIT_test.py index df6ab0cdb1317d..5134f930d314b7 100755 --- a/chrome/browser/ash/PRESUBMIT_test.py +++ b/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,23 +74,23 @@ 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. @@ -103,18 +98,18 @@ 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__':