[Extensions][TaskScheduler] Update ActivityLog for scheduling migration

chrome/browser/extensions/activity_log/activity_database_unittest.cc

Index: chrome/browser/extensions/activity_log/activity_database_unittest.cc
diff --git a/chrome/browser/extensions/activity_log/activity_database_unittest.cc b/chrome/browser/extensions/activity_log/activity_database_unittest.cc
index cdd657875f9348ef2cc66f504af3fe591d3fc188..ee07d8a554e1e93866948de376d204d2450ba59b 100644
--- a/chrome/browser/extensions/activity_log/activity_database_unittest.cc
+++ b/chrome/browser/extensions/activity_log/activity_database_unittest.cc
@@ -13,10 +13,12 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/test/simple_test_clock.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/activity_log/activity_action_constants.h"
 #include "chrome/browser/extensions/activity_log/activity_database.h"
+#include "chrome/browser/extensions/activity_log/activity_log_task_runner.h"
 #include "chrome/browser/extensions/activity_log/fullstream_ui_policy.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_system.h"
@@ -26,11 +28,24 @@
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/common/dom_action_types.h"
 #include "sql/statement.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using content::BrowserThread;
+#define TEST_ON_ACTIVITY_LOG_RUNNER(test_class, test_name)                 \

[Devlin, 2017/07/12 19:30:36]

gab@, I'm curious what you think of a pattern like this.  It allows us to still
write tests like
TEST_MACRO() {
  <test stuff>
}

But I'm not sure if it's either too magical, or risks some bad interaction with
task scheduling.

+  class test_class##_##test_name##_Runner : public test_class {            \
+   public:                                                                 \
+    void RunTest();                                                        \
+  };                                                                       \
+  TEST_F(test_class##_##test_name##_Runner, test_name) {                   \
+    base::RunLoop run_loop;                                                \
+    GetActivityLogTaskRunner()->PostTask(                                  \
+        FROM_HERE, base::Bind(&test_class##_##test_name##_Runner::RunTest, \
+                              base::Unretained(this)));                    \
+    content::RunAllBlockingPoolTasksUntilIdle();                           \
+  }                                                                        \
+  void test_class##_##test_name##_Runner::RunTest()
 
 namespace constants = activity_log_constants;
 
@@ -134,6 +149,10 @@ class ActivityDatabaseTest : public ChromeRenderViewHostTestHarness {
     db_delegate_->Record(db, action);
   }
 
+  int CountActions(ActivityDatabase* db, const std::string& api_name_pattern) {
+    return CountActions(&db->db_, api_name_pattern);
+  }
+
   int CountActions(sql::Connection* db, const std::string& api_name_pattern) {
     if (!db->DoesTableExist(ActivityDatabaseTestPolicy::kTableName))
       return -1;
@@ -148,12 +167,16 @@ class ActivityDatabaseTest : public ChromeRenderViewHostTestHarness {
     return statement.ColumnInt(0);
   }
 
+  void RecordBatchedActions(ActivityDatabase* db) {
+    db->RecordBatchedActionsWhileTesting();
+  }
+
  private:
   ActivityDatabaseTestPolicy* db_delegate_;
 };
 
 // Check that the database is initialized properly.
