From cec9cef7196c57d8ae1480b2b5f9907640d0418b Mon Sep 17 00:00:00 2001 From: Mario Sanchez Prada Date: Sun, 15 Dec 2019 11:54:57 +0000 Subject: [PATCH] [mojo] Update presubmit checks to prevent introducing old Mojo types 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 Reviewed-by: Kentaro Hara Commit-Queue: Mario Sanchez Prada Cr-Commit-Position: refs/heads/master@{#724971} --- PRESUBMIT.py | 13 +++++++++++-- PRESUBMIT_test.py | 23 +++++++++++++++++------ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 228569c1a255a7..5598c46e542cde 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -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 diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index a04074cf881c63..96d8ec3ce57125 100755 --- a/PRESUBMIT_test.py +++ b/PRESUBMIT_test.py @@ -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<>;', @@ -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: @@ -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):