Android: Ensure that no tests run during instrumentation test listing.

To support parameterized tests, our test runner does not use the default
test listing logic, and instead requests that tests be run, and relies
on BaseJUnit4ClassRunner to no-op them all when in this mode.

This scheme works only if tests actually use BaseJUnit4ClassRunner (or a
subclass). The consequence of not using this runner is that the tests
will run during test listing.

This changes our test runner to enforce that all tests use
BaseJUnit4ClassRunner so that we never have tests mistakenly being run
during test listing.

Bug: 999007
Change-Id: I226fe55b5716c713d47a6027c088ca6774ae0c10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1777089Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692128}

base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java

@@ -252,9 +252,9 @@ public class BaseChromiumAndroidJUnitRunner extends AndroidJUnitRunner {
         }
         builder.addFromRunnerArgs(runnerArgs);
         builder.addApkToScan(getContext().getPackageCodePath());
-        // See crbug://841695. TestLoader.isTestClass is incorrectly deciding that
-        // InstrumentationTestSuite is a test class.
-        builder.removeTestClass("android.test.InstrumentationTestSuite");
+
+        // Ignore tests from framework / support library classes.
+        builder.removeTestPackage("android");
         builder.setClassLoader(new ForgivingClassLoader());
         return builder.build();
     }
@@ -293,6 +293,12 @@ public class BaseChromiumAndroidJUnitRunner extends AndroidJUnitRunner {
             }
         }
 
+        @Override
+        public TestRequestBuilder removeTestPackage(String testPackage) {
+            mExcludedPrefixes.add(testPackage);
+            return this;
+        }
+
         @Override
         public TestRequestBuilder addFromRunnerArgs(RunnerArgs runnerArgs) {
             mExcludedPrefixes.addAll(runnerArgs.notTestPackages);

base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java

@@ -224,7 +224,6 @@ public class BaseJUnit4ClassRunner extends AndroidJUnit4ClassRunner {
         if (BaseChromiumAndroidJUnitRunner.shouldListTests(
                     InstrumentationRegistry.getArguments())) {
             for (Description child : getDescription().getChildren()) {
-                notifier.fireTestStarted(child);
                 notifier.fireTestFinished(child);
             }
             return;

base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java

@@ -10,6 +10,7 @@ import org.json.JSONArray;
 import org.json.JSONObject;
 import org.junit.runner.Description;
 import org.junit.runner.notification.Failure;
+import org.junit.runners.model.InitializationError;
 
 import org.chromium.base.Log;
 
@@ -47,23 +48,33 @@ public class TestListInstrumentationRunListener extends InstrumentationRunListen
         }
     }
 
-    /**
-     * Store the test method description to a Map at the beginning of a test run.
-     */
     @Override
-    public void testStarted(Description desc) throws Exception {
-        if (mTestClassJsonMap.containsKey(desc.getTestClass())) {
-            ((JSONArray) mTestClassJsonMap.get(desc.getTestClass()).get("methods"))
-                .put(getTestMethodJSON(desc));
-        } else {
+    public void testFinished(Description desc) throws Exception {
         Class<?> testClass = desc.getTestClass();
-            mTestClassJsonMap.put(desc.getTestClass(), new JSONObject()
+        JSONObject classEntry = mTestClassJsonMap.get(testClass);
+        if (classEntry == null) {
+            classEntry =
+                    new JSONObject()
                             .put("class", testClass.getName())
                             .put("superclass", testClass.getSuperclass().getName())
                             .put("annotations",
                                     getAnnotationJSON(Arrays.asList(testClass.getAnnotations())))
-                    .put("methods", new JSONArray().put(getTestMethodJSON(desc))));
+                            .put("methods", new JSONArray());
+            mTestClassJsonMap.put(testClass, classEntry);
+        }
+        ((JSONArray) classEntry.get("methods")).put(getTestMethodJSON(desc));
     }
+
+    /**
+     * Store the test method description to a Map at the beginning of a test run.
+     */
+    @Override
+    public void testStarted(Description desc) throws Exception {
+        // BaseJUnit4ClassRunner only fires testFinished(), so a call to testStarted means a
+        // different runner is active, and the test is actually being executed rather than just
+        // listed.
+        throw new InitializationError("All tests must use @RunWith(BaseJUnit4ClassRunner.class) or "
+                + "a subclass thereof. Found that this test does not: " + desc.getTestClass());
     }
 
     /**

build/android/pylib/instrumentation/instrumentation_test_instance.py

@@ -40,13 +40,6 @@ _EXCLUDE_UNLESS_REQUESTED_ANNOTATIONS = [
 _VALID_ANNOTATIONS = set(_DEFAULT_ANNOTATIONS +
                          _EXCLUDE_UNLESS_REQUESTED_ANNOTATIONS)
 
-# These test methods are inherited from android.test base test class and
-# should be permitted for not having size annotation. For more, please check
-# https://developer.android.com/reference/android/test/AndroidTestCase.html
-# https://developer.android.com/reference/android/test/ServiceTestCase.html
-_TEST_WITHOUT_SIZE_ANNOTATIONS = [
-    'testAndroidTestCaseSetupProperly', 'testServiceTestCaseSetUpProperly']
-
 _EXTRA_DRIVER_TEST_LIST = (
     'org.chromium.test.driver.OnDeviceInstrumentationDriver.TestList')
 _EXTRA_DRIVER_TEST_LIST_FILE = (
@@ -257,8 +250,7 @@ def FilterTests(tests, filter_str=None, annotations=None,
       continue
 
     # Enforce that all tests declare their size.
-    if (not any(a in _VALID_ANNOTATIONS for a in t['annotations'])
-        and t['method'] not in _TEST_WITHOUT_SIZE_ANNOTATIONS):
+    if not any(a in _VALID_ANNOTATIONS for a in t['annotations']):
       raise MissingSizeAnnotationError(GetTestName(t))
 
     if (not annotation_filter(t['annotations'])