Componentize ssl_config_service_manager_pref.cc

chrome/browser/net/ssl_config_service_manager_pref_unittest.cc

Index: chrome/browser/net/ssl_config_service_manager_pref_unittest.cc
diff --git a/chrome/browser/net/ssl_config_service_manager_pref_unittest.cc b/chrome/browser/net/ssl_config_service_manager_pref_unittest.cc
index e3afe0c8c8fa5f3d53c6339341a3a17b3fdc0240..59952adf326c1733887ef8ee5e8ae32d53cb71db 100644
--- a/chrome/browser/net/ssl_config_service_manager_pref_unittest.cc
+++ b/chrome/browser/net/ssl_config_service_manager_pref_unittest.cc
@@ -2,42 +2,37 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/net/ssl_config_service_manager.h"
+#include "components/ssl_config/ssl_config_service_manager.h"
 
 #include "base/command_line.h"
+#include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/testing_pref_store.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/values.h"
-#include "chrome/browser/prefs/pref_service_mock_factory.h"
-#include "chrome/common/chrome_switches.h"
-#include "chrome/common/pref_names.h"
-#include "chrome/test/base/testing_pref_service_syncable.h"
-#include "chrome/test/base/testing_profile.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
-#include "content/public/test/test_browser_thread.h"
+#include "components/pref_registry/testing_pref_service_syncable.h"
+#include "components/ssl_config/ssl_config_prefs.h"
+#include "components/ssl_config/ssl_config_switches.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/ssl/ssl_config_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ListValue;
 using base::Value;
-using content::BrowserThread;
 using net::SSLConfig;
 using net::SSLConfigService;
+using ssl_config::SSLConfigServiceManager;
 
 class SSLConfigServiceManagerPrefTest : public testing::Test {
  public:
-  SSLConfigServiceManagerPrefTest()
-      : ui_thread_(BrowserThread::UI, &message_loop_),
-        io_thread_(BrowserThread::IO, &message_loop_) {}
+  SSLConfigServiceManagerPrefTest() {}
 
  protected:
   base::MessageLoop message_loop_;
-  content::TestBrowserThread ui_thread_;
-  content::TestBrowserThread io_thread_;
 };
 
 // Test channel id with no user prefs.
