Reland "[Chromecast] Use base::FeatureList to control features."

chromecast/browser/test/cast_features_browsertest.cc

Index: chromecast/browser/test/cast_features_browsertest.cc
diff --git a/chromecast/browser/test/cast_features_browsertest.cc b/chromecast/browser/test/cast_features_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3c9cbc45d2e5213b7d8b8ccd1a96685a6f33ad63
--- /dev/null
+++ b/chromecast/browser/test/cast_features_browsertest.cc
@@ -0,0 +1,295 @@
+// Copyright 2017 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 "chromecast/base/cast_features.h"
+
+#include <cstdint>
+#include <unordered_set>
+
+#include "base/feature_list.h"
+#include "base/macros.h"
+#include "base/metrics/field_trial_params.h"
+#include "chromecast/base/pref_names.h"
+#include "chromecast/browser/cast_browser_process.h"
+#include "chromecast/browser/test/cast_browser_test.h"
+#include "components/prefs/pref_service.h"
+#include "components/prefs/scoped_user_pref_update.h"
+
+// PLEASE READ:
+// 1) These tests are run in pairs to simulate a restart of cast_shell. Each
+//    test (ex. FooTest) is accompanied by a test which is guaranteed to run
+//    before it (ex. PRE_FooTest). PRE_ is a special prefix known to gtest
+//    which enforces that a given prefixed test runs *before* the unprefixed
+//    test of the same name. If the prefixed test fails, the unprefixed test
+//    will not run. The tests use this behavior to test persisted state between
+//    subsequent cast_shell runs. HOWEVER, tests which do not share a common
+//    name have NO guarantee of ordering (ex. BarTest may run before, after, or
+//    during PRE_FooTest). Therefore, tests with different names should be
+//    entirely independent.
+//
+// 2) Because of the last note in (1), when testing persistent features, do not
+//    re-use the same feature to test in more than one pair of tests. Because
+//    the order of execution is not guaranteed between test pairs, this may
+//    cause the tests to flake, depending on how gtest decides to schedule them.
+//    For example, FooTest and BarTest should not both try to read and write
+//    kEnableFoo in the PrefStore. This makes the tests flaky and co-dependent.
+
+namespace chromecast {
+namespace shell {
+namespace {
+
+// For use in TestFeaturesActivateOnBoot only.
+const base::Feature kEnableFoo("enable_foo", base::FEATURE_DISABLED_BY_DEFAULT);
+const base::Feature kEnableBar("enable_bar", base::FEATURE_ENABLED_BY_DEFAULT);
+const base::Feature kEnableBaz("enable_baz", base::FEATURE_DISABLED_BY_DEFAULT);
+const base::Feature kEnableBat("enable_bat", base::FEATURE_ENABLED_BY_DEFAULT);

[halliwell, 2017/04/27 00:04:23]

nit: would it make sense to have a more consistent naming convention for these
groups of features?

[slan, 2017/04/27 15:36:02]

Made it a little more formulaic, added guidance.

+
+// For use in TestParamsActivateOnBoot only.
+const base::Feature kEnableParams("enable_params",
+                                  base::FEATURE_DISABLED_BY_DEFAULT);
+
+// For use in TestOnlyWellFormedFeaturesPersisted only.
+const base::Feature kEnableAlpha("enable_alpha",
+                                 base::FEATURE_DISABLED_BY_DEFAULT);
+const base::Feature kEnableBravo("enable_bravo",
+                                 base::FEATURE_ENABLED_BY_DEFAULT);
+const base::Feature kEnableCharlie("enable_charlie",
+                                   base::FEATURE_DISABLED_BY_DEFAULT);
+const base::Feature kEnableDelta("enable_delta",
+                                 base::FEATURE_ENABLED_BY_DEFAULT);
+
+}  // namespace
+
+class CastFeaturesBrowserTest : public CastBrowserTest {
+ public:
+  CastFeaturesBrowserTest() {}
+  ~CastFeaturesBrowserTest() override {}
+
+  // CastBrowserTest implementation:
+  void TearDownOnMainThread() override {
+    CastBrowserTest::TearDownOnMainThread();
+    ASSERT_TRUE(features_declared_);
+
+    // If the test failed, scrub any state from prefs.

[halliwell, 2017/04/27 00:04:23]

What is this block for exactly?  does it cover clearing state on disk?  What
happens if tests crash (i.e. failure that won't run this cleanup)?  Does it make
more sense to clean everything up at the start (I think there's a setup function
that's called once before all tests run)?

[slan, 2017/04/27 15:36:02]

All really good points. On crash, the device would be left in a bad state.

Spent about 30 minutes trying to make SetUpTestCase (the method to which you
refer) work for us here, but couldn't make it work. We need too many things from
the browser (mostly the BrowserThread and the PathService) to effectively modify
the PrefStore in a non-hacky way, and the browser is not set up at that point.

So... I decided to go a little more low-tech and add a PRE_PRE_* test to each
group which clears the set of features used by each test. I also added some
guidance up top. This allows us to remove any of the declared feature nonsense,
and always ensures that the test run is good, no matter the state of the device.

+    if (HasFailure()) {
+      ClearDeclaredFeaturesFromPrefs();
+      ClearExperimentIds();
+    }
+  }
+
+  static PrefService* pref_service() {
+    return CastBrowserProcess::GetInstance()->pref_service();
+  }
+
+  // Write |dcs_features| to the pref store. This method is intended to be
+  // overridden in internal test to utilize the real production codepath for
+  // setting features from the server.
+  virtual void SetFeatures(const base::DictionaryValue& dcs_features) {
+    auto pref_features = GetOverriddenFeaturesForStorage(dcs_features);
+    ScopedUserPrefUpdate<base::DictionaryValue, base::Value::Type::DICTIONARY>
+        dict(pref_service(), prefs::kLatestDCSFeatures);
+    dict->MergeDictionary(&pref_features);
+    pref_service()->CommitPendingWrite();
+  }
+
+  // Declare the features that will used for this test. This function MUST be
+  // caled for every test case.
+  void DeclareFeatures(std::vector<base::Feature> features) {
+    DCHECK(!features_declared_);

[halliwell, 2017/04/27 00:04:23]

here and below, should these be asserts running on all build flavours?

[slan, 2017/04/27 15:36:02]

No more DCHECKS after changes above.

+    features_declared_ = true;
+    features_.swap(features);
+  }
+
+  // Clears only the features applicable to the current test (set in
+  // DelcareFeatures() above). DeclareFeatures() must be called before this

[halliwell, 2017/04/27 00:04:23]

DeclareFeatures

[slan, 2017/04/27 15:36:02]

Irrelevant

+  // method.
+  void ClearDeclaredFeaturesFromPrefs() {
+    DCHECK(features_declared_) << "DeclareFeatures() must be called at the "
+                               << "beginning of the test.";
+    ScopedUserPrefUpdate<base::DictionaryValue, base::Value::Type::DICTIONARY>
+        dict(pref_service(), prefs::kLatestDCSFeatures);
+    for (auto f : features_)
+      dict->Remove(f.name, nullptr);
+    pref_service()->CommitPendingWrite();
+  }
+
+  // Write |experiment_ids| to the pref store. This method is intended to be
+  // overridden in internal test to utilize the real production codepath for
+  // setting features from the server.
+  virtual void SetExperimentIds(
+      const std::unordered_set<int32_t>& experiment_ids) {
+    base::ListValue list;
+    for (auto id : experiment_ids)
+      list.AppendInteger(id);
+    pref_service()->Set(prefs::kActiveDCSExperiments, list);
+    pref_service()->CommitPendingWrite();
+  }
+
+  // Clear the set experiment id's.
+  void ClearExperimentIds() {
+    pref_service()->Set(prefs::kActiveDCSExperiments, base::ListValue());
+    pref_service()->CommitPendingWrite();
+  }
+
+ private:
+  bool features_declared_ = false;
+  std::vector<base::Feature> features_;
+
+  DISALLOW_COPY_AND_ASSIGN(CastFeaturesBrowserTest);
+};
+
+// Test that set features activate on the next boot. Part 1 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_TestFeaturesActivateOnBoot) {
+  DeclareFeatures({kEnableFoo, kEnableBar, kEnableBaz, kEnableBat});
+
+  // Default values should be returned.
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableFoo));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableBar));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableBaz));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableBat));
+
+  // Set the features to be used on next boot.
+  base::DictionaryValue features;
+  features.SetBoolean("enable_foo", true);
+  features.SetBoolean("enable_bat", false);
+  SetFeatures(features);
+
+  // Default values should still be returned until next boot.
+  EXPECT_FALSE(base::FeatureList::IsEnabled(kEnableFoo));
+  EXPECT_TRUE(base::FeatureList::IsEnabled(kEnableBar));
+  EXPECT_FALSE(base::FeatureList::IsEnabled(kEnableBaz));
+  EXPECT_TRUE(base::FeatureList::IsEnabled(kEnableBat));
+}
+
+// Test that features activate on the next boot. Part 2 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, TestFeaturesActivateOnBoot) {
+  DeclareFeatures({kEnableFoo, kEnableBar, kEnableBaz, kEnableBat});
+
+  // Overriden values set in test case above should be set.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableFoo));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableBar));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableBaz));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableBat));
+
+  ClearDeclaredFeaturesFromPrefs();
+}
+
+// Test that features with params activate on boot. Part 1 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, PRE_TestParamsActivateOnBoot) {
+  DeclareFeatures({kEnableParams});
+
+  // Default value should be returned.
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableParams));
+
+  // Set the features to be used on next boot.
+  base::DictionaryValue features;
+  auto params = base::MakeUnique<base::DictionaryValue>();
+  params->SetBoolean("bool_param", true);
+  params->SetBoolean("bool_param_2", false);
+  params->SetString("str_param", "foo");
+  params->SetDouble("doub_param", 3.14159);
+  params->SetInteger("int_param", 76543);
+  features.Set("enable_params", std::move(params));
+  SetFeatures(features);
+
+  // Default value should still be returned until next boot.
+  EXPECT_FALSE(base::FeatureList::IsEnabled(kEnableParams));
+}
+
+// Test that features with params activate on boot. Part 2 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, TestParamsActivateOnBoot) {
+  DeclareFeatures({kEnableParams});
+
+  // Check that the feature is now enabled.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableParams));
+
+  // Check that the params are populated and correct.
+  ASSERT_TRUE(base::GetFieldTrialParamByFeatureAsBool(
+      kEnableParams, "bool_param", false /* default_value */));
+  ASSERT_FALSE(base::GetFieldTrialParamByFeatureAsBool(
+      kEnableParams, "bool_param_2", true /* default_value */));
+  ASSERT_EQ("foo",
+            base::GetFieldTrialParamValueByFeature(kEnableParams, "str_param"));
+  ASSERT_EQ(76543, base::GetFieldTrialParamByFeatureAsInt(
+                       kEnableParams, "int_param", 0 /* default_value */));
+  ASSERT_EQ(3.14159, base::GetFieldTrialParamByFeatureAsDouble(
+                         kEnableParams, "doub_param", 0.0 /* default_value */));
+
+  // Check that no extra parameters are set.
+  std::map<std::string, std::string> params_out;
+  ASSERT_TRUE(base::GetFieldTrialParamsByFeature(kEnableParams, &params_out));
+  ASSERT_EQ(5u, params_out.size());
+
+  ClearDeclaredFeaturesFromPrefs();
+}
+
+// Test that only well-formed features are persisted to disk. Part 1 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_TestOnlyWellFormedFeaturesPersisted) {
+  DeclareFeatures({kEnableAlpha, kEnableBravo, kEnableCharlie, kEnableDelta});
+
+  // Default values should be returned.
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableAlpha));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableBravo));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableCharlie));
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableDelta));
+
+  // Set both good parameters...
+  base::DictionaryValue features;
+  features.SetBoolean("enable_alpha", true);
+  features.SetBoolean("enable_delta", false);
+
+  // ... and bad parameters.
+  features.SetString("enable_bravo", "False");
+  features.Set("enable_charlie", base::MakeUnique<base::ListValue>());
+
+  SetFeatures(features);
+}
+
+// Test that only well-formed features are persisted to disk. Part 2 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       TestOnlyWellFormedFeaturesPersisted) {
+  DeclareFeatures({kEnableAlpha, kEnableBravo, kEnableCharlie, kEnableDelta});
+
+  // Only the well-formed parameters should be overriden.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableAlpha));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableDelta));
+
+  // The other should take default values.
+  ASSERT_TRUE(base::FeatureList::IsEnabled(kEnableBravo));
+  ASSERT_FALSE(base::FeatureList::IsEnabled(kEnableCharlie));
+
+  ClearDeclaredFeaturesFromPrefs();
+}
+
+// Test that experiment ids are persisted to disk. Part 1 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest,
+                       PRE_TestExperimentIdsPersisted) {
+  DeclareFeatures(std::vector<base::Feature>());
+
+  // No experiments should be set.
+  ASSERT_TRUE(GetDCSExperimentIds().empty());
+
+  // Set a test list of experiments.
+  std::unordered_set<int32_t> ids({1234, 1, 4000});
+  SetExperimentIds(ids);
+
+  // No experiments should be set, still.
+  ASSERT_TRUE(GetDCSExperimentIds().empty());
+}
+
+// Test that experiment ids are persisted to disk. Part 2 of 2.
+IN_PROC_BROWSER_TEST_F(CastFeaturesBrowserTest, TestExperimentIdsPersisted) {
+  DeclareFeatures(std::vector<base::Feature>());
+
+  // The experiments set in the last run should be active now.
+  std::unordered_set<int32_t> ids({1234, 1, 4000});
+  ASSERT_EQ(ids, GetDCSExperimentIds());
+
+  ClearExperimentIds();
+}
+
+}  // namespace shell
+}  // namespace chromecast