Initial implementation of FeatureList in base/.

base/metrics/feature_list_unittest.cc

+// 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.
+
+#include "base/metrics/feature_list.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+
+namespace {
+
+const char kFeatureOnName[] = "Example1";

[Ilya Sherman, 2015/09/01 03:55:26]

I think the tests would be easier to follow if this line were something like

  const char kFeatureOnByDefaultName[] = "Example_OnByDefault";

and the other lines were changed similarly.

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

+struct Feature kExampleFeatureOn {
+  kFeatureOnName, true
+};
+
+const char kFeatureOffName[] = "Example2";
+struct Feature kExampleFeatureOff {
+  kFeatureOffName, false
+};
+
+class FeatureListTest : public testing::Test {

[Ilya Sherman, 2015/09/01 03:55:26]

nit: The test harness class should be declared outside the anonymous namespace,
so that the ODR is less likely to be violated by tests.

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

+ public:
+  FeatureListTest() : feature_list_(new FeatureList) {
+    RegisterFeatureListInstance();
+  }
+  ~FeatureListTest() override { ClearFeatureListInstance(); }
+
+  void RegisterFeatureListInstance() {
+    FeatureList::SetInstance(make_scoped_ptr(feature_list_));
+  }

[Ilya Sherman, 2015/09/01 03:55:26]

This doesn't make much sense as a public method if the caller can't control the
value of |feature_list_|.  Perhaps this method should accept a pointer to the
desired feature list?

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

+  void ClearFeatureListInstance() { FeatureList::ClearInstanceForTesting(); }
+
+  FeatureList* feature_list() { return feature_list_; }

[Ilya Sherman, 2015/09/01 03:55:26]

This can return a pointer to freed memory if ClearFeatureListInstance() has been
called.  Probably that method should set |feature_list_| to nullptr.

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

+
+ private:
+  // Weak. Owned by the FeatureList::SetInstance().
+  FeatureList* feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(FeatureListTest);
+};
+
+}  // namespace
+
+TEST_F(FeatureListTest, DefaultStates) {
+  EXPECT_TRUE(FeatureList::IsEnabled(kExampleFeatureOn));
+  EXPECT_FALSE(FeatureList::IsEnabled(kExampleFeatureOff));
+}
+
+TEST_F(FeatureListTest, InitializeFromCommandLine) {
+  struct {
+    const char* enable_features;
+    const char* disable_features;
+    bool expected_feature_on_state;
+    bool expected_feature_off_state;
+  } test_cases[] = {
+      {"", "", true, false},
+      {"Example2", "", true, true},
+      {"Example2", "Example1", false, true},
+      {"Example1,Example2", "", true, true},
+      {"", "Example1,Example2", false, false},
+      // In the case an entry is both, disable takes precedence.
+      {"Example1", "Example1,Example2", false, false},
+  };
+
+  for (size_t i = 0; i < arraysize(test_cases); i++) {

[Ilya Sherman, 2015/09/01 03:55:26]

nit: ++i

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

+    const auto& test_case = test_cases[i];
+
+    FeatureList feature_list;
+    feature_list.InitializeFromCommandLine(test_case.enable_features,
+                                           test_case.disable_features);
+    feature_list.FinalizeInitialization();

[Ilya Sherman, 2015/09/01 03:55:26]

It seems more appropriate to set the singleton, and later to call
FeatureList::ClearInstanceForTesting().  That more closely mirrors what actual
client code would do (except for the resetting part, of course).

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done. I was originally thinking to keep the class flexible - such that if
someone wants to use this as a free-standing instance outside of the singleton,
the class lets you do this. But I actually don't have a reason in mind why
someone would want to do this, so restricted things as you suggested.

+
+    EXPECT_EQ(test_case.expected_feature_on_state,
+              feature_list.IsFeatureEnabled(kExampleFeatureOn))
+        << i;
+    EXPECT_EQ(test_case.expected_feature_off_state,
+              feature_list.IsFeatureEnabled(kExampleFeatureOff))
+        << i;
+  }
+}
+
+TEST_F(FeatureListTest, CheckFeatureIdentity) {

[Ilya Sherman, 2015/09/01 03:55:26]

This purpose of this test is not immediately obvious.  It might be worth writing
a brief comment explaining what it's doing.  (Better yet would be to test the
public API rather than the private one, if possible.)

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

I don't think a test using the public API is possible. Such a test would need to
somehow test that a DCHECK was hit - and I don't think there's a mechanism for
this. (Let me know if one exists and I'll gladly change this.)

Added a comment per your other suggestion.

[Ilya Sherman, 2015/09/01 20:17:17]

I was thinking we could use a gtest Death Test: [
https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests ].  (Caveat:
I haven't tried this recently, and remember it being somewhat finicky.)

[Alexei Svitkine (slow), 2015/09/01 21:18:38]

Right, then the test would have to only be enabled if DCHECKs are on. I suppose
it's possible, but I feel like it would be a bit convoluted so, would rather
leave as-is.

+  EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kExampleFeatureOn));
+  EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kExampleFeatureOn));
+  EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kExampleFeatureOff));
+  EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kExampleFeatureOff));

[Ilya Sherman, 2015/09/01 03:55:26]

Why are these lines written twice each?

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

The first time would register it and the second time would check against the
registered entry. Clarified in the comment.

+
+  struct Feature kExampleFeatureOn2 {
+    kFeatureOnName, true
+  };
+  EXPECT_FALSE(feature_list()->CheckFeatureIdentity(kExampleFeatureOn2));
+}
+
+}  // namespace base