Sync: add a test for downloading a tombstoned bookmark

Sync: add a test for downloading a tombstoned bookmark

This CL adds the first test that injects a tombstone into FakeServer (a
fancy way of saying deleting the entity).

As part of this CL, a util to wait for a bookmark count to match was
added. A util function to trigger a sync cycle given a ModelTypeSet was
also added.

BUG=365774
Committed: https://crrev.com/42417f22bf7c3ac27565c7771c49fd2b663b24a2
Cr-Commit-Position: refs/heads/master@{#329205}

[pval...(no longer on Chromium), 2015-05-07 21:47:16]

Patchset #1 (id:1) has been deleted

[pval...(no longer on Chromium), 2015-05-07 22:43:45]

Patchset #1 (id:20001) has been deleted

[pval...(no longer on Chromium), 2015-05-07 22:54:03]

Patchset #1 (id:40001) has been deleted

[pval...(no longer on Chromium), 2015-05-07 22:58:46]

The CQ bit was checked by pvalenzuela@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-07 22:59:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137543002/60001

[pval...(no longer on Chromium), 2015-05-07 23:00:16]

pvalenzuela@chromium.org changed reviewers:
+ zea@chromium.org

[commit-bot: I haz the power, 2015-05-08 00:10:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-08 00:10:10]

Dry run: This issue passed the CQ dry run.

[pval...(no longer on Chromium), 2015-05-08 17:28:10]

pvalenzuela@chromium.org changed reviewers:
+ maniscalco@chromium.org
- zea@chromium.org

[pval...(no longer on Chromium), 2015-05-08 17:28:11]

zea is out, so I'm switching reviewers. maniscalco@, please take a look? :-)

[maniscalco, 2015-05-08 20:13:18]

Looking good.  Just a few comments...

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/bookmarks_helper.cc:769:
expected_count_(expected_count) {}
nit: DCHECK expected_count >= 0 ?  Either here or in the Await method below?

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/bookmarks_helper.cc:772: int actual_count =
CountBookmarksWithTitlesMatching(0, title_);
I see we're passing profile 0 here.  Instead, should this class have an "int
profile" ctor parameter that's saved in member variable and then passed as an
argument to CountBookmarksWithtitlesMatching?

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/bookmarks_helper.cc:781: std::string
title_;
nit: These data members can be made const.

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc:326:
ASSERT_TRUE(AwaitCountBookmarksWithTitlesMatching(0 /* profile */,
nit: I like how you added a comment here to the reader knows what the 0 is for. 
Another option along these same lines would be to create a const int that's
scoped to this test (profile_id?) and pass that here and elsewhere above
(CountBookmarksWithtitlesMatching).  And if most/all of the test cases in this
file deal with profile 0, you could even make it a file scoped constant.

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
(right):

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc:80:
TriggerSyncForModelTypes(0, kExperimentsType);
Nice to have this in a method.  Are there others tests where you'd want to use
it?

Maybe here?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

[pval...(no longer on Chromium), 2015-05-08 22:34:19]

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/bookmarks_helper.cc:769:
expected_count_(expected_count) {}
On 2015/05/08 20:13:18, maniscalco wrote:
> nit: DCHECK expected_count >= 0 ?  Either here or in the Await method below?

Done.

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/bookmarks_helper.cc:772: int actual_count =
CountBookmarksWithTitlesMatching(0, title_);
On 2015/05/08 20:13:18, maniscalco wrote:
> I see we're passing profile 0 here.  Instead, should this class have an "int
> profile" ctor parameter that's saved in member variable and then passed as an
> argument to CountBookmarksWithtitlesMatching?

yeah. this was a mistake. thanks for catching this. I don't like that we need
both the PSS pointer (required by the Checker infrastructure) and index
(required by the CountBookmarks... helper), but this seems like the best
solution.

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/bookmarks_helper.cc:781: std::string
title_;
On 2015/05/08 20:13:18, maniscalco wrote:
> nit: These data members can be made const.

Done.

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc:326:
ASSERT_TRUE(AwaitCountBookmarksWithTitlesMatching(0 /* profile */,
On 2015/05/08 20:13:18, maniscalco wrote:
> nit: I like how you added a comment here to the reader knows what the 0 is
for. 
> Another option along these same lines would be to create a const int that's
> scoped to this test (profile_id?) and pass that here and elsewhere above
> (CountBookmarksWithtitlesMatching).  And if most/all of the test cases in this
> file deal with profile 0, you could even make it a file scoped constant.

constant added in this file. PTAL.

I think we could also work to make some of these calls cleaner (avoid passing
the index in single client test methods), but this seems like enough improvement
for now.

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
(right):

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc:80:
TriggerSyncForModelTypes(0, kExperimentsType);
On 2015/05/08 20:13:18, maniscalco wrote:
> Nice to have this in a method.  Are there others tests where you'd want to use
> it?
> 
> Maybe here?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

I had originally skipped this one, but it looks like it's really easy to use my
method here. See the changed file in the latest patchset.

[maniscalco, 2015-05-08 23:56:14]

LGTM

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/1137543002/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc:326:
ASSERT_TRUE(AwaitCountBookmarksWithTitlesMatching(0 /* profile */,
On 2015/05/08 22:34:19, pvalenzuela wrote:
> On 2015/05/08 20:13:18, maniscalco wrote:
> > nit: I like how you added a comment here to the reader knows what the 0 is
> for. 
> > Another option along these same lines would be to create a const int that's
> > scoped to this test (profile_id?) and pass that here and elsewhere above
> > (CountBookmarksWithtitlesMatching).  And if most/all of the test cases in
this
> > file deal with profile 0, you could even make it a file scoped constant.
> 
> constant added in this file. PTAL.
> 
> I think we could also work to make some of these calls cleaner (avoid passing
> the index in single client test methods), but this seems like enough
improvement
> for now.

Agreed.

[pval...(no longer on Chromium), 2015-05-11 17:47:11]

The CQ bit was checked by pvalenzuela@chromium.org

[commit-bot: I haz the power, 2015-05-11 17:53:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1137543002/80001

[commit-bot: I haz the power, 2015-05-11 18:45:17]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-05-11 18:46:40]

Patchset 2 (id:??) landed as
https://crrev.com/42417f22bf7c3ac27565c7771c49fd2b663b24a2
Cr-Commit-Position: refs/heads/master@{#329205}