Make BackgroundBudgetService calls asynchronous.

chrome/browser/budget_service/background_budget_service_unittest.cc

Index: chrome/browser/budget_service/background_budget_service_unittest.cc
diff --git a/chrome/browser/budget_service/background_budget_service_unittest.cc b/chrome/browser/budget_service/background_budget_service_unittest.cc
index 98ee3dffd1c31561276982627bb305b26d3d05aa..81091892d2687a95eb823c274b2d736457a7d0af 100644
--- a/chrome/browser/budget_service/background_budget_service_unittest.cc
+++ b/chrome/browser/budget_service/background_budget_service_unittest.cc
@@ -6,6 +6,7 @@
 #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"
@@ -31,7 +32,7 @@ const double kSecondsToAccumulate = 864000.0;
 
 class BackgroundBudgetServiceTest : public testing::Test {
  public:
-  BackgroundBudgetServiceTest() {}
+  BackgroundBudgetServiceTest() : budget_(0.0) {}
   ~BackgroundBudgetServiceTest() override {}
 
   BackgroundBudgetService* GetService() {
@@ -52,17 +53,47 @@ class BackgroundBudgetServiceTest : public testing::Test {
     return clock;
   }
 
+  void GotBudget(int id, double budget) {
+    budget_ = budget;
+    last_callback_ = id;
+    if (run_loop_)
+      run_loop_->Quit();
+  }
+  void StoredBudget(int id) {

[Michael van Ouwerkerk, 2016/06/10 12:10:19]

nit: add a blank line above this one

[harkness, 2016/06/10 14:52:57]

Done.

+    last_callback_ = id;
+    if (run_loop_)
+      run_loop_->Quit();
+  }
+
+  void WaitForCallback(int id) {
+    if (last_callback_ != id) {
+      run_loop_.reset(new base::RunLoop);
+      run_loop_->Run();
+    }
+  }
+
+  // Budget for callbacks to set.
+  double budget_;
+
+  // ID of the last callback processed.
+  int last_callback_;
+
  private:
   content::TestBrowserThreadBundle thread_bundle_;
   TestingProfile profile_;
+
+  std::unique_ptr<base::RunLoop> run_loop_;
 };
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetNoBudgetOrSES) {
   const GURL origin(kTestOrigin);
 
-  double budget = GetService()->GetBudget(origin);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), 1));
+  WaitForCallback(1);

[Michael van Ouwerkerk, 2016/06/10 12:10:19]

I think we should be able to do this without passing around callback ids, and
storing last_callback_ and run_loop_:

base::RunLoop run_loop;
GetService()->GetBudget(origin,
base::Bind(&BackgroundBudgetServiceTest::GotBudget, base::Unretained(this),
run_loop.QuitClosure()));
run_loop.Run();

...and run the quit closure in GotBudget.

[harkness, 2016/06/10 14:52:56]

Updated the code as discussed in person.

 
-  EXPECT_DOUBLE_EQ(budget, 0.0);
+  EXPECT_DOUBLE_EQ(budget_, 0.0);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetNoBudgetSESExists) {
@@ -71,9 +102,12 @@ TEST_F(BackgroundBudgetServiceTest, GetBudgetNoBudgetSESExists) {
   // Set a starting SES for the url but no stored budget info.
   SetSiteEngagementScore(origin, kTestSES);
 
-  double budget = GetService()->GetBudget(origin);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), 1));
+  WaitForCallback(1);
 
-  EXPECT_DOUBLE_EQ(budget, kTestSES);
+  EXPECT_DOUBLE_EQ(budget_, kTestSES);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetNoElapsedTime) {
@@ -82,11 +116,17 @@ TEST_F(BackgroundBudgetServiceTest, GetBudgetNoElapsedTime) {
   std::unique_ptr<BackgroundBudgetService> service(
       new BackgroundBudgetService(profile()));
 
-  service->StoreBudget(origin, kTestBudget);
+  service->StoreBudget(origin, kTestBudget,
+                       base::Bind(&BackgroundBudgetServiceTest::StoredBudget,
+                                  base::Unretained(this), 1));
+  WaitForCallback(1);
 
-  double budget = service->GetBudget(origin);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), 2));
+  WaitForCallback(2);
 
