diff --git a/PRESUBMIT.py b/PRESUBMIT.py index b883367b7f833..895ab1814fb9f 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -5182,23 +5182,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 @@ -6748,8 +6748,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 0e7ae32f39253..4fd8663b1426b 100755 --- a/PRESUBMIT_test.py +++ b/PRESUBMIT_test.py @@ -365,6 +365,15 @@ class CheckEachPerfettoTestDataFileHasDepsEntry(unittest.TestCase): 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 CheckAddedDepsHaveTestApprovalsTest(unittest.TestCase): 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 @@ class CheckAddedDepsHaveTestApprovalsTest(unittest.TestCase): 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 @@ class CheckAddedDepsHaveTestApprovalsTest(unittest.TestCase): 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 @@ class PydepsNeedsUpdatingTest(unittest.TestCase): 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 @@ class PydepsNeedsUpdatingTest(unittest.TestCase): 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 f11fae2ede811..6a28ea9a24f26 100644 --- a/PRESUBMIT_test_mocks.py +++ b/PRESUBMIT_test_mocks.py @@ -10,6 +10,9 @@ import re 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 @@ class MockInputApi(object): 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,28 @@ class MockInputApi(object): # 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): + # Actual presubmit calls normpath, but too many tests break to do this + # right in MockFile(). + for f in files: + f._local_path = os.path.normpath(f._local_path) + 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 + + def mock_glob(pattern, *args, **kwargs): + return fnmatch.filter(files_that_exist, pattern) + + # Do not stub these in the constructor to not break existing tests. + self.os_path.exists = mock_exists + self.glob = mock_glob def AffectedFiles(self, file_filter=None, include_deletes=True): for file in self.files: @@ -146,7 +170,7 @@ class MockInputApi(object): 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 +271,7 @@ class MockFile(object): 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 +297,7 @@ class MockFile(object): class MockAffectedFile(MockFile): - - def AbsoluteLocalPath(self): - return self._local_path + pass class MockChange(object): @@ -301,3 +323,6 @@ class MockChange(object): def GitFootersFromDescription(self): return self.footers + + def RepositoryRoot(self): + return _REPO_ROOT diff --git a/chrome/android/webapk/PRESUBMIT_test.py b/chrome/android/webapk/PRESUBMIT_test.py index 6b6fbba854d99..0ed8795e0edba 100755 --- a/chrome/android/webapk/PRESUBMIT_test.py +++ b/chrome/android/webapk/PRESUBMIT_test.py @@ -16,148 +16,161 @@ from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi # Mocks os.walk() class MockOsWalkFileSystem(object): - def __init__(self, file_paths): - self.file_paths = file_paths + def __init__(self, file_paths): + self.file_paths = file_paths - def walk(self, top): - if not top.endswith('/'): - top += '/' + def walk(self, top): + if not top.endswith('/'): + top += '/' - files = [] - dirs = [] - for f in self.file_paths: - if f.startswith(top): - remaining = f[len(top):] - slash_index = remaining.find('/') - if slash_index >= 0: - dir_name = remaining[:slash_index] - if not dir_name in dirs: - dirs.append(dir_name) - else: - files.append(remaining) + files = [] + dirs = [] + for f in self.file_paths: + if f.startswith(top): + remaining = f[len(top):] + slash_index = remaining.find('/') + if slash_index >= 0: + dir_name = remaining[:slash_index] + if not dir_name in dirs: + dirs.append(dir_name) + else: + files.append(remaining) - yield top[:-1], dirs, files + yield top[:-1], dirs, files - for name in dirs: - for result in self.walk(top + name): - yield result + for name in dirs: + for result in self.walk(top + name): + yield result -class CustomMockInputApi(MockInputApi): - def makeMockAffectedFiles(self, file_names): - mock_files = [] - for file_name in file_names: - mock_files.append( - MockAffectedFile(file_name, ['new_content'], action='A')) - return mock_files +def make_mock_affected_files(file_names, extra_files=[]): + D = 'chrome/android/webapk' + ret = [] + for file_name in file_names: + ret.append( + MockAffectedFile(f'{D}/{file_name}', ['new_content'], action='A')) + for mock_file in extra_files: + ret.append( + MockAffectedFile(f'{D}/{mock_file.LocalPath()}', + mock_file.NewContents(), + action=mock_file.Action())) + + return ret class ShellApkVersion(unittest.TestCase): - UPDATE_CURRENT_VERSION_MESSAGE = ( - 'current_shell_apk_version in ' - 'shell_apk/current_version/current_version.gni needs to updated due to ' - 'changes in:') + UPDATE_CURRENT_VERSION_MESSAGE = ( + 'current_shell_apk_version in ' + 'shell_apk/current_version/current_version.gni needs to updated due to ' + 'changes in:') - def testCheckWamMintTriggerRule(self): - COMMON_SRC_FILE_PATH = ( - 'libs/common/src/org/chromium/webapk/lib/common/A.java') - COMMON_JUNIT_FILE_PATH = ( - 'libs/common/junit/src/org/chromium/webapk/lib/common/B.java') - SHELL_APK_SRC_FILE_PATH = ( - 'shell_apk/src/org/chromium/webapk/shell_apk/C.java') - SHELL_APK_JUNIT_FILE_PATH = ( - 'shell_apk/junit/src/org/chromium/webapk/shell_apk/D.java') - changed_java_file_paths = [ - COMMON_SRC_FILE_PATH, COMMON_JUNIT_FILE_PATH, SHELL_APK_SRC_FILE_PATH, - SHELL_APK_JUNIT_FILE_PATH - ] + def testCheckWamMintTriggerRule(self): + COMMON_SRC_FILE_PATH = ( + 'libs/common/src/org/chromium/webapk/lib/common/A.java') + COMMON_JUNIT_FILE_PATH = ( + 'libs/common/junit/src/org/chromium/webapk/lib/common/B.java') + SHELL_APK_SRC_FILE_PATH = ( + 'shell_apk/src/org/chromium/webapk/shell_apk/C.java') + SHELL_APK_JUNIT_FILE_PATH = ( + 'shell_apk/junit/src/org/chromium/webapk/shell_apk/D.java') + changed_java_file_paths = [ + COMMON_SRC_FILE_PATH, COMMON_JUNIT_FILE_PATH, SHELL_APK_SRC_FILE_PATH, + SHELL_APK_JUNIT_FILE_PATH + ] - SHELL_APK_RES_FILE_PATH = 'shell_apk/res/mipmap-xxxxxxhdpi/app_icon.png' - CURRENT_VERSION_FILE_PATH = 'shell_apk/current_version/current_version.gni' + SHELL_APK_RES_FILE_PATH = 'shell_apk/res/mipmap-xxxxxxhdpi/app_icon.png' + CURRENT_VERSION_FILE_PATH = 'shell_apk/current_version/current_version.gni' - # template_shell_apk_version not updated. There should be an error about - # template_shell_apk_version needing to be updated. - input_api = CustomMockInputApi() - input_api.files = input_api.makeMockAffectedFiles( - changed_java_file_paths + [SHELL_APK_RES_FILE_PATH]) - input_api.files += [ - MockAffectedFile(CURRENT_VERSION_FILE_PATH, 'variable=O', - 'variable=N', action='M') - ] - errors = PRESUBMIT._CheckCurrentVersionIncreaseRule(input_api, - MockOutputApi()) - self.assertEqual(1, len(errors)) - self.assertEqual(self.UPDATE_CURRENT_VERSION_MESSAGE, errors[0].message) - self.assertEqual([COMMON_SRC_FILE_PATH, SHELL_APK_SRC_FILE_PATH, - SHELL_APK_RES_FILE_PATH], - errors[0].items) + # template_shell_apk_version not updated. There should be an error about + # template_shell_apk_version needing to be updated. + input_api = MockInputApi() + input_api.InitFiles( + make_mock_affected_files( + changed_java_file_paths + [SHELL_APK_RES_FILE_PATH], [ + MockAffectedFile(CURRENT_VERSION_FILE_PATH, + 'variable=O', + 'variable=N', + action='M') + ])) + errors = PRESUBMIT._CheckCurrentVersionIncreaseRule(input_api, + MockOutputApi()) + self.assertEqual(1, len(errors)) + self.assertEqual(self.UPDATE_CURRENT_VERSION_MESSAGE, errors[0].message) + self.assertEqual([COMMON_SRC_FILE_PATH, SHELL_APK_SRC_FILE_PATH, + SHELL_APK_RES_FILE_PATH], + errors[0].items) - # template_shell_apk_version updated. There should be no errors. - input_api.files = input_api.makeMockAffectedFiles( - changed_java_file_paths + [SHELL_APK_RES_FILE_PATH]) - input_api.files += [ - MockAffectedFile(CURRENT_VERSION_FILE_PATH, - ['current_shell_apk_version=1'], - ['current_shell_apk_version=2'], action='M') - ] - errors = PRESUBMIT._CheckCurrentVersionIncreaseRule(input_api, - MockOutputApi()) - self.assertEqual([], errors) + # template_shell_apk_version updated. There should be no errors. + input_api.InitFiles( + make_mock_affected_files( + changed_java_file_paths + [SHELL_APK_RES_FILE_PATH], [ + MockAffectedFile(CURRENT_VERSION_FILE_PATH, + ['current_shell_apk_version=1'], + ['current_shell_apk_version=2'], + action='M') + ])) + errors = PRESUBMIT._CheckCurrentVersionIncreaseRule(input_api, + MockOutputApi()) + self.assertEqual([], errors) class OverlappingResourceFileNames(unittest.TestCase): - RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE = ( - 'Resources in different top level res/ directories [\'shell_apk/res\', ' - '\'shell_apk/res_template\', \'libs/common/res_splash\'] should have ' - 'different names:') + RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE = ( + 'Resources in different top level res/ directories [\'shell_apk/res\', ' + '\'shell_apk/res_template\', \'libs/common/res_splash\'] should have ' + 'different names:') - def testAddFileSameNameWithinResDirectory(self): - # Files within a res/ directory can have same file name. - MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml', - 'libs/common/res_splash/values/dimens.xml'] - input_api = CustomMockInputApi() - input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk + def testAddFileSameNameWithinResDirectory(self): + # Files within a res/ directory can have same file name. + MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml', + 'libs/common/res_splash/values/dimens.xml'] + input_api = MockInputApi() + input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk - input_api.files = input_api.makeMockAffectedFiles([ - 'shell_apk/res/values-v22/values.xml']) - errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule( - input_api, MockOutputApi()) - self.assertEqual(0, len(errors)) + input_api.InitFiles( + make_mock_affected_files(['shell_apk/res/values-v22/values.xml'])) + errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule( + input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) - def testAddFileSameNameAcrossResDirectories(self): - # Adding a file to a res/ directory with the same file name as a file in a - # different res/ directory is illegal. - MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml', - 'libs/common/res_splash/values/dimens.xml'] - input_api = CustomMockInputApi() - input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk - input_api.files = input_api.makeMockAffectedFiles([ - 'shell_apk/res/values-v17/dimens.xml', - 'libs/common/res_splash/values-v22/colors.xml']) - errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule( - input_api, MockOutputApi()) - self.assertEqual(1, len(errors)) - self.assertEqual(self.RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE, - errors[0].message) - errors[0].items.sort() - self.assertEqual(['colors.xml', 'dimens.xml'], errors[0].items) + def testAddFileSameNameAcrossResDirectories(self): + # Adding a file to a res/ directory with the same file name as a file in a + # different res/ directory is illegal. + MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml', + 'libs/common/res_splash/values/dimens.xml'] + input_api = MockInputApi() + input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk + input_api.InitFiles( + make_mock_affected_files([ + 'shell_apk/res/values-v17/dimens.xml', + 'libs/common/res_splash/values-v22/colors.xml' + ])) + errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule( + input_api, MockOutputApi()) + self.assertEqual(1, len(errors)) + self.assertEqual(self.RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE, + errors[0].message) + errors[0].items.sort() + self.assertEqual(['colors.xml', 'dimens.xml'], errors[0].items) - def testAddTwoFilesWithSameNameDifferentResDirectories(self): - # Adding two files with the same file name but in different res/ - # directories is illegal. - MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml', - 'libs/common/res_splash/values/dimens.xml'] - input_api = CustomMockInputApi() - input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk - input_api.files = input_api.makeMockAffectedFiles([ - 'shell_apk/res/values/values.xml', - 'libs/common/res_splash/values-v22/values.xml']) - errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule( - input_api, MockOutputApi()) - self.assertEqual(1, len(errors)) - self.assertEqual(self.RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE, - errors[0].message) - self.assertEqual(['values.xml'], errors[0].items) + def testAddTwoFilesWithSameNameDifferentResDirectories(self): + # Adding two files with the same file name but in different res/ + # directories is illegal. + MOCK_FILE_SYSTEM_FILES = ['shell_apk/res/values/colors.xml', + 'libs/common/res_splash/values/dimens.xml'] + input_api = MockInputApi() + input_api.os_walk = MockOsWalkFileSystem(MOCK_FILE_SYSTEM_FILES).walk + input_api.InitFiles( + make_mock_affected_files([ + 'shell_apk/res/values/values.xml', + 'libs/common/res_splash/values-v22/values.xml' + ])) + errors = PRESUBMIT._CheckNoOverlappingFileNamesInResourceDirsRule( + input_api, MockOutputApi()) + self.assertEqual(1, len(errors)) + self.assertEqual(self.RESOURCES_SHOULD_HAVE_DIFFERENT_FILE_NAMES_MESSAGE, + errors[0].message) + self.assertEqual(['values.xml'], errors[0].items) if __name__ == '__main__': - unittest.main() + unittest.main() diff --git a/chrome/browser/PRESUBMIT_test.py b/chrome/browser/PRESUBMIT_test.py index 17a0d16ce5cf4..c3a195aad44a0 100755 --- a/chrome/browser/PRESUBMIT_test.py +++ b/chrome/browser/PRESUBMIT_test.py @@ -22,11 +22,7 @@ _INVALID_DEP2 = "+third_party/blink/public/web/web_nothing.h," 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 df6ab0cdb1317..5134f930d314b 100755 --- a/chrome/browser/ash/PRESUBMIT_test.py +++ b/chrome/browser/ash/PRESUBMIT_test.py @@ -20,12 +20,6 @@ _CHROME_BROWSER_ASH = os.path.join('chrome', 'browser', 'ash') 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 @@ class DepsFileProhibitingChromeExistsFileTest(unittest.TestCase): # 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 @@ class DepsFileProhibitingChromeExistsFileTest(unittest.TestCase): 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 @@ class DepsFileProhibitingChromeExistsFileTest(unittest.TestCase): 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__': diff --git a/third_party/blink/PRESUBMIT.py b/third_party/blink/PRESUBMIT.py index c40256cc7bf01..ba60a2a393a73 100644 --- a/third_party/blink/PRESUBMIT.py +++ b/third_party/blink/PRESUBMIT.py @@ -147,7 +147,7 @@ def _CommonChecks(input_api, output_api): return results -def _FilterPaths(input_api): +def FilterPaths(input_api): """Returns input files with certain paths removed.""" files = [] for f in input_api.AffectedFiles(): @@ -157,19 +157,19 @@ def _FilterPaths(input_api): # non-Chromium authors. if 'web_tests' + input_api.os_path.sep in file_path: continue - if '/PRESUBMIT' in file_path: + if input_api.os_path.sep + 'PRESUBMIT' in file_path: continue # Skip files that were generated by bison. if re.search( - 'third_party/blink/renderer/' - 'core/xml/xpath_grammar_generated\.(cc|h)$', file_path): + 'third_party.blink.renderer.' + 'core.xml.xpath_grammar_generated\.(cc|h)$', file_path): continue files.append(file_path) return files def _CheckStyle(input_api, output_api): - files = _FilterPaths(input_api) + files = FilterPaths(input_api) # Do not call check_blink_style.py with empty affected file list if all # input_api.AffectedFiles got filtered. if not files: diff --git a/third_party/blink/PRESUBMIT_test.py b/third_party/blink/PRESUBMIT_test.py index 7ec7bdd79f04d..b5c52268d9367 100755 --- a/third_party/blink/PRESUBMIT_test.py +++ b/third_party/blink/PRESUBMIT_test.py @@ -13,8 +13,8 @@ import subprocess import sys import unittest -sys.path.append( - os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', '..')) +_THIS_DIR = os.path.dirname(os.path.abspath(__file__)) +sys.path.append(os.path.join(_THIS_DIR, '..', '..')) import mock from PRESUBMIT_test_mocks import MockInputApi @@ -46,37 +46,31 @@ class PresubmitTest(unittest.TestCase): diff_file_chromium_h = ['another diff'] diff_file_test_expectations = ['more diff'] mock_input_api = MockInputApi() - mock_python_file = MockAffectedFile('file_blink.py', ['lint me']) - mock_input_api.files = [ - MockAffectedFile('file_blink.h', diff_file_blink_h), - MockAffectedFile('file_chromium.h', diff_file_chromium_h), - MockAffectedFile( - mock_input_api.os_path.join('web_tests', 'TestExpectations'), - diff_file_test_expectations), + B = 'third_party/blink' + mock_python_file = MockAffectedFile(f'{B}/file_blink.py', ['lint me']) + mock_input_api.InitFiles([ + MockAffectedFile(f'{B}/file_blink.h', diff_file_blink_h), + MockAffectedFile(f'{B}/file_chromium.h', diff_file_chromium_h), + MockAffectedFile(f'{B}/web_tests/TestExpectations', + diff_file_test_expectations), mock_python_file, - ] - mock_input_api.presubmit_local_path = mock_input_api.os_path.join( - 'path', 'to', 'third_party', 'blink') - with mock.patch.object(mock_python_file, - 'AbsoluteLocalPath', - return_value=mock_input_api.os_path.join( - 'path', 'to', 'third_party', 'blink', - 'file_blink.py')): - # Access to a protected member _CheckStyle - # pylint: disable=W0212 - PRESUBMIT._CheckStyle(mock_input_api, MockOutputApi()) - mock_input_api.canned_checks.GetPylint.assert_called_once_with( - mock.ANY, - mock.ANY, - files_to_check=[r'file_blink\.py'], - pylintrc=mock_input_api.os_path.join('tools', 'blinkpy', - 'pylintrc')) + ]) + # Access to a protected member _CheckStyle + # pylint: disable=W0212 + PRESUBMIT._CheckStyle(mock_input_api, MockOutputApi()) + mock_input_api.canned_checks.GetPylint.assert_called_once_with( + mock.ANY, + mock.ANY, + files_to_check=[r'file_blink\.py'], + pylintrc=mock_input_api.os_path.join('tools', 'blinkpy', + 'pylintrc')) capture = Capture() # pylint: disable=E1101 subprocess.Popen.assert_called_with(capture, stderr=-1) self.assertEqual(6, len(capture.value)) - self.assertEqual('file_blink.h', capture.value[3]) + self.assertEqual(os.path.join(_THIS_DIR, 'file_blink.h'), + capture.value[3]) @mock.patch('subprocess.Popen') def testCheckChangeOnUploadWithEmptyAffectedFileList(self, _): @@ -94,28 +88,24 @@ class PresubmitTest(unittest.TestCase): self.assertEqual(0, subprocess.Popen.call_count) def test_FilterPaths(self): - """This verifies that _FilterPaths removes expected paths.""" + """This verifies that FilterPaths removes expected paths.""" diff_file_chromium1_h = ['some diff'] diff_web_tests_html = ['more diff'] diff_presubmit = ['morer diff'] diff_test_expectations = ['morest diff'] mock_input_api = MockInputApi() - mock_input_api.files = [ - MockAffectedFile('file_chromium1.h', diff_file_chromium1_h), - MockAffectedFile( - mock_input_api.os_path.join('web_tests', 'some_tests.html'), - diff_web_tests_html), - MockAffectedFile( - mock_input_api.os_path.join('web_tests', 'TestExpectations'), - diff_test_expectations), - # Note that this path must have a slash, whereas most other paths - # must have os-standard path separators. - MockAffectedFile('blink/PRESUBMIT', diff_presubmit), - ] - # Access to a protected member _FilterPaths - # pylint: disable=W0212 - filtered = PRESUBMIT._FilterPaths(mock_input_api) - self.assertEqual(['file_chromium1.h'], filtered) + B = 'third_party/blink' + mock_input_api.InitFiles([ + MockAffectedFile(f'{B}/file_chromium1.h', diff_file_chromium1_h), + MockAffectedFile(f'{B}/web_tests/some_tests.html', + diff_web_tests_html), + MockAffectedFile(f'{B}/web_tests/TestExpectations', + diff_test_expectations), + MockAffectedFile(f'{B}/blink/PRESUBMIT', diff_presubmit), + ]) + filtered = PRESUBMIT.FilterPaths(mock_input_api) + self.assertEqual([os.path.join(_THIS_DIR, 'file_chromium1.h')], + filtered) def testCheckPublicHeaderWithBlinkMojo(self): """This verifies that _CheckForWrongMojomIncludes detects -blink mojo @@ -125,12 +115,12 @@ class PresubmitTest(unittest.TestCase): mock_input_api = MockInputApi() potentially_bad_content = \ '#include "public/platform/modules/cache_storage.mojom-blink.h"' - mock_input_api.files = [ + mock_input_api.InitFiles([ MockAffectedFile( mock_input_api.os_path.join('third_party', 'blink', 'public', 'a_header.h'), [potentially_bad_content], None) - ] + ]) # Access to a protected member _CheckForWrongMojomIncludes # pylint: disable=W0212 errors = PRESUBMIT._CheckForWrongMojomIncludes(mock_input_api, @@ -151,12 +141,12 @@ class PresubmitTest(unittest.TestCase): #include "public/platform/modules/cache_storage.mojom-blink-forward.h" #include "public/platform/modules/cache_storage.mojom-blink-test-utils.h" """ - mock_input_api.files = [ + mock_input_api.InitFiles([ MockAffectedFile( mock_input_api.os_path.join('third_party', 'blink', 'renderer', 'core', 'a_header.h'), [potentially_bad_content], None) - ] + ]) # Access to a protected member _CheckForWrongMojomIncludes # pylint: disable=W0212 errors = PRESUBMIT._CheckForWrongMojomIncludes(mock_input_api, @@ -185,9 +175,9 @@ class CxxDependencyTest(unittest.TestCase): def runCheck(self, filename, file_contents): mock_input_api = MockInputApi() - mock_input_api.files = [ + mock_input_api.InitFiles([ MockAffectedFile(filename, file_contents), - ] + ]) # Access to a protected member # pylint: disable=W0212 return PRESUBMIT._CheckForForbiddenChromiumCode( diff --git a/third_party/blink/web_tests/PRESUBMIT.py b/third_party/blink/web_tests/PRESUBMIT.py index 864ac3ba047ed..4cc3935f086e0 100644 --- a/third_party/blink/web_tests/PRESUBMIT.py +++ b/third_party/blink/web_tests/PRESUBMIT.py @@ -174,12 +174,12 @@ def _TestsCorrespondingToAffectedBaselines(input_api, test_prefix = baseline_match['test_prefix'] # Getting the test name from the baseline path is not as easy as the # other direction. Try all extensions as a heuristic instead. + abs_prefix = input_api.os_path.join(input_api.PresubmitLocalPath(), + test_prefix) for extension in [ 'html', 'xml', 'xhtml', 'xht', 'pl', 'htm', 'php', 'svg', 'mht', 'pdf', 'js' ]: - abs_prefix = input_api.os_path.join(input_api.PresubmitLocalPath(), - test_prefix) test_paths.update(input_api.glob(f'{abs_prefix}*.{extension}')) return [ input_api.os_path.relpath(test_path, input_api.PresubmitLocalPath()) diff --git a/third_party/blink/web_tests/PRESUBMIT_test.py b/third_party/blink/web_tests/PRESUBMIT_test.py index 7b5b8580d135a..66c83320d5f7e 100755 --- a/third_party/blink/web_tests/PRESUBMIT_test.py +++ b/third_party/blink/web_tests/PRESUBMIT_test.py @@ -13,73 +13,54 @@ import posixpath from unittest import mock import PRESUBMIT -sys.path.append( +_DIR_SOURCE_ROOT = os.path.normpath( os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', '..', '..')) +sys.path.append(_DIR_SOURCE_ROOT) + from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi, MockAffectedFile class PresubmitTest(unittest.TestCase): def testTestsCorrespondingToAffectedBaselines(self): - test_files = [ - '/chromium/src/third_party/blink/web_tests/' - 'path/to/test-without-behavior-change.html', - '/chromium/src/third_party/blink/web_tests/' - 'path/to/test-with-behavior-change.html', - '/chromium/src/third_party/blink/web_tests/' - 'path/to/test-with-variants.html', - '/chromium/src/third_party/blink/web_tests/' - 'path/to/test-multiglobals.any.js', - ] + D = 'third_party/blink/web_tests' mock_input_api = MockInputApi() - mock_input_api.os_path = posixpath - mock_input_api.presubmit_local_path = '/chromium/src/third_party/blink/web_tests' - mock_input_api.glob = mock.Mock( - side_effect=functools.partial(fnmatch.filter, test_files)) - mock_input_api.files = [ + mock_input_api.InitFiles([ + MockAffectedFile(f'{D}/subdir/without-behavior-change.html', []), + MockAffectedFile(f'{D}/subdir/with-behavior-change.html', []), + MockAffectedFile(f'{D}/subdir/with-variants.html', []), + MockAffectedFile(f'{D}/subdir/multiglobals.any.js', []), MockAffectedFile( - '/chromium/src/third_party/blink/web_tests/' - 'path/to/test-without-behavior-change.html', []), + f'{D}/flag-specific/fake-flag/' + 'subdir/with-behavior-change-expected.txt', []), MockAffectedFile( - '/chromium/src/third_party/blink/web_tests/' - 'path/to/test-with-behavior-change.html', []), + f'{D}/platform/fake-platform/virtual/fake-suite/' + 'subdir/with-variants_query-param-expected.txt', []), + MockAffectedFile(f'{D}/subdir/multiglobals.any-expected.txt', []), MockAffectedFile( - '/chromium/src/third_party/blink/web_tests/' - 'flag-specific/fake-flag/' - 'path/to/test-with-behavior-change-expected.txt', []), - MockAffectedFile( - '/chromium/src/third_party/blink/web_tests/' - 'platform/fake-platform/virtual/fake-suite/' - 'path/to/test-with-variants_query-param-expected.txt', []), - MockAffectedFile( - '/chromium/src/third_party/blink/web_tests/' - 'path/to/test-multiglobals.any-expected.txt', []), - MockAffectedFile( - '/chromium/src/third_party/blink/web_tests/' - 'path/to/test-multiglobals.any.worker-expected.txt', []), - ] + f'{D}/subdir/multiglobals.any.worker-expected.txt', []), + ]) tests = PRESUBMIT._TestsCorrespondingToAffectedBaselines( mock_input_api) self.assertEqual( set(tests), { - 'path/to/test-multiglobals.any.js', - 'path/to/test-with-behavior-change.html', - # `path/to/test-with-variants.html` is not checked; see inline - # comments. + os.path.normpath('subdir/multiglobals.any.js'), + os.path.normpath('subdir/with-behavior-change.html'), + # `with-variants.html` is not checked; see inline comments. }) def testCheckForDoctypeHTML(self): """This verifies that we correctly identify missing DOCTYPE html tags. """ - - file1 = MockAffectedFile("some/dir/file1.html", [ + D = 'third_party/blink/web_tests/external/wpt' + file1 = MockAffectedFile(f"{D}/some/dir/file1.html", [ "<!DOCTYPE html>", "<html>", "<body>", "<p>Test</p>", "</body>", "</html>" ]) file2 = MockAffectedFile( "some/dir2/file2.html", ["<html>", "<body>", "<p>Test</p>", "</body>", "</html>"]) - file3 = MockAffectedFile("file3.html", [ + file3 = MockAffectedFile(f"{D}/file3.html", [ "<!--Some comment-->", "<!docTYPE htML>", "<html>", "<body>", "<p>Test</p>", "</body>", "</html>" ]) @@ -87,9 +68,9 @@ class PresubmitTest(unittest.TestCase): ["<script></script>", "<!DOCTYPE html>"]) file5 = MockAffectedFile("file5.html", []) file6 = MockAffectedFile( - "file6.not_html", + f"{D}/file6.not_html", ["<html>", "<body>", "<p>Test</p>", "</body>", "</html>"]) - file7 = MockAffectedFile("file7.html", [ + file7 = MockAffectedFile(f"{D}/file7.html", [ "<!DOCTYPE html >", "<html>", "<body>", "<p>Test</p>", "</body>", "</html>" ]) @@ -98,19 +79,19 @@ class PresubmitTest(unittest.TestCase): "</body>", "</html>" ]) file9 = MockAffectedFile( - "some/dir/quirk-file9.html", + f"{D}/some/dir/quirk-file9.html", ["<html>", "<body>", "<p>Test</p>", "</body>", "</html>"]) file10 = MockAffectedFile( - "old/file10.html", + f"{D}/old/file10.html", ["<html>", "<body>", "<p>New content</p>", "</body>", "</html>"], ["<html>", "<body>", "<p>Old content</p>", "</body>", "</html>"], action="M") mock_input_api = MockInputApi() - mock_input_api.files = [ + mock_input_api.InitFiles([ file1, file2, file3, file4, file5, file6, file7, file8, file9, file10 - ] + ]) messages = PRESUBMIT._CheckForDoctypeHTML(mock_input_api, MockOutputApi()) @@ -123,14 +104,12 @@ class PresubmitTest(unittest.TestCase): """This test makes sure that we don't raise <!DOCTYPE html> errors for WPT importer. """ + D = 'third_party/blink/web_tests/external/wpt' error_file = MockAffectedFile( - "/chromium/src/third_party/blink/web_tests/" - "external/wpt/some/dir/doctype_error.html", + f"{D}/external/wpt/some/dir/doctype_error.html", ["<html>", "<body>", "<p>Test</p>", "</body>", "</html>"]) mock_input_api = MockInputApi() - mock_input_api.os_path = posixpath - mock_input_api.presubmit_local_path = "/chromium/src/third_party/blink/web_tests" - mock_input_api.files = [error_file] + mock_input_api.InitFiles([error_file]) messages = PRESUBMIT._CheckForDoctypeHTML(mock_input_api, MockOutputApi())