📰 Add tests for Tile and TileGroup

chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java

Index: chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java b/chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java
index a1344931264634fd8e6d2afe2839ae0d1a941f93..bdc4a89cc417aa0b9c2f2c674f0f0c2154cd6534 100644
--- a/chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java
@@ -12,6 +12,7 @@
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
 
@@ -39,20 +40,26 @@
     String[] value();
 
     /**
-     * Add this rule to tests to activate the {@link EnableFeatures} annotations and choose flags
-     * to enable, or get rid of exceptions when the production code tries to check for enabled
-     * features.
+     * Add this rule to tests get rid of exceptions when the production code tries to check for
+     * enabled features. It also activates the {@link EnableFeatures} annotation and allows to
+     * choose which features to enable. All are disabled by default.

[Bernhard Bauer, 2017/03/03 11:54:38]

Ooh, there is one thing I had been thinking of: would it be possible / worth it
to use a Map<String, Boolean> for feature state? The idea being that tests would
explicitly set any feature to enabled or disabled that is checked by the code
under test, and the code would assert if an unknown feature is checked (as
opposed to now, where all features get disabled as soon as you add the rule). We
could do that in a followup though.

[dgn, 2017/03/03 13:48:30]

Yes we could do that. But I think it becomes interesting only when we define
default states for features. Otherwise turning on only what you need is easy
enough for unit tests, considering that we mock most things. Which features do
you think would benefit most? I'll do it as follow up, yes.

[Bernhard Bauer, 2017/03/03 14:01:22]

Well, what I'm a bit worried about is if we make a change that makes our code
check a feature it didn't check before. We will just default to off, which may
not match the real-world behavior if the feature in Chrome defaults to on, so we
would avoid catching a hypothetical bug that only happens if the feature is on.
If we require the feature to be set, we would at least have to explicitly define
the feature state we're assuming.

[dgn, 2017/03/03 15:53:45]

Okay got it. In that case, I reverted the change that makes the rule have some
effect by default, since it would not be useful for that later state. I just
added the current empty annotation at the class level.

      */
-    public static class Processor extends AnnotationProcessor<EnableFeatures> {
+    class Processor extends AnnotationProcessor<EnableFeatures> {
         public Processor() {
-            super(EnableFeatures.class);
+            super(EnableFeatures.class, true);
         }
 
         @Override
         protected void before() throws Throwable {
-            Set<String> enabledFeatures = new HashSet<>();
-            String[] values = getAnnotation().value();
-            enabledFeatures.addAll(Arrays.asList(values));
+            Set<String> enabledFeatures;
+
+            if (getAnnotation() != null) {
+                String[] values = getAnnotation().value();
+                enabledFeatures = new HashSet<>(Arrays.asList(values));
+            } else {
+                enabledFeatures = Collections.emptySet();
+            }
+
             ChromeFeatureList.setTestEnabledFeatures(enabledFeatures);
         }