Refactor password sync integration tests

Refactor password sync integration tests

Refactors the password sync integration tests so they no longer require
self-notifications or access to progress markers.  This will allow them
to work in the future when we refactor the integration tests to no
longer depend on sync internals.

Introduces two new Await*() functions for passwords.  One is used to
wait until all profiles have matching passwords state.  The other waits
until a single profile matches the verifier.  Together, these are enough
to replace most uses of AwaitQuiescence(),
AwaitCommitActivityCompletion(), and other await conditions that will
not be supported in the new style of integration tests.

Refactors some two client tests into multi (ie. three) client tests.
These are the tests that formerly relied on
AwaitCommitActivityCompletion() to ensure that certain changes were
committed to the server.  Using three clients allows us to set up
a "canary" client that can be used to infer that items are committed
based on the fact that it has received them.

Removes a few redundant tests.

Note to sheriffs: The TwoClientPasswordTest.Delete test may be flaky.
I believe it was flaky before the refactor, and it may still be flaky
now.  Please disable it and assign the bug to me if it causes problems.

BUG=97780, 95742
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269198

[rlarocque, 2014-05-02 19:20:20]

Please review.

[Nicolas Zea, 2014-05-06 00:16:37]

mostly LG

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

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:79:
ASSERT_TRUE(AwaitProfileContainsSamePasswordFormsAsVerifier(2));
Hmm, given that we're calling this, why do we care about waiting for client 1 to
pick up changes? Either way we'll be waiting here for those changes.

Unlike the passphrase cases, do we care about getting the data at model
association time vs change processor time?

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:105:
ASSERT_TRUE(AwaitProfileContainsSamePasswordFormsAsVerifier(2));
Same with this test

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:134:
GetSyncService(1)->SetSyncSetupCompleted();
SetupSync should be setting these bits appropriately. Is that not the case?

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

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/passwords_helper.cc:181: LOG(INFO) <<
"Profile " << i << " does not contain the same password"
any reason this was changed?

[rlarocque, 2014-05-06 00:33:36]

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

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:79:
ASSERT_TRUE(AwaitProfileContainsSamePasswordFormsAsVerifier(2));
On 2014/05/06 00:16:37, Nicolas Zea wrote:
> Hmm, given that we're calling this, why do we care about waiting for client 1
to
> pick up changes? Either way we'll be waiting here for those changes.
> 
> Unlike the passphrase cases, do we care about getting the data at model
> association time vs change processor time?

The expression ASSERT_EQ(1, GetPasswordCount(1)) is probably a left-over from
the old test.

It might still have some value, though.  It's not necessarily about asserting
that client 1 has exactly one password.  In combination with line 81, it asserts
that all stores have exactly one password.

If we assume that the verifier profile is managed correctly then the check is
not useful.

If you feel strongly about it I could remove this or have it assert against a
different profile.

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:134:
GetSyncService(1)->SetSyncSetupCompleted();
On 2014/05/06 00:16:37, Nicolas Zea wrote:
> SetupSync should be setting these bits appropriately. Is that not the case?

These lines are a left-over from the old integration test.  If I remove them,
the tests fail.

I didn't bother to investigate too much.  My goals for this CL did not involve
cleaning up this particular mess.

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

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/passwords_helper.cc:181: LOG(INFO) <<
"Profile " << i << " does not contain the same password"
On 2014/05/06 00:16:37, Nicolas Zea wrote:
> any reason this was changed?

ERROR isn't quite right anymore.  In some ways it was never right, but now it
actually matters.

Now that we use these functions in an Await() function, it's normal and expected
for this function to fail from time to time.  It's only an error when the
profiles don't match after 40 seconds of waiting.

[Nicolas Zea, 2014-05-06 18:42:15]

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

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:79:
ASSERT_TRUE(AwaitProfileContainsSamePasswordFormsAsVerifier(2));
On 2014/05/06 00:33:36, rlarocque wrote:
> On 2014/05/06 00:16:37, Nicolas Zea wrote:
> > Hmm, given that we're calling this, why do we care about waiting for client
1
> to
> > pick up changes? Either way we'll be waiting here for those changes.
> > 
> > Unlike the passphrase cases, do we care about getting the data at model
> > association time vs change processor time?
> 
> The expression ASSERT_EQ(1, GetPasswordCount(1)) is probably a left-over from
> the old test.
> 
> It might still have some value, though.  It's not necessarily about asserting
> that client 1 has exactly one password.  In combination with line 81, it
asserts
> that all stores have exactly one password.
> 
> If we assume that the verifier profile is managed correctly then the check is
> not useful.
> 
> If you feel strongly about it I could remove this or have it assert against a
> different profile.

