Introduce @Batch annotation for batching Instrumentation tests

Introduces the @Batch annotation, which causes the test runner to run
tests without a separate instrumentation run for each test (in other
words, no Activity/process restart).

Lowers the per-test overhead from around 10 seconds on my device to <5ms
for all tests with this new annotation. (Bots from ~2s to <5ms)

The BATCH.UNIT_TESTS batch is for unit tests that don't require any
globally shared setup/teardown and don't change global state.

The Batch.PER_CLASS batch is for integration tests that don't require
a restart between each test, but do require a restart before/after the
suite runs. It is equivalent to @Batch('unique_name').

In the future, batches may be added at other layers for suites that
agree on known starting conditions, and reset to those starting
conditions.

This change also removes some dead handling of junit3 tests so that we
can re-interpret an array of tests as a collection of tests to run in a
single instrumentation call.

Some followup work will be required to fix reported duration, as each
test individually is reported as taking the duration the batch of tests
took.

Bug: 989569
Change-Id: I2fee81c33363789ca63174432a3b90b4fb472097
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2199706
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771630}

base/BUILD.gn

@@ -3594,6 +3594,7 @@ if (is_android) {
       "test/android/javatests/src/org/chromium/base/test/util/AdvancedMockContext.java",
       "test/android/javatests/src/org/chromium/base/test/util/AnnotationProcessingUtils.java",
       "test/android/javatests/src/org/chromium/base/test/util/AnnotationRule.java",
+      "test/android/javatests/src/org/chromium/base/test/util/Batch.java",
       "test/android/javatests/src/org/chromium/base/test/util/CallbackHelper.java",
       "test/android/javatests/src/org/chromium/base/test/util/CloseableOnMainThread.java",
       "test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java",

base/test/android/javatests/src/org/chromium/base/test/util/Batch.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base.test.util;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Annotation to indicate that this collection of tests is safe to run in batches, where the
+ * Instrumentation Runner (and hence the process) does not need to be restarted between these tests.
+ *
+ * The value passed to this annotation determines which test suites may be run together in the same
+ * batch - batches that share a batch name will run in the same Instrumentation invocation. The
+ * default value (empty) means the suite runs as its own batch, and the process is restarted before
+ * and after the suite.
+ *
+ * This makes the tests run significantly faster, but you must be careful not to persist changes to
+ * global state that could cause other tests in the batch to fail.
+ *
+ * @Before/@After will run as expected before/after each test method.
+ * @BeforeClass/@AfterClass may be used for one-time initialization across all tests within a single
+ * suite. Tests wishing to share one-time initialization across suites in the same batch will need
+ * to explicitly coordinate.
+ */
+@Target(ElementType.TYPE)
+@Retention(RetentionPolicy.RUNTIME)
+public @interface Batch {
+    public String value();
+
+    /**
+     * Batch name for test suites that are not safe to run batched across multiple suites. The
+     * process will not be restarted before each test within this suite, but will be restarted
+     * before and after this suite runs.
+     */
+    public static final String PER_CLASS = "";
+
+    /**
+     * Globally shared name for unit tests that can all be batched together.
+     *
+     * Unit tests must be careful not to persist any changes to global state, or flakes are likely
+     * to occur.
+     *
+     * An exception to this is loading Chrome's native library (eg. using NativeLibraryTestRule).
+     * Your unit tests must assume that the native library may have already been loaded by another
+     * test.
+     */
+    public static final String UNIT_TESTS = "UnitTests";
+}

build/android/pylib/instrumentation/instrumentation_test_instance.py

@@ -940,7 +940,8 @@ class InstrumentationTestInstance(test_instance.TestInstance):
             'class': c['class'],
             'method': m['method'],
             'annotations': a,
-            'is_junit4': c['superclass'] == 'java.lang.Object'
+            # TODO(https://crbug.com/1084729): Remove is_junit4.
+            'is_junit4': True
         })
     return inflated_tests
 

build/android/pylib/instrumentation/instrumentation_test_instance_test.py

