[Sync] Add tests for migration triggered by notifications

[Sync] Add tests for migration triggered by notifications

Add support for disabling/enabling notifications to the test server, and support for triggering notifications.

BUG=92928
TEST=


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

[akalin, 2011-08-26 00:54:08]

+rsimha for integration test stuff, +zea for everything else

[Raghu Simha, 2011-08-26 01:30:21]

I have one question below.

If there's a way of adding an extra check to IsSynced to detect that another
sync cycle is coming up even though we just synced, we may not need
AwaitNextSyncCycleCompletion. If there's no way to do so, then test changes
LGTM.

http://codereview.chromium.org/7753023/diff/1/chrome/browser/sync/profile_syn...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/7753023/diff/1/chrome/browser/sync/profile_syn...
chrome/browser/sync/profile_sync_service_harness.h:78: // cycle has completed.
Is it a bug or a valid case that there can be an impending full sync cycle even
though we are already synced? If it's a bug, this workaround seems OK.

However, if it's a valid case, then it might be possible to add an extra check
to IsSynced() so that it returns true iff we're fully synced AND there are no
impending sync cycles.

http://codereview.chromium.org/7753023/diff/1/chrome/test/live_sync/migration...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7753023/diff/1/chrome/test/live_sync/migration...
chrome/test/live_sync/migration_errors_test.cc:12: #include
"chrome/test/live_sync/bookmarks_helper.h"
While you're editing this file, could I ask you to switch the 2 lines above? :)

[akalin, 2011-08-26 01:58:16]

http://codereview.chromium.org/7753023/diff/1/chrome/browser/sync/profile_syn...
> chrome/browser/sync/profile_sync_service_harness.h:78: // cycle has
> completed.
> Is it a bug or a valid case that there can be an impending full sync
> cycle even though we are already synced? If it's a bug, this workaround
> seems OK.
>
> However, if it's a valid case, then it might be possible to add an extra
> check to IsSynced() so that it returns true iff we're fully synced AND
> there are no impending sync cycles.

Well, it's neither.  The case in this CL is that the sync integration
test has triggered the server to send out a notification, so we know
that a sync cycle is coming, but there's no way for the client to know
that (in general).  I couldn't think of a better way than adding this
method.  What do you think?

[Raghu Simha, 2011-08-26 03:13:18]

On 2011/08/26 01:58:16, akalin wrote:
> Well, it's neither.  The case in this CL is that the sync integration
> test has triggered the server to send out a notification, so we know
> that a sync cycle is coming, but there's no way for the client to know
> that (in general).  I couldn't think of a better way than adding this
> method.  What do you think?

I see. You're right about the fact that there's no way for the client to know
that a sync cycle is impending (i.e., there's nothing you can add to IsSynced()
that will allow the client to predict an upcoming sync cycle). I'm fine with
leaving AwaitNextSyncCycleCompletion() as it is.

Curious: Do you think it will be useful for AwaitNextSyncCycleCompletion to be a
permanent addition to the framework, with a comment warning people that it
shouldn't be called unless tests have explicitly triggered errors /
notifications on the server prior to the call? (Right now, it seems like it's
you've only added it as a temporary workaround)

My LGTM still stands :)

[Raghu Simha, 2011-08-26 03:14:46]

On 2011/08/26 01:58:16, akalin wrote:
> Well, it's neither.  The case in this CL is that the sync integration
> test has triggered the server to send out a notification, so we know
> that a sync cycle is coming, but there's no way for the client to know
> that (in general).  I couldn't think of a better way than adding this
> method.  What do you think?

I see. You're right about the fact that there's no way for the client to know
that a sync cycle is impending (i.e., there's nothing you can add to IsSynced()
that will allow the client to predict an upcoming sync cycle). I'm fine with
leaving AwaitNextSyncCycleCompletion() as it is.

Curious: Do you think it will be useful for AwaitNextSyncCycleCompletion to be a
permanent addition to the framework, with a comment warning people that it
shouldn't be called unless tests have explicitly triggered errors /
notifications on the server prior to the call? (Right now, it seems like it's
you've only added it as a temporary workaround)

My LGTM still stands :)

[Nicolas Zea, 2011-08-26 21:42:55]

http://codereview.chromium.org/7753023/diff/1/chrome/test/live_sync/migration...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7753023/diff/1/chrome/test/live_sync/migration...
chrome/test/live_sync/migration_errors_test.cc:365:
IN_PROC_BROWSER_TEST_F(MigrationReconfigureTest, SetSyncTabsAndMigrate) {
Looks like this needs to be FAILS as well? (Although as I mentioned in the other
code review, if these are timing out after a minute, I'd prefer disabled)

http://codereview.chromium.org/7753023/diff/1/net/tools/testserver/xmppserver...
File net/tools/testserver/xmppserver_test.py (right):

http://codereview.chromium.org/7753023/diff/1/net/tools/testserver/xmppserver...
net/tools/testserver/xmppserver_test.py:324: def
AssertSentDataLength(expectedLength):
Pull this out into it's own method since it's used below as well?

[akalin, 2011-08-26 23:42:09]

All comments addressed, PTAL.

http://codereview.chromium.org/7753023/diff/1/chrome/test/live_sync/migration...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7753023/diff/1/chrome/test/live_sync/migration...
chrome/test/live_sync/migration_errors_test.cc:12: #include
"chrome/test/live_sync/bookmarks_helper.h"
On 2011/08/26 01:30:21, rsimha wrote:
> While you're editing this file, could I ask you to switch the 2 lines above?
:)

Done.

http://codereview.chromium.org/7753023/diff/1/chrome/test/live_sync/migration...
chrome/test/live_sync/migration_errors_test.cc:365:
IN_PROC_BROWSER_TEST_F(MigrationReconfigureTest, SetSyncTabsAndMigrate) {
On 2011/08/26 21:42:56, nzea wrote:
> Looks like this needs to be FAILS as well? (Although as I mentioned in the
other
> code review, if these are timing out after a minute, I'd prefer disabled)

Done.

http://codereview.chromium.org/7753023/diff/1/net/tools/testserver/xmppserver...
File net/tools/testserver/xmppserver_test.py (right):

http://codereview.chromium.org/7753023/diff/1/net/tools/testserver/xmppserver...
net/tools/testserver/xmppserver_test.py:324: def
AssertSentDataLength(expectedLength):
On 2011/08/26 21:42:56, nzea wrote:
> Pull this out into it's own method since it's used below as well?

Done.

[akalin, 2011-08-26 23:44:15]

> Curious: Do you think it will be useful for AwaitNextSyncCycleCompletion to
> be a
> permanent addition to the framework, with a comment warning people that it
> shouldn't be called unless tests have explicitly triggered errors /
> notifications on the server prior to the call? (Right now, it seems like
> it's
> you've only added it as a temporary workaround)

Hmm, maybe, if we write other integration tests that trigger
notifications (plausible, since we could test nudge behavior that
way).  The current use case will evolve to need something like
AwaitNextConfigurationDone() or something, though.

[Nicolas Zea, 2011-08-27 00:48:01]

LGTM

[commit-bot: I haz the power, 2011-08-27 06:53:27]

Change committed as 98558