-  EXPECT_NEAR(budget, kTestBudget, kTestBudget);
+  EXPECT_NEAR(budget_, kTestBudget, kTestBudget);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetElapsedTime) {
@@ -95,73 +135,119 @@ TEST_F(BackgroundBudgetServiceTest, GetBudgetElapsedTime) {
   BackgroundBudgetService* service = GetService();
   base::SimpleTestClock* clock = SetClockForTesting();
   base::Time starting_time = clock->Now();
+  int callback_id = 1;
 
   // Set initial SES and budget values.
   const GURL origin(kTestOrigin);
   SetSiteEngagementScore(origin, kTestSES);
-  service->StoreBudget(origin, kTestBudget);
-
-  double budget = service->GetBudget(origin);
-  EXPECT_DOUBLE_EQ(budget, kTestBudget);
+  service->StoreBudget(origin, kTestBudget,
+                       base::Bind(&BackgroundBudgetServiceTest::StoredBudget,
+                                  base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+  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);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+  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);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+  EXPECT_NEAR(budget_, kTestBudget, kTestSES * 3600.0 / kSecondsToAccumulate);
+  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;
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+  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);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+  EXPECT_DOUBLE_EQ(budget_, kTestSES);
 
   // Now, change the SES score to the maximum amount and reinitialize budget.
   SetSiteEngagementScore(origin, kMaxSES);
-  service->StoreBudget(origin, kTestBudget);
+  service->StoreBudget(origin, kTestBudget,
+                       base::Bind(&BackgroundBudgetServiceTest::StoredBudget,
+                                  base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+
   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);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+  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);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+  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);
+  service->StoreBudget(origin, kTestBudget,
+                       base::Bind(&BackgroundBudgetServiceTest::StoredBudget,
+                                  base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
   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);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
+  EXPECT_LT(budget_, kTestBudget);
+  EXPECT_GT(budget_, kLowSES);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetConsumedOverTime) {
@@ -169,32 +255,45 @@ TEST_F(BackgroundBudgetServiceTest, GetBudgetConsumedOverTime) {
   // can control so that we can fast forward in time.
   BackgroundBudgetService* service = GetService();
   base::SimpleTestClock* clock = SetClockForTesting();
+  int callback_id = 1;
 
   // Set initial SES and budget values.
   const GURL origin(kTestOrigin);
   SetSiteEngagementScore(origin, kTestSES);
-  service->StoreBudget(origin, kTestBudget);
-  double budget = 0.0;
+  service->StoreBudget(origin, kTestBudget,
+                       base::Bind(&BackgroundBudgetServiceTest::StoredBudget,
+                                  base::Unretained(this), callback_id));
+  WaitForCallback(callback_id);
+  callback_id++;
 
   // 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);
+    GetService()->GetBudget(origin,
+                            base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                       base::Unretained(this), callback_id));
+    WaitForCallback(callback_id);
+    callback_id++;
 
     if (i % 10 == 0) {
       double cost = BackgroundBudgetService::GetCost(
           BackgroundBudgetService::CostType::SILENT_PUSH);
-      service->StoreBudget(origin, budget - cost);
+      service->StoreBudget(
+          origin, budget_ - cost,
+          base::Bind(&BackgroundBudgetServiceTest::StoredBudget,
+                     base::Unretained(this), callback_id));
+      WaitForCallback(callback_id);
+      callback_id++;
     }
   }
 
   // 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) {
@@ -210,9 +309,12 @@ TEST_F(BackgroundBudgetServiceTest, GetBudgetInvalidBudget) {
   update_map->SetStringWithoutPathExpansion(origin.spec(), "20#2.0");
 
   // Get the budget, expect that it will return SES.
-  double budget = GetService()->GetBudget(origin);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), 1));
+  WaitForCallback(1);
 
-  EXPECT_DOUBLE_EQ(budget, kTestSES);
+  EXPECT_DOUBLE_EQ(budget_, kTestSES);
 }
 
 TEST_F(BackgroundBudgetServiceTest, GetBudgetNegativeTime) {
@@ -225,17 +327,32 @@ TEST_F(BackgroundBudgetServiceTest, GetBudgetNegativeTime) {
   // Set initial SES and budget values.
   const GURL origin(kTestOrigin);
   SetSiteEngagementScore(origin, kTestSES);
-  service->StoreBudget(origin, kTestBudget);
+  service->StoreBudget(origin, kTestBudget,
+                       base::Bind(&BackgroundBudgetServiceTest::StoredBudget,
+                                  base::Unretained(this), 1));
+  WaitForCallback(1);
 
   // 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);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), 2));
+  WaitForCallback(2);
+
+  // Store the updated budget.
+  service->StoreBudget(origin, budget_,
+                       base::Bind(&BackgroundBudgetServiceTest::StoredBudget,
+                                  base::Unretained(this), 3));
+  WaitForCallback(3);
+  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);
+  GetService()->GetBudget(origin,
+                          base::Bind(&BackgroundBudgetServiceTest::GotBudget,
+                                     base::Unretained(this), 4));
+  WaitForCallback(4);
+  EXPECT_NEAR(original_budget, budget_, 0.01);
 }