@@ -497,15 +497,17 @@ class InstrumentationTestInstanceTest(unittest.TestCase):
     ]
 
     expected_tests = [
-      {
-        'annotations': {
-          'Feature': {'value': ['Foo']},
-          'MediumTest': None,
+        {
+            'annotations': {
+                'Feature': {
+                    'value': ['Foo']
+                },
+                'MediumTest': None,
+            },
+            'class': 'org.chromium.test.SampleTest',
+            'is_junit4': True,
+            'method': 'testMethod2',
         },
-        'class': 'org.chromium.test.SampleTest',
-        'is_junit4': False,
-        'method': 'testMethod2',
-      },
     ]
 
     o._excluded_annotations = [('SmallTest', None)]
@@ -556,16 +558,18 @@ class InstrumentationTestInstanceTest(unittest.TestCase):
     ]
 
     expected_tests = [
-      {
-        'annotations': {
-          'Feature': {'value': ['Foo']},
-          'SmallTest': None,
-          'TestValue': '1',
+        {
+            'annotations': {
+                'Feature': {
+                    'value': ['Foo']
+                },
+                'SmallTest': None,
+                'TestValue': '1',
+            },
+            'class': 'org.chromium.test.SampleTest',
+            'is_junit4': True,
+            'method': 'testMethod1',
         },
-        'class': 'org.chromium.test.SampleTest',
-        'is_junit4': False,
-        'method': 'testMethod1',
-      },
     ]
 
     o._annotations = [('TestValue', '1')]
@@ -724,24 +728,28 @@ class InstrumentationTestInstanceTest(unittest.TestCase):
     ]
 
     expected_tests = [
-      {
-        'annotations': {
-          'Feature': {'value': ['Baz']},
-          'MediumTest': None,
+        {
+            'annotations': {
+                'Feature': {
+                    'value': ['Baz']
+                },
+                'MediumTest': None,
+            },
+            'class': 'org.chromium.test.SampleTest',
+            'is_junit4': True,
+            'method': 'testMethod2',
         },
-        'class': 'org.chromium.test.SampleTest',
-        'is_junit4': False,
-        'method': 'testMethod2',
-      },
-      {
-        'annotations': {
-          'Feature': {'value': ['Bar']},
-          'SmallTest': None,
+        {
+            'annotations': {
+                'Feature': {
+                    'value': ['Bar']
+                },
+                'SmallTest': None,
+            },
+            'class': 'org.chromium.test.SampleTest2',
+            'is_junit4': True,
+            'method': 'testMethod1',
         },
-        'class': 'org.chromium.test.SampleTest2',
-        'is_junit4': False,
-        'method': 'testMethod1',
-      },
     ]
 
     o._annotations = [('Feature', 'Bar'), ('Feature', 'Baz')]

build/android/pylib/local/device/local_device_instrumentation_test_run.py

@@ -102,6 +102,8 @@ RENDER_TEST_MODEL_SDK_CONFIGS = {
     'Nexus 5X': [23],
 }
 
+_TEST_BATCH_MAX_GROUP_SIZE = 256
+
 
 @contextlib.contextmanager
 def _LogTestEndpoints(device, test_name):
@@ -461,6 +463,31 @@ class LocalDeviceInstrumentationTestRun(
         self._test_instance.total_external_shards)
     return tests
 
+  #override
+  def _GroupTests(self, tests):
+    batched_tests = dict()
+    other_tests = []
+    for test in tests:
+      if 'Batch' in test['annotations']:
+        batch_name = test['annotations']['Batch']['value']
+        if not batch_name:
+          batch_name = test['class']
+        if not batch_name in batched_tests:
+          batched_tests[batch_name] = []
+        batched_tests[batch_name].append(test)
+      else:
+        other_tests.append(test)
+
+    all_tests = []
+    for _, tests in batched_tests.items():
+      tests.sort()  # Ensure a consistent ordering across external shards.
+      all_tests.extend([
+          tests[i:i + _TEST_BATCH_MAX_GROUP_SIZE]
+          for i in range(0, len(tests), _TEST_BATCH_MAX_GROUP_SIZE)
+      ])
+    all_tests.extend(other_tests)
+    return all_tests
+
   #override
   def _GetUniqueTestName(self, test):
     return instrumentation_test_instance.GetUniqueTestName(test)
@@ -509,12 +536,9 @@ class LocalDeviceInstrumentationTestRun(
           device.adb, suffix='.json', dir=device.GetExternalStoragePath())
       extras[EXTRA_TRACE_FILE] = trace_device_file.name
 
+    target = '%s/%s' % (self._test_instance.test_package,
+                        self._test_instance.junit4_runner_class)
     if isinstance(test, list):
-      if not self._test_instance.driver_apk:
-        raise Exception('driver_apk does not exist. '
-                        'Please build it and try again.')
-      if any(t.get('is_junit4') for t in test):
-        raise Exception('driver apk does not support JUnit4 tests')
 
       def name_and_timeout(t):
         n = instrumentation_test_instance.GetTestName(t)
@@ -523,26 +547,15 @@ class LocalDeviceInstrumentationTestRun(
 
       test_names, timeouts = zip(*(name_and_timeout(t) for t in test))
 
-      test_name = ','.join(test_names)
+      test_name = instrumentation_test_instance.GetTestName(test[0]) + '_batch'
+      extras['class'] = ','.join(test_names)
       test_display_name = test_name
-      target = '%s/%s' % (
-          self._test_instance.driver_package,
-          self._test_instance.driver_name)
-      extras.update(
-          self._test_instance.GetDriverEnvironmentVars(
-              test_list=test_names))
       timeout = sum(timeouts)
     else:
+      assert test['is_junit4']
       test_name = instrumentation_test_instance.GetTestName(test)
       test_display_name = self._GetUniqueTestName(test)
-      if test['is_junit4']:
-        target = '%s/%s' % (
-            self._test_instance.test_package,
-            self._test_instance.junit4_runner_class)
-      else:
-        target = '%s/%s' % (
-            self._test_instance.test_package,
-            self._test_instance.junit3_runner_class)
+
       extras['class'] = test_name
       if 'flags' in test and test['flags']:
         flags_to_add.extend(test['flags'])

build/android/pylib/local/device/local_device_test_run.py

@@ -137,6 +137,7 @@ class LocalDeviceTestRun(test_run.TestRun):
       with signal_handler.AddSignalHandler(signal.SIGTERM, stop_tests):
         tries = 0
         while tries < self._env.max_tries and tests:
+          grouped_tests = self._GroupTests(tests)
           logging.info('STARTING TRY #%d/%d', tries + 1, self._env.max_tries)
           if tries > 0 and self._env.recover_devices:
             if any(d.build_version_sdk == version_codes.LOLLIPOP_MR1
@@ -171,12 +172,14 @@ class LocalDeviceTestRun(test_run.TestRun):
 
           try:
             if self._ShouldShard():
-              tc = test_collection.TestCollection(self._CreateShards(tests))
+              tc = test_collection.TestCollection(
+                  self._CreateShards(grouped_tests))
               self._env.parallel_devices.pMap(
                   run_tests_on_device, tc, try_results).pGet(None)
             else:
-              self._env.parallel_devices.pMap(
-                  run_tests_on_device, tests, try_results).pGet(None)
+              self._env.parallel_devices.pMap(run_tests_on_device,
+                                              grouped_tests,
+                                              try_results).pGet(None)
           except TestsTerminated:
             for unknown_result in try_results.GetUnknown():
               try_results.AddResult(
@@ -236,9 +239,16 @@ class LocalDeviceTestRun(test_run.TestRun):
     if total_shards < 0 or shard_index < 0 or total_shards <= shard_index:
       raise InvalidShardingSettings(shard_index, total_shards)
 
-    return [
-        t for t in tests
-        if hash(self._GetUniqueTestName(t)) % total_shards == shard_index]
+    sharded_tests = []
+    for t in self._GroupTests(tests):
+      if (hash(self._GetUniqueTestName(t[0] if isinstance(t, list) else t)) %
+          total_shards == shard_index):
+        if isinstance(t, list):
+          sharded_tests.extend(t)
+        else:
+          sharded_tests.append(t)
+
+    return sharded_tests
 
   def GetTool(self, device):
     if str(device) not in self._tools:
@@ -260,6 +270,10 @@ class LocalDeviceTestRun(test_run.TestRun):
   def _GetTests(self):
     raise NotImplementedError
 
+  def _GroupTests(self, tests):
+    # pylint: disable=no-self-use
+    return tests
+
   def _RunTest(self, device, test):
     raise NotImplementedError
 

components/external_intents/android/javatests/src/org/chromium/components/external_intents/ExternalNavigationHandlerTest.java

@@ -34,6 +34,7 @@ import org.chromium.base.ContextUtils;
 import org.chromium.base.IntentUtils;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.test.BaseJUnit4ClassRunner;
+import org.chromium.base.test.util.Batch;
 import org.chromium.base.test.util.DisableIf;
 import org.chromium.components.external_intents.ExternalNavigationDelegate.StartActivityIfNeededResult;
 import org.chromium.components.external_intents.ExternalNavigationHandler.OverrideUrlLoadingResult;
@@ -56,6 +57,7 @@ import java.util.regex.Pattern;
 // clang-format off
 @DisableIf.Build(message = "Flaky on K - see https://crbug.com/851444",
         sdk_is_less_than = Build.VERSION_CODES.LOLLIPOP)
+@Batch(Batch.UNIT_TESTS)
 public class ExternalNavigationHandlerTest {
     // clang-format on
     @Rule

testing/android/docs/instrumentation.md

@@ -72,6 +72,18 @@ Instrumentation tests in Chromium use a wide variety of annotations to control
 and manipulate test execution. Some of these are implemented in Chromium, while
 others are pulled in from outside. They include:
 
+#### Test Batching
+
+The [`@Batch("group_name")`](https://chromium.googlesource.com/chromium/src/+/master/base/test/android/javatests/src/org/chromium/base/test/util/UnitTest.java)
+annotation is used to run all tests with the same batch group name in the same
+instrumentation invocation. In other words, the browser process is not
+restarted between these tests, and so any changes to global state, like
+launching an Activity, will persist between tests within a batch group. The
+benefit of this is that these tests run significantly faster - the per-test cost
+of restarting the process can be as high as 10 seconds (usually around 2
+seconds), and that doesn't count the cost of starting an Activity like
+ChromeTabbedActivity.
+
 #### Size annotations
 
 Size annotations are used primarily by the test runner to determine the length
@@ -207,7 +219,7 @@ up to date, and whether the build was an official one.
 @Restriction(
     // Possible values include:
     //
-    // base: 
+    // base:
     //  - Restriction.RESTRICTION_TYPE_LOW_END_DEVICE
     //    Restricts the test to low-end devices as determined by SysUtils.isLowEndDevice().
     //

testing/android/docs/junit4.md

@@ -187,19 +187,19 @@ The CommandLineFlags annonations are more fully documented in the [CommandLineFl
     -   When using @UiThreadTest, **it would cause `setUp` and `tearDown` to
         run in Ui Thread** as well. Avoid that by calling [`runOnUiThread`][9]
         or [`runOnMainSync`][10] with a Runnable.
-    
+
     ```java
     // Wrong test
     public class Test {
         @Rule
         public ActivityTestRule<MyActivity> mRule = new ActivityTestRule<>(
-                MyActivity.class>
+                MyActivity.class);
 
         @Before
         public void setUp() {
             // Cause failure because this also runs on Ui Thread, while it
             // is intended for Instrumentation worker thread
-            mRule.launchActivity() 
+            mRule.launchActivity();
         }
 
         @UiThreadTest
@@ -216,11 +216,11 @@ The CommandLineFlags annonations are more fully documented in the [CommandLineFl
     public class Test {
         @Rule
         public ActivityTestRule<MyActivity> mRule = new ActivityTestRule<>(
-                MyActivity.class>
+                MyActivity.class);
 
         @Before
         public void setUp() {
-            mRule.launchActivity()
+            mRule.launchActivity();
         }
 
         public void test() {
@@ -241,7 +241,7 @@ The CommandLineFlags annonations are more fully documented in the [CommandLineFl
 
 1.  Errorprone expects all public methods starting with `test...` to be
     annotated with `@Test`. Failure to meet that expectation will cause
-    errorprone to fail with something like this: 
+    errorprone to fail with something like this:
 
         [JUnit4TestNotRun] Test method will not be run; please add @Test annotation
 
@@ -250,8 +250,8 @@ The CommandLineFlags annonations are more fully documented in the [CommandLineFl
 
 ## Common questions
 
--   Q: Are `@Test` and `@LargeTest/@MediumTest/@SmallTest` annotation both
-    necessary?
+-   Q: Are `@Test` and `@LargeTest/@MediumTest/@SmallTest` annotation
+    both necessary?
     -   A: Yes, both are required for now. We plan to refactor this in the
         future.
 -   Q: Isn't the inheritance of the Test classes just migrated to inheritance
@@ -284,4 +284,4 @@ If you have any other questions, feel free to report in [this bug][7].
 [10]: https://developer.android.com/reference/android/support/test/rule/UiThreadTestRule.html#runOnUiThread(java.lang.Runnable)
 [11]: /chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestRule.java
 [12]: https://bugs.chromium.org/p/chromium/issues/detail?id=734553
-[13]: /base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java
\ No newline at end of file
+[13]: /base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java