I'd rather just rely on the verifier to be correct, and test against that. It
seems to me that client 1 isn't necessary for this test (and the one below).

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

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/passwords_helper.cc:181: LOG(INFO) <<
"Profile " << i << " does not contain the same password"
On 2014/05/06 00:33:36, rlarocque wrote:
> On 2014/05/06 00:16:37, Nicolas Zea wrote:
> > any reason this was changed?
> 
> ERROR isn't quite right anymore.  In some ways it was never right, but now it
> actually matters.
> 
> Now that we use these functions in an Await() function, it's normal and
expected
> for this function to fail from time to time.  It's only an error when the
> profiles don't match after 40 seconds of waiting.

If it's expected for this to be called, I think it should probably be a
DVLOG(1), rather than printing non-useful messages.

[rlarocque, 2014-05-06 23:40:47]

Rebased and addressed some recent comments.  PTAL.

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

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:79:
ASSERT_TRUE(AwaitProfileContainsSamePasswordFormsAsVerifier(2));
On 2014/05/06 18:42:15, Nicolas Zea wrote:
> On 2014/05/06 00:33:36, rlarocque wrote:
> > On 2014/05/06 00:16:37, Nicolas Zea wrote:
> > > Hmm, given that we're calling this, why do we care about waiting for
client
> 1
> > to
> > > pick up changes? Either way we'll be waiting here for those changes.
> > > 
> > > Unlike the passphrase cases, do we care about getting the data at model
> > > association time vs change processor time?
> > 
> > The expression ASSERT_EQ(1, GetPasswordCount(1)) is probably a left-over
from
> > the old test.
> > 
> > It might still have some value, though.  It's not necessarily about
asserting
> > that client 1 has exactly one password.  In combination with line 81, it
> asserts
> > that all stores have exactly one password.
> > 
> > If we assume that the verifier profile is managed correctly then the check
is
> > not useful.
> > 
> > If you feel strongly about it I could remove this or have it assert against
a
> > different profile.
> 
> I'd rather just rely on the verifier to be correct, and test against that. It
> seems to me that client 1 isn't necessary for this test (and the one below).

Done.

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:105:
ASSERT_TRUE(AwaitProfileContainsSamePasswordFormsAsVerifier(2));
On 2014/05/06 00:16:37, Nicolas Zea wrote:
> Same with this test

Done.

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

https://codereview.chromium.org/267743003/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/passwords_helper.cc:181: LOG(INFO) <<
"Profile " << i << " does not contain the same password"
On 2014/05/06 18:42:15, Nicolas Zea wrote:
> On 2014/05/06 00:33:36, rlarocque wrote:
> > On 2014/05/06 00:16:37, Nicolas Zea wrote:
> > > any reason this was changed?
> > 
> > ERROR isn't quite right anymore.  In some ways it was never right, but now
it
> > actually matters.
> > 
> > Now that we use these functions in an Await() function, it's normal and
> expected
> > for this function to fail from time to time.  It's only an error when the
> > profiles don't match after 40 seconds of waiting.
> 
> If it's expected for this to be called, I think it should probably be a
> DVLOG(1), rather than printing non-useful messages.

Done.

[Nicolas Zea, 2014-05-06 23:46:58]

https://codereview.chromium.org/267743003/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc
(right):

https://codereview.chromium.org/267743003/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:59:
IN_PROC_BROWSER_TEST_F(MultipleClientPasswordsSyncTest, DisablePasswords) {
My point is that these tests don't need to be multiple client tests. Client 1
isn't doing anything useful here, these can be two-client tests.

[rlarocque, 2014-05-07 00:06:41]

https://codereview.chromium.org/267743003/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc
(right):

https://codereview.chromium.org/267743003/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:59:
IN_PROC_BROWSER_TEST_F(MultipleClientPasswordsSyncTest, DisablePasswords) {
On 2014/05/06 23:46:58, Nicolas Zea wrote:
> My point is that these tests don't need to be multiple client tests. Client 1
> isn't doing anything useful here, these can be two-client tests.

Actually, client 1 is doing something.

The goal of this particular test is to:
1) Disable sync on client B.
2) Add some items on client A.
3) Wait for all the items created on client A to make it to the server.
4) Enable sync on client B.
5) Wait for client B to come back into sync.

The tricky part is step 3.  It used to rely on AwaitCommitActivityCompletion(). 
But that function has lots of problems, and can't reasonably be supported in the
future.

Client 1 is a hack to work around this problem.  When it is synced with client
0, then we can infer that all of client 0's items were committed to the server. 
It's ugly, but it's the only solution that doesn't require us to inspect
internal state (other than the state of the passwords stores).

