Change ScopedFeatureList to overrides FeatureList not reset

base/test/scoped_feature_list_unittest.cc

+// 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 "base/test/scoped_feature_list.h"
+
+#include <string>
+
+#include "testing/gmock/include/gmock/gmock.h"

[Ilya Sherman, 2017/04/20 20:53:07]

nit: Is gmock being used?

[chaopeng, 2017/04/21 02:36:22]

Done.

+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+
+class ScopedFeatureListTest : public testing::Test {
+ public:
+  void SetUp() override {

[Ilya Sherman, 2017/04/20 20:53:08]

nit: Please do this work in the constructor instead.  It's rare that SetUp() is
preferable to doing work in the constructor.

[chaopeng, 2017/04/21 02:36:22]

Done.

+    // Clear default feature list;
+    feature_list.Init();
+  }
+
+  void ExpectFeatures(const std::string& enabled_features,
+                      const std::string& disabled_features) {
+    base::FeatureList* list = base::FeatureList::GetInstance();
+    std::string actual_enabled_features;
+    std::string actual_disabled_features;
+
+    list->GetFeatureOverrides(&actual_enabled_features,
+                              &actual_disabled_features);
+
+    EXPECT_EQ(enabled_features, actual_enabled_features);
+    EXPECT_EQ(disabled_features, actual_disabled_features);
+  }

[Ilya Sherman, 2017/04/20 20:53:07]

nit: This doesn't need to be part of the class AFAICT, so please move it into an
anonymous namespace instead.

[chaopeng, 2017/04/21 02:36:22]

Done.

+
+ private:
+  test::ScopedFeatureList feature_list;

[Ilya Sherman, 2017/04/20 20:53:08]

nit: Private members should end with a trailing underscore.

[chaopeng, 2017/04/21 02:36:22]

Done.

+};
+
+TEST_F(ScopedFeatureListTest, BasicScoped) {
+  ExpectFeatures(std::string(), std::string());
+  {
+    test::ScopedFeatureList feature_list1;
+    feature_list1.InitFromCommandLine(std::string(), "OverlayScrollbar");
+    ExpectFeatures(std::string(), "OverlayScrollbar");
+  }
+  ExpectFeatures(std::string(), std::string());
+}
+
+TEST_F(ScopedFeatureListTest, TestInitWithFeatureListAndOverrides) {
+  ExpectFeatures(std::string(), std::string());
+  {
+    test::ScopedFeatureList feature_list1;

[Ilya Sherman, 2017/04/20 20:53:07]

I think it would be more realistic for the outer list to not be a
ScopedFeatureList, but rather a regular feature list.  This does mean that the
test will duplicate some of the work that the ScopedFeatureList does, but I
think it will be clearer as a result.

Please try to structure these tests in the arrange/act/assert style, as
described here: https://goto.google.com/unit-test-practices#structure

+    std::vector<std::string> features{"OverlayScrollbar"};
+    feature_list1.InitWithFeatureListAndOverrides(std::vector<std::string>(),
+                                                  features);
+    ExpectFeatures(std::string(), "OverlayScrollbar");
+
+    {
+      test::ScopedFeatureList feature_list2;
+      ExpectFeatures(std::string(), "OverlayScrollbar");
+      // Given enabled list should override given feature list.
+      feature_list2.InitWithFeatureListAndOverrides(features,
+                                                    std::vector<std::string>());
+      ExpectFeatures("OverlayScrollbar", std::string());
+    }
+
+    {
+      test::ScopedFeatureList feature_list2;
+      ExpectFeatures(std::string(), "OverlayScrollbar");
+      // Given same feature should not appear duplicate in feature list.
+      feature_list2.InitWithFeatureListAndOverrides(std::vector<std::string>(),
+                                                    features);
+      ExpectFeatures(std::string(), "OverlayScrollbar");
+    }
+  }
+  ExpectFeatures(std::string(), std::string());
+}
+
+}  // namespace base