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

 // Copyright (c) 2012 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/base64.h"
 #include "base/string_split.h"
 #include "chrome/browser/metrics/proto/study.pb.h"
+#include "chrome/browser/metrics/variations/resource_request_allowed_notifier_test_util.h"
 #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"
 
 namespace chrome_variations {
 
 namespace {
 
 // A test class used to validate expected functionality in VariationsService.
 class TestVariationsService : public VariationsService {
  public:
-  TestVariationsService() : VariationsService(),
-                            fetch_attempted_(false) {
+  explicit TestVariationsService(TestRequestAllowedNotifier* test_notifier)
+      : VariationsService(test_notifier),
+        fetch_attempted_(false) {
+    // Set this so StartRepeatedVariationsSeedFetch can be called in tests.
+    SetCreateTrialsFromSeedCalledForTesting(true);
   }
-  virtual ~TestVariationsService() {}
+
+  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;
   }
 
  private:
   bool fetch_attempted_;
 
   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_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestNetworkChangeNotifier);
-};
-
 // Converts |time| to Study proto format.
 int64 TimeToProtoTime(const base::Time& time) {
   return (time - base::Time::UnixEpoch()).InSeconds();
 }
 
 // Populates |seed| with simple test data. The resulting seed will contain one
 // study called "test", which contains one experiment called "abc" with
 // probability weight 100. |seed|'s study field will be cleared before adding
 // the new study.
 TrialsSeed CreateTestSeed() {
   TrialsSeed seed;
   Study* study = seed.add_study();
   study->set_name("test");
   study->set_default_experiment_name("abc");
   Study_Experiment* experiment = study->add_experiment();
   experiment->set_name("abc");
   experiment->set_probability_weight(100);
   return seed;
 }
 
 }  // namespace
 
-// A test fixture class for VariationsService tests that require network state
-// simulations.
-class VariationsServiceNetworkTest : public testing::Test {
- public:
-  VariationsServiceNetworkTest()
-    : ui_thread(content::BrowserThread::UI, &message_loop) { }
-  ~VariationsServiceNetworkTest() { }
-
-  void SetWasOfflineDuringLastRequestAttempt(bool offline) {
-    test_service.SetWasOfflineDuringLastRequestAttemptForTesting(offline);
-  }
-
-  void SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::ConnectionType type) {
-    notifier.SimulateNetworkConnectionChange(type);
-  }
-
-  bool fetch_attempted() const {
-    return test_service.fetch_attempted();
-  }
-
- private:
-  MessageLoopForUI message_loop;
-  content::TestBrowserThread ui_thread;
-  TestNetworkChangeNotifier notifier;
-  TestVariationsService test_service;
-
-  DISALLOW_COPY_AND_ASSIGN(VariationsServiceNetworkTest);
-};
-
 TEST(VariationsServiceTest, CheckStudyChannel) {
   const chrome::VersionInfo::Channel channels[] = {
     chrome::VersionInfo::CHANNEL_CANARY,
     chrome::VersionInfo::CHANNEL_DEV,
     chrome::VersionInfo::CHANNEL_BETA,
     chrome::VersionInfo::CHANNEL_STABLE,
   };
   const Study_Channel study_channels[] = {
     Study_Channel_CANARY,
     Study_Channel_DEV,
@@ skipping 265 matching lines @@
 
   // Store good seed data to test if loading from prefs works.
   TrialsSeed seed = CreateTestSeed();
 
   std::string serialized_seed;
   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);
 
-  VariationsService variations_service;
+  TestVariationsService variations_service(new TestRequestAllowedNotifier);
   TrialsSeed loaded_seed;
   EXPECT_TRUE(
       variations_service.LoadTrialsSeedFromPref(&pref_service, &loaded_seed));
 
   std::string serialized_loaded_seed;
   loaded_seed.SerializeToString(&serialized_loaded_seed);
   // Check that the loaded data is the same as the original.
   EXPECT_EQ(serialized_seed, serialized_loaded_seed);
   // Make sure the pref hasn't been changed.
   EXPECT_FALSE(
@@ skipping 13 matching lines @@
 }
 
 TEST(VariationsServiceTest, StoreSeed) {
   TestingPrefService pref_service;
 
   VariationsService::RegisterPrefs(&pref_service);
   const base::Time now = base::Time::Now();
 
   TrialsSeed seed = CreateTestSeed();
 
-  VariationsService variations_service;
+  TestVariationsService variations_service(new TestRequestAllowedNotifier);
   std::string serialized_seed;
   seed.SerializeToString(&serialized_seed);
   EXPECT_TRUE(
       variations_service.StoreSeedData(serialized_seed, now, &pref_service));
   // Make sure the pref was actually set.
   EXPECT_FALSE(
       pref_service.FindPreference(prefs::kVariationsSeed)->IsDefaultValue());
 
   std::string loaded_serialized_seed =
       pref_service.GetString(prefs::kVariationsSeed);
@@ skipping 78 matching lines @@
       &total_probability);
   ASSERT_TRUE(valid);
   Study_Experiment* repeated_group = study.add_experiment();
   repeated_group->set_name("abc");
   repeated_group->set_probability_weight(1);
   valid = VariationsService::ValidateStudyAndComputeTotalProbability(study,
       &total_probability);
   EXPECT_FALSE(valid);
 }
 
