Have the VariationsService attempt to fetch the seed when an update is ready.

chrome/browser/metrics/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_service.h"
+#include "chrome/browser/upgrade_detector.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_pref_service.h"
+#include "content/public/browser/notification_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() : fetch_attempted_(false) {}
+  virtual ~TestVariationsService() {}
+
+  bool fetch_attempted() { return fetch_attempted_; }
+
+  // Simulate that an auto-update is ready by sending the appropriate
+  // notification to the Observe method. Note that the source and details are
+  // not necessary for this notification type.
+  void SimulateUpgradeAvailable() {
+    Observe(chrome::NOTIFICATION_UPGRADE_RECOMMENDED,
+        content::Source<UpgradeDetector>(NULL),
+        content::NotificationService::NoDetails());

[Ilya Sherman, 2012/08/07 20:31:51]

nit: This seems a little odd.  Why not dispatch the notification in the usual
way, and have coverage for being registered for the notification as well?

[SteveT, 2012/08/08 18:11:39]

That makes way more sense, and yes, I do like the extra bit of coverage we get
from that. Resolved this. Thanks for the suggestion!

+  }
+
+ protected:
+  virtual void FetchVariationsSeed() {

[Ilya Sherman, 2012/08/07 20:31:51]

nit: OVERRIDE

[SteveT, 2012/08/08 18:11:39]

Done.

+    fetch_attempted_ = true;
+  }
+
+ private:
+  bool fetch_attempted_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestVariationsService);
+};
+
 // 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.
 chrome_variations::TrialsSeed CreateTestSeed() {
   chrome_variations::TrialsSeed seed;
   chrome_variations::Study* study = seed.add_study();
   study->set_name("test");
   study->set_default_experiment_name("abc");
   chrome_variations::Study_Experiment* experiment = study->add_experiment();
   experiment->set_name("abc");
   experiment->set_probability_weight(100);
   return seed;
 }
 
 }  // namespace
 
+TEST(VariationsServiceTest, AttemptFetchOnAutoUpdate) {
+  // Simulate an auto-update and ensure that the VariationsService attempts
+  // to fetch the variations seed.
+  MessageLoopForUI message_loop;
+  content::TestBrowserThread ui_thread(content::BrowserThread::UI,
+                                       &message_loop);

[Ilya Sherman, 2012/08/07 20:31:51]

nit: Why is this needed?  If it's just to satisfy the DCHECK, perhaps it would
be cleaner to have the VariationsService inherit from base::NonThreadSafe and
replace the check with DCHECK(CalledOnValidThread())?

[SteveT, 2012/08/08 18:11:39]

Yes, this was just to satisfy the DCHECKs on BrowserThread::UI in
variations_service.cc. I replaced those checks with NonThreadSafe and
CalledOnValidThread, which I agree is cleaner.

However, now that we've started calling into UpgradeDetector::GetInstance, we
need to keep this TestBrowserThread around, as UpgradeDetector's init code
requires it (or else _it_ DCHECKs). Does this seem fine to you? I've seen
creating TestBrowserThreads for testing similar classes (that run on a specific
Thread) as a common pattern. Is this something we'd like to move away from?

[Ilya Sherman, 2012/08/08 21:33:47]

It's an ok pattern; it's just often overkill and adds a bit of boilerplate noise
to the test implementation, so I prefer to remove it when it's not really
needed.  If it's needed for the UpgradeDetector code in this case, then that's
fine.

+  TestVariationsService test_service;
+  ASSERT_FALSE(test_service.fetch_attempted());

[Ilya Sherman, 2012/08/07 20:31:51]

nit: EXPECT_FALSE

[SteveT, 2012/08/08 18:11:39]

Done.

+  test_service.SimulateUpgradeAvailable();
+  ASSERT_TRUE(test_service.fetch_attempted());

[Ilya Sherman, 2012/08/07 20:31:51]

nit: EXPECT_TRUE

[SteveT, 2012/08/08 18:11:39]

Done.

+}
+
 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 399 matching lines @@
   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);
 }
 
 }  // namespace chrome_variations