Make BackgroundBudgetService calls asynchronous.

chrome/browser/budget_service/background_budget_service_unittest.cc

 // Copyright 2016 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 <stdint.h>
 #include <string>
 
 #include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
 #include "base/test/simple_test_clock.h"
 #include "chrome/browser/budget_service/background_budget_service.h"
 #include "chrome/browser/budget_service/background_budget_service_factory.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
 const char kTestOrigin[] = "https://example.com";
 const double kTestBudget = 10.0;
 const double kTestSES = 48.0;
 const double kLowSES = 1.0;
 const double kMaxSES = 100.0;
 // Mirrors definition in BackgroundBudgetService, this is 10 days of seconds.
 const double kSecondsToAccumulate = 864000.0;
 
 }  // namespace
 
 class BackgroundBudgetServiceTest : public testing::Test {
  public:
-  BackgroundBudgetServiceTest() {}
+  BackgroundBudgetServiceTest() : budget_(0.0) {}
   ~BackgroundBudgetServiceTest() override {}
 
   BackgroundBudgetService* GetService() {
     return BackgroundBudgetServiceFactory::GetForProfile(&profile_);
   }
 
   void SetSiteEngagementScore(const GURL& url, double score) {
     SiteEngagementService* service = SiteEngagementService::Get(&profile_);
     service->ResetScoreForURL(url, score);
   }
 
   Profile* profile() { return &profile_; }
 
   base::SimpleTestClock* SetClockForTesting() {
     base::SimpleTestClock* clock = new base::SimpleTestClock();
     BackgroundBudgetServiceFactory::GetForProfile(&profile_)
         ->SetClockForTesting(base::WrapUnique(clock));
     return clock;
   }
 
+  double GetBudget() {
+    const GURL origin(kTestOrigin);
+    base::RunLoop run_loop;
+    GetService()->GetBudget(
+        origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                           base::Unretained(this), run_loop.QuitClosure()));
+    run_loop.Run();
+    return budget_;
+  }
+
+  void GotBudget(base::Closure run_loop_closure, double budget) {
+    budget_ = budget;
+    run_loop_closure.Run();
+  }
+
+  void StoreBudget(double budget) {
+    const GURL origin(kTestOrigin);

[johnme, 2016/06/20 13:35:22]

Nit: perhaps simpler to just pass GURL(kTestOrigin) as a parameter to
StoreBudget. Ditto elsewhere. Up to you.

[harkness, 2016/06/20 14:05:08]

Putting it here allowed me to remove it from a bunch of the tests, so I'm going
to leave it here.

+    std::unique_ptr<BackgroundBudgetService> service(
+        new BackgroundBudgetService(profile()));

[johnme, 2016/06/20 13:35:22]

1. No need for unique_ptr here (`BackgroundBudgetService service(profile_);`
should be equivalent).

2. Why do you create a new instance of the service here, rather than using
GetService to fetch the singleton (per profile anyway) instance from the
factory?

[harkness, 2016/06/20 14:05:08]

Yeah, I had that in one of my test cases, probably left over from
pre-KeyedServiceFactory days, and that happened to be the one I copy/pasted.
oops.

+    base::RunLoop run_loop;
+    service->StoreBudget(origin, budget, run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  // Budget for callbacks to set.
+  double budget_;
+
  private:
   content::TestBrowserThreadBundle thread_bundle_;
   TestingProfile profile_;
 };
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetNoBudgetOrSES) {
-  const GURL origin(kTestOrigin);
-
-  double budget = GetService()->GetBudget(origin);
-
+  double budget = GetBudget();

[johnme, 2016/06/20 13:35:22]

Nit: I guess you can just inline this into the EXPECT_. Ditto with other calls
to getBudget (you never use the return value more than once).

[harkness, 2016/06/20 14:05:08]

I swapped them in calls where it was only used once, but left the assignment in
the methods where I did test it multiple times. In those tests, I left all
checks to be on budget for consistency.

   EXPECT_DOUBLE_EQ(budget, 0.0);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetNoBudgetSESExists) {
+  // Set a starting SES for the url but no stored budget info.
   const GURL origin(kTestOrigin);
-
-  // Set a starting SES for the url but no stored budget info.
   SetSiteEngagementScore(origin, kTestSES);
 
-  double budget = GetService()->GetBudget(origin);
-
+  double budget = GetBudget();
   EXPECT_DOUBLE_EQ(budget, kTestSES);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetNoElapsedTime) {
-  const GURL origin(kTestOrigin);
-
-  std::unique_ptr<BackgroundBudgetService> service(
-      new BackgroundBudgetService(profile()));
-
-  service->StoreBudget(origin, kTestBudget);
-
-  double budget = service->GetBudget(origin);
-
+  StoreBudget(kTestBudget);
+  double budget = GetBudget();
   EXPECT_NEAR(budget, kTestBudget, kTestBudget);

[johnme, 2016/06/20 13:35:22]

The 3rd parameter to EXPECT_NEAR is the allowable absolute error. `kTestBudget`
doesn't seem like a reasonable value for this? Why not just use EXPECT_DOUBLE_EQ
here (and many of the other places you use EXPECT_NEAR) for consistency?

[harkness, 2016/06/20 14:05:08]

You're right, this one had a bad bound on it. The other ones actually have
reasonable bounds, so I converted this one, but left the others.

 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetElapsedTime) {
   // Manually construct a BackgroundBudgetServie with a clock that the test
   // can control so that we can fast forward in time.
   BackgroundBudgetService* service = GetService();
   base::SimpleTestClock* clock = SetClockForTesting();
   base::Time starting_time = clock->Now();
 
   // Set initial SES and budget values.
   const GURL origin(kTestOrigin);
   SetSiteEngagementScore(origin, kTestSES);
-  service->StoreBudget(origin, kTestBudget);
+  std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop());
+  service->StoreBudget(origin, kTestBudget, run_loop->QuitClosure());
+  run_loop->Run();
 
