Skip to content

Commit

Permalink
Revert "Reland "Improve mocking of os_path.exists() in PRESUBMIT_test…
Browse files Browse the repository at this point in the history 
….py""

This reverts commit 3ba398e.

Reason for revert: crbug.com/372313790

Original change's description:
> Reland "Improve mocking of os_path.exists() in PRESUBMIT_test.py"
>
> 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}

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 <[email protected]>
Owners-Override: Sophey Dong <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Auto-Submit: Sophey Dong <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1365901}
  • Loading branch information
Sophey Dong authored and Chromium LUCI CQ committed Oct 9, 2024
1 parent cac6fcf commit 58cf9bb
Show file tree
Hide file tree
Showing 8 changed files with 303 additions and 299 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'):
# f.LocalPath is relative to repo (src, or internal repo).
# os_path.exists is relative to src repo.
# Therefore if os_path.exists is true, it means f.LocalPath is relative
# to src and we can conclude that the pydeps is in src.
exists = input_api.os_path.exists(f.LocalPath())
if f.Action() == 'D' and f.LocalPath() in _ALL_PYDEPS_FILES:
results.append(
output_api.PresubmitError(
'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
'remove %s' % f.LocalPath()))
elif (f.Action() != 'D' and exists
and f.LocalPath() not in _ALL_PYDEPS_FILES):
results.append(
output_api.PresubmitError(
'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
'include %s' % f.LocalPath()))
if input_api.os_path.exists(f.LocalPath()):
if f.Action() == 'D' and f.LocalPath() in _ALL_PYDEPS_FILES:
results.append(
output_api.PresubmitError(
'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
'remove %s' % f.LocalPath()))
elif f.Action() != 'D' and f.LocalPath(
) not in _ALL_PYDEPS_FILES:
results.append(
output_api.PresubmitError(
'Please update _ALL_PYDEPS_FILES within //PRESUBMIT.py to '
'include %s' % f.LocalPath()))

if results:
return results
Expand Down Expand Up @@ -6771,8 +6771,8 @@ def CheckStrings(input_api, output_api):
return []

affected_png_paths = [
f.LocalPath() for f in input_api.AffectedFiles()
if f.LocalPath().endswith('.png')
f.AbsoluteLocalPath() for f in input_api.AffectedFiles()
if (f.LocalPath().endswith('.png'))
]

# Check for screenshots. Developers can upload screenshots using
Expand Down
62 changes: 40 additions & 22 deletions PRESUBMIT_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,15 +365,6 @@ def testDeleteSha256Success(self):

class CheckAddedDepsHaveTestApprovalsTest(unittest.TestCase):

def setUp(self):
self.input_api = input_api = MockInputApi()
input_api.environ = {}
input_api.owners_client = self.FakeOwnersClient()
input_api.gerrit = self.fakeGerrit()
input_api.change.issue = 123
self.mockOwnersAndReviewers("owner", set(["reviewer"]))
self.mockListSubmodules([])

