Create CommandLineTestRule

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

Index: base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java
diff --git a/base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java b/base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java
index 3e97532defd47333206199f04b187891c359919a..f2905cce4efda85d5e821cd40253398b67c585c1 100644
--- a/base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java
+++ b/base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java
@@ -5,8 +5,12 @@
 package org.chromium.base.test.util;
 
 import android.content.Context;
+import android.support.test.InstrumentationRegistry;
 
-import junit.framework.Assert;
+import org.junit.Assert;
+import org.junit.rules.TestRule;
+import org.junit.runner.Description;
+import org.junit.runners.model.Statement;
 
 import org.chromium.base.BaseChromiumApplication;
 import org.chromium.base.CommandLine;
@@ -60,36 +64,64 @@ public final class CommandLineFlags {
     }
 
     /**
-     * Sets up the CommandLine with the appropriate flags.
+     * Sets up the CommandLine flags using test method or class.

[jbudorick, 2017/01/11 20:21:33]

nit: "... using the given test method or class."

[the real yoland, 2017/03/16 22:52:59]

No longer relevant

      *
      * This will add the difference of the sets of flags specified by {@link CommandLineFlags.Add}
      * and {@link CommandLineFlags.Remove} to the {@link org.chromium.base.CommandLine}. Note that
      * trying to remove a flag set externally, i.e. by the command-line flags file, will not work.
      */
     public static void setUp(Context targetContext, AnnotatedElement element) {
+        Set<String> flags = getFlags(element);
+        setUp(targetContext, flags);

[jbudorick, 2017/01/11 20:21:33]

nit: just do

  setUp(targetContext, getFlags(element));

[the real yoland, 2017/03/16 22:52:59]

No longer relevant

+    }
+
+    /**
+     * Sets up the CommandLine flags using a set of flag strings.
+     *
+     * This will add the difference of the sets of flags specified by {@link CommandLineFlags.Add}

[jbudorick, 2017/01/11 20:21:33]

Update this sentence:

  "This will add {@code flags} to the {@link org.chromium.base.CommandLine}."

updateFlags, which this does not call, is responsible for getting the set of
flags specified by various @CommandLineFlags.Add and @CommandLineFlags.Remove
annotations.

[the real yoland, 2017/03/16 22:52:59]

Done

+     * and {@link CommandLineFlags.Remove} to the {@link org.chromium.base.CommandLine}. Note that
+     * trying to remove a flag set externally, i.e. by the command-line flags file, will not work.
+
+     */
+    public static void setUp(Context targetContext, Set<String> flags) {
         Assert.assertNotNull("Unable to get a non-null target context.", targetContext);
-        CommandLine.reset();
         BaseChromiumApplication.initCommandLine(targetContext);
-        Set<String> flags = getFlags(element);
         for (String flag : flags) {
             CommandLine.getInstance().appendSwitch(flag);
         }
     }
 