@@ -46,7 +41,8 @@ TEST_F(SSLConfigServiceManagerPrefTest, ChannelIDWithoutUserPrefs) {
   SSLConfigServiceManager::RegisterPrefs(local_state.registry());
 
   scoped_ptr<SSLConfigServiceManager> config_manager(
-      SSLConfigServiceManager::CreateDefaultManager(&local_state));
+      SSLConfigServiceManager::CreateDefaultManager(
+          &local_state, base::ThreadTaskRunnerHandle::Get()));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());
@@ -63,7 +59,8 @@ TEST_F(SSLConfigServiceManagerPrefTest, GoodDisabledCipherSuites) {
   SSLConfigServiceManager::RegisterPrefs(local_state.registry());
 
   scoped_ptr<SSLConfigServiceManager> config_manager(
-      SSLConfigServiceManager::CreateDefaultManager(&local_state));
+      SSLConfigServiceManager::CreateDefaultManager(
+          &local_state, base::ThreadTaskRunnerHandle::Get()));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());
@@ -75,7 +72,7 @@ TEST_F(SSLConfigServiceManagerPrefTest, GoodDisabledCipherSuites) {
   base::ListValue* list_value = new base::ListValue();
   list_value->Append(new base::StringValue("0x0004"));
   list_value->Append(new base::StringValue("0x0005"));
-  local_state.SetUserPref(prefs::kCipherSuiteBlacklist, list_value);
+  local_state.SetUserPref(ssl_config::prefs::kCipherSuiteBlacklist, list_value);
 
   // Pump the message loop to notify the SSLConfigServiceManagerPref that the
   // preferences changed.
@@ -98,7 +95,8 @@ TEST_F(SSLConfigServiceManagerPrefTest, BadDisabledCipherSuites) {
   SSLConfigServiceManager::RegisterPrefs(local_state.registry());
 
   scoped_ptr<SSLConfigServiceManager> config_manager(
-      SSLConfigServiceManager::CreateDefaultManager(&local_state));
+      SSLConfigServiceManager::CreateDefaultManager(
+          &local_state, base::ThreadTaskRunnerHandle::Get()));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());
@@ -112,7 +110,7 @@ TEST_F(SSLConfigServiceManagerPrefTest, BadDisabledCipherSuites) {
   list_value->Append(new base::StringValue("TLS_NOT_WITH_A_CIPHER_SUITE"));
   list_value->Append(new base::StringValue("0x0005"));
   list_value->Append(new base::StringValue("0xBEEFY"));
-  local_state.SetUserPref(prefs::kCipherSuiteBlacklist, list_value);
+  local_state.SetUserPref(ssl_config::prefs::kCipherSuiteBlacklist, list_value);
 
   // Pump the message loop to notify the SSLConfigServiceManagerPref that the
   // preferences changed.
@@ -131,16 +129,12 @@ TEST_F(SSLConfigServiceManagerPrefTest, BadDisabledCipherSuites) {
 // TLS versions from 1.0 up to 1.1 or 1.2 are enabled.
 TEST_F(SSLConfigServiceManagerPrefTest, NoCommandLinePrefs) {
   scoped_refptr<TestingPrefStore> local_state_store(new TestingPrefStore());
-
-  PrefServiceMockFactory factory;
-  factory.set_user_prefs(local_state_store);
-  scoped_refptr<PrefRegistrySimple> registry = new PrefRegistrySimple;
-  scoped_ptr<PrefService> local_state(factory.Create(registry.get()));
-
-  SSLConfigServiceManager::RegisterPrefs(registry.get());
+  TestingPrefServiceSimple local_state;
+  SSLConfigServiceManager::RegisterPrefs(local_state.registry());
 
   scoped_ptr<SSLConfigServiceManager> config_manager(
-      SSLConfigServiceManager::CreateDefaultManager(local_state.get()));
+      SSLConfigServiceManager::CreateDefaultManager(
+          &local_state, base::ThreadTaskRunnerHandle::Get()));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());
@@ -155,15 +149,15 @@ TEST_F(SSLConfigServiceManagerPrefTest, NoCommandLinePrefs) {
             ssl_config.version_max);
 
   // The settings should not be added to the local_state.
-  EXPECT_FALSE(local_state->HasPrefPath(prefs::kSSLVersionMin));
-  EXPECT_FALSE(local_state->HasPrefPath(prefs::kSSLVersionMax));
+  EXPECT_FALSE(local_state.HasPrefPath(ssl_config::prefs::kSSLVersionMin));
+  EXPECT_FALSE(local_state.HasPrefPath(ssl_config::prefs::kSSLVersionMax));
 
   // Explicitly double-check the settings are not in the preference store.
   std::string version_min_str;
   std::string version_max_str;
-  EXPECT_FALSE(local_state_store->GetString(prefs::kSSLVersionMin,
+  EXPECT_FALSE(local_state_store->GetString(ssl_config::prefs::kSSLVersionMin,
                                             &version_min_str));
-  EXPECT_FALSE(local_state_store->GetString(prefs::kSSLVersionMax,
+  EXPECT_FALSE(local_state_store->GetString(ssl_config::prefs::kSSLVersionMax,
                                             &version_max_str));
 }
 
@@ -171,21 +165,16 @@ TEST_F(SSLConfigServiceManagerPrefTest, NoCommandLinePrefs) {
 // respected and that they do not persist to the preferences files.
 TEST_F(SSLConfigServiceManagerPrefTest, CommandLinePrefs) {
   scoped_refptr<TestingPrefStore> local_state_store(new TestingPrefStore());
-
-  base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
-  command_line.AppendSwitchASCII(switches::kSSLVersionMin, "tls1.1");
-  command_line.AppendSwitchASCII(switches::kSSLVersionMax, "tls1");
-
-  PrefServiceMockFactory factory;
-  factory.set_user_prefs(local_state_store);
-  factory.SetCommandLine(&command_line);
-  scoped_refptr<PrefRegistrySimple> registry = new PrefRegistrySimple;
-  scoped_ptr<PrefService> local_state(factory.Create(registry.get()));
-
-  SSLConfigServiceManager::RegisterPrefs(registry.get());
+  TestingPrefServiceSimple local_state;
+  local_state.SetUserPref(ssl_config::prefs::kSSLVersionMin,
+                          new base::StringValue("tls1.1"));
+  local_state.SetUserPref(ssl_config::prefs::kSSLVersionMax,
+                          new base::StringValue("tls1"));
+  SSLConfigServiceManager::RegisterPrefs(local_state.registry());
 
   scoped_ptr<SSLConfigServiceManager> config_manager(
-      SSLConfigServiceManager::CreateDefaultManager(local_state.get()));
+      SSLConfigServiceManager::CreateDefaultManager(
+          &local_state, base::ThreadTaskRunnerHandle::Get()));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());
@@ -198,18 +187,18 @@ TEST_F(SSLConfigServiceManagerPrefTest, CommandLinePrefs) {
 
   // Explicitly double-check the settings are not in the preference store.
   const PrefService::Preference* version_min_pref =
-      local_state->FindPreference(prefs::kSSLVersionMin);
+      local_state.FindPreference(ssl_config::prefs::kSSLVersionMin);
   EXPECT_FALSE(version_min_pref->IsUserModifiable());

[Abhishek, 2015/09/10 13:32:54]

Currently this test is failing here. I tried solving it but I don't know how to
solve it? Please help me!

[droger, 2015/09/10 14:49:38]

I discussed with sdefresne who is more familiar with this code.
It turns out that that what this is testing is:
1) the command line pref store is not user modifiable
2) setting a preference through a switch does not set it in the user pref store
3) the command line switch is correctly converted to a preference.

None of this is relevant to ssl_config_service_manager_pref (and we can see that
it is not even called here), and furthermore these behaviors are already tested
elsewhere.
1) is tested in PrefValueStoreTest.PrefValueUserModifiable
3) is already tested by CommandLinePrefStoreTest.SimpleStringPref

