Enable CommandLineFlags annonations on Rules.

Enable CommandLineFlags.add and CommandLineFlags.remove on Junit4 Rules. The annotations are read in the order: 1. Annotations on Rule Classes and Rule Class Ancestors 2. Annotations on Test Class Ancestors 2. Annotations on Test Class 3 Annotations on Test method Meaning that annotations on the test class or method can remove command line flags set by rules. The order in which annotations on different rules are applied is undefined, but annotations on each rule's ancestors are read before annotations on the rule itself Note that if the @rule annotation annotates a method (see http://junit.org/junit4/javadoc/4.12/org/junit/Rule.html) then this will only read CommandLine annotations on the formal return type of the method, and not the actual type of the returned rule. This CL also (as a test of this) adds command line flags to BottomSheetTestRule, and removes them from tests using this rule. BUG=734553 Change-Id: I1501cb6ca4ae3963775b299b1695304e471ac0ab Reviewed-on: https://chromium-review.googlesource.com/539563 Commit-Queue: Anthony Berent <aberent@chromium.org> Reviewed-by: John Budorick <jbudorick@chromium.org> Reviewed-by: Yoland Yan <yolandyan@chromium.org> Reviewed-by: Bernhard Bauer <bauerb@chromium.org> Cr-Commit-Position: refs/heads/master@{#481568}
marcosholgado · Jun 22, 2017 · 4b4ec5c · 4b4ec5c
1 parent d43235d
commit 4b4ec5c
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 43 deletions.
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
@@ -7,6 +7,7 @@
 import android.content.Context;
 
 import org.junit.Assert;
+import org.junit.Rule;
 
 import org.chromium.base.BaseChromiumApplication;
 import org.chromium.base.CommandLine;
@@ -19,6 +20,7 @@
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 import java.lang.reflect.AnnotatedElement;
+import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.HashSet;
@@ -32,7 +34,24 @@
  * so a derived class can add a command-line flag that a base class has removed (or vice versa).
  * Similarly, uses of these annotations on a test method will take precedence over uses on the
  * containing class.
+ * <p>
+ * These annonations may also be used on Junit4 Rule classes and on their base classes. Note,
+ * however that the annotation processor only looks at the declared type of the Rule, not its actual
+ * type, so in, for example:
  *
+ * <pre>
+ *     &#64Rule
+ *     TestRule mRule = new ChromeActivityTestRule();
+ * </pre>
+ *
+ * will only look for CommandLineFlags annotations on TestRule, not for CommandLineFlags annotations
+ * on ChromeActivityTestRule.
+ * <p>
+ * In addition a rule may not remove flags added by an independently invoked rule, although it may
+ * remove flags added by its base classes.
+ * <p>
+ * Uses of these annotations on the test class or methods take precedence over uses on Rule classes.
+ * <p>
  * Note that this class should never be instantiated.
  */
 public final class CommandLineFlags {
@@ -81,12 +100,42 @@ public static void setUp(Context targetContext, AnnotatedElement element) {
         }
     }
 
-    private static Set<String> getFlags(AnnotatedElement element) {
+    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);
+                }
+            }
+            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);
+                }
+            }
+        }
+
+        // Add the flags from the parent. Override any flags defined by the rules.
         AnnotatedElement parent = (element instanceof Method)
                 ? ((Method) element).getDeclaringClass()
-                : ((Class) element).getSuperclass();
-        Set<String> flags = (parent == null) ? new HashSet<String>() : getFlags(parent);
+                : ((Class<?>) element).getSuperclass();
+        if (parent != null) updateFlagsForElement(parent, flags);
 
+        // Flags on the element itself override all other flag sources.
         if (element.isAnnotationPresent(CommandLineFlags.Add.class)) {
             flags.addAll(
                     Arrays.asList(element.getAnnotation(CommandLineFlags.Add.class).value()));
@@ -104,8 +153,6 @@ private static Set<String> getFlags(AnnotatedElement element) {
             }
             flags.removeAll(flagsToRemove);
         }