-  double budget = service->GetBudget(origin);
-  EXPECT_DOUBLE_EQ(budget, kTestBudget);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(

[johnme, 2016/06/20 13:35:22]

It seems like you can use the helper methods here as well. Ditto in
GetBudgetConsumedOverTime and GetBudgetNegativeTime.

[harkness, 2016/06/20 14:05:08]

Done.

+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_DOUBLE_EQ(budget_, kTestBudget);
 
   // Query for the budget after 1 second has passed.
   clock->SetNow(starting_time + base::TimeDelta::FromSeconds(1));
-  budget = service->GetBudget(origin);
-  EXPECT_NEAR(budget, kTestBudget, kTestSES * 1.0 / kSecondsToAccumulate);
-  EXPECT_GT(budget, kTestBudget);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_NEAR(budget_, kTestBudget, kTestSES * 1.0 / kSecondsToAccumulate);
+  EXPECT_GT(budget_, kTestBudget);
 
   // Query for the budget after 1 hour has passed.
   clock->SetNow(starting_time + base::TimeDelta::FromHours(1));
-  budget = service->GetBudget(origin);
-  EXPECT_NEAR(budget, kTestBudget, kTestSES * 3600.0 / kSecondsToAccumulate);
-  EXPECT_GT(budget, kTestBudget);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_NEAR(budget_, kTestBudget, kTestSES * 3600.0 / kSecondsToAccumulate);

[johnme, 2016/06/20 13:35:22]

This is quite an elaborate calculation for the allowable absolute error - could
you add an explanatory comment?

[harkness, 2016/06/20 14:05:08]

Updated it to be a more bounds-based calculation.

+  EXPECT_GT(budget_, kTestBudget);
 
   // Query for the budget after 5 days have passed. The budget should be
   // increasing, but not up the SES score.
   clock->SetNow(starting_time + base::TimeDelta::FromDays(5));
-  budget = service->GetBudget(origin);
-  EXPECT_GT(budget, kTestBudget);
-  EXPECT_LT(budget, kTestSES);
-  double moderate_ses_budget = budget;
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_GT(budget_, kTestBudget);
+  EXPECT_LT(budget_, kTestSES);
+  double moderate_ses_budget = budget_;
 
   // Query for the budget after 10 days have passed. By this point, the budget
   // should converge to the SES score.
   clock->SetNow(starting_time + base::TimeDelta::FromDays(10));
-  budget = service->GetBudget(origin);
-  EXPECT_DOUBLE_EQ(budget, kTestSES);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_DOUBLE_EQ(budget_, kTestSES);
 
   // Now, change the SES score to the maximum amount and reinitialize budget.
   SetSiteEngagementScore(origin, kMaxSES);
-  service->StoreBudget(origin, kTestBudget);
+  run_loop.reset(new base::RunLoop());
+  service->StoreBudget(origin, kTestBudget, run_loop->QuitClosure());
+  run_loop->Run();
+
   starting_time = clock->Now();
 
   // Query for the budget after 1 second has passed.
   clock->SetNow(starting_time + base::TimeDelta::FromSeconds(1));
-  budget = service->GetBudget(origin);
-  EXPECT_NEAR(budget, kTestBudget, kMaxSES * 1.0 / kSecondsToAccumulate);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_NEAR(budget_, kTestBudget, kMaxSES * 1.0 / kSecondsToAccumulate);
 
   // Query for the budget after 5 days have passed. Again, the budget should be
   // approaching the SES, but not have reached it.
   clock->SetNow(starting_time + base::TimeDelta::FromDays(5));
-  budget = service->GetBudget(origin);
-  EXPECT_GT(budget, kTestBudget);
-  EXPECT_LT(budget, kMaxSES);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_GT(budget_, kTestBudget);
+  EXPECT_LT(budget_, kMaxSES);
 
   // The budget after 5 days with max SES should be greater than the budget
   // after 5 days with moderate SES.
-  EXPECT_GT(budget, moderate_ses_budget);
+  EXPECT_GT(budget_, moderate_ses_budget);
 
   // Now, change the SES score to a low amount and reinitialize budget.
   SetSiteEngagementScore(origin, kLowSES);
-  service->StoreBudget(origin, kTestBudget);
+  run_loop.reset(new base::RunLoop());
+  service->StoreBudget(origin, kTestBudget, run_loop->QuitClosure());
+  run_loop->Run();
   starting_time = clock->Now();
 
   // Query for the budget after 5 days have passed. Again, the budget should be
   // approaching the SES, this time decreasing, but not have reached it.
   clock->SetNow(starting_time + base::TimeDelta::FromDays(5));
-  budget = service->GetBudget(origin);
-  EXPECT_LT(budget, kTestBudget);
-  EXPECT_GT(budget, kLowSES);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_LT(budget_, kTestBudget);
+  EXPECT_GT(budget_, kLowSES);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetConsumedOverTime) {
   // Manually construct a BackgroundBudgetService with a clock that the test
   // can control so that we can fast forward in time.
   BackgroundBudgetService* service = GetService();
   base::SimpleTestClock* clock = SetClockForTesting();
 
   // Set initial SES and budget values.
   const GURL origin(kTestOrigin);
   SetSiteEngagementScore(origin, kTestSES);
-  service->StoreBudget(origin, kTestBudget);
-  double budget = 0.0;
+  std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop());
+  service->StoreBudget(origin, kTestBudget, run_loop->QuitClosure());
+  run_loop->Run();
 
   // Measure over 200 hours. In each hour a message is received, and for 1 in
   // 10, budget is consumed.
   for (int i = 0; i < 200; i++) {
     // Query for the budget after 1 hour has passed.
     clock->Advance(base::TimeDelta::FromHours(1));
-    budget = service->GetBudget(origin);
+    run_loop.reset(new base::RunLoop());
+    GetService()->GetBudget(
+        origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                           base::Unretained(this), run_loop->QuitClosure()));
+    run_loop->Run();
 
     if (i % 10 == 0) {
       double cost = BackgroundBudgetService::GetCost(
           BackgroundBudgetService::CostType::SILENT_PUSH);
-      service->StoreBudget(origin, budget - cost);
+      run_loop.reset(new base::RunLoop());
+      service->StoreBudget(origin, budget_ - cost, run_loop->QuitClosure());
+      run_loop->Run();
     }
   }
 
   // With a SES of 48.0, the origin will get a budget of 2.4 per day, but the
   // old budget will also decay. At the end, we expect the budget to be lower
   // than the starting budget.
