[Android] Add support for command-line flags via annotation.

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

Index: base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java
diff --git a/base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java b/base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java
new file mode 100644
index 0000000000000000000000000000000000000000..56d718dcd436383c07b5fc4e9ee56010c90038a8
--- /dev/null
+++ b/base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java
@@ -0,0 +1,94 @@
+// Copyright 2015 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;
+
+import android.app.Activity;
+import android.test.ActivityInstrumentationTestCase2;
+
+import org.chromium.base.BaseChromiumApplication;
+import org.chromium.base.CommandLine;
+import org.chromium.base.test.util.CommandLineFlags;
+
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Set;
+
+/**
+ * Base class for all Activity-based Instrumentation tests.
+ *
+ * @param <T> The Activity type.
+ */
+public class BaseActivityInstrumentationTestCase<T extends Activity>
+        extends ActivityInstrumentationTestCase2<T> {
+    private static final String TAG = "BaseActivityInstrumentationTestCase";

[nyquist, 2015/01/28 23:10:36]

Nit: I don't think this field is used.

[jbudorick, 2015/01/29 04:46:45]

Removed.

+
+    /**
+     * Creates a instance for running tests against an Activity of the given class.
+     *
+     * @param activityClass The type of activity that will be tested.
+     */
+    public BaseActivityInstrumentationTestCase(Class<T> activityClass) {
+        super(activityClass);
+    }
+
+    /**
+     * Sets up the CommandLine with the appropriate flags.
+     *
+     * This will add the difference of the sets of flags specified by @CommandLineFlags.Add and

[nyquist, 2015/01/28 23:10:36]

Nit: How about using {@link CommandLineFlags.Add} and similarly for Remove?

[jbudorick, 2015/01/29 04:46:45]

Done.

+     * @CommandLineFlags.Remove to the CommandLine. Note that trying to remove a flag set

[Ted C, 2015/01/29 00:39:51]

This caveat scares me.  If a test is explicitly trying to remove a flag and it
exists in the command line flag file, it will break in an odd and hard to debug
way.

Maybe when removing flags, we should check whether it exists in the CommandLine
already and fail the test if that ever happens.

[jbudorick, 2015/01/29 04:46:45]

Understandably so. Neither base::CommandLine nor org.chromium.base.CommandLine
support removal at the moment, though.
That sounds reasonable.

[jbudorick, 2015/01/29 04:46:45]

added {@link org.chromium.base.CommandLine} here

+     * externally, i.e. by the command-line flags file, will not work.
+     */
+    @Override
+    protected void setUp() throws Exception {
+        super.setUp();
+
+        CommandLine.reset();
+        BaseChromiumApplication.initCommandLine(getInstrumentation().getTargetContext());

[Ted C, 2015/01/29 00:39:51]

You might want to do what is in CommandLineTestUtils which works around where
the target context is null on older versions of android.

        try {
            Assert.assertTrue(CriteriaHelper.pollForCriteria(new Criteria() {
                @Override
                public boolean isSatisfied() {
                    return instrumentation.getTargetContext() != null
                            &&
instrumentation.getTargetContext().getApplicationContext() != null;
                }
            }));
        } catch (InterruptedException e) {
            Log.i("CommandLineTestUtils", "failed to get a non-null target
application context");
        }

[jbudorick, 2015/01/29 04:46:45]

Criteria & CriteriaHelper are in content/. I've replicated the functionality
here for now, though doing so is a bit ugly. Moving them into base/ would
theoretically be possible -- neither depends on anything outside of base/ -- but
given how heavily they're currently used, doing so doesn't seem practical.

+
+        Class<?> testClass = getClass();
+        Method testMethod = testClass.getMethod(getName());
+
+        Set<String> flags = getClassFlags(testClass);
+
+        if (testMethod.isAnnotationPresent(CommandLineFlags.Add.class)) {
+            flags.addAll(
+                    Arrays.asList(testMethod.getAnnotation(CommandLineFlags.Add.class).value()));
+        }
+
+        if (testMethod.isAnnotationPresent(CommandLineFlags.Remove.class)) {
+            flags.removeAll(
+                    Arrays.asList(testMethod.getAnnotation(CommandLineFlags.Remove.class).value()));
+        }
+
+        Iterator<String> flagIter = flags.iterator();
+        while (flagIter.hasNext()) {

[nyquist, 2015/01/28 23:10:36]

Nit: I often find it easier to read:
for (String flag : flags) {
  ...

This would also remove the necessity of importing java.util.Iterator.

[jbudorick, 2015/01/29 04:46:45]

Done.

+            CommandLine.getInstance().appendSwitch(flagIter.next());
+        }
+    }
+
+    private Set<String> getClassFlags(Class<?> testClass) {

[nyquist, 2015/01/28 23:10:36]

Nit: This method should be static to signal that it does not access any instance
members.

[jbudorick, 2015/01/29 04:46:45]

Done.

+        Class<?> superclass = testClass.getSuperclass();
+        Set<String> flags = null;

[nyquist, 2015/01/28 23:10:36]

Nit: You are always initializing this field below, so you don't need to
initialize it to null first.

[jbudorick, 2015/01/29 04:46:45]

Done.

+        if (superclass != null) {

[nyquist, 2015/01/28 23:10:36]

Optional nit: My personal preference is to not flip null-checks like this when
you have both if and else. I find it easier to read if a then do X else do Y.
I'm fine either way though.

You could also just do:
Set<String> flags = superclass == null ? new HashSet<String>() :
getClassFlags(superclass);

[jbudorick, 2015/01/29 04:46:45]

ternary conditional it is

+            flags = getClassFlags(superclass);
+        } else {
+            flags = new HashSet<String>();
+        }
+
+        if (testClass.isAnnotationPresent(CommandLineFlags.Add.class)) {

[Ted C, 2015/01/29 00:39:51]

Can this and the if below be abstracted out to a helper function with the same
logic above?

I think Method and Class both extend from AnnotatedElement, so it might make it
shareable.

[jbudorick, 2015/01/29 04:46:45]

Done.

+            flags.addAll(
+                    Arrays.asList(testClass.getAnnotation(CommandLineFlags.Add.class).value()));
+        }
+
+        if (testClass.isAnnotationPresent(CommandLineFlags.Remove.class)) {
+            flags.removeAll(
+                    Arrays.asList(testClass.getAnnotation(CommandLineFlags.Remove.class).value()));
+        }
+
+        return flags;
+    }
+}