-TEST_F(VariationsServiceNetworkTest, DoNotFetchIfOffline) {
-  SetWasOfflineDuringLastRequestAttempt(true);
-  SimulateNetworkConnectionChange(net::NetworkChangeNotifier::CONNECTION_NONE);
-  EXPECT_FALSE(fetch_attempted());
-}
+TEST(VariationsServiceTest, ResourceRequestAllowedNotifierTest) {
+  MessageLoopForUI message_loop;
+  content::TestBrowserThread ui_thread(content::BrowserThread::UI,
+                                       &message_loop);
 
-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());
+  // Pass ownership to TestVariationsService, but keep a weak pointer to
+  // manipulate it for this test.
+  TestRequestAllowedNotifier* test_notifier = new TestRequestAllowedNotifier;
+  TestVariationsService test_service(test_notifier);
+  test_service.StartRepeatedVariationsSeedFetch();
+  EXPECT_FALSE(test_service.fetch_attempted());

[Alexei Svitkine (slow), 2012/09/24 15:59:36]

Can you add a comment explaining why this is expected to be false? (I assume
because by default, TestRequestAllowedNotifier returns that the network is not
available.)

Can you add a second test to this file that will check what happens if
TestRequestAllowedNotifier will initially return that the network is available?
(In which case, StartRepeatedVariationsSeedFetch() should result in
fetch_attempted() being true.)

[SteveT, 2012/09/24 18:04:27]

I have to push back on this because I believe that the individual criteria were
supposed to be mocked and tested in the RRAN unit tests, and not here in
VariationsServiceTest, which does not care about the more granular criteria
checks.

But this comment brings up an issue in my code: This was succeeding only because
the EULA accepted state was false (hence the first fetch_attempted returned
false). Since I don't want to test EULA (or network) criteria here, the correct
thing to do is to force the first result to be false with
ResourceRequestsAllowedOverride.

(I also think this might have been related to why my non-CrOS tests were failing
- rerunning bots now)

[Alexei Svitkine (slow), 2012/09/24 18:24:20]

Sorry, what I actually meant was:

Can you add a second test to this file that will check what happens if
TestRequestAllowedNotifier will initially return that requests "are allowed"?
(In which case, StartRepeatedVariationsSeedFetch() should result in
fetch_attempted() being true.)

[SteveT, 2012/09/24 18:46:21]

Oh, yes, of course :)

+  test_notifier->NotifyObservers();
+  EXPECT_TRUE(test_service.fetch_attempted());
 }
 
 }  // namespace chrome_variations