Skip to content

Commit

Permalink
Reland "Improve mocking of os_path.exists() in PRESUBMIT_test.py"
Browse files Browse the repository at this point in the history 
This reverts commit 74e5e08.

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 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 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 <[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}

Bug: None
Change-Id: I303d4d1abc685e3bad854e9a5ebe8ae2efe48869
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5912982
Commit-Queue: Andrew Grieve <[email protected]>
Auto-Submit: Andrew Grieve <[email protected]>
Reviewed-by: Ted Choc <[email protected]>
Owners-Override: Andrew Grieve <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1365664}
  • Loading branch information
@agrieve
agrieve authored and Chromium LUCI CQ committed Oct 8, 2024
1 parent f06ae8b commit 3ba398e
Show file tree
Hide file tree
Showing 8 changed files with 299 additions and 303 deletions.
36 changes: 18 additions & 18 deletions PRESUBMIT.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5181,23 +5181,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
Expand Down Expand Up @@ -6771,8 +6771,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
Expand Down
62 changes: 22 additions & 40 deletions PRESUBMIT_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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):
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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))

Expand All @@ -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))
Expand Down Expand Up @@ -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."""
Expand Down
36 changes: 28 additions & 8 deletions PRESUBMIT_test_mocks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -80,16 +83,32 @@ def __init__(self):
self.files = []
self.is_committing = False
self.change = MockChange([])
self.presubmit_local_path = os.path.dirname(__file__)
self.presubmit_local_path = os.path.dirname(
os.path.abspath(sys.argv[0]))
self.is_windows = sys.platform == 'win32'
self.no_diffs = False
# Although this makes assumptions about command line arguments used by
# test scripts that create mocks, it is a convenient way to set up the
# verbosity via the input api.
self.verbose = '--verbose' in sys.argv

def CreateMockFileInPath(self, f_list):
self.os_path.exists = lambda x: x in f_list
def InitFiles(self, files):
# 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

self.os_path.exists = mock_exists

def AffectedFiles(self, file_filter=None, include_deletes=True):
for file in self.files:
Expand Down Expand Up @@ -146,7 +165,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)
Expand Down Expand Up @@ -247,7 +266,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
Expand All @@ -273,9 +292,7 @@ def replace(self, altsep, sep):


class MockAffectedFile(MockFile):

def AbsoluteLocalPath(self):
return self._local_path
pass


class MockChange(object):
Expand All @@ -301,3 +318,6 @@ def AffectedFiles(self,

def GitFootersFromDescription(self):
return self.footers

def RepositoryRoot(self):
return _REPO_ROOT
Loading

0 comments on commit 3ba398e

Please sign in to comment.