From b92fa52719c8826a96d85e19f0b204c76998f523 Mon Sep 17 00:00:00 2001 From: Yoland Yan Date: Mon, 28 Aug 2017 17:37:06 +0000 Subject: [PATCH] Add Presubmit Warnings for Junit4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These presubmit warnings prevent people from adding new JUnit3 tests or use inheritance in JUnit4 testing, it also prevent people from using the deprecated JUnit framework. For more on JUnit4 migration, please check //testing/android/docs/junit4.md Bug: 640116 Change-Id: I941b595f6fbc01ac60a2647ab0af64482596d9cc Reviewed-on: https://chromium-review.googlesource.com/634603 Commit-Queue: Yoland Yan Reviewed-by: Paweł Hajdan Jr. Cr-Commit-Position: refs/heads/master@{#497790} --- PRESUBMIT.py | 54 +++++++++++++++++++++++++ PRESUBMIT_test.py | 89 ++++++++++++++++++++++++++++++++++++++++- PRESUBMIT_test_mocks.py | 6 ++- 3 files changed, 147 insertions(+), 2 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 144c5ccc855c14..a29e0a0756a4b4 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -1802,6 +1802,58 @@ def _CheckAndroidCrLogUsage(input_api, output_api): return results +def _CheckAndroidTestJUnitFrameworkImport(input_api, output_api): + """Checks that junit.framework.* is no longer used.""" + deprecated_junit_framework_pattern = input_api.re.compile( + r'^import junit\.framework\..*;', + input_api.re.MULTILINE) + sources = lambda x: input_api.FilterSourceFile( + x, white_list=(r'.*\.java$',), black_list=None) + errors = [] + for f in input_api.AffectedFiles(sources): + for line_num, line in f.ChangedContents(): + if deprecated_junit_framework_pattern.search(line): + errors.append("%s:%d" % (f.LocalPath(), line_num)) + + results = [] + if errors: + results.append(output_api.PresubmitError( + 'APIs from junit.framework.* are deprecated, please use JUnit4 framework' + '(org.junit.*) from //third_party/junit. Contact yolandyan@chromium.org' + ' if you have any question.', errors)) + return results + + +def _CheckAndroidTestJUnitInheritance(input_api, output_api): + """Checks that if new Java test classes have inheritance. + Either the new test class is JUnit3 test or it is a JUnit4 test class + with a base class, either case is undesirable. + """ + class_declaration_pattern = input_api.re.compile(r'^public class \w*Test ') + + sources = lambda x: input_api.FilterSourceFile( + x, white_list=(r'.*Test\.java$',), black_list=None) + errors = [] + for f in input_api.AffectedFiles(sources): + if not f.OldContents(): + class_declaration_start_flag = False + for line_num, line in f.ChangedContents(): + if class_declaration_pattern.search(line): + class_declaration_start_flag = True + if class_declaration_start_flag and ' extends ' in line: + errors.append('%s:%d' % (f.LocalPath(), line_num)) + if '{' in line: + class_declaration_start_flag = False + + results = [] + if errors: + results.append(output_api.PresubmitPromptWarning( + 'The newly created files include Test classes that inherits from base' + ' class. Please do not use inheritance in JUnit4 tests or add new' + ' JUnit3 tests. Contact yolandyan@chromium.org if you have any' + ' questions.', errors)) + return results + def _CheckAndroidTestAnnotationUsage(input_api, output_api): """Checks that android.test.suitebuilder.annotation.* is no longer used.""" deprecated_annotation_import_pattern = input_api.re.compile( @@ -2292,6 +2344,8 @@ def _AndroidSpecificOnUploadChecks(input_api, output_api): results.extend(_CheckAndroidCrLogUsage(input_api, output_api)) results.extend(_CheckAndroidNewMdpiAssetLocation(input_api, output_api)) results.extend(_CheckAndroidToastUsage(input_api, output_api)) + results.extend(_CheckAndroidTestJUnitInheritance(input_api, output_api)) + results.extend(_CheckAndroidTestJUnitFrameworkImport(input_api, output_api)) results.extend(_CheckAndroidTestAnnotationUsage(input_api, output_api)) return results diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index f8feb78f1815ca..649872cb9d5a59 100755 --- a/PRESUBMIT_test.py +++ b/PRESUBMIT_test.py @@ -742,7 +742,6 @@ def mock_check_output(cmd, shell=False): self.assertTrue('File is stale' in str(results[0])) self.assertTrue('File is stale' in str(results[1])) - class AndroidDeprecatedTestAnnotationTest(unittest.TestCase): def testCheckAndroidTestAnnotationUsage(self): mock_input_api = MockInputApi() @@ -788,7 +787,95 @@ def testCheckAndroidTestAnnotationUsage(self): self.assertTrue('UsedDeprecatedSmokeAnnotation.java:1' in msgs[0].items, 'UsedDeprecatedSmokeAnnotation not found in errors') +class AndroidDeprecatedJUnitFrameworkTest(unittest.TestCase): + def testCheckAndroidTestAnnotationUsage(self): + mock_input_api = MockInputApi() + mock_output_api = MockOutputApi() + mock_input_api.files = [ + MockAffectedFile('LalaLand.java', [ + 'random stuff' + ]), + MockAffectedFile('CorrectUsage.java', [ + 'import org.junit.ABC', + 'import org.junit.XYZ;', + ]), + MockAffectedFile('UsedDeprecatedJUnit.java', [ + 'import junit.framework.*;', + ]), + MockAffectedFile('UsedDeprecatedJUnitAssert.java', [ + 'import junit.framework.Assert;', + ]), + ] + msgs = PRESUBMIT._CheckAndroidTestJUnitFrameworkImport( + mock_input_api, mock_output_api) + self.assertEqual(1, len(msgs), + 'Expected %d items, found %d: %s' + % (1, len(msgs), msgs)) + self.assertEqual(2, len(msgs[0].items), + 'Expected %d items, found %d: %s' + % (2, len(msgs[0].items), msgs[0].items)) + self.assertTrue('UsedDeprecatedJUnit.java:1' in msgs[0].items, + 'UsedDeprecatedJUnit.java not found in errors') + self.assertTrue('UsedDeprecatedJUnitAssert.java:1' + in msgs[0].items, + 'UsedDeprecatedJUnitAssert not found in errors') + +class AndroidJUnitBaseClass(unittest.TestCase): + def testCheckAndroidTestAnnotationUsage(self): + mock_input_api = MockInputApi() + mock_output_api = MockOutputApi() + + mock_input_api.files = [ + MockAffectedFile('LalaLand.java', [ + 'random stuff' + ]), + MockAffectedFile('CorrectTest.java', [ + '@RunWith(ABC.class);' + 'public class CorrectTest {', + '}', + ]), + MockAffectedFile('HistoricallyIncorrectTest.java', [ + 'public class Test extends BaseCaseA {', + '}', + ], old_contents=[ + 'public class Test extends BaseCaseB {', + '}', + ]), + MockAffectedFile('CorrectTestWithInterface.java', [ + '@RunWith(ABC.class);' + 'public class CorrectTest implement Interface {', + '}', + ]), + MockAffectedFile('IncorrectTest.java', [ + 'public class IncorrectTest extends TestCase {', + '}', + ]), + MockAffectedFile('IncorrectTestWithInterface.java', [ + 'public class Test implements X extends BaseClass {', + '}', + ]), + MockAffectedFile('IncorrectTestMultiLine.java', [ + 'public class Test implements X, Y, Z', + ' extends TestBase {', + '}', + ]), + ] + msgs = PRESUBMIT._CheckAndroidTestJUnitInheritance( + mock_input_api, mock_output_api) + self.assertEqual(1, len(msgs), + 'Expected %d items, found %d: %s' + % (1, len(msgs), msgs)) + self.assertEqual(3, len(msgs[0].items), + 'Expected %d items, found %d: %s' + % (3, len(msgs[0].items), msgs[0].items)) + self.assertTrue('IncorrectTest.java:1' in msgs[0].items, + 'IncorrectTest not found in errors') + self.assertTrue('IncorrectTestWithInterface.java:1' + in msgs[0].items, + 'IncorrectTestWithInterface not found in errors') + self.assertTrue('IncorrectTestMultiLine.java:2' in msgs[0].items, + 'IncorrectTestMultiLine not found in errors') class LogUsageTest(unittest.TestCase): diff --git a/PRESUBMIT_test_mocks.py b/PRESUBMIT_test_mocks.py index 162570ef4c85b4..c6aa84ab600276 100644 --- a/PRESUBMIT_test_mocks.py +++ b/PRESUBMIT_test_mocks.py @@ -97,13 +97,14 @@ class MockFile(object): MockInputApi for presubmit unittests. """ - def __init__(self, local_path, new_contents, action='A'): + def __init__(self, local_path, new_contents, old_contents=None, action='A'): self._local_path = local_path self._new_contents = new_contents self._changed_contents = [(i + 1, l) for i, l in enumerate(new_contents)] self._action = action self._scm_diff = "--- /dev/null\n+++ %s\n@@ -0,0 +1,%d @@\n" % (local_path, len(new_contents)) + self._old_contents = old_contents for l in new_contents: self._scm_diff += "+%s\n" % l @@ -125,6 +126,9 @@ def AbsoluteLocalPath(self): def GenerateScmDiff(self): return self._scm_diff + def OldContents(self): + return self._old_contents + def rfind(self, p): """os.path.basename is called on MockFile so we need an rfind method.""" return self._local_path.rfind(p)