-  EXPECT_GT(budget, 0.0);
-  EXPECT_LT(budget, kTestBudget);
+  EXPECT_GT(budget_, 0.0);
+  EXPECT_LT(budget_, kTestBudget);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetInvalidBudget) {
   const GURL origin(kTestOrigin);
 
   // Set a starting SES for the url.
   SetSiteEngagementScore(origin, kTestSES);
 
   // Set a badly formatted budget in the user preferences.
   DictionaryPrefUpdate update(profile()->GetPrefs(),
                               prefs::kBackgroundBudgetMap);
   base::DictionaryValue* update_map = update.Get();
   update_map->SetStringWithoutPathExpansion(origin.spec(), "20#2.0");
 
   // Get the budget, expect that it will return SES.
-  double budget = GetService()->GetBudget(origin);
-
+  double budget = GetBudget();
   EXPECT_DOUBLE_EQ(budget, kTestSES);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetNegativeTime) {
   // Manually construct a BackgroundBudgetService with a clock that the test
   // can control so that we can fast forward in time.
   BackgroundBudgetService* service = GetService();
   base::SimpleTestClock* clock = SetClockForTesting();
   base::Time starting_time = clock->Now();
 
   // Set initial SES and budget values.
   const GURL origin(kTestOrigin);
   SetSiteEngagementScore(origin, kTestSES);
-  service->StoreBudget(origin, kTestBudget);
+  std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop());
+  service->StoreBudget(origin, kTestBudget, run_loop->QuitClosure());
+  run_loop->Run();
 
   // Move time forward an hour and get the budget.
   clock->SetNow(starting_time + base::TimeDelta::FromHours(1));
-  double budget = service->GetBudget(origin);
-  service->StoreBudget(origin, budget);
-  EXPECT_NE(kTestBudget, budget);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+
+  // Store the updated budget.
+  run_loop.reset(new base::RunLoop());
+  service->StoreBudget(origin, budget_, run_loop->QuitClosure());
+  run_loop->Run();
+  EXPECT_NE(kTestBudget, budget_);
+  double original_budget = budget_;
 
   // Now move time backwards a day and make sure that the current
   // budget matches the budget of the most foward time.
   clock->SetNow(starting_time - base::TimeDelta::FromDays(1));
-  double back_budget = service->GetBudget(origin);
-  EXPECT_NEAR(budget, back_budget, 0.01);
+  run_loop.reset(new base::RunLoop());
+  GetService()->GetBudget(
+      origin, base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                         base::Unretained(this), run_loop->QuitClosure()));
+  run_loop->Run();
+  EXPECT_NEAR(original_budget, budget_, 0.01);
 }