def calculate(self, old_include_rules, old_specific_include_rules,
new_include_rules, new_specific_include_rules):
return PRESUBMIT._CalculateAddedDeps(
Expand Down Expand Up @@ -470,6 +461,15 @@ class fakeGerrit(object):
def IsOwnersOverrideApproved(self, issue):
return False

def setUp(self):
self.input_api = input_api = MockInputApi()
input_api.environ = {}
input_api.owners_client = self.FakeOwnersClient()
input_api.gerrit = self.fakeGerrit()
input_api.change.issue = 123
self.mockOwnersAndReviewers("owner", set(["reviewer"]))
self.mockListSubmodules([])

def mockOwnersAndReviewers(self, owner, reviewers):

def mock(*args, **kwargs):
Expand All @@ -485,9 +485,11 @@ def mock(*args, **kwargs):
self.input_api.ListSubmodules = mock

def testApprovedAdditionalDep(self):
self.input_api.InitFiles([
MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']),
])
old_deps = """include_rules = []""".splitlines()
new_deps = """include_rules = ["+v8/123"]""".splitlines()
self.input_api.files = [
MockAffectedFile("pdf/DEPS", new_deps, old_deps)
]

# mark the additional dep as approved.
os_path = self.input_api.os_path
Expand All @@ -499,9 +501,11 @@ def testApprovedAdditionalDep(self):
self.assertEqual([], results)

def testUnapprovedAdditionalDep(self):
self.input_api.InitFiles([
MockAffectedFile('pdf/DEPS', ['include_rules=["+v8/123"]']),
])
old_deps = """include_rules = []""".splitlines()
new_deps = """include_rules = ["+v8/123"]""".splitlines()
self.input_api.files = [
MockAffectedFile('pdf/DEPS', new_deps, old_deps),
]

# pending.
os_path = self.input_api.os_path
Expand Down Expand Up @@ -788,19 +792,23 @@ def testAddedPydep(self):
if not self.mock_input_api.platform.startswith('linux'):
return []

self.mock_input_api.InitFiles([
self.mock_input_api.files = [
MockAffectedFile('new.pydeps', [], action='A'),
])
]

self.mock_input_api.CreateMockFileInPath([
x.LocalPath()
for x in self.mock_input_api.AffectedFiles(include_deletes=True)
])
results = self._RunCheck()
self.assertEqual(1, len(results))
self.assertIn('PYDEPS_FILES', str(results[0]))

def testPydepNotInSrc(self):
self.mock_input_api.InitFiles([
self.mock_input_api.files = [
MockAffectedFile('new.pydeps', [], action='A'),
])
self.mock_input_api.os_path.exists = lambda x: False
]
self.mock_input_api.CreateMockFileInPath([])
results = self._RunCheck()
self.assertEqual(0, len(results))

Expand All @@ -809,8 +817,12 @@ def testRemovedPydep(self):
if not self.mock_input_api.platform.startswith('linux'):
return []

self.mock_input_api.InitFiles([
self.mock_input_api.files = [
MockAffectedFile(PRESUBMIT._ALL_PYDEPS_FILES[0], [], action='D'),
]
self.mock_input_api.CreateMockFileInPath([
x.LocalPath()
for x in self.mock_input_api.AffectedFiles(include_deletes=True)
])
results = self._RunCheck()
self.assertEqual(1, len(results))
Expand Down Expand Up @@ -3735,7 +3747,13 @@ class StringTest(unittest.TestCase):

def makeInputApi(self, files):
input_api = MockInputApi()
input_api.InitFiles(files)
input_api.files = files
# Override os_path.exists because the presubmit uses the actual
# os.path.exists.
input_api.CreateMockFileInPath([
x.LocalPath()
for x in input_api.AffectedFiles(include_deletes=True)
])
return input_api

""" CL modified and added messages, but didn't add any screenshots."""
Expand Down
36 changes: 8 additions & 28 deletions PRESUBMIT_test_mocks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@
import subprocess
import sys


_REPO_ROOT = os.path.abspath(os.path.dirname(__file__))

# TODO(dcheng): It's kind of horrible that this is copy and pasted from
# presubmit_canned_checks.py, but it's far easier than any of the alternatives.
def _ReportErrorFileAndLine(filename, line_num, dummy_line):
Expand Down Expand Up @@ -83,32 +80,16 @@ def __init__(self):
self.files = []
self.is_committing = False
self.change = MockChange([])
self.presubmit_local_path = os.path.dirname(
os.path.abspath(sys.argv[0]))
self.presubmit_local_path = os.path.dirname(__file__)
self.is_windows = sys.platform == 'win32'
self.no_diffs = False
# Although this makes assumptions about command line arguments used by
# test scripts that create mocks, it is a convenient way to set up the
# verbosity via the input api.
self.verbose = '--verbose' in sys.argv

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

def AffectedFiles(self, file_filter=None, include_deletes=True):
for file in self.files:
Expand Down Expand Up @@ -165,7 +146,7 @@ def ReadFile(self, filename, mode='r'):
if hasattr(filename, 'AbsoluteLocalPath'):
filename = filename.AbsoluteLocalPath()
for file_ in self.files:
if filename in (file_.LocalPath(), file_.AbsoluteLocalPath()):
if file_.LocalPath() == filename:
return '\n'.join(file_.NewContents())
# Otherwise, file is not in our mock API.
raise IOError("No such file or directory: '%s'" % filename)
Expand Down Expand Up @@ -266,7 +247,7 @@ def LocalPath(self):
return self._local_path

def AbsoluteLocalPath(self):
return os.path.join(_REPO_ROOT, self._local_path)
return self._local_path

def GenerateScmDiff(self):
return self._scm_diff
Expand All @@ -292,7 +273,9 @@ def replace(self, altsep, sep):


class MockAffectedFile(MockFile):
pass

def AbsoluteLocalPath(self):
return self._local_path


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

def GitFootersFromDescription(self):
return self.footers

def RepositoryRoot(self):
return _REPO_ROOT
Loading

0 comments on commit 58cf9bb

Please sign in to comment.