Add Presubmit Warnings for Junit4

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 <yolandyan@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497790}

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
 

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):
 

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)