-TEST_F(ActivityDatabaseTest, Init) {
+TEST_ON_ACTIVITY_LOG_RUNNER(ActivityDatabaseTest, Init) {
   base::ScopedTempDir temp_dir;
   base::FilePath db_file;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
@@ -170,7 +193,7 @@ TEST_F(ActivityDatabaseTest, Init) {
 }
 
 // Check that actions are recorded in the db.
-TEST_F(ActivityDatabaseTest, RecordAction) {
+TEST_ON_ACTIVITY_LOG_RUNNER(ActivityDatabaseTest, RecordAction) {
   base::ScopedTempDir temp_dir;
   base::FilePath db_file;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
@@ -189,7 +212,7 @@ TEST_F(ActivityDatabaseTest, RecordAction) {
   ASSERT_EQ(1, CountActions(&db, "brewster"));
 }
 
-TEST_F(ActivityDatabaseTest, BatchModeOff) {
+TEST_ON_ACTIVITY_LOG_RUNNER(ActivityDatabaseTest, BatchModeOff) {
   base::ScopedTempDir temp_dir;
   base::FilePath db_file;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
@@ -202,12 +225,12 @@ TEST_F(ActivityDatabaseTest, BatchModeOff) {
 
   scoped_refptr<Action> action = CreateAction(base::Time::Now(), "brewster");
   Record(activity_db, action);
-  ASSERT_EQ(1, CountActions(&activity_db->db_, "brewster"));
+  ASSERT_EQ(1, CountActions(activity_db, "brewster"));
 
   activity_db->Close();
 }
 
-TEST_F(ActivityDatabaseTest, BatchModeOn) {
+TEST_ON_ACTIVITY_LOG_RUNNER(ActivityDatabaseTest, BatchModeOn) {
   base::ScopedTempDir temp_dir;
   base::FilePath db_file;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
@@ -219,17 +242,16 @@ TEST_F(ActivityDatabaseTest, BatchModeOn) {
   activity_db->SetBatchModeForTesting(true);
   scoped_refptr<Action> action = CreateAction(base::Time::Now(), "brewster");
   Record(activity_db, action);
-  ASSERT_EQ(0, CountActions(&activity_db->db_, "brewster"));
+  ASSERT_EQ(0, CountActions(activity_db, "brewster"));
 
   // Artificially trigger and then stop the timer.
-  activity_db->SetTimerForTesting(0);
-  base::RunLoop().RunUntilIdle();

[Devlin, 2017/07/12 19:30:36]

We can remove this here because SetTimerForTesting() just kicked off a timer to
execute RecordBatchedActionsWhileTesting(), so doing that immediately followed
by a RunLoop().RunUntilIdle() was equivalent to just calling the record method
directly.

But, it raised a question.  The reason I changed this was because RunLoops don't
seem to work on other threads (or at least, not whichever thread the task
scheduler put us on).  The migration doc says

"If test code was previously using RunLoop to execute things off the main thread
(as TestBrowserThreadBundle grouped everything under a single MessageLoop),
flushing tasks will now require asking for that explicitly."  Silly question -
how do we "ask for that explicitly"?  The usual suspects of
content::RunAllBlockingPoolTasksUntilIdle() and TaskScheduler::FlushForTesting()
don't work.  Even though we don't strictly need it here, it seems like a useful
thing to be able to do; is there a canonical blocking pool run loop equivalent?

-  ASSERT_EQ(1, CountActions(&activity_db->db_, "brewster"));
+  RecordBatchedActions(activity_db);
+  ASSERT_EQ(1, CountActions(activity_db, "brewster"));
 
   activity_db->Close();
 }
 
-TEST_F(ActivityDatabaseTest, BatchModeFlush) {
+TEST_ON_ACTIVITY_LOG_RUNNER(ActivityDatabaseTest, BatchModeFlush) {
   base::ScopedTempDir temp_dir;
   base::FilePath db_file;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
@@ -241,17 +263,17 @@ TEST_F(ActivityDatabaseTest, BatchModeFlush) {
   activity_db->SetBatchModeForTesting(true);
   scoped_refptr<Action> action = CreateAction(base::Time::Now(), "brewster");
   Record(activity_db, action);
-  ASSERT_EQ(0, CountActions(&activity_db->db_, "brewster"));
+  ASSERT_EQ(0, CountActions(activity_db, "brewster"));
 
   // Request an immediate database flush.
   activity_db->AdviseFlush(ActivityDatabase::kFlushImmediately);
-  ASSERT_EQ(1, CountActions(&activity_db->db_, "brewster"));
+  ASSERT_EQ(1, CountActions(activity_db, "brewster"));
 
   activity_db->Close();
 }
 
 // Check that nothing explodes if the DB isn't initialized.
-TEST_F(ActivityDatabaseTest, InitFailure) {
+TEST_ON_ACTIVITY_LOG_RUNNER(ActivityDatabaseTest, InitFailure) {
   base::ScopedTempDir temp_dir;
   base::FilePath db_file;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());