Clean up TestProfileSyncService and related tests

Clean up TestProfileSyncService and related tests

This CL refactors many of the tests in profile_sync_service_unittest.cc.
It continues the refactoring work begun in r233533, r235661, and
r235854.

The JsController tests have been deleted.  There is not much point in
testing the JsController here; it can be more easily tested on its own
in sync_js_controller_uniittest.cc.  The SyncJsController unit tests
have been updated so they now cover the few cases that were formerly
only exercised in the ProfileSyncService unit tests.

It converts all remaining uncoverted tests from relying on the
TestProfileSyncService to using a real ProfileSyncService with an
injected backend.  The injected backend makes it easier to create the
scenarios we want to test.  We can inject a specially crafted SBH rather
than fiddling with "synchronous init" and "fail initial download" flags.

Since the TestProfileSyncService no longer needs to support the wide
variety of test scenarios required by the tests in
profile_sync_service_unittest.cc, we can greatly simplify its
implementation.  Many of its parameters and associated code have been
removed.

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

[rlarocque, 2013-11-19 20:41:09]

Here's another test refactoring patch.  Please review.

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

https://codereview.chromium.org/67683005/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_unittest.cc:87: class
ProfileSyncServiceTest : public ::testing::Test {
The diffs here may be a bit confusing.

In a previous commit, I added the ProfileSyncServiceSimpleTest harness to
supplement the ProfileSyncServiceTest.  This CL converts all tests to use
ProfileSyncServiceSimpleTest.  Then, to make things extra confusing, it renames
ProfileSyncServiceSimpleTest to ProfileSyncServiceTest.

[rlarocque, 2013-11-22 21:23:01]

On 2013/11/19 20:41:09, rlarocque wrote:
> Here's another test refactoring patch.  Please review.
> 
>
https://codereview.chromium.org/67683005/diff/1/chrome/browser/sync/profile_s...
> File chrome/browser/sync/profile_sync_service_unittest.cc (right):
> 
>
https://codereview.chromium.org/67683005/diff/1/chrome/browser/sync/profile_s...
> chrome/browser/sync/profile_sync_service_unittest.cc:87: class
> ProfileSyncServiceTest : public ::testing::Test {
> The diffs here may be a bit confusing.
> 
> In a previous commit, I added the ProfileSyncServiceSimpleTest harness to
> supplement the ProfileSyncServiceTest.  This CL converts all tests to use
> ProfileSyncServiceSimpleTest.  Then, to make things extra confusing, it
renames
> ProfileSyncServiceSimpleTest to ProfileSyncServiceTest.

Ping.

[rlarocque, 2013-11-25 18:22:32]

+Pavel

[rlarocque, 2013-11-27 21:56:17]

On 2013/11/25 18:22:32, rlarocque wrote:
> +Pavel

ping.

[tim (not reviewing), 2013-11-27 22:32:30]

On 2013/11/27 21:56:17, rlarocque wrote:
> On 2013/11/25 18:22:32, rlarocque wrote:
> > +Pavel
> 
> ping.

This patch LGTM but I didn't review a few of the intermediary patches, so Pavel
/ Nicolas should review in better context. One thing I noticed while looking
back was that you mention the SBH coverage we used to get was removed from the
ProfileSyncService unittests, which is good in that it keeps the unit tests more
targeted, but are we sure the SBHTests were already covering what we removed
from here?  I didn't see any changes to them.

[pavely, 2013-12-02 19:00:55]

LGTM with small comment.

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

https://codereview.chromium.org/67683005/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_unittest.cc:32:
ACTION(ReturnNewDataTypeManager) {
I don't think you are using ReturnNewDataTypeManager anywhere. Could you check?

[rlarocque, 2013-12-02 21:08:58]

Re: Tim's comment

I don't think we've lost any meaningful coverage of the sync backend here.  The
test harnesses were putting the backend into a very strange state in order to
trick it into not downloading updates at startup.  That's not a very realistic
scenario.

I think that our best coverage of backend and PSS interaction is the integration
tests.  This has always been the case.  The integration tests are the only tests
where we're allowed to make meaningful requests to the server during init.

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

https://codereview.chromium.org/67683005/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_unittest.cc:32:
ACTION(ReturnNewDataTypeManager) {
On 2013/12/02 19:00:55, pavely wrote:
> I don't think you are using ReturnNewDataTypeManager anywhere. Could you
check?

It's used in ExpectDataTypeManagerCreation() on line 144.

[commit-bot: I haz the power, 2013-12-02 22:47:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/67683005/100001

[commit-bot: I haz the power, 2013-12-03 01:05:05]

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

[commit-bot: I haz the power, 2013-12-03 01:06:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/67683005/100001

[commit-bot: I haz the power, 2013-12-03 11:28:51]

Change committed as 238348