Activate the VariationsService for ChromeOS and ensure that it does not ping the server until the E…

chrome/browser/metrics/variations/variations_service_unittest.cc

Index: chrome/browser/metrics/variations/variations_service_unittest.cc
diff --git a/chrome/browser/metrics/variations/variations_service_unittest.cc b/chrome/browser/metrics/variations/variations_service_unittest.cc
index 0de0800342379ff326e0489f17d10e0fd2ac7ad0..2341afcebd8c7e06d55ca166e4a9254b5bd2477a 100644
--- a/chrome/browser/metrics/variations/variations_service_unittest.cc
+++ b/chrome/browser/metrics/variations/variations_service_unittest.cc
@@ -8,6 +8,7 @@
 #include "chrome/browser/metrics/variations/variations_service.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_pref_service.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -16,19 +17,53 @@ namespace chrome_variations {
 
 namespace {
 
+class TestRequestAllowedNotifier : public ResourceRequestAllowedNotifier {
+ public:
+  TestRequestAllowedNotifier();
+  virtual ~TestRequestAllowedNotifier();
+
+  virtual bool ResourceRequestsAllowed() OVERRIDE;
+
+  void SetRequestsAllowed(bool allowed);
+
+  // Notify observers that requests are allowed.
+  void NotifyObservers();
+
+ private:
+  bool requests_allowed_;
+};
+
+TestRequestAllowedNotifier::TestRequestAllowedNotifier()
+    : requests_allowed_(true) {
+}
+
+TestRequestAllowedNotifier::~TestRequestAllowedNotifier() {
+}
+
+bool TestRequestAllowedNotifier::ResourceRequestsAllowed() {
+  return requests_allowed_;
+}
+
+void TestRequestAllowedNotifier::SetRequestsAllowed(bool allowed) {
+  requests_allowed_ = allowed;
+}
+
+// Notify observers that requests are allowed.
+void TestRequestAllowedNotifier::NotifyObservers() {
+  requests_allowed_ = true;
+  MaybeNotifyObservers();
+}
+
 // A test class used to validate expected functionality in VariationsService.
 class TestVariationsService : public VariationsService {
  public:
-  TestVariationsService() : VariationsService(),
-                            fetch_attempted_(false) {
-  }
-  virtual ~TestVariationsService() {}
+  explicit TestVariationsService(ResourceRequestAllowedNotifier* notifier);
+  virtual ~TestVariationsService();
 
   bool fetch_attempted() const { return fetch_attempted_; }
-  void SetFetchAttempted(bool attempted) { fetch_attempted_ = attempted; }
 
  protected:
-  virtual void FetchVariationsSeed() OVERRIDE {
+  virtual void DoActualFetch() OVERRIDE {
     fetch_attempted_ = true;
   }
 
@@ -38,31 +73,17 @@ class TestVariationsService : public VariationsService {
   DISALLOW_COPY_AND_ASSIGN(TestVariationsService);
 };
 
-// Override NetworkChangeNotifier to simulate connection type changes for tests.
-class TestNetworkChangeNotifier : public net::NetworkChangeNotifier {
- public:
-  TestNetworkChangeNotifier()
-    : net::NetworkChangeNotifier(),
-      connection_type_to_return_(
-          net::NetworkChangeNotifier::CONNECTION_UNKNOWN) {
-  }
-
-  void SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::ConnectionType type) {
-    connection_type_to_return_ = type;
-    net::NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange();
-    MessageLoop::current()->RunAllPending();
-  }
-
- private:
-  virtual ConnectionType GetCurrentConnectionType() const OVERRIDE {
-    return connection_type_to_return_;
-  }
-
-  net::NetworkChangeNotifier::ConnectionType connection_type_to_return_;
+TestVariationsService::TestVariationsService(
+    ResourceRequestAllowedNotifier* notifier)
+    : VariationsService(),
+      fetch_attempted_(false) {
+  SetResourceRequestAllowedNotifierForTesting(notifier);
+  // Set this so StartRepeatedVariationsSeedFetch can be called in tests.
+  create_trials_from_seed_called_ = true;
+}
 
-  DISALLOW_COPY_AND_ASSIGN(TestNetworkChangeNotifier);
-};
+TestVariationsService::~TestVariationsService() {
+}
 
 // Converts |time| to Study proto format.
 int64 TimeToProtoTime(const base::Time& time) {
@@ -84,38 +105,38 @@ TrialsSeed CreateTestSeed() {
   return seed;
 }
 
-}  // namespace
-
-// A test fixture class for VariationsService tests that require network state
-// simulations.
-class VariationsServiceNetworkTest : public testing::Test {
+// Fixture for tests that instantiate VariationsService.
+// TODO(stevet): This is not great since it creates useless overhead for non-

[SteveT, 2012/09/19 19:27:09]

REVIEWERS: Any thoughts here? I might be able to just create a
CreateVariationsTestLocalState() helper in an anon-namespace which returns a
TestingPerfService with the EulaAccepted pref already set to true (on CrOS)...
and then use that the way Jesse's original tests used it (rather than have this
fixture class).

Does that seem neater?

[Ilya Sherman, 2012/09/19 19:49:01]

Either way seems fine to me, or Alexei's suggestion also seems fine.  If you
keep this approach, a couple of nits below...

[SteveT, 2012/09/20 19:40:18]

This is gone now since we're using that virtual IsEulaAccepted method.

+// ChromeOS tests. Find a better way to handle this.
+class VariationsServiceCreatedTest : public testing::Test {
  public:
-  VariationsServiceNetworkTest()
-    : ui_thread(content::BrowserThread::UI, &message_loop) { }
-  ~VariationsServiceNetworkTest() { }
-
-  void SetWasOfflineDuringLastRequestAttempt(bool offline) {
-    test_service.SetWasOfflineDuringLastRequestAttemptForTesting(offline);
+  VariationsServiceCreatedTest()
+      : scoped_testing_local_state_(
+            static_cast<TestingBrowserProcess*>(g_browser_process)),
+        local_state_(scoped_testing_local_state_.Get()) {
   }
 
-  void SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::ConnectionType type) {
-    notifier.SimulateNetworkConnectionChange(type);
-  }
+  // Expose local_state for tests that need to touch the VariationsSeed.
+  TestingPrefService* local_state() { return local_state_; }
 
-  bool fetch_attempted() const {
-    return test_service.fetch_attempted();
+#if defined(OS_CHROMEOS)
+  void SetUp() {

[Ilya Sherman, 2012/09/19 19:49:01]

nit: OVERRIDE

[SteveT, 2012/09/20 19:40:18]

This class has been removed.

+    // Have to set the Eula state before creating the notifier as it checks this
+    // at startup.
+    // TODO(stevet): Share this pref name with the wizard_controller file that
+    // defines it.
+    local_state_->SetUserPref("EulaAccepted", Value::CreateBooleanValue(true));

[Alexei Svitkine (slow), 2012/09/19 19:44:28]

I made a comment about this in the test for new notifier class. Ideally, you
should be able to use a test notifier here too to simulate the state you want,
rather than murking around in the prefs.

[SteveT, 2012/09/20 19:40:18]

This class has been removed.

   }
-
+#endif
  private:
-  MessageLoopForUI message_loop;
-  content::TestBrowserThread ui_thread;
-  TestNetworkChangeNotifier notifier;
-  TestVariationsService test_service;
+  ScopedTestingLocalState scoped_testing_local_state_;
+  TestingPrefService* local_state_;

[Ilya Sherman, 2012/09/19 19:49:01]

nit: This doesn't look like it needs to be a separate member variable -- you
could just use scoped_testing_local_state_ throughout (and rename that one to
local_state_ if you get rid of this one).

[SteveT, 2012/09/20 19:40:18]

This class has been removed.

 
-  DISALLOW_COPY_AND_ASSIGN(VariationsServiceNetworkTest);
+  DISALLOW_COPY_AND_ASSIGN(VariationsServiceCreatedTest);
 };
 
+}  // namespace
+
 TEST(VariationsServiceTest, CheckStudyChannel) {
   const chrome::VersionInfo::Channel channels[] = {
     chrome::VersionInfo::CHANNEL_CANARY,
@@ -387,11 +408,7 @@ TEST(VariationsServiceTest, IsStudyExpired) {
   }
 }
 
-TEST(VariationsServiceTest, LoadSeed) {
-  TestingPrefService pref_service;
-
-  VariationsService::RegisterPrefs(&pref_service);
-
+TEST_F(VariationsServiceCreatedTest, LoadSeed) {
   // Store good seed data to test if loading from prefs works.
   TrialsSeed seed = CreateTestSeed();
 
@@ -399,12 +416,12 @@ TEST(VariationsServiceTest, LoadSeed) {
   seed.SerializeToString(&serialized_seed);
   std::string base64_serialized_seed;
   ASSERT_TRUE(base::Base64Encode(serialized_seed, &base64_serialized_seed));
-  pref_service.SetString(prefs::kVariationsSeed, base64_serialized_seed);
+  local_state()->SetString(prefs::kVariationsSeed, base64_serialized_seed);
 
   VariationsService variations_service;
   TrialsSeed loaded_seed;
   EXPECT_TRUE(
-      variations_service.LoadTrialsSeedFromPref(&pref_service, &loaded_seed));
+      variations_service.LoadTrialsSeedFromPref(local_state(), &loaded_seed));
 
   std::string serialized_loaded_seed;
   loaded_seed.SerializeToString(&serialized_loaded_seed);
@@ -412,25 +429,22 @@ TEST(VariationsServiceTest, LoadSeed) {
   EXPECT_EQ(serialized_seed, serialized_loaded_seed);
   // Make sure the pref hasn't been changed.
   EXPECT_FALSE(
-      pref_service.FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
+      local_state()->FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
   EXPECT_EQ(base64_serialized_seed,
-            pref_service.GetString(prefs::kVariationsSeed));
+            local_state()->GetString(prefs::kVariationsSeed));
 
   // Check that loading a bad seed returns false and clears the pref.
-  pref_service.ClearPref(prefs::kVariationsSeed);
-  pref_service.SetString(prefs::kVariationsSeed, "this should fail");
+  local_state()->ClearPref(prefs::kVariationsSeed);
+  local_state()->SetString(prefs::kVariationsSeed, "this should fail");
   EXPECT_FALSE(
-      pref_service.FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
+      local_state()->FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
   EXPECT_FALSE(
-      variations_service.LoadTrialsSeedFromPref(&pref_service, &loaded_seed));
+      variations_service.LoadTrialsSeedFromPref(local_state(), &loaded_seed));
   EXPECT_TRUE(
-      pref_service.FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
+      local_state()->FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
 }
 
-TEST(VariationsServiceTest, StoreSeed) {
-  TestingPrefService pref_service;
-
-  VariationsService::RegisterPrefs(&pref_service);
+TEST_F(VariationsServiceCreatedTest, StoreSeed) {
   const base::Time now = base::Time::Now();
 
   TrialsSeed seed = CreateTestSeed();
@@ -439,13 +453,13 @@ TEST(VariationsServiceTest, StoreSeed) {
   std::string serialized_seed;
   seed.SerializeToString(&serialized_seed);
   EXPECT_TRUE(
-      variations_service.StoreSeedData(serialized_seed, now, &pref_service));
+      variations_service.StoreSeedData(serialized_seed, now, local_state()));
   // Make sure the pref was actually set.
   EXPECT_FALSE(
-      pref_service.FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
+      local_state()->FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
 
   std::string loaded_serialized_seed =
-      pref_service.GetString(prefs::kVariationsSeed);
+      local_state()->GetString(prefs::kVariationsSeed);
   std::string decoded_serialized_seed;
   ASSERT_TRUE(base::Base64Decode(loaded_serialized_seed,
                                  &decoded_serialized_seed));
@@ -453,11 +467,11 @@ TEST(VariationsServiceTest, StoreSeed) {
   EXPECT_EQ(serialized_seed, decoded_serialized_seed);
 
   // Check if trying to store a bad seed leaves the pref unchanged.
-  pref_service.ClearPref(prefs::kVariationsSeed);
+  local_state()->ClearPref(prefs::kVariationsSeed);
   EXPECT_FALSE(
-      variations_service.StoreSeedData("this should fail", now, &pref_service));
+      variations_service.StoreSeedData("this should fail", now, local_state()));
   EXPECT_TRUE(
-      pref_service.FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
+      local_state()->FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
 }
 
 TEST(VariationsServiceTest, ValidateStudy) {
@@ -534,53 +548,20 @@ TEST(VariationsServiceTest, ValidateStudy) {
   EXPECT_FALSE(valid);
 }
 
-TEST_F(VariationsServiceNetworkTest, DoNotFetchIfOffline) {
-  SetWasOfflineDuringLastRequestAttempt(true);
-  SimulateNetworkConnectionChange(net::NetworkChangeNotifier::CONNECTION_NONE);
-  EXPECT_FALSE(fetch_attempted());
-}
-
-TEST_F(VariationsServiceNetworkTest, DoNotFetchIfOnlineToOnline) {
-  SetWasOfflineDuringLastRequestAttempt(false);
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_ETHERNET);
-  EXPECT_FALSE(fetch_attempted());
-}
-
-TEST_F(VariationsServiceNetworkTest, FetchOnReconnect) {
-  SetWasOfflineDuringLastRequestAttempt(true);
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_ETHERNET);
-  EXPECT_TRUE(fetch_attempted());
-}
-
-TEST_F(VariationsServiceNetworkTest, NoFetchOnWardriving) {
-  SetWasOfflineDuringLastRequestAttempt(false);
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
-  EXPECT_FALSE(fetch_attempted());
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_3G);
-  EXPECT_FALSE(fetch_attempted());
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_4G);
-  EXPECT_FALSE(fetch_attempted());
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
-  EXPECT_FALSE(fetch_attempted());
-}
-
-TEST_F(VariationsServiceNetworkTest, NoFetchOnFlakyConnection) {
-  SetWasOfflineDuringLastRequestAttempt(false);
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
-  EXPECT_FALSE(fetch_attempted());
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_NONE);
-  EXPECT_FALSE(fetch_attempted());
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
-  EXPECT_FALSE(fetch_attempted());
+TEST_F(VariationsServiceCreatedTest, ResourceRequestAllowedNotifierTest) {
+  MessageLoopForUI message_loop;
+  content::TestBrowserThread ui_thread(content::BrowserThread::UI,
+                                       &message_loop);
+
+  TestRequestAllowedNotifier* notifier = new TestRequestAllowedNotifier;
+  // Pass ownership to TestVariationsService, but keep a weak pointer to
+  // manipulate it for this test.
+  TestVariationsService test_service(notifier);
+  notifier->SetRequestsAllowed(false);
+  test_service.StartRepeatedVariationsSeedFetch();
+  EXPECT_FALSE(test_service.fetch_attempted());
+  notifier->NotifyObservers();
+  EXPECT_TRUE(test_service.fetch_attempted());
 }
 
 }  // namespace chrome_variations