ProfileSyncServiceTest refactoring

ProfileSyncServiceTest refactoring

Move following tests:
  TestStartupWithOldSyncData
  FailToOpenDatabase
  FailToDownloadControlTypes

Remove redundant parameters from StartSyncServiceAndSetInitialSyncEnded
and remove StartSyncService function.

TestStartupWithOldSyncData goes to SyncBackendHost tests. It verifies that SyncBackendHost cleans Sync Data directory before initialization.

FailToOpenDatabase goes to SyncManagerImpl test. It verifies that if SyncManager fails to open database it reports initialization failure.

FailToDownloadControlTypes is already covered by SyncBackendHostTest.SilentlyFailToDownloadControlTypes

BUG=312994
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235854

[rlarocque, 2013-11-11 22:30:46]

See r233945, a ProfileSyncServiceStartupTest refactor that landed last week.

See also https://codereview.chromium.org/69583002/, an in-progress CL that will
do something similar with the ProfileSyncServiceTests.

https://codereview.chromium.org/68483005/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://codereview.chromium.org/68483005/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:567:
TEST_F(ProfileSyncServiceStartupTest, TestStartupWithOldSyncData) {
I don't understand how this works.

The ProfileSyncServiceStartupTests were recently changed to use a
MockSyncBackendHost.  The MockSyncBackendHost does almost nothing.  The
MockSyncBackendHost shouldn't be deleting any files.  What actually deletes the
files in this test case?

https://codereview.chromium.org/68483005/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:632:
sync_->set_storage_option(syncer::STORAGE_INVALID);
I think your patch is a bit behind trunk.  sync_ is currently defined as a
ProfileSyncService*.  Only TestProfileSyncService has the set_storage_option(). 
I doubt this will compile.

[pavely, 2013-11-13 18:20:15]

I did rebase, moved tests to better places.

PTAL.

[rlarocque, 2013-11-13 18:58:35]

Thanks!  Once we combine this with the other CL I'm working on, we should
finally be able to get rid of synchronous backend init.

LGTM.

https://codereview.chromium.org/68483005/diff/70001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc (right):

https://codereview.chromium.org/68483005/diff/70001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc:701:
InitializeBackend(true);
I think InitializeBackend() will instantiate a FakeSyncManager.  The
FakeSyncManager has no syncable::Directory.  So no one will bother to create an
on-disk sync DB in this test.

I guess that's OK.  The test only promises to test the directory cleanup code,
and it certainly does that.

[pavely, 2013-11-18 19:26:56]

https://codereview.chromium.org/68483005/diff/70001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc (right):

https://codereview.chromium.org/68483005/diff/70001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc:701:
InitializeBackend(true);
On 2013/11/13 18:58:35, rlarocque wrote:
> I think InitializeBackend() will instantiate a FakeSyncManager.  The
> FakeSyncManager has no syncable::Directory.  So no one will bother to create
an
> on-disk sync DB in this test.
> 
> I guess that's OK.  The test only promises to test the directory cleanup code,
> and it certainly does that.

Yes, you are right. It doesn't test creation of sync db.

[commit-bot: I haz the power, 2013-11-18 19:31:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/68483005/180001

[commit-bot: I haz the power, 2013-11-18 19:58:16]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-18 20:09:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavely@chromium.org/68483005/440001

[commit-bot: I haz the power, 2013-11-18 23:54:36]

Change committed as 235854