Ensure tests have an active task runner

chrome/browser/background/background_mode_manager_unittest.cc

Index: chrome/browser/background/background_mode_manager_unittest.cc
diff --git a/chrome/browser/background/background_mode_manager_unittest.cc b/chrome/browser/background/background_mode_manager_unittest.cc
index 7b5850325e73e3af1c3fe55b10043225e9df126c..82f8cb0a39b8ec73d84f7932eb2d0878ea56383f 100644
--- a/chrome/browser/background/background_mode_manager_unittest.cc
+++ b/chrome/browser/background/background_mode_manager_unittest.cc
@@ -7,6 +7,8 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/thread_task_runner_handle.h"
 #include "chrome/browser/background/background_mode_manager.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
@@ -44,6 +46,13 @@ using testing::StrictMock;
 
 namespace {
 
+scoped_ptr<TestingProfileManager> CreateTestingProfileManager() {
+  scoped_ptr<TestingProfileManager> profile_manager(
+      new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
+  EXPECT_TRUE(profile_manager->SetUp());
+  return profile_manager.Pass();
+}
+
 // Helper class that tracks state transitions in BackgroundModeManager and
 // exposes them via getters (or gmock for EnableLaunchOnStartup).
 class TestBackgroundModeManager : public StrictMock<BackgroundModeManager> {
@@ -146,7 +155,8 @@ class AdvancedTestBackgroundModeManager : public TestBackgroundModeManager {
 
 class BackgroundModeManagerTest : public testing::Test {
  public:
-  BackgroundModeManagerTest() {}
+  BackgroundModeManagerTest()
+      : dummy_task_runner_(new base::TestSimpleTaskRunner()) {}
   ~BackgroundModeManagerTest() override {}
 
   void SetUp() override {
@@ -156,25 +166,10 @@ class BackgroundModeManagerTest : public testing::Test {
   }
 
  protected:
-  scoped_refptr<extensions::Extension> CreateExtension(
-      extensions::Manifest::Location location,
-      const std::string& data,
-      const std::string& id) {
-    scoped_ptr<base::DictionaryValue> parsed_manifest(
-        extensions::api_test_utils::ParseDictionary(data));
-    return extensions::api_test_utils::CreateExtension(
-        location, parsed_manifest.get(), id);
-  }
-
-  // From views::MenuModelAdapter::IsCommandEnabled with modification.
-  bool IsCommandEnabled(ui::MenuModel* model, int id) const {
-    int index = 0;
-    if (ui::MenuModel::GetModelAndIndexForCommandId(id, &model, &index))
-      return model->IsEnabledAt(index);
-
-    return false;
-  }
-
+  // We use a dummy task runner instead of the full test browser thread bundle
+  // to avoid having the browser's shutdown logic try to access a non-existent

[Andrew T Wilson (Slow), 2015/04/22 14:48:03]

I guess I'm not sure I understand - are you saying that there are shutdown tasks
that this dummy_task_runner_ won't execute? I'm not 100% clear why this class
gets a dummy_task_runner_ but BackgroundModeManagerWithExtensionTest does not?

[Sami, 2015/04/22 18:06:45]

Disclaimer: that was just the first thing I managed to make work -- I don't
actually know anything about these tests.

I've now done some more digging and the underlying problem is that
chrome::DecrementKeepAliveCount() uses the absence of a MessageLoop to infer
that it's in a unit test and shouldn't do a real browser shutdown. Since my
patch is adding a MessageLoop, things go wrong when the shutdown sequence is
trying to touch things that aren't there.

Unfortunately I can't seem to find the set of objects that actually need to be
around for the shutdown to work (MessageCenter is enough for everything except
CrOS which wants an ash::Shell). BackgroundModeManagerWithExtensionTest works
around this by artificially incrementing the browser keepalive count to prevent
the shutdown, but we can't really do that in BackgroundModeManagerTest because
it is explicitly checking what the current keepalive count is.

The best solution I could think of is to add a
chrome::DisableShutdownForTesting() that this test can use to keep its existing
behavior. WDYT?

+  // objects such as the MessageCenter during teardown.
+  base::ThreadTaskRunnerHandle dummy_task_runner_;
   scoped_ptr<base::CommandLine> command_line_;
 
   scoped_ptr<TestingProfileManager> profile_manager_;
@@ -182,24 +177,18 @@ class BackgroundModeManagerTest : public testing::Test {
   TestingProfile* profile_;
 
  private:
-  scoped_ptr<TestingProfileManager> CreateTestingProfileManager() {
-    scoped_ptr<TestingProfileManager> profile_manager
-        (new TestingProfileManager(TestingBrowserProcess::GetGlobal()));
-    EXPECT_TRUE(profile_manager->SetUp());
-    return profile_manager.Pass();
-  }
-
   DISALLOW_COPY_AND_ASSIGN(BackgroundModeManagerTest);
 };
 
-class BackgroundModeManagerWithExtensionsTest
-    : public BackgroundModeManagerTest {
+class BackgroundModeManagerWithExtensionsTest : public testing::Test {
  public:
   BackgroundModeManagerWithExtensionsTest() {}
   ~BackgroundModeManagerWithExtensionsTest() override {}
 
   void SetUp() override {
-    BackgroundModeManagerTest::SetUp();
+    command_line_.reset(new base::CommandLine(base::CommandLine::NO_PROGRAM));
+    profile_manager_ = CreateTestingProfileManager();
+    profile_ = profile_manager_->CreateTestingProfile("p1");
 
     // Aura clears notifications from the message center at shutdown.
     message_center::MessageCenter::Initialize();
@@ -266,6 +255,25 @@ class BackgroundModeManagerWithExtensionsTest
   }
 
  protected:
+  scoped_refptr<extensions::Extension> CreateExtension(
+      extensions::Manifest::Location location,
+      const std::string& data,
+      const std::string& id) {
+    scoped_ptr<base::DictionaryValue> parsed_manifest(
+        extensions::api_test_utils::ParseDictionary(data));
+    return extensions::api_test_utils::CreateExtension(
+        location, parsed_manifest.get(), id);
+  }
+
+  // From views::MenuModelAdapter::IsCommandEnabled with modification.
+  bool IsCommandEnabled(ui::MenuModel* model, int id) const {
+    int index = 0;
+    if (ui::MenuModel::GetModelAndIndexForCommandId(id, &model, &index))
+      return model->IsEnabledAt(index);
+
+    return false;
+  }
+
   void AddEphemeralApp(const extensions::Extension* extension,
                        ExtensionService* service) {
     extensions::ExtensionPrefs* prefs =
@@ -282,6 +290,12 @@ class BackgroundModeManagerWithExtensionsTest
 
   scoped_ptr<TestBackgroundModeManager> manager_;
 
+  scoped_ptr<base::CommandLine> command_line_;
+
+  scoped_ptr<TestingProfileManager> profile_manager_;
+  // Test profile used by all tests - this is owned by profile_manager_.
+  TestingProfile* profile_;
+
  private:
   // Required for extension service.
   content::TestBrowserThreadBundle thread_bundle_;