-    private static Set<String> getFlags(AnnotatedElement element) {
+    /**
+     * @return a set of strings to represent the commandline flags

[jbudorick, 2017/01/11 20:21:33]

This should say something like

  /** Determines the set of command-line flags that should be added for the
given element.
   *
   * This determines the difference of the sets of flags specified by {@link
CommandLineFlags.Add}
   * and {@link CommandLineFlags.Remove}.
   */

[the real yoland, 2017/03/16 22:52:59]

No longer relevant

+     */
+    @VisibleForTesting

[jbudorick, 2017/01/11 20:21:33]

This shouldn't be necessary.

[the real yoland, 2017/03/16 22:52:59]

No longer relevant

+    public static Set<String> getFlags(AnnotatedElement element) {
         AnnotatedElement parent = (element instanceof Method)
                 ? ((Method) element).getDeclaringClass()
-                : ((Class) element).getSuperclass();
+                : ((Class<?>) element).getSuperclass();
         Set<String> flags = (parent == null) ? new HashSet<String>() : getFlags(parent);
+        return updateFlags(flags, element.getAnnotation(CommandLineFlags.Add.class),
+                element.getAnnotation(CommandLineFlags.Remove.class));
+    }
 
-        if (element.isAnnotationPresent(CommandLineFlags.Add.class)) {
-            flags.addAll(
-                    Arrays.asList(element.getAnnotation(CommandLineFlags.Add.class).value()));
-        }
+    /**
+     * @return a set of strings to represent the commandline flags

[jbudorick, 2017/01/11 20:21:33]

Same as above.

[the real yoland, 2017/03/16 22:52:59]

No longer relevant

+     */
+    @VisibleForTesting

[jbudorick, 2017/01/11 20:21:33]

This shouldn't be necessary.

[the real yoland, 2017/03/16 22:52:59]

No longer relevant

+    public static Set<String> getFlags(Description desc) {

[jbudorick, 2017/01/11 20:21:33]

Do we need to provide both versions of getFlags?

[the real yoland, 2017/03/16 22:52:59]

No longer relevant

+        Set<String> flags = getFlags(desc.getTestClass());
+        return updateFlags(flags, desc.getAnnotation(CommandLineFlags.Add.class),

[jbudorick, 2017/01/11 20:21:32]

Why does this call updateFlags a second time?

[the real yoland, 2017/03/16 22:52:59]

No longer relevant

+                desc.getAnnotation(CommandLineFlags.Remove.class));
+    }
 
-        if (element.isAnnotationPresent(CommandLineFlags.Remove.class)) {
-            List<String> flagsToRemove =
-                    Arrays.asList(element.getAnnotation(CommandLineFlags.Remove.class).value());
+    private static Set<String> updateFlags(Set<String> flags, CommandLineFlags.Add addAnnotation,
+            CommandLineFlags.Remove removeAnnotation) {
+        Assert.assertNotNull("flags set can not be null", flags);
+        if (addAnnotation != null) {
+            flags.addAll(Arrays.asList(addAnnotation.value()));
+        }
+        if (removeAnnotation != null) {
+            List<String> flagsToRemove = Arrays.asList(removeAnnotation.value());
             for (String flagToRemove : flagsToRemove) {
                 // If your test fails here, you have tried to remove a command-line flag via
                 // CommandLineFlags.Remove that was loaded into CommandLine via something other
@@ -99,7 +131,6 @@ public final class CommandLineFlags {
             }
             flags.removeAll(flagsToRemove);
         }
-
         return flags;
     }
 
@@ -150,4 +181,48 @@ public final class CommandLineFlags {
             super(PARAMETER_TAG, parameterReader);
         }
     }
+
+    /**
+     * TestRule to used by JUnit4 style instrumentation test to set CommandLine flags.
+     *
+     * If you need to add CommandLine flag to your junit4 style instrumentation test,
+     * use CommandLineFlag.Add and CommandLineFlags.Remove annotation on your test method or test
+     * class. Then add CommandLineTestRule in your test. For example
+     *
+     * <code>
+     * @CommandLineFlags.Add(...)
+     * public class ChromeActivityTest {
+     *     @Rule CommandLineTestRule mCommandLineRule = new CommandLineTestRule();
+     *
+     *     @CommandLineFlags.Remove(...)
+     *     @Test
+     *     public void test() {...}
+     * </code>
+     *
+     * TODO(yolandyan): Add error message in BaseJUnit4ClassRunner for tests with CommandLineFlag
+     * annotations but did not have a CommandLineTestRule field
+     */
+    public static class CommandLineTestRule implements TestRule {

[jbudorick, 2017/01/11 20:21:32]

A client would use this as

  @Rule CommandLineFlags.CommandLineTestRule mCommandLineRule = new
CommandLineFlags.CommandLineTestRule()

Could we either:
 1) rename this s.t. CommandLineFlags.<new name> isn't as repetitive (e.g. name
it "Rule" s.t. a client would do CommandLineFlags.Rule), or
 2) move this to its own file & have it be standalone.

[the real yoland, 2017/03/16 22:52:59]

Moved CommandLineTestRule to its own file

+        private class CommandLineTestRuleStatement extends Statement {
+            private Statement mBase;
+            private Set<String> mFlags;
+
+            public CommandLineTestRuleStatement(Statement base, Set<String> flags) {
+                mBase = base;
+                mFlags = flags;
+            }
+
+            @Override
+            public void evaluate() throws Throwable {
+                Context targetContext = InstrumentationRegistry.getContext();
+                CommandLineFlags.setUp(targetContext, mFlags);
+                mBase.evaluate();
+            }
+        }
+
+        @Override
+        public Statement apply(final Statement base, Description desc) {
+            return new CommandLineTestRuleStatement(base, getFlags(desc));
+        }
+    }
 }