Add integraton test for sync backup/rollback.

Add integraton test for sync backup/rollback.

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

[pval...(no longer on Chromium), 2014-06-03 22:08:18]

I'm glad the TriggerError prototype worked here.

+rlarocque for reviewing the client state verification (you'll also need a
review from him since I'm not a committer yet)

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:24:
#if defined(OS_WIN) || defined(OS_MACOSX) || (defined(OS_LINUX) &&
!defined(OS_CHROMEOS))
I think conditionally disabling the test on Chrome SS is a better pattern to
follow.

For an example, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:70:
return pss_->backend_mode() == ProfileSyncService::BACKUP &&
Richard knows more about verifying client state than I do, so I'll let him
comment on this. My concern here is that we're modifying PSS only for test
verification. Is there any other existing way we can verify that backup/rollback
has completed?

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:71:
pss_->is_syncing();
Based on the changes to PSS in this CL, doesn't pss_->is_syncing() return false
when backup is complete?

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:97:
ASSERT_TRUE(checker.Wait());
Why wait for the checker here? As far as I can tell no rollback is happening at
this point.

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
File sync/test/fake_server/fake_server.cc (right):

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
sync/test/fake_server/fake_server.cc:226: if (error_type_ !=
sync_pb::SyncEnums::SUCCESS) {
there are a few other in-flight changes here that might make merging difficult.
would you like me to commit the TriggerError infrastructure so that you don't
have to deal with it?

[rlarocque, 2014-06-03 22:59:48]

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:769: bool is_syncing() const {
How does this differ from ShouldPushChanges()?

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:24:
#if defined(OS_WIN) || defined(OS_MACOSX) || (defined(OS_LINUX) &&
!defined(OS_CHROMEOS))
On 2014/06/03 22:08:18, pvalenzuela wrote:
> I think conditionally disabling the test on Chrome SS is a better pattern to
> follow.
> 
> For an example, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

It would be even better if you could disable only the execution of the test,
while still compiling it for all platforms.  Large #ifdefs make it more likely
that compiles will fail on one platform but not another.  That's less of a
problem in unit tests, but it's still a problem.

I'd recommend using the pattern:
#ifdef OS_CHROMEOS
#define MAYBE_SyncClientBackupRollbackTest DISABLED_...
#else
#define MAYBE_SyncClientBackupRollbackTest SyncClientBackupRollbackTest
#endif.

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:41:
class BackupModeChecker {
I think there are better ways to test than this.

Polling tends to be flaky.  Speaking abstractly, it's possible that the state
could briefly change from X to Y to X again in between polls and this test would
never notice most of the time.  Except that every now and then there would be
one lucky trybot or buildbot that polls at state Y and behaves differently.

The second issue is that we should avoid verifications against internal state. 
It makes it harder to refactor that internal state later on.  More importantly,
this internal state is not visible to users.

I can see that you're basing several expectations on bookmarks appearing and
disappearing.  Could you make a checker that registers as a bookmarks observer
and verifies that state instead?  That way we'd have more confidence that the
bookmarks the user sees are being modified as expected.

The ProfilesMatchStatusChecker in autofill_helper.cc provides an example of how
this sort of test can be implemented.

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
File sync/test/fake_server/fake_server.cc (right):

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
sync/test/fake_server/fake_server.cc:483: void FakeServer::TriggerError(const
sync_pb::SyncEnums::ErrorType& error_type) {
I think we should look at mock_connection_manager.cc and take it as a warning. 
This style of injecting errors works when there's one or two conditions we care
about, but it can eventually lead to a hairball once we have many different
conditions and triggering logic.

This is OK for now, but we should keep an eye on this style of error injection
to make sure it doesn't get out of control.

[haitaol1, 2014-06-04 18:21:29]

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:769: bool is_syncing() const {
That should work too. Removed this.

On 2014/06/03 22:59:48, rlarocque wrote:
> How does this differ from ShouldPushChanges()?

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:24:
#if defined(OS_WIN) || defined(OS_MACOSX) || (defined(OS_LINUX) &&
!defined(OS_CHROMEOS))
On 2014/06/03 22:59:48, rlarocque wrote:
> On 2014/06/03 22:08:18, pvalenzuela wrote:
> > I think conditionally disabling the test on Chrome SS is a better pattern to
> > follow.
> > 
> > For an example, see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...
> 
> It would be even better if you could disable only the execution of the test,
> while still compiling it for all platforms.  Large #ifdefs make it more likely
> that compiles will fail on one platform but not another.  That's less of a
> problem in unit tests, but it's still a problem.
> 
> I'd recommend using the pattern:
> #ifdef OS_CHROMEOS
> #define MAYBE_SyncClientBackupRollbackTest DISABLED_...
> #else
> #define MAYBE_SyncClientBackupRollbackTest SyncClientBackupRollbackTest
> #endif.

Done.

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:24:
#if defined(OS_WIN) || defined(OS_MACOSX) || (defined(OS_LINUX) &&
!defined(OS_CHROMEOS))
On 2014/06/03 22:08:18, pvalenzuela wrote:
> I think conditionally disabling the test on Chrome SS is a better pattern to
> follow.
> 
> For an example, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

Done.

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:41:
class BackupModeChecker {
The exit condition here is not bookmark change. I want to wait until PSS has
backed up all user's data, which is true when association is finished.

On 2014/06/03 22:59:48, rlarocque wrote:
> I think there are better ways to test than this.
> 
> Polling tends to be flaky.  Speaking abstractly, it's possible that the state
> could briefly change from X to Y to X again in between polls and this test
would
> never notice most of the time.  Except that every now and then there would be
> one lucky trybot or buildbot that polls at state Y and behaves differently.
> 
> The second issue is that we should avoid verifications against internal state.

> It makes it harder to refactor that internal state later on.  More
importantly,
> this internal state is not visible to users.
> 
> I can see that you're basing several expectations on bookmarks appearing and
> disappearing.  Could you make a checker that registers as a bookmarks observer
> and verifies that state instead?  That way we'd have more confidence that the
> bookmarks the user sees are being modified as expected.
> 
> The ProfilesMatchStatusChecker in autofill_helper.cc provides an example of
how
> this sort of test can be implemented.

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:70:
return pss_->backend_mode() == ProfileSyncService::BACKUP &&
Yes, Richard pointed to an existing funciton.

On 2014/06/03 22:08:18, pvalenzuela wrote:
> Richard knows more about verifying client state than I do, so I'll let him
> comment on this. My concern here is that we're modifying PSS only for test
> verification. Is there any other existing way we can verify that
backup/rollback
> has completed?

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:71:
pss_->is_syncing();
Backup is a continuous process as long as user's not signed in.

On 2014/06/03 22:08:18, pvalenzuela wrote:
> Based on the changes to PSS in this CL, doesn't pss_->is_syncing() return
false
> when backup is complete?

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:97:
ASSERT_TRUE(checker.Wait());
To make sure backup is finished in order to set rollback expections.

On 2014/06/03 22:08:18, pvalenzuela wrote:
> Why wait for the checker here? As far as I can tell no rollback is happening
at
> this point.

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
File sync/test/fake_server/fake_server.cc (right):

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
sync/test/fake_server/fake_server.cc:226: if (error_type_ !=
sync_pb::SyncEnums::SUCCESS) {
I rebased on ToT. Please take a look to verify.

On 2014/06/03 22:08:18, pvalenzuela wrote:
> there are a few other in-flight changes here that might make merging
difficult.
> would you like me to commit the TriggerError infrastructure so that you don't
> have to deal with it?

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
sync/test/fake_server/fake_server.cc:483: void FakeServer::TriggerError(const
sync_pb::SyncEnums::ErrorType& error_type) {
Added a todo for Patrick.

On 2014/06/03 22:59:48, rlarocque wrote:
> I think we should look at mock_connection_manager.cc and take it as a warning.

> This style of injecting errors works when there's one or two conditions we
care
> about, but it can eventually lead to a hairball once we have many different
> conditions and triggering logic.
> 
> This is OK for now, but we should keep an eye on this style of error injection
> to make sure it doesn't get out of control.

[rlarocque, 2014-06-04 18:30:28]

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:79:
logging::SetMinLogLevel(-1);
Remove this before commit?

https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:97:
ASSERT_TRUE(checker.Wait());
On 2014/06/04 18:21:30, haitaol1 wrote:
> To make sure backup is finished in order to set rollback expections.
> 
> On 2014/06/03 22:08:18, pvalenzuela wrote:
> > Why wait for the checker here? As far as I can tell no rollback is happening
> at
> > this point.
> 

In the real world, outside of tests, what would stop a user from signing in and
setting up sync before the backup is finished?

[haitaol1, 2014-06-04 20:17:01]

On 2014/06/04 18:30:28, rlarocque wrote:
>
https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
> File
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
> (right):
> 
>
https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
> chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:79:
> logging::SetMinLogLevel(-1);
> Remove this before commit?

done 

> 
>
https://codereview.chromium.org/310103004/diff/20001/chrome/browser/sync/test...
> chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:97:
> ASSERT_TRUE(checker.Wait());
> On 2014/06/04 18:21:30, haitaol1 wrote:
> > To make sure backup is finished in order to set rollback expections.
> > 
> > On 2014/06/03 22:08:18, pvalenzuela wrote:
> > > Why wait for the checker here? As far as I can tell no rollback is
happening
> > at
> > > this point.
> > 
> 
> In the real world, outside of tests, what would stop a user from signing in
and
> setting up sync before the backup is finished?

That is unavoidable. In that case, User's data is backed up to last chrome
session.

[rlarocque, 2014-06-04 22:54:06]

Two minor comments.  Otherwise I have no more comments, so LGTM.

https://codereview.chromium.org/310103004/diff/60001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/310103004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:114:
// Let server to return rolllback command on next sync request.
s/rolllback/rollback

https://codereview.chromium.org/310103004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:131:
}
Verify tier2_c is not present?

[rlarocque, 2014-06-04 22:55:20]

> > In the real world, outside of tests, what would stop a user from signing in
> and
> > setting up sync before the backup is finished?
> 
> That is unavoidable. In that case, User's data is backed up to last chrome
> session.

I see.  So the feature behaves differently based on timing.

I guess that changes my view of this test.  Now I'm thinking of it as:
1. Start up.
2. Sleep 15 seconds.
3. Try to sign in and roll back.
with some extra optimizations to make the success or failure faster by observing
internal-only events.

I realize many will not see it that way, but this perspective works for me. 
That's the only way I can look at it and see a valid test.

[haitaol1, 2014-06-05 17:44:11]

https://codereview.chromium.org/310103004/diff/60001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/310103004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:114:
// Let server to return rolllback command on next sync request.
On 2014/06/04 22:54:07, rlarocque wrote:
> s/rolllback/rollback

Done.

https://codereview.chromium.org/310103004/diff/60001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:131:
}
It's verified because tier1_b only has one child for http://www.nhl.com.

On 2014/06/04 22:54:07, rlarocque wrote:
> Verify tier2_c is not present?

[haitaol1, 2014-06-05 17:44:21]

The CQ bit was checked by haitaol@chromium.org

[pval...(no longer on Chromium), 2014-06-05 17:46:14]

lgtm

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
File sync/test/fake_server/fake_server.cc (right):

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
sync/test/fake_server/fake_server.cc:226: if (error_type_ !=
sync_pb::SyncEnums::SUCCESS) {
On 2014/06/04 18:21:30, haitaol1 wrote:
> I rebased on ToT. Please take a look to verify.
> 
> On 2014/06/03 22:08:18, pvalenzuela wrote:
> > there are a few other in-flight changes here that might make merging
> difficult.
> > would you like me to commit the TriggerError infrastructure so that you
don't
> > have to deal with it?
> 

latest version (patchset 5) looks good

https://codereview.chromium.org/310103004/diff/20001/sync/test/fake_server/fa...
sync/test/fake_server/fake_server.cc:483: void FakeServer::TriggerError(const
sync_pb::SyncEnums::ErrorType& error_type) {
On 2014/06/04 18:21:30, haitaol1 wrote:
> Added a todo for Patrick.
> 
> On 2014/06/03 22:59:48, rlarocque wrote:
> > I think we should look at mock_connection_manager.cc and take it as a
warning.
> 
> > This style of injecting errors works when there's one or two conditions we
> care
> > about, but it can eventually lead to a hairball once we have many different
> > conditions and triggering logic.
> > 
> > This is OK for now, but we should keep an eye on this style of error
injection
> > to make sure it doesn't get out of control.
> 

Great; thanks for TODO. I'll clean this up in a later CL.

[commit-bot: I haz the power, 2014-06-05 17:46:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/310103004/80001

[commit-bot: I haz the power, 2014-06-05 18:25:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13448)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/16536)

[commit-bot: I haz the power, 2014-06-05 18:28:18]

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

[commit-bot: I haz the power, 2014-06-05 18:28:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[haitaol1, 2014-06-05 21:14:29]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-06-05 21:16:46]

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

[haitaol1, 2014-06-05 22:55:04]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-06-05 22:55:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/310103004/120001

[commit-bot: I haz the power, 2014-06-06 03:52:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-06 03:55:45]

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

[commit-bot: I haz the power, 2014-06-06 03:55:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[haitaol1, 2014-06-10 21:09:19]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-06-10 21:12:53]

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

[commit-bot: I haz the power, 2014-06-11 03:17:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-11 06:34:07]

Change committed as 276281