Correctly integrate StoragePartition into TestingProfile.

chrome/browser/sync_file_system/local_file_sync_service_unittest.cc

 // Copyright (c) 2012 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 "base/basictypes.h"
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/location.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/sync_file_system/local_file_sync_service.h"
 #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"
 #include "webkit/browser/fileapi/syncable/canned_syncable_file_system.h"
 #include "webkit/browser/fileapi/syncable/file_change.h"
 #include "webkit/browser/fileapi/syncable/local_file_change_tracker.h"
 #include "webkit/browser/fileapi/syncable/local_file_sync_context.h"
 #include "webkit/browser/fileapi/syncable/local_file_sync_status.h"
 #include "webkit/browser/fileapi/syncable/mock_sync_status_observer.h"
 #include "webkit/browser/fileapi/syncable/sync_file_metadata.h"
 #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;
 using ::testing::InvokeWithoutArgs;
 using ::testing::StrictMock;
 
 namespace sync_file_system {
 
 namespace {
 
@@ skipping 55 matching lines @@
     : public testing::Test,
       public LocalFileSyncService::Observer {
  protected:
   LocalFileSyncServiceTest() : num_changes_(0) {}
 
   virtual ~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_));
 
     file_system_->SetUp();
 
     base::RunLoop run_loop;
     SyncStatusCode status = SYNC_STATUS_UNKNOWN;
     local_service_->MaybeInitializeFileSystemContext(
         GURL(kOrigin), file_system_->file_system_context(),
         AssignAndQuitCallback(&run_loop, &status));
     run_loop.Run();
 
     local_service_->AddChangeObserver(this);
 
     EXPECT_EQ(base::PLATFORM_FILE_OK, file_system_->OpenFileSystem());
 
     file_system_->file_system_context()->sync_context()->
         set_mock_notify_changes_duration_in_sec(0);
   }
 
   virtual void TearDown() OVERRIDE {
     local_service_->Shutdown();
     file_system_->TearDown();
     RevokeSyncableFileSystem();
-
-    thread_helper_.TearDown();
   }
 
   // LocalChangeObserver overrides.
   virtual void OnLocalChangeAvailable(int64 num_changes) OVERRIDE {
     num_changes_ = num_changes;
   }
 
   void PrepareForProcessRemoteChange(
       const FileSystemURL& url,
       const tracked_objects::Location& where,
@@ skipping 19 matching lines @@
         change, local_path, url,
         AssignAndQuitCallback(&run_loop, &sync_status));
     run_loop.Run();
     return sync_status;
   }
 
   int64 GetNumChangesInTracker() const {
     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_;
   scoped_ptr<LocalFileSyncService> local_service_;
 
   int64 num_changes_;
 };
 
 // More complete tests for PrepareForProcessRemoteChange and ApplyRemoteChange
 // are also in content_unittest:LocalFileSyncContextTest.
@@ skipping 79 matching lines @@
 // Flaky: http://crbug.com/171487
 #define MAYBE_LocalChangeObserverMultipleContexts\
     DISABLED_LocalChangeObserverMultipleContexts
 #else
 #define MAYBE_LocalChangeObserverMultipleContexts\
     LocalChangeObserverMultipleContexts
 #endif
 
 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;
   SyncStatusCode status = SYNC_STATUS_UNKNOWN;
   local_service_->MaybeInitializeFileSystemContext(
       GURL(kOrigin2), file_system2.file_system_context(),
       AssignAndQuitCallback(&run_loop, &status));
   run_loop.Run();
 
   EXPECT_EQ(base::PLATFORM_FILE_OK, file_system2.OpenFileSystem());
@@ skipping 404 matching lines @@
   all_origins.insert(kOrigin2);
   all_origins.insert(kOrigin3);
   while (!all_origins.empty()) {
     ASSERT_TRUE(NextOriginToProcess(&origin));
     ASSERT_TRUE(ContainsKey(all_origins, origin));
     all_origins.erase(origin);
   }
 }
 
 }  // namespace sync_file_system