Simplify CommandLineFlags, don't allow .Remove on classes

This is part 1 of 3(?). The end goal is make CommandLineFlags work for
batched tests.

Part 1 (this CL) gets rid of ordering concerns between
class/method/field level flags by only allowing the test methods
themselves to remove flags. We only had one existing case of wanting to
do this, and it was actually a no-op.

Part 2 will split out performing class-level CommandLineFlags parsing,
and method-level CommandLine parsing.

Part 3 will remove the reset() when applying CommandLineFlags
modifications, and have CommandLineFlags undo the changes it makes for
the test/class when the test/class finishes running.

Bug: 989569
Change-Id: Icc2d3c9afca13febf55cc0e5dbaeff882ca2e9b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2216288Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772722}

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

@@ -72,11 +72,14 @@ public final class CommandLineFlags {
     /**
      * Removes command-line flags from the {@link org.chromium.base.CommandLine} from this test.
      *
-     * Note that this can only remove flags added via {@link Add} above.
+     * Note that this can only be applied to test methods. This restriction is due to complexities
+     * in resolving the order that annotations are applied, and given how rare it is to need to
+     * remove command line flags, this annotation must be applied directly to each test method
+     * wishing to remove a flag.
      */
     @Inherited
     @Retention(RetentionPolicy.RUNTIME)
-    @Target({ElementType.METHOD, ElementType.TYPE})
+    @Target({ElementType.METHOD})
     public @interface Remove {
         String[] value();
     }
@@ -93,7 +96,9 @@ public final class CommandLineFlags {
         CommandLineInitUtil.initCommandLine(getTestCmdLineFile());
         Set<String> enableFeatures = new HashSet<String>(getFeatureValues(ENABLE_FEATURES));
         Set<String> disableFeatures = new HashSet<String>(getFeatureValues(DISABLE_FEATURES));
-        Set<String> flags = getFlags(element);
+
+        Set<String> flags = new HashSet<>();
+        updateFlagsForElement(element, flags);
         for (String flag : flags) {
             String[] parsedFlags = flag.split("=", 2);
             if (parsedFlags.length == 1) {
@@ -120,32 +125,20 @@ public final class CommandLineFlags {
         }
     }
 
-    private static Set<String> getFlags(AnnotatedElement type) {
-        Set<String> rule_flags = new HashSet<>();
-        updateFlagsForElement(type, rule_flags);
-        return rule_flags;
-    }
-
     private static void updateFlagsForElement(AnnotatedElement element, Set<String> flags) {
         if (element instanceof Class<?>) {
             // Get flags from rules within the class.
             for (Field field : ((Class<?>) element).getFields()) {
                 if (field.isAnnotationPresent(Rule.class)) {
                     // The order in which fields are returned is undefined, so, for consistency,
-                    // a rule must not remove a flag added by a different rule. Ensure this by
-                    // initially getting the flags into a new set.
-                    Set<String> rule_flags = getFlags(field.getType());
-                    flags.addAll(rule_flags);
+                    // a rule must only ever add flags.
+                    updateFlagsForElement(field.getType(), flags);
                 }
             }
             for (Method method : ((Class<?>) element).getMethods()) {
-                if (method.isAnnotationPresent(Rule.class)) {
-                    // The order in which methods are returned is undefined, so, for consistency,
-                    // a rule must not remove a flag added by a different rule. Ensure this by
-                    // initially getting the flags into a new set.
-                    Set<String> rule_flags = getFlags(method.getReturnType());
-                    flags.addAll(rule_flags);
-                }
+                Assert.assertFalse("@Rule annotations on methods are unsupported. Cause: "
+                                + method.toGenericString(),
+                        method.isAnnotationPresent(Rule.class));
             }
         }
 

base/test/android/junit/src/org/chromium/base/test/TestListInstrumentationRunListenerTest.java

@@ -13,6 +13,7 @@ import org.robolectric.annotation.Config;
 
 import org.chromium.base.test.params.ParameterizedCommandLineFlags;
 import org.chromium.base.test.params.ParameterizedCommandLineFlags.Switches;
+import org.chromium.base.test.util.Batch;
 import org.chromium.base.test.util.CommandLineFlags;
 
 import java.util.Arrays;
@@ -31,9 +32,8 @@ public class TestListInstrumentationRunListenerTest {
         public void testB() {}
     }
 
-    @CommandLineFlags.Remove("hello")
-    private static class ChildClass extends ParentClass {
-    }
+    @Batch("foo")
+    private static class ChildClass extends ParentClass {}
 
     private static class Groups {
         // clang-format off
@@ -141,8 +141,8 @@ public class TestListInstrumentationRunListenerTest {
             " 'CommandLineFlags$Add': {",
             "  'value': ['hello']",
             " },",
-            " 'CommandLineFlags$Remove': {",
-            "  'value': ['hello']",
+            " 'Batch': {",
+            "  'value': 'foo'",
             " }",
             "}"
         );
@@ -201,4 +201,3 @@ public class TestListInstrumentationRunListenerTest {
         Assert.assertEquals(expectedJsonString, json.toString());
     }
 }
-

chrome/android/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java

@@ -16,12 +16,13 @@ import android.support.test.filters.SmallTest;
 
 import org.junit.After;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
+import org.chromium.base.CommandLine;
 import org.chromium.base.test.util.CallbackHelper;
-import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.Feature;
 import org.chromium.base.test.util.FlakyTest;
@@ -45,7 +46,6 @@ import java.util.concurrent.TimeoutException;
  * Tests for the first run experience.
  */
 @RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Remove(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
 public class FirstRunTest {
     @Rule
     public SyncTestRule mSyncTestRule = new SyncTestRule() {
@@ -118,6 +118,12 @@ public class FirstRunTest {
     private final TestObserver mTestObserver = new TestObserver();
     private FirstRunActivity mActivity;
 
+    @Before
+    public void setUp() {
+        Assert.assertFalse(
+                CommandLine.getInstance().hasSwitch(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE));
+    }
+
     @After
     public void tearDown() {
         if (mActivity != null) mActivity.finish();