0

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

This reverts commit 58cf9bbd16.

Reason for reland: Fixed broken test

Original change's description:
> Revert "Reland "Improve mocking of os_path.exists() in PRESUBMIT_test.py""
>
> This reverts commit 3ba398ee58.
>
> Reason for revert: crbug.com/372313790
>
> Original change's description:
> > Reland "Improve mocking of os_path.exists() in PRESUBMIT_test.py"
> >
> > This reverts commit 74e5e087cb.
> >
> > Reason for reland: Fixed blink & webapk presubmit tests
> >
> > Original change's description:
> > > Revert "Improve mocking of os_path.exists() in PRESUBMIT_test.py"
> > >
> > > This reverts commit b35a9a9465.
> > >
> > > 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 upcoming 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 <tedchoc@chromium.org>
> > > > Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> > > > 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 <agrieve@chromium.org>
> > > Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> > > Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
> > > Owners-Override: Łukasz Anforowicz <lukasza@chromium.org>
> > > Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
> > > Cr-Commit-Position: refs/heads/main@{#1364999}
> >
> > Bug: None
> > Change-Id: I303d4d1abc685e3bad854e9a5ebe8ae2efe48869
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5912982
> > Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> > Auto-Submit: Andrew Grieve <agrieve@chromium.org>
> > Reviewed-by: Ted Choc <tedchoc@chromium.org>
> > Owners-Override: Andrew Grieve <agrieve@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1365664}
>
> Bug: None
> Change-Id: Iaebc98243a14fff08442948fea4d69531f6103ee
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5918609
> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Owners-Override: Sophey Dong <sophey@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Auto-Submit: Sophey Dong <sophey@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1365901}

Bug: None
Change-Id: I998b8bea9e513d24652ce3d92bf88f677c987538
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5921545
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Owners-Override: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1368995}
This commit is contained in:
Andrew Grieve
2024-10-15 20:20:08 +00:00
committed by Chromium LUCI CQ
parent 794759e291
commit 713b89b89f
10 changed files with 323 additions and 343 deletions

@ -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

@ -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."""

@ -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

@ -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()

@ -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 '

@ -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__':

@ -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:

@ -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(

@ -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())

@ -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())