Correctly integrate StoragePartition into TestingProfile.

chrome/browser/sync_file_system/local_file_sync_service_unittest.cc

Index: chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
diff --git a/chrome/browser/sync_file_system/local_file_sync_service_unittest.cc b/chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
index 59722dcc81bc03a08b2a6d0247453483f894aed1..35cc430b82002205234dd12507b1b045f64fe49e 100644
--- a/chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
+++ b/chrome/browser/sync_file_system/local_file_sync_service_unittest.cc
@@ -14,6 +14,8 @@
 #include "chrome/browser/sync_file_system/mock_local_change_processor.h"
 #include "chrome/browser/sync_file_system/sync_file_system_test_util.h"
 #include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webkit/browser/fileapi/file_system_context.h"
@@ -27,6 +29,7 @@
 #include "webkit/browser/fileapi/syncable/sync_status_code.h"
 #include "webkit/browser/fileapi/syncable/syncable_file_system_util.h"
 
+using content::BrowserThread;
 using fileapi::FileSystemURL;
 using ::testing::_;
 using ::testing::AtLeast;
@@ -102,12 +105,10 @@ class LocalFileSyncServiceTest
   virtual void SetUp() OVERRIDE {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
 
-    thread_helper_.SetUp();
-
     file_system_.reset(new CannedSyncableFileSystem(
         GURL(kOrigin),
-        thread_helper_.io_task_runner(),
-        thread_helper_.file_task_runner()));
+        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO),
+        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)));
 
     local_service_.reset(new LocalFileSyncService(&profile_));
 
@@ -132,8 +133,6 @@ class LocalFileSyncServiceTest
     local_service_->Shutdown();
     file_system_->TearDown();
     RevokeSyncableFileSystem();
-
-    thread_helper_.TearDown();
   }
 
   // LocalChangeObserver overrides.
@@ -173,11 +172,11 @@ class LocalFileSyncServiceTest
     return file_system_->file_system_context()->change_tracker()->num_changes();
   }
 
+  content::TestBrowserThreadBundle thread_bundle_;

[kinuko, 2013/07/12 04:35:44]

MultiThreadTestHelper internally uses TestBrowserThreadBundle, and I'm not very
sure if this change is really necessary.

This change is also making some tests from multi-threaded to single-threaded,
and thus losing some test coverage.

Does your change require test code which uses TestProfile to run
single-threaded? If not can we continue to use the test helper?

[awong, 2013/07/12 07:17:39]

Nope...it's not required. I thought it was just extra cleanup I could do while
here.
 
Regarding this, my general experience has been that multi-threaded tests don't
actually increase test coverage much unless the test was very specifically
designed to test parallelsim.  I had a long write-up on this in the FAQ here:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/TestBr...
(search for "too bland").  For brevity, the summary is If a test mostly has a
pattern of

  SpawnAsyncTask();
  WaitForTaskCompleteion();
  SpawnNextAsyncTask();
  WaitForTaskCompleteion();

Then there really isn't much parallelism and spawning extra threads just adds
overhead (esp on valgrind) + complexity/flakiness.

At first glance, I didn't see much coverage lost. Having said that, I don't
understand this test super well.  If you tell me that the assertions these tests
are guaranteeing partially rely on having real threads, then I'll believe you
and add it back in. 
Nope...not required to be single threaded.  If you find the helper to be a
meaningful abstraction (regardless of whether we use real threads or not), I can
totally add it back in.  Your code, your call. :)

[kinuko, 2013/07/12 07:28:27]

Right, in most cases I'd agree with you and just use single-thread, but this
test goes through a heavily threaded class under the surface, which chains tasks
spanning on three threads.  And multi-threaded tests actually helped me find out
some edge cases while I'm developing.

Putting aside the fact that such class should be cleaned up in a simpler model
(there's a TODO for that :)), I'd like to keep this test multi-threaded for now.
 (Reg: to use a helper class or not, I don't have strong opinion on it. Now that
we have ThreadBundle the helper class is not adding much)

+
   ScopedEnableSyncFSDirectoryOperation enable_directory_operation_;
   TestingProfile profile_;
 
-  MultiThreadTestHelper thread_helper_;
-
   base::ScopedTempDir temp_dir_;
 
   scoped_ptr<CannedSyncableFileSystem> file_system_;
@@ -277,9 +276,10 @@ TEST_F(LocalFileSyncServiceTest, LocalChangeObserver) {
 
 TEST_F(LocalFileSyncServiceTest, MAYBE_LocalChangeObserverMultipleContexts) {
   const char kOrigin2[] = "http://foo";
-  CannedSyncableFileSystem file_system2(GURL(kOrigin2),
-                                        thread_helper_.io_task_runner(),
-                                        thread_helper_.file_task_runner());
+  CannedSyncableFileSystem file_system2(
+      GURL(kOrigin2),
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO),
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
   file_system2.SetUp();
 
   base::RunLoop run_loop;