Add four new sync integration tests for custom search engines.

Add four new sync integration tests for custom search engines.

BUG=0
TEST=Ensure that sync integration tests TwoClientSearchEnginesSyncTest.* pass.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110448

[SteveT, 2011-11-15 15:29:14]

Hey Raghu,

Here's that patch with the 4 shiny new tests I promised. PTAL. Thanks, as
always!


Steve

[Raghu Simha, 2011-11-16 01:14:40]

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/search_engines_helper.h (right):

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/search_engines_helper.h:55: // verifier, if
it is used. If profile -1 is specified, this only affects the
All these changes involving |profile| == -1 might be unnecessary if you call
DisableVerifier() prior to tests that involve conflicts. See my comment in
two_client_search_engines_sync_test.cc.

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc
(right):

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc:167:
EditSearchEngine(-1, "test0", "test0", "test0.com", "http://www.test0.com/");
In general, for tests that involve intentional commit races, conflicts and
merges, we avoid using the verifier because we cannot predict which client's
updates win (and therefore get applied across all clients). The simple thing to
do in such tests is to call DisableVerifier() at the start, and then not worry
about the verifier while making conflicting changes.

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc:200:
GetVerifierService()->Add(CreateTestTemplateURL(3));
Same comment as above -- no need to update the verifier in tests that have
intentional races, conflicts or merges.

[SteveT, 2011-11-16 15:43:32]

All done. Taking your advice and disabled the verifier in those tests.

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/search_engines_helper.h (right):

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/search_engines_helper.h:55: // verifier, if
it is used. If profile -1 is specified, this only affects the
Removed all these comments.

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc
(right):

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc:167:
EditSearchEngine(-1, "test0", "test0", "test0.com", "http://www.test0.com/");
That makes way more sense. Thanks for pointing this out. Using DisableVerifier
instead and removing all these calls to the verifier.

http://codereview.chromium.org/8571005/diff/2001/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc:200:
GetVerifierService()->Add(CreateTestTemplateURL(3));
On 2011/11/16 01:14:41, rsimha wrote:
> Same comment as above -- no need to update the verifier in tests that have
> intentional races, conflicts or merges.

Done.

[Raghu Simha, 2011-11-16 19:44:42]

Looking a lot better now. 3 more small changes, then LGTM pending green trybots.

Thanks for adding these tests!

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/search_engines_helper.cc (right):

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/search_engines_helper.cc:164: for (int it =
0; it < test()->num_clients(); ++it) {
|it| can be initialized to 1, since there's no point in comparing Profile 0 with
itself.

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc
(right):

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc:166:
ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
Call AwaitQuiescence() instead of AwaitMutualSyncCycleCompletion() here, since
both clients are busy committing / downloading changes.

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc:193:
ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
Same comment as above -- call AwaitQuiescence().

[SteveT, 2011-11-16 21:25:38]

Cool, thanks for the review. Fixes in. Will commit once we are green on the sync
bots.

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/search_engines_helper.cc (right):

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/search_engines_helper.cc:164: for (int it =
0; it < test()->num_clients(); ++it) {
On 2011/11/16 19:44:43, rsimha wrote:
> |it| can be initialized to 1, since there's no point in comparing Profile 0
with
> itself.

Done.

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc
(right):

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc:166:
ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
On 2011/11/16 19:44:43, rsimha wrote:
> Call AwaitQuiescence() instead of AwaitMutualSyncCycleCompletion() here, since
> both clients are busy committing / downloading changes.

Done.

http://codereview.chromium.org/8571005/diff/5002/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc:193:
ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
On 2011/11/16 19:44:43, rsimha wrote:
> Same comment as above -- call AwaitQuiescence().

Done.

[commit-bot: I haz the power, 2011-11-17 02:30:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/8571005/7003

[commit-bot: I haz the power, 2011-11-17 06:06:30]

Change committed as 110448