The fake server will provide an alternative eventually, but it's not quite there
yet and I'm not sure we want to give up on the ability to run these tests
against an out-of-process server.

Another option would be to declare these tests to be useless and non-portable
(they'll never work on Android or iOS) and delete them.  I'd be OK with that
plan.  I figured keeping them would be the less controversial choice.

[Nicolas Zea, 2014-05-07 17:07:15]

On 2014/05/07 00:06:41, rlarocque wrote:
>
https://codereview.chromium.org/267743003/diff/40001/chrome/browser/sync/test...
> File
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc
> (right):
> 
>
https://codereview.chromium.org/267743003/diff/40001/chrome/browser/sync/test...
>
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:59:
> IN_PROC_BROWSER_TEST_F(MultipleClientPasswordsSyncTest, DisablePasswords) {
> On 2014/05/06 23:46:58, Nicolas Zea wrote:
> > My point is that these tests don't need to be multiple client tests. Client
1
> > isn't doing anything useful here, these can be two-client tests.
> 
> Actually, client 1 is doing something.
> 
> The goal of this particular test is to:
> 1) Disable sync on client B.
> 2) Add some items on client A.
> 3) Wait for all the items created on client A to make it to the server.
> 4) Enable sync on client B.
> 5) Wait for client B to come back into sync.
> 
> The tricky part is step 3.  It used to rely on
AwaitCommitActivityCompletion(). 
> But that function has lots of problems, and can't reasonably be supported in
the
> future.
> 
> Client 1 is a hack to work around this problem.  When it is synced with client
> 0, then we can infer that all of client 0's items were committed to the
server. 
> It's ugly, but it's the only solution that doesn't require us to inspect
> internal state (other than the state of the passwords stores).

Right, but why is step 3 necessary at all? It seems step 5 supersedes it.

> 
> The fake server will provide an alternative eventually, but it's not quite
there
> yet and I'm not sure we want to give up on the ability to run these tests
> against an out-of-process server.
> 
> Another option would be to declare these tests to be useless and non-portable
> (they'll never work on Android or iOS) and delete them.  I'd be OK with that
> plan.  I figured keeping them would be the less controversial choice.

[rlarocque, 2014-05-07 17:19:01]

On 2014/05/07 17:07:15, Nicolas Zea wrote:
> On 2014/05/07 00:06:41, rlarocque wrote:
> >
>
https://codereview.chromium.org/267743003/diff/40001/chrome/browser/sync/test...
> > File
> chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/267743003/diff/40001/chrome/browser/sync/test...
> >
>
chrome/browser/sync/test/integration/multiple_client_passwords_sync_test.cc:59:
> > IN_PROC_BROWSER_TEST_F(MultipleClientPasswordsSyncTest, DisablePasswords) {
> > On 2014/05/06 23:46:58, Nicolas Zea wrote:
> > > My point is that these tests don't need to be multiple client tests.
Client
> 1
> > > isn't doing anything useful here, these can be two-client tests.
> > 
> > Actually, client 1 is doing something.
> > 
> > The goal of this particular test is to:
> > 1) Disable sync on client B.
> > 2) Add some items on client A.
> > 3) Wait for all the items created on client A to make it to the server.
> > 4) Enable sync on client B.
> > 5) Wait for client B to come back into sync.
> > 
> > The tricky part is step 3.  It used to rely on
> AwaitCommitActivityCompletion(). 
> > But that function has lots of problems, and can't reasonably be supported in
> the
> > future.
> > 
> > Client 1 is a hack to work around this problem.  When it is synced with
client
> > 0, then we can infer that all of client 0's items were committed to the
> server. 
> > It's ugly, but it's the only solution that doesn't require us to inspect
> > internal state (other than the state of the passwords stores).
> 
> Right, but why is step 3 necessary at all? It seems step 5 supersedes it.
> 

If we don't wait for step 3, then it's possible that we end up with:

1) Disable sync on client B
2) Add some items to client A
3) Enable sync on client B
4) Client A commits to server
5) Client B downloads these commits

Steps 3-5 are a regular sync test that don't prove anything about disabled sync.

[Nicolas Zea, 2014-05-07 17:28:52]

I see. It strikes me then that this test could still succeed even if the client
2 was not disabled, simply because it's not guaranteed that if client 1 has
synced items, client 2 would have attempted to.

Given the nature of these tests as basically generic "not syncing", I'd rather
just remove them altogether. I don't think they're contributing enough.

LGTM otherwise

[rlarocque, 2014-05-07 23:49:32]

Removed the "disable" tests.

[rlarocque, 2014-05-07 23:49:40]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-07 23:53:03]

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

[commit-bot: I haz the power, 2014-05-09 09:38:32]

Change committed as 269198