Add new E2E sync multi profiles tests. This should not affect any existing

Add new E2E sync multi profiles tests. This should not affect any existing
test cases.

The multi profiles tests creates multiple profiles each in different user data
dirs and signs in each one separately. Then each profile performs sync related
operations and verifies results on the other sync'ed profile.

The CL also adds some helper functions to force a client to do a sync cycle.

Note: These tests are DISABLED and do not run on the normal bots.

BUG=431366
Committed: https://crrev.com/c956a156641baacbf4748330f379d4086681ccd0
Cr-Commit-Position: refs/heads/master@{#311534}

[shadi, 2014-12-06 01:47:43]

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

[shadi, 2014-12-06 01:50:06]

PTAL. The tests pass with current test servers setup locally.

[pval...(no longer on Chromium), 2014-12-09 18:44:08]

apologies for the slow review

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/bookmarks_helper.cc:673: void
NotifyBookmarksSync(int profile) {
Did you investigate using P2P invalidations instead of this way of forcing sync?
I think invalidations would be a cleaner way of doing this as we don't need to
write any more test hooks.

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_e2e_test.cc (right):

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:23:
IN_PROC_BROWSER_TEST_F(TwoClientE2ETest, DISABLED_SanitySetup) {
I'm not sure if we did this in the other test, but we should comment on why
these tests are disabled.

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:30: // ALl profiles
should sync same bookmarks.
nit: ALl -> All

https://codereview.chromium.org/772513004/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/772513004/diff/1/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1260:
'browser/sync/test/integration/two_client_e2e_test.cc',
add to GN file as well?

[shadi, 2014-12-10 18:10:10]

PTAL

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/bookmarks_helper.cc:673: void
NotifyBookmarksSync(int profile) {
On 2014/12/09 18:44:08, pvalenzuela wrote:
> Did you investigate using P2P invalidations instead of this way of forcing
sync?
> I think invalidations would be a cleaner way of doing this as we don't need to
> write any more test hooks.
Yes, I looked into it; an observer_list destructor CHECK fails when creating a
new P2P invalidation service. In a nutshell, 
invalidation::ProfileInvalidationProviderFactory::GetInstance()->SetTestingFactoryAndUse(...)
in sync_test.cc calls ContextShutdown() on the profile created by the test. The
shutdown fails because of the observers attached to it because of the way we
create the profile (I think). 
https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...

I am not very familiar in how invalidations work, but reading the code it looks
like there could be only one invalidation service to use, either the default one
or the test one. A test one can be added using SetTestingFactoryAndUse()
function, and this is failing to happen...

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_e2e_test.cc (right):

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:23:
IN_PROC_BROWSER_TEST_F(TwoClientE2ETest, DISABLED_SanitySetup) {
On 2014/12/09 18:44:08, pvalenzuela wrote:
> I'm not sure if we did this in the other test, but we should comment on why
> these tests are disabled.
I added a #define E2E_ONLY() above  to include add disabled tests.

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:30: // ALl profiles
should sync same bookmarks.
On 2014/12/09 18:44:08, pvalenzuela wrote:
> nit: ALl -> All

Done.

https://codereview.chromium.org/772513004/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/772513004/diff/1/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1260:
'browser/sync/test/integration/two_client_e2e_test.cc',
On 2014/12/09 18:44:08, pvalenzuela wrote:
> add to GN file as well?
I don't think sync_integration_test target is added to GN builds. The previous
CL failed because login_ui tests were using the new login_utils file part of
browser_tests target.

[pval...(no longer on Chromium), 2014-12-11 19:24:52]

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/bookmarks_helper.cc:673: void
NotifyBookmarksSync(int profile) {
On 2014/12/10 18:10:10, shadi wrote:
> On 2014/12/09 18:44:08, pvalenzuela wrote:
> > Did you investigate using P2P invalidations instead of this way of forcing
> sync?
> > I think invalidations would be a cleaner way of doing this as we don't need
to
> > write any more test hooks.
> Yes, I looked into it; an observer_list destructor CHECK fails when creating a
> new P2P invalidation service. In a nutshell, 
>
invalidation::ProfileInvalidationProviderFactory::GetInstance()->SetTestingFactoryAndUse(...)
> in sync_test.cc calls ContextShutdown() on the profile created by the test.
The
> shutdown fails because of the observers attached to it because of the way we
> create the profile (I think). 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...
> 
> I am not very familiar in how invalidations work, but reading the code it
looks
> like there could be only one invalidation service to use, either the default
one
> or the test one. A test one can be added using SetTestingFactoryAndUse()
> function, and this is failing to happen...

Thanks for the explanation here; it's very useful for future discussions.

Maybe add a TODO to use invalidations instead of force notifying?

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/two_client_e2e_test.cc (right):

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:23:
IN_PROC_BROWSER_TEST_F(TwoClientE2ETest, DISABLED_SanitySetup) {
On 2014/12/10 18:10:10, shadi wrote:
> On 2014/12/09 18:44:08, pvalenzuela wrote:
> > I'm not sure if we did this in the other test, but we should comment on why
> > these tests are disabled.
> I added a #define E2E_ONLY() above  to include add disabled tests.

cool; I think this may be how we end up designating e2e-only tests long-term

https://codereview.chromium.org/772513004/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/772513004/diff/1/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1260:
'browser/sync/test/integration/two_client_e2e_test.cc',
On 2014/12/10 18:10:10, shadi wrote:
> On 2014/12/09 18:44:08, pvalenzuela wrote:
> > add to GN file as well?
> I don't think sync_integration_test target is added to GN builds. The previous
> CL failed because login_ui tests were using the new login_utils file part of
> browser_tests target.

gotcha. thanks.

[Nicolas Zea, 2014-12-11 19:45:34]

https://codereview.chromium.org/772513004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/bookmarks_helper.h (right):

https://codereview.chromium.org/772513004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/bookmarks_helper.h:138: // Sends sync
refresh notification for bookmarks model set of profile |profile|.
nit: "bookmarks model set of" -> "bookmarks to" (here and below)

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

https://codereview.chromium.org/772513004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:32:
DisableVerifier();
Are we ever expecting to use the verifier in E2E tests? Should we have it
disabled by default (in the test fixture itself)?

https://codereview.chromium.org/772513004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:51:
base::TimeDelta::FromSeconds(5),
Were you planning to try to add some logic to wait for an OnSyncCycleCompleted
event or something like that?

[shadi, 2014-12-17 22:43:11]

Patchset #3 (id:40001) has been deleted

[shadi, 2014-12-17 22:48:09]

Patchset #3 (id:60001) has been deleted

[shadi, 2014-12-17 22:50:16]

This patch replaces the timeouts to force sync refresh with an obrserver for
OnSyncCycleCompleted. I added a new class P2PSyncRefresher (please suggest
better naming if possible) to do so. The benefit is that this model is not
dependent on the datatype of the commit like my previous implementation was.

PTAL.

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/772513004/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/bookmarks_helper.cc:673: void
NotifyBookmarksSync(int profile) {
On 2014/12/11 19:24:52, pvalenzuela wrote:
> On 2014/12/10 18:10:10, shadi wrote:
> > On 2014/12/09 18:44:08, pvalenzuela wrote:
> > > Did you investigate using P2P invalidations instead of this way of forcing
> > sync?
> > > I think invalidations would be a cleaner way of doing this as we don't
need
> to
> > > write any more test hooks.
> > Yes, I looked into it; an observer_list destructor CHECK fails when creating
a
> > new P2P invalidation service. In a nutshell, 
> >
>
invalidation::ProfileInvalidationProviderFactory::GetInstance()->SetTestingFactoryAndUse(...)
> > in sync_test.cc calls ContextShutdown() on the profile created by the test.
> The
> > shutdown fails because of the observers attached to it because of the way we
> > create the profile (I think). 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/keyed_s...
> > 
> > I am not very familiar in how invalidations work, but reading the code it
> looks
> > like there could be only one invalidation service to use, either the default
> one
> > or the test one. A test one can be added using SetTestingFactoryAndUse()
> > function, and this is failing to happen...
> 
> Thanks for the explanation here; it's very useful for future discussions.
> 
> Maybe add a TODO to use invalidations instead of force notifying?

Done.

https://codereview.chromium.org/772513004/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/bookmarks_helper.h (right):

https://codereview.chromium.org/772513004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/bookmarks_helper.h:138: // Sends sync
refresh notification for bookmarks model set of profile |profile|.
On 2014/12/11 19:45:34, Nicolas Zea wrote:
> nit: "bookmarks model set of" -> "bookmarks to" (here and below)

Done.

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

https://codereview.chromium.org/772513004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:32:
DisableVerifier();
On 2014/12/11 19:45:34, Nicolas Zea wrote:
> Are we ever expecting to use the verifier in E2E tests? Should we have it
> disabled by default (in the test fixture itself)?
So far I think a verifier is useless in the E2E setup. In theory, the verifier
is the second profile in these tests. I'll defer cleaning up verifier code to a
separate CL.

https://codereview.chromium.org/772513004/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_e2e_test.cc:51:
base::TimeDelta::FromSeconds(5),
On 2014/12/11 19:45:34, Nicolas Zea wrote:
> Were you planning to try to add some logic to wait for an OnSyncCycleCompleted
> event or something like that?

Yes. I ended up creating SyncRefresher. PTAL.

[pval...(no longer on Chromium), 2014-12-18 01:47:25]

This looks really cool.

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/p2p_sync_refresher.h (right):

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/p2p_sync_refresher.h:18: class
P2PSyncRefresher : public ProfileSyncServiceObserver {
The prefix of P2P immediately makes me think of the P2PInvalidationService and
networked peers. In reality, this thing is just an observer that replaces
notifications.

I'm trying to think of a new name that doesn't have this prefix. The first thing
that came to mind is pretty silly:
TestOnlyInProcessInvalidationServiceReplacement. Obviously this isn't a good
name. Any ideas?

[Nicolas Zea, 2014-12-18 20:55:25]

Nice job! LGTM with some nits

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/p2p_sync_refresher.h (right):

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/p2p_sync_refresher.h:13: // This class
observe ProfileSyncService events and emit refresh notifications
grammar nit: observes and emits

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/p2p_sync_refresher.h:21:
~P2PSyncRefresher() override;
Hmm, I'm pretty sure this is supposed to be virtual, without the override
keyword.

[shadi, 2014-12-19 19:15:12]

Fixed nits, but still not sure on final class name.

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/p2p_sync_refresher.h (right):

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/p2p_sync_refresher.h:13: // This class
observe ProfileSyncService events and emit refresh notifications
On 2014/12/18 20:55:25, Nicolas Zea wrote:
> grammar nit: observes and emits

Done.

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/p2p_sync_refresher.h:18: class
P2PSyncRefresher : public ProfileSyncServiceObserver {
On 2014/12/18 01:47:25, pvalenzuela wrote:
> The prefix of P2P immediately makes me think of the P2PInvalidationService and
> networked peers. In reality, this thing is just an observer that replaces
> notifications.
> 
> I'm trying to think of a new name that doesn't have this prefix. The first
thing
> that came to mind is pretty silly:
> TestOnlyInProcessInvalidationServiceReplacement. Obviously this isn't a good
> name. Any ideas?

I wanted to capture in the name that we are sending refresh notification to sync
clients. what about SyncClientsRefreshOnCommitObserver? :)

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/p2p_sync_refresher.h:21:
~P2PSyncRefresher() override;
On 2014/12/18 20:55:25, Nicolas Zea wrote:
> Hmm, I'm pretty sure this is supposed to be virtual, without the override
> keyword.

Done.

[pval...(no longer on Chromium), 2014-12-22 23:49:39]

lgtm

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/p2p_sync_refresher.h (right):

https://codereview.chromium.org/772513004/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/p2p_sync_refresher.h:18: class
P2PSyncRefresher : public ProfileSyncServiceObserver {
On 2014/12/19 19:15:12, shadi wrote:
> On 2014/12/18 01:47:25, pvalenzuela wrote:
> > The prefix of P2P immediately makes me think of the P2PInvalidationService
and
> > networked peers. In reality, this thing is just an observer that replaces
> > notifications.
> > 
> > I'm trying to think of a new name that doesn't have this prefix. The first
> thing
> > that came to mind is pretty silly:
> > TestOnlyInProcessInvalidationServiceReplacement. Obviously this isn't a good
> > name. Any ideas?
> 
> I wanted to capture in the name that we are sending refresh notification to
sync
> clients. what about SyncClientsRefreshOnCommitObserver? :)
> 

ok, fair enough, I am fine with this name

[shadi, 2015-01-13 23:54:08]

The CQ bit was checked by shadi@chromium.org

[commit-bot: I haz the power, 2015-01-13 23:55:04]

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

[commit-bot: I haz the power, 2015-01-14 00:18:23]

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

[commit-bot: I haz the power, 2015-01-14 00:18:24]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[shadi, 2015-01-14 01:10:05]

The CQ bit was checked by shadi@chromium.org

[commit-bot: I haz the power, 2015-01-14 01:11:07]

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

[commit-bot: I haz the power, 2015-01-14 01:22:11]

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

[commit-bot: I haz the power, 2015-01-14 01:22:12]

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

[shadi, 2015-01-14 18:41:01]

Patchset #5 (id:120001) has been deleted

[shadi, 2015-01-14 18:42:53]

The CQ bit was checked by shadi@chromium.org

[commit-bot: I haz the power, 2015-01-14 18:43:38]

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

[commit-bot: I haz the power, 2015-01-14 19:47:05]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-01-14 19:48:02]

Patchset 5 (id:??) landed as
https://crrev.com/c956a156641baacbf4748330f379d4086681ccd0
Cr-Commit-Position: refs/heads/master@{#311534}