Skip to content

Commit

Permalink
[mojo] Update presubmit checks to prevent introducing old Mojo types
Browse files Browse the repository at this point in the history
Following the work started in CL1796371, this change changes the logic
of the PRESUBMIT.py script in order to raise ERRORS from now on when
deprecated mojo types are introduced in Blink, and raise WARNINGS when
those old types are introduced everywhere else but //components/arc,
which hasn't been migrated yet (see crrev.com/c/1868870).

We didn't do this before to prevent too many "false positives" when
moving code around, but since all of Blink has been migrated now and
nearly everything outside of Blink (but //components/arc) is nearly
finished, this seems like a good moment to flip the switch.

See the "Mojo Bindings Conversion CheatSheet" for more information:
https://docs.google.com/document/d/1Jwfbzbe8ozaoilhqj5mAPYbYGpgZCen_XAAAdwmyP1E

Bug: 955171
Change-Id: I1f7b62ff6841935a43c462dc266d3f3aa632aa80
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1961908
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#724971}
  • Loading branch information
mariospr authored and Commit Bot committed Dec 15, 2019
1 parent 86ca2fb commit cec9cef
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 8 deletions.
13 changes: 11 additions & 2 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -1887,24 +1887,33 @@ def CheckForMatch(affected_file, line_num, line, func_name, message, error):
def _CheckNoDeprecatedMojoTypes(input_api, output_api):
"""Make sure that old Mojo types are not used."""
warnings = []
errors = []

file_filter = lambda f: f.LocalPath().endswith(('.cc', '.mm', '.h'))
for f in input_api.AffectedFiles(file_filter=file_filter):
# Only need to check Blink for warnings for now.
if not f.LocalPath().startswith('third_party/blink'):
# Don't check //components/arc, not yet migrated (see crrev.com/c/1868870).
if f.LocalPath().startswith('components/arc'):
continue

for line_num, line in f.ChangedContents():
for func_name, message in _DEPRECATED_MOJO_TYPES:
problems = _GetMessageForMatchingType(input_api, f, line_num, line,
func_name, message)

if problems:
# Violations raise errors inside Blink and warnings everywhere else.
if f.LocalPath().startswith('third_party/blink'):
errors.extend(problems)
else:
warnings.extend(problems)

result = []
if (warnings):
result.append(output_api.PresubmitPromptWarning(
'Banned Mojo types were used.\n' + '\n'.join(warnings)))
if (errors):
result.append(output_api.PresubmitError(
'Banned Mojo types were used.\n' + '\n'.join(errors)))
return result


Expand Down
23 changes: 17 additions & 6 deletions PRESUBMIT_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2074,8 +2074,9 @@ def testBannedMojoFunctions(self):
self.assertTrue('content/renderer/ok/file3.cc' not in results[0].message)

def testDeprecatedMojoTypes(self):
ok_paths = ['some/cpp']
warning_paths = ['third_party/blink']
ok_paths = ['components/arc']
warning_paths = ['some/cpp']
error_paths = ['third_party/blink']
test_cases = [
{
'type': 'mojo::AssociatedBinding<>;',
Expand Down Expand Up @@ -2144,7 +2145,7 @@ def testDeprecatedMojoTypes(self):
]

# Build the list of MockFiles considering paths that should trigger warnings
# as well as paths that should not trigger anything.
# as well as paths that should trigger errors.
input_api = MockInputApi()
input_api.files = []
for test_case in test_cases:
Expand All @@ -2154,20 +2155,30 @@ def testDeprecatedMojoTypes(self):
for path in warning_paths:
input_api.files.append(MockFile(os.path.join(path, test_case['file']),
[test_case['type']]))
for path in error_paths:
input_api.files.append(MockFile(os.path.join(path, test_case['file']),
[test_case['type']]))

results = PRESUBMIT._CheckNoDeprecatedMojoTypes(input_api, MockOutputApi())

# Only warnings for now for all deprecated Mojo types.
self.assertEqual(1, len(results))
# warnings are results[0], errors are results[1]
self.assertEqual(2, len(results))

for test_case in test_cases:
# Check that no warnings or errors have been triggered for these paths.
# Check that no warnings nor errors have been triggered for these paths.
for path in ok_paths:
self.assertFalse(path in results[0].message)
self.assertFalse(path in results[1].message)

# Check warnings have been triggered for these paths.
for path in warning_paths:
self.assertTrue(path in results[0].message)
self.assertFalse(path in results[1].message)

# Check errors have been triggered for these paths.
for path in error_paths:
self.assertFalse(path in results[0].message)
self.assertTrue(path in results[1].message)


class NoProductionCodeUsingTestOnlyFunctionsTest(unittest.TestCase):
Expand Down

0 comments on commit cec9cef

Please sign in to comment.