[Origin Trials] Support updates of disabled token list

chrome/browser/chrome_origin_trials_browsertest.cc

Index: chrome/browser/chrome_origin_trials_browsertest.cc
diff --git a/chrome/browser/chrome_origin_trials_browsertest.cc b/chrome/browser/chrome_origin_trials_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..315e262b4f0c0615ba955e025d50e19d0817f27d
--- /dev/null
+++ b/chrome/browser/chrome_origin_trials_browsertest.cc
@@ -0,0 +1,254 @@
+// 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/command_line.h"
+#include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+static const char kNewPublicKey[] = "new public key";
+
+static const std::vector<std::string> kOneDisabledFeatureList = {"A"};
+static const std::vector<std::string> kTwoDisabledFeaturesList = {"A", "B"};
+static const std::vector<std::string> kThreeDisabledFeaturesList = {"A", "B",
+                                                                    "C"};
+static const std::vector<std::string> kSpacesInDisabledFeaturesList = {"A",
+                                                                       "B C"};
+static const char kOneDisabledFeatureSwitch[] = "A";
+static const char kTwoDisabledFeaturesSwitch[] = "A|B";
+static const char kThreeDisabledFeaturesSwitch[] = "A|B|C";
+static const char kSpacesInDisabledFeaturesSwitch[] = "A|B C";
+
+static const std::vector<std::string> kOneDisabledTokenList = {"t1"};
+static const std::vector<std::string> kTwoDisabledTokensList = {"t1", "t2"};
+static const std::vector<std::string> kThreeDisabledTokensList = {"t1", "t2",
+                                                                  "t3"};
+static const char kOneDisabledTokenSwitch[] = "t1";
+static const char kTwoDisabledTokensSwitch[] = "t1|t2";
+static const char kThreeDisabledTokensSwitch[] = "t1|t2|t3";
+
+class ChromeOriginTrialsTest : public InProcessBrowserTest {
+ protected:
+  ChromeOriginTrialsTest() {}
+
+  void AddDisabledFeaturesToPrefs(const std::vector<std::string>& features) {
+    base::ListValue disabled_feature_list;
+    disabled_feature_list.AppendStrings(features);
+    ListPrefUpdate update(local_state(), prefs::kOriginTrialDisabledFeatures);
+    update->Swap(&disabled_feature_list);
+  }
+
+  void AddDisabledTokensToPrefs(const std::vector<std::string>& tokens) {
+    base::ListValue disabled_token_list;
+    disabled_token_list.AppendStrings(tokens);
+    ListPrefUpdate update(local_state(), prefs::kOriginTrialDisabledTokens);
+    update->Swap(&disabled_token_list);
+  }
+
+  PrefService* local_state() { return g_browser_process->local_state(); }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ChromeOriginTrialsTest);
+};
+
+// Tests to verify that the public key is correctly read from prefs and
+// added to the command line
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest, NoPublicKeySet) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  EXPECT_FALSE(command_line->HasSwitch(switches::kOriginTrialPublicKey));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest, PRE_PublicKeySetOnCommandLine) {
+  local_state()->Set(prefs::kOriginTrialPublicKey, base::Value(kNewPublicKey));
+  ASSERT_EQ(kNewPublicKey,
+            local_state()->GetString(prefs::kOriginTrialPublicKey));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest, PublicKeySetOnCommandLine) {
+  ASSERT_EQ(kNewPublicKey,
+            local_state()->GetString(prefs::kOriginTrialPublicKey));
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+
+  EXPECT_TRUE(command_line->HasSwitch(switches::kOriginTrialPublicKey));
+
+  if (command_line->HasSwitch(switches::kOriginTrialPublicKey)) {

[Marijn Kruisselbrink, 2017/03/14 05:54:42]

rather than having this if, would it make sense to just change the previous
EXPECT_TRUE into an ASSERT_TRUE?

[chasej, 2017/03/14 20:00:08]

I created a helper method GetCommandLineSwitch(), since there were multiple
lines of boilerplate code to get and assert. I can't use ASSERT_TRUE
because the helper method returns a value. I did remove the extra
"if command_line->HasSwitch" check, as it will just return an empty string
if not found.

+    std::string actual_key =
+        command_line->GetSwitchValueASCII(switches::kOriginTrialPublicKey);
+    EXPECT_EQ(kNewPublicKey, actual_key);
+  }
+}
+
+// Tests to verify that disabled features are correctly read from prefs and
+// added to the command line
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest, NoDisabledFeatures) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  EXPECT_FALSE(command_line->HasSwitch(switches::kOriginTrialDisabledFeatures));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       PRE_OneDisabledFeatureSetOnCommandLine) {
+  AddDisabledFeaturesToPrefs(kOneDisabledFeatureList);
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledFeatures));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       OneDisabledFeatureSetOnCommandLine) {
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledFeatures));
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+
+  EXPECT_TRUE(command_line->HasSwitch(switches::kOriginTrialDisabledFeatures));
+
+  if (command_line->HasSwitch(switches::kOriginTrialDisabledFeatures)) {
+    std::string switch_value = command_line->GetSwitchValueASCII(
+        switches::kOriginTrialDisabledFeatures);
+    EXPECT_EQ(kOneDisabledFeatureSwitch, switch_value);
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,

[Marijn Kruisselbrink, 2017/03/14 05:54:42]

Rather than having all these essentially identical tests, just with different
data, would it make sense to write this as parameterized tests (using
IN_PROC_BROWSER_TEST_P/INSTANTIATE_TEST_CASE_P? I assume that would still work
correctly even with the PRE_ tests, although I've never tried that.

[chasej, 2017/03/14 20:00:08]

Done. Thanks for the suggestion. I had to add separate fixtures, but
the result is still much shorter and cleaner.

+                       PRE_TwoDisabledFeaturesSetOnCommandLine) {
+  AddDisabledFeaturesToPrefs(kTwoDisabledFeaturesList);
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledFeatures));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       TwoDisabledFeaturesSetOnCommandLine) {
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledFeatures));
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+
+  EXPECT_TRUE(command_line->HasSwitch(switches::kOriginTrialDisabledFeatures));
+
+  if (command_line->HasSwitch(switches::kOriginTrialDisabledFeatures)) {
+    std::string switch_value = command_line->GetSwitchValueASCII(
+        switches::kOriginTrialDisabledFeatures);
+    EXPECT_EQ(kTwoDisabledFeaturesSwitch, switch_value);
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       PRE_ThreeDisabledFeaturesSetOnCommandLine) {
+  AddDisabledFeaturesToPrefs(kThreeDisabledFeaturesList);
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledFeatures));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       ThreeDisabledFeaturesSetOnCommandLine) {
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledFeatures));
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+
+  EXPECT_TRUE(command_line->HasSwitch(switches::kOriginTrialDisabledFeatures));
+
+  if (command_line->HasSwitch(switches::kOriginTrialDisabledFeatures)) {
+    std::string switch_value = command_line->GetSwitchValueASCII(
+        switches::kOriginTrialDisabledFeatures);
+    EXPECT_EQ(kThreeDisabledFeaturesSwitch, switch_value);
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       PRE_SpacesInDisabledFeaturesSetOnCommandLine) {
+  AddDisabledFeaturesToPrefs(kSpacesInDisabledFeaturesList);
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledFeatures));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       SpacesInDisabledFeaturesSetOnCommandLine) {
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledFeatures));
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+
+  EXPECT_TRUE(command_line->HasSwitch(switches::kOriginTrialDisabledFeatures));
+
+  if (command_line->HasSwitch(switches::kOriginTrialDisabledFeatures)) {
+    std::string switch_value = command_line->GetSwitchValueASCII(
+        switches::kOriginTrialDisabledFeatures);
+    EXPECT_EQ(kSpacesInDisabledFeaturesSwitch, switch_value);
+  }
+}
+
+// Tests to verify that disabled tokens are correctly read from prefs and
+// added to the command line
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest, NoDisabledTokens) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  EXPECT_FALSE(command_line->HasSwitch(switches::kOriginTrialDisabledTokens));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       PRE_OneDisabledTokenSetOnCommandLine) {
+  AddDisabledTokensToPrefs(kOneDisabledTokenList);
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledTokens));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       OneDisabledTokenSetOnCommandLine) {
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledTokens));
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+
+  EXPECT_TRUE(command_line->HasSwitch(switches::kOriginTrialDisabledTokens));
+
+  if (command_line->HasSwitch(switches::kOriginTrialDisabledTokens)) {
+    std::string switch_value =
+        command_line->GetSwitchValueASCII(switches::kOriginTrialDisabledTokens);
+    EXPECT_EQ(kOneDisabledTokenSwitch, switch_value);
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       PRE_TwoDisabledTokensSetOnCommandLine) {
+  AddDisabledTokensToPrefs(kTwoDisabledTokensList);
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledTokens));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       TwoDisabledTokensSetOnCommandLine) {
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledTokens));
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+
+  EXPECT_TRUE(command_line->HasSwitch(switches::kOriginTrialDisabledTokens));
+
+  if (command_line->HasSwitch(switches::kOriginTrialDisabledTokens)) {
+    std::string switch_value =
+        command_line->GetSwitchValueASCII(switches::kOriginTrialDisabledTokens);
+    EXPECT_EQ(kTwoDisabledTokensSwitch, switch_value);
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       PRE_ThreeDisabledTokensSetOnCommandLine) {
+  AddDisabledTokensToPrefs(kThreeDisabledTokensList);
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledTokens));
+}
+
+IN_PROC_BROWSER_TEST_F(ChromeOriginTrialsTest,
+                       ThreeDisabledTokensSetOnCommandLine) {
+  ASSERT_TRUE(local_state()->HasPrefPath(prefs::kOriginTrialDisabledTokens));
+
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+
+  EXPECT_TRUE(command_line->HasSwitch(switches::kOriginTrialDisabledTokens));
+
+  if (command_line->HasSwitch(switches::kOriginTrialDisabledTokens)) {
+    std::string switch_value =
+        command_line->GetSwitchValueASCII(switches::kOriginTrialDisabledTokens);
+    EXPECT_EQ(kThreeDisabledTokensSwitch, switch_value);
+  }
+}
+
+}  // namespace