-
-        return flags;
     }
 
     private CommandLineFlags() {
@@ -129,18 +176,16 @@ public void run(Context targetContext, Method testMethod) {
      * instructs to run the test with default command-line flags.
      *
      * Example:
+     *
      * @ParameterizedTest.Set(tests = {
-     *             @ParameterizedTest(parameters = {
-     *                         @Parameter(
-     *                                 tag = CommandLineFlags.Parameter.PARAMETER_TAG)}),
-     *             @ParameterizedTest(parameters = {
-     *                         @Parameter(
-     *                                 tag = CommandLineFlags.Parameter.PARAMETER_TAG,
-     *                                 arguments = {
-     *                                     @Parameter.Argument(
-     *                                         name = CommandLineFlags.Parameter.ADD_ARG,
-     *                                         stringArray = {'arg1', 'arg2'})
-     *             })})})
+     * @ParameterizedTest(parameters = {
+     * @Parameter( tag = CommandLineFlags.Parameter.PARAMETER_TAG)}),
+     * @ParameterizedTest(parameters = {
+     * @Parameter( tag = CommandLineFlags.Parameter.PARAMETER_TAG,
+     * arguments = {
+     * @Parameter.Argument( name = CommandLineFlags.Parameter.ADD_ARG,
+     * stringArray = {'arg1', 'arg2'})
+     * })})})
      *
      * Note that because the entire instrumentation test process needs to be restarted to apply
      * modified command-line arguments, this annotation is handled by test_runner.py, not by

diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/appmenu/ChromeHomeAppMenuTest.java
@@ -18,10 +18,8 @@
 import org.junit.runner.RunWith;
 
 import org.chromium.base.ThreadUtils;
-import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.Restriction;
 import org.chromium.base.test.util.UrlUtils;
-import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.test.BottomSheetTestRule;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.content.browser.test.util.Criteria;
@@ -31,9 +29,6 @@
  * Tests for the app menu when Chrome Home is enabled.
  */
 @RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Add({BottomSheetTestRule.ENABLE_CHROME_HOME,
-        ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
-        BottomSheetTestRule.DISABLE_NETWORK_PREDICTION_FLAG})
 @Restriction(RESTRICTION_TYPE_PHONE) // ChromeHome is only enabled on phones
 public class ChromeHomeAppMenuTest {
     private static final String TEST_URL = UrlUtils.encodeHtmlDataUri("<html>foo</html>");

diff --git a/...oid/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java b/...oid/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetTest.java
@@ -22,11 +22,9 @@
 import org.junit.runner.RunWith;
 
 import org.chromium.base.ThreadUtils;
-import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.Restriction;
 import org.chromium.base.test.util.RetryOnFailure;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.ntp.cards.ItemViewType;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.chrome.test.SuggestionsBottomSheetTestRule;
@@ -37,9 +35,6 @@
  * Instrumentation tests for {@link SuggestionsBottomSheetContent}.
  */
 @RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Add({SuggestionsBottomSheetTestRule.ENABLE_CHROME_HOME,
-        ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
-        SuggestionsBottomSheetTestRule.DISABLE_NETWORK_PREDICTION_FLAG})
 @Restriction(RESTRICTION_TYPE_PHONE) // ChromeHome is only enabled on phones
 public class SuggestionsBottomSheetTest {
     @Rule

diff --git a/...ests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetUiCaptureTest.java b/...ests/src/org/chromium/chrome/browser/suggestions/SuggestionsBottomSheetUiCaptureTest.java
@@ -19,12 +19,10 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.Feature;
 import org.chromium.base.test.util.Restriction;
 import org.chromium.base.test.util.ScreenShooter;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.widget.bottomsheet.BottomSheet;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.chrome.test.SuggestionsBottomSheetTestRule;
@@ -33,9 +31,6 @@
  * Tests for the appearance of Article Snippets.
  */
 @RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Add({SuggestionsBottomSheetTestRule.ENABLE_CHROME_HOME,
-        ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
-        SuggestionsBottomSheetTestRule.DISABLE_NETWORK_PREDICTION_FLAG})
 @Restriction(RESTRICTION_TYPE_PHONE) // ChromeHome is only enabled on phones
 public class SuggestionsBottomSheetUiCaptureTest {
     @Rule

diff --git a/.../src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentControllerTest.java b/.../src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentControllerTest.java
@@ -17,10 +17,8 @@
 import org.junit.runner.RunWith;
 
 import org.chromium.base.ThreadUtils;
-import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.Restriction;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.ChromeTabbedActivity;
 import org.chromium.chrome.browser.bookmarks.BookmarkSheetContent;
 import org.chromium.chrome.browser.bookmarks.BookmarkUtils;
@@ -36,9 +34,6 @@
 
 /** This class tests the functionality of the {@link BottomSheetContentController}. */
 @RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Add({"enable-features=ChromeHome",
-        ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
-        BottomSheetTestRule.DISABLE_NETWORK_PREDICTION_FLAG})
 @Restriction(RESTRICTION_TYPE_PHONE) // ChromeHome is only enabled on phones
 public class BottomSheetContentControllerTest {
     private BottomSheetTestRule.Observer mObserver;

diff --git a/...javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetObserverTest.java b/...javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetObserverTest.java
@@ -17,10 +17,8 @@
 import org.junit.runner.RunWith;
 
 import org.chromium.base.test.util.CallbackHelper;
-import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.Restriction;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.bookmarks.BookmarkSheetContent;
 import org.chromium.chrome.browser.download.DownloadSheetContent;
 import org.chromium.chrome.browser.history.HistorySheetContent;
@@ -33,9 +31,6 @@
 
 /** This class tests the functionality of the {@link BottomSheetObserver}. */
 @RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Add({BottomSheetTestRule.ENABLE_CHROME_HOME,
-        ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
-        BottomSheetTestRule.DISABLE_NETWORK_PREDICTION_FLAG})
 @Restriction(RESTRICTION_TYPE_PHONE) // ChromeHome is only enabled on phones
 public class BottomSheetObserverTest {
     @Rule

diff --git a/chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestRule.java b/chrome/test/android/javatests/src/org/chromium/chrome/test/BottomSheetTestRule.java
@@ -4,10 +4,15 @@
 
 package org.chromium.chrome.test;
 
+import static org.chromium.chrome.browser.ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE;
+import static org.chromium.chrome.test.BottomSheetTestRule.ENABLE_CHROME_HOME;
+import static org.chromium.chrome.test.ChromeActivityTestRule.DISABLE_NETWORK_PREDICTION_FLAG;
+
 import android.support.v7.widget.RecyclerView;
 
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.CallbackHelper;
+import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.preferences.ChromePreferenceManager;
@@ -20,6 +25,8 @@
 /**
  * Junit4 rule for tests testing the Chrome Home bottom sheet.
  */
+@CommandLineFlags.Add({ENABLE_CHROME_HOME, DISABLE_FIRST_RUN_EXPERIENCE,
+        DISABLE_NETWORK_PREDICTION_FLAG})
 public class BottomSheetTestRule extends ChromeTabbedActivityTestRule {
     /** An observer used to record events that occur with respect to the bottom sheet. */
     public static class Observer extends EmptyBottomSheetObserver {
@@ -158,7 +165,7 @@ public BottomSheetContentController getBottomSheetContentController() {
     /**
      * Set the bottom sheet's state on the UI thread.
      *
-     * @param state The state to set the sheet to.
+     * @param state   The state to set the sheet to.
      * @param animate If the sheet should animate to the provided state.
      */
     public void setSheetState(final int state, final boolean animate) {
@@ -190,7 +197,7 @@ public BottomSheetContent getBottomSheetContent() {
 
     /**
      * @param itemId The id of the MenuItem corresponding to the {@link BottomSheetContent} to
-     *            select.
+     *               select.
      */
     public void selectBottomSheetContent(final int itemId) {
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {

diff --git a/testing/android/docs/junit4.md b/testing/android/docs/junit4.md
@@ -145,6 +145,25 @@ public class MyRule implements TestRule {
 }
 ```
 
+## Command Line Flags
+
+In our Junit3 tests command line flags (set by the CommandLineFlag annotations) were inherited from the
+test base classes. As an example, ChromeActivityTestBase is annotated with:
+
+```java
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE, ...
+```
+
+and as a result any test in a class derived from ChromeActivityTestBase will disable the first run experience.
+
+The Junit4 tests classes are not however, derived from test base classes; instead their behavior is defined by
+test rules. To support this our Junit4 test runner will examine the command line flag annotations on all rules
+referenced with @Rule annotations in the test class. In addition, where one rule is derived from another, the
+command line flags propogate through the hierarchy of rules. See, for example, [BottomSheetTestRule][11]
+
+Note:- This has only recently been implemented, so is not yet used in all tests. See [this bug][12]
+
+The CommandLineFlags annonations are more fully documented in the [CommandLineFlags class][13]
 
 ## Common Errors
 
@@ -263,3 +282,6 @@ If you have any other questions, feel free to report in [this bug][7].
 [8]: http://junit.org/junit4/javadoc/4.12/org/junit/rules/RuleChain.html
 [9]: https://developer.android.com/reference/android/app/Instrumentation.html#runOnMainSync(java.lang.Runnable)
 [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