From 5a66ae7641e695205554eaf6e4e70295f9bffa46 Mon Sep 17 00:00:00 2001 From: Andrew Grieve Date: Fri, 13 Dec 2024 07:21:53 -0800 Subject: [PATCH] Add a PRESUBMIT to ensure Robolectric tests use BaseRobolectricTestRunner Bug: 383779576 Change-Id: Ica45a4591554acb2ff2f1c7f658185de0353ca5f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6092006 Commit-Queue: Yaron Friedman Auto-Submit: Andrew Grieve Reviewed-by: Yaron Friedman Cr-Commit-Position: refs/heads/main@{#1395951} --- PRESUBMIT.py | 27 +++++++++++++++++++++---- PRESUBMIT_test.py | 50 +++++++++++++++++++++++++++++++++++------------ 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index bdc02df51b78d2..58f3b56982e7d5 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -7363,19 +7363,20 @@ def CheckPythonShebang(input_api, output_api): return result -def CheckBatchAnnotation(input_api, output_api): +def CheckAndroidTestAnnotations(input_api, output_api): """Checks that tests have either @Batch or @DoNotBatch annotation. If this is not an instrumentation test, disregard.""" batch_annotation = input_api.re.compile(r'^\s*@Batch') do_not_batch_annotation = input_api.re.compile(r'^\s*@DoNotBatch') - robolectric_test = input_api.re.compile(r'[rR]obolectric') + robolectric_test = input_api.re.compile(r'@RunWith\((.*?)RobolectricTestRunner') test_class_declaration = input_api.re.compile(r'^\s*public\sclass.*Test') uiautomator_test = input_api.re.compile(r'[uU]i[aA]utomator') test_annotation_declaration = input_api.re.compile(r'^\s*public\s@interface\s.*{') missing_annotation_errors = [] extra_annotation_errors = [] + wrong_robolectric_test_runner_errors = [] def _FilterFile(affected_file): return input_api.FilterSourceFile( @@ -7388,9 +7389,20 @@ def _FilterFile(affected_file): do_not_batch_matched = None is_instrumentation_test = True test_annotation_declaration_matched = None + has_base_robolectric_rule = False for line in f.NewContents(): - if robolectric_test.search(line) or uiautomator_test.search(line): - # Skip Robolectric and UiAutomator tests. + if 'BaseRobolectricTestRule' in line: + has_base_robolectric_rule = True + continue + if m := robolectric_test.search(line): + is_instrumentation_test = False + if m.group(1) == '' and not has_base_robolectric_rule: + path = str(f.LocalPath()) + # These two spots cannot use it. + if 'webapk' not in path and 'build' not in path: + wrong_robolectric_test_runner_errors.append(path) + break + if uiautomator_test.search(line): is_instrumentation_test = False break if not batch_matched: @@ -7432,6 +7444,13 @@ def _FilterFile(affected_file): """ Robolectric tests do not need a @Batch or @DoNotBatch annotations. """, extra_annotation_errors)) + if wrong_robolectric_test_runner_errors: + results.append( + output_api.PresubmitPromptWarning( + """ +Robolectric tests should use either @RunWith(BaseRobolectricTestRule.class) (or +a subclass of it), or use "@Rule BaseRobolectricTestRule". +""", wrong_robolectric_test_runner_errors)) return results diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index a2c70635be53b7..b4d989378213c7 100755 --- a/PRESUBMIT_test.py +++ b/PRESUBMIT_test.py @@ -4998,10 +4998,10 @@ def testMissingParentheses(self): self.assertRegex(error.message, r'DCHECK_IS_ON().+parentheses') -class CheckBatchAnnotation(unittest.TestCase): - """Test the CheckBatchAnnotation presubmit check.""" +class CheckAndroidTestAnnotations(unittest.TestCase): + """Test the CheckAndroidTestAnnotations presubmit check.""" - def testTruePositives(self): + def testBatchTruePositives(self): """Examples of when there is no @Batch or @DoNotBatch is correctly flagged. """ mock_input = MockInputApi() @@ -5010,16 +5010,16 @@ def testTruePositives(self): MockFile('path/TwoTest.java', ['public class TwoTest']), MockFile('path/ThreeTest.java', [ '@Batch(Batch.PER_CLASS)', - 'import org.chromium.base.test.BaseRobolectricTestRunner;', + '@RunWith(BaseRobolectricTestRunner.class)', 'public class Three {' ]), MockFile('path/FourTest.java', [ '@DoNotBatch(reason = "placeholder reason 1")', - 'import org.chromium.base.test.BaseRobolectricTestRunner;', + '@RunWith(BaseRobolectricTestRunner.class)', 'public class Four {' ]), ] - errors = PRESUBMIT.CheckBatchAnnotation(mock_input, MockOutputApi()) + errors = PRESUBMIT.CheckAndroidTestAnnotations(mock_input, MockOutputApi()) self.assertEqual(2, len(errors)) self.assertEqual(2, len(errors[0].items)) self.assertIn('OneTest.java', errors[0].items[0]) @@ -5028,7 +5028,7 @@ def testTruePositives(self): self.assertIn('ThreeTest.java', errors[1].items[0]) self.assertIn('FourTest.java', errors[1].items[1]) - def testAnnotationsPresent(self): + def testBatchAnnotationsPresent(self): """Examples of when there is @Batch or @DoNotBatch is correctly flagged.""" mock_input = MockInputApi() mock_input.files = [ @@ -5060,17 +5060,17 @@ def testAnnotationsPresent(self): 'public class Five extends BaseTestB {' ]), MockFile('path/SixTest.java', [ - 'import org.chromium.base.test.BaseRobolectricTestRunner;', + '@RunWith(BaseRobolectricTestRunner.class)', 'public class Six extends BaseTestA {' ], [ - 'import org.chromium.base.test.BaseRobolectricTestRunner;', + '@RunWith(BaseRobolectricTestRunner.class)', 'public class Six extends BaseTestB {' ]), MockFile('path/SevenTest.java', [ - 'import org.robolectric.annotation.Config;', + '@RunWith(BaseRobolectricTestRunner.class)', 'public class Seven extends BaseTestA {' ], [ - 'import org.robolectric.annotation.Config;', + '@RunWith(BaseRobolectricTestRunner.class)', 'public class Seven extends BaseTestB {' ]), MockFile( @@ -5086,9 +5086,35 @@ def testAnnotationsPresent(self): ['public @interface SomeAnnotation {'], ), ] - errors = PRESUBMIT.CheckBatchAnnotation(mock_input, MockOutputApi()) + errors = PRESUBMIT.CheckAndroidTestAnnotations(mock_input, MockOutputApi()) self.assertEqual(0, len(errors)) + def testWrongRobolectricTestRunner(self): + mock_input = MockInputApi() + mock_input.files = [ + MockFile('path/OneTest.java', [ + '@RunWith(RobolectricTestRunner.class)', + 'public class ThreeTest {' + ]), + MockFile('path/TwoTest.java', [ + 'import org.chromium.base.test.BaseRobolectricTestRule;', + '@RunWith(RobolectricTestRunner.class)', + 'public class TwoTest {' + ]), + MockFile('path/ThreeTest.java', [ + '@RunWith(FooRobolectricTestRunner.class)', + 'public class ThreeTest {' + ]), + MockFile('webapks/FourTest.java', [ + '@RunWith(RobolectricTestRunner.class)', + 'public class ThreeTest {' + ]), + ] + errors = PRESUBMIT.CheckAndroidTestAnnotations(mock_input, MockOutputApi()) + self.assertEqual(1, len(errors)) + self.assertEqual(1, len(errors[0].items)) + self.assertIn('OneTest.java', errors[0].items[0]) + class CheckMockAnnotation(unittest.TestCase): """Test the CheckMockAnnotation presubmit check."""