Fix flakiness in AccessibilityFeaturesApiTest.Set tests

chrome/browser/chromeos/extensions/accessibility_features_apitest.cc

 // Copyright 2014 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.
 
 #include <string>
 #include <vector>
 
 #include "base/json/json_writer.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/stringprintf.h"
@@ skipping 143 matching lines @@
 
     scoped_ptr<base::ListValue> disabled_list(new base::ListValue);
     for (size_t i = 0; i < disabled_features.size(); ++i)
       disabled_list->AppendString(disabled_features[i]);
     test_arg.Set(kDisabledFeaturesKey, disabled_list.release());
 
     return base::JSONWriter::Write(&test_arg, result);
   }
 };
 
-// Disabled now as this test is being flaky, see http://crbug.com/384266.
-INSTANTIATE_TEST_CASE_P(
-    DISABLED_AccessibilityFeatureaApiTestInstantiatePermission,
-    AccessibilityFeaturesApiTest,
-    testing::Bool());
+INSTANTIATE_TEST_CASE_P(AccessibilityFeatureaApiTestInstantiatePermission,
+                        AccessibilityFeaturesApiTest,
+                        testing::Bool());
 
 // Tests that an extension with read permission can read accessibility features
 // state, while an extension that doesn't have the permission cannot.
 IN_PROC_BROWSER_TEST_P(AccessibilityFeaturesApiTest, Get) {
+  // WARNING: Make sure that spoken feedback is not among enabled_features
+  // (see |Set| test for the reason).
   std::vector<std::string> enabled_features;
   enabled_features.push_back("largeCursor");
   enabled_features.push_back("stickyKeys");
   enabled_features.push_back("highContrast");
 
   std::vector<std::string> disabled_features;
   disabled_features.push_back("spokenFeedback");
   disabled_features.push_back("screenMagnifier");
   disabled_features.push_back("autoclick");
   disabled_features.push_back("virtualKeyboard");
 
   ASSERT_TRUE(
       InitPrefServiceForTest(GetPrefs(), enabled_features, disabled_features));
 
   std::string test_arg;
   ASSERT_TRUE(GenerateTestArg(
       "getterTest", enabled_features, disabled_features, &test_arg));
   EXPECT_TRUE(
-      RunPlatformAppTestWithArg(GetTestExtensionPath(), test_arg.c_str()));
+      RunPlatformAppTestWithArg(GetTestExtensionPath(), test_arg.c_str()))
+      << message_;
 }
 
 // Tests that an extension with modify permission can modify accessibility
 // features, while an extension that doesn't have the permission can't.
 IN_PROC_BROWSER_TEST_P(AccessibilityFeaturesApiTest, Set) {
+  // WARNING: Make sure that spoken feedback does not get enabled at this point
+  // (before the test app is loaded), as that may break the test:
+  // |RunPlatformAppTestWithArg| waits for the test extension to load by
+  // waiting for EXTENSION_LOADED notification to be observed. It also assumes
+  // that there is only one extension being loaded during this time (it finishes
+  // when the first notification is seen). Enabling spoken feedback here would
+  // break this assumption as it would induce loading of ChromeVox extension.
   std::vector<std::string> enabled_features;
-  enabled_features.push_back("spokenFeedback");
   enabled_features.push_back("stickyKeys");
   enabled_features.push_back("autoclick");
   enabled_features.push_back("virtualKeyboard");
 
   std::vector<std::string> disabled_features;
+  disabled_features.push_back("spokenFeedback");
   disabled_features.push_back("largeCursor");
   disabled_features.push_back("highContrast");
   disabled_features.push_back("screenMagnifier");
 
   ASSERT_TRUE(
       InitPrefServiceForTest(GetPrefs(), enabled_features, disabled_features));
 
   std::string test_arg;
   ASSERT_TRUE(GenerateTestArg(
       "setterTest", enabled_features, disabled_features, &test_arg));
 
   // The test extension attempts to flip all feature values.
   ASSERT_TRUE(
-      RunPlatformAppTestWithArg(GetTestExtensionPath(), test_arg.c_str()));
+      RunPlatformAppTestWithArg(GetTestExtensionPath(), test_arg.c_str()))
+      << message_;
 
   // The test tries to flip the feature states.
   if (ShouldModifyingFeatureSucceed()) {
     VerifyPrefServiceState(GetPrefs(), disabled_features, enabled_features);
   } else {
     VerifyPrefServiceState(GetPrefs(), enabled_features, disabled_features);
   }
 }
 
 // Tests that an extension with read permission is notified when accessibility
 // features change.
 IN_PROC_BROWSER_TEST_F(AccessibilityFeaturesApiTest, ObserveFeatures) {
+  // WARNING: Make sure that spoken feedback is not among enabled_features
+  // (see |Set| test for the reason).
   std::vector<std::string> enabled_features;
   enabled_features.push_back("largeCursor");
   enabled_features.push_back("stickyKeys");
   enabled_features.push_back("highContrast");
 
   std::vector<std::string> disabled_features;
   disabled_features.push_back("screenMagnifier");
 
   ASSERT_TRUE(
       InitPrefServiceForTest(GetPrefs(), enabled_features, disabled_features));
 
   std::string test_arg;
   ASSERT_TRUE(GenerateTestArg(
       "observerTest", enabled_features, disabled_features, &test_arg));
 
   // The test extension is supposed to report result twice when runnign this
   // test. First time when in initializes it's feature listeners, and second
   // time, when gets all expected events. This is done so the extension is
   // running when the accessibility features are flipped; oterwise, the
   // extension may not see events.
   ASSERT_TRUE(RunPlatformAppTestWithArg(kTestExtensionPathReadPermission,
-                                        test_arg.c_str()));
+                                        test_arg.c_str()))
+      << message_;

[Peter Lundblad, 2014/06/18 02:37:03]

nit: would this line fit on the previous one?

[tbarzic, 2014/06/18 03:07:36]

It probably would, but I find it more readable this way.

 
   // This should flip all features.
   ASSERT_TRUE(
       InitPrefServiceForTest(GetPrefs(), disabled_features, enabled_features));
 
   // Catch the second result notification sent by the test extension.
   ResultCatcher result_catcher;
   ASSERT_TRUE(result_catcher.GetNextResult()) << result_catcher.message();
 }
 
 }  // namespace
 
 }  // namespace extensions