This leaves only 2), which is really some kind of integration test. I'm not sure
how much value there is in testing that. I would suggest splitting this
particular test to a different file, or maybe delete it if owners agree, because
there may be very little value in it.

[Ryan Sleevi, 2015/09/24 20:59:56]

Well, it was a regression test based on a real bug, so I'm kind of inclined to
keep it, since we know the testing of the other components wasn't sufficient in
the past :)

[Abhishek, 2015/09/28 05:30:33]

Thanks for updating it!

Sure, I'll keep it and break it into another file as suggested by droger.

 
   const PrefService::Preference* version_max_pref =
-      local_state->FindPreference(prefs::kSSLVersionMax);
+      local_state.FindPreference(ssl_config::prefs::kSSLVersionMax);
   EXPECT_FALSE(version_max_pref->IsUserModifiable());

[Abhishek, 2015/09/10 13:32:54]

ditto

 
   std::string version_min_str;
   std::string version_max_str;
-  EXPECT_FALSE(local_state_store->GetString(prefs::kSSLVersionMin,
+  EXPECT_FALSE(local_state_store->GetString(ssl_config::prefs::kSSLVersionMin,
                                             &version_min_str));
-  EXPECT_FALSE(local_state_store->GetString(prefs::kSSLVersionMax,
+  EXPECT_FALSE(local_state_store->GetString(ssl_config::prefs::kSSLVersionMax,
                                             &version_max_str));
 }
 
@@ -217,19 +206,14 @@ TEST_F(SSLConfigServiceManagerPrefTest, CommandLinePrefs) {
 TEST_F(SSLConfigServiceManagerPrefTest, NoSSL3) {
   scoped_refptr<TestingPrefStore> local_state_store(new TestingPrefStore());
 
-  base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
-  command_line.AppendSwitchASCII(switches::kSSLVersionMin, "ssl3");
-
-  PrefServiceMockFactory factory;
-  factory.set_user_prefs(local_state_store);
-  factory.SetCommandLine(&command_line);
-  scoped_refptr<PrefRegistrySimple> registry = new PrefRegistrySimple;
-  scoped_ptr<PrefService> local_state(factory.Create(registry.get()));
-
-  SSLConfigServiceManager::RegisterPrefs(registry.get());
+  TestingPrefServiceSimple local_state;
+  local_state.SetUserPref(ssl_config::prefs::kSSLVersionMin,
+                          new base::StringValue("ssl3"));
+  SSLConfigServiceManager::RegisterPrefs(local_state.registry());
 
   scoped_ptr<SSLConfigServiceManager> config_manager(
-      SSLConfigServiceManager::CreateDefaultManager(local_state.get()));
+      SSLConfigServiceManager::CreateDefaultManager(
+          &local_state, base::ThreadTaskRunnerHandle::Get()));
   ASSERT_TRUE(config_manager.get());
   scoped_refptr<SSLConfigService> config_service(config_manager->Get());
   ASSERT_TRUE(config_service.get());