reland of [sync] Add Sessions integration tests

reland of [sync] Add Sessions integration tests

Add integration tests for a number of new scenarios:
* Multi window, multi tab
* Navigation hierarchy changes
* Tab movement

Add a new checker for waiting to see a SessionsHierarchy on
the FakeServer. Refactor some existing tests to use this instead of
waiting for sync cycle completion then manually verifying a match.

Add several new helper methods to SessionsHelper, and refactor some
existing ones.

Add the ability to open a new Browser from an existing Profile
to SyncTest.

R=zea@chromium.org, skym@chromium.org

BUG=695241
Review-Url: https://codereview.chromium.org/2725813003
Cr-Commit-Position: refs/heads/master@{#454481}
Committed: https://chromium.googlesource.com/chromium/src/+/1a3b9e6a70c04989ec277114f50417164fc9f32f

[Patrick Noland, 2017-03-01 17:59:41]

Description was changed from

==========
reland of [sync] Add Sessions integration tests

Add integration tests for a number of new scenarios:
* Multi window, multi tab
* Navigation hierarchy changes
* Tab movement

Add a new checker for waiting to see a SessionsHierarchy on
the FakeServer. Refactor some existing tests to use this instead of
waiting for sync cycle completion then manually verifying a match.

Add several new helper methods to SessionsHelper, and refactor some
existing ones.

Add the ability to open a new Browser from an existing Profile
to SyncTest.

R=zea@chromium.org, skym@chromium.org

BUG=695241

FREEZE.unindexed


[autofill] Add rogerm@ to OWNERS for autofill components, browser and renderer

Also add sebsg to autofill renderer OWNERS

Review-Url: https://codereview.chromium.org/2703613005
Cr-Commit-Position: refs/heads/master@{#451389}
==========

to

==========
reland of [sync] Add Sessions integration tests

Add integration tests for a number of new scenarios:
* Multi window, multi tab
* Navigation hierarchy changes
* Tab movement

Add a new checker for waiting to see a SessionsHierarchy on
the FakeServer. Refactor some existing tests to use this instead of
waiting for sync cycle completion then manually verifying a match.

Add several new helper methods to SessionsHelper, and refactor some
existing ones.

Add the ability to open a new Browser from an existing Profile
to SyncTest.

R=zea@chromium.org, skym@chromium.org

BUG=695241
==========

[Patrick Noland, 2017-03-01 22:58:24]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 22:58:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2017-03-01 23:46:33]

Nicolas, Sky, PTAL as this contains non-trivial changes

Notably:
- Fixed the order of an EXPECT_GE in ExpectUniqueSampleGE
- Removed the code that added a sync cycle delay to
SingleClientSessionsSyncTests
- Switched CookieJarMismatch to use UpdatedProgressMarkerChecker instead of
WaitForURLOnServer
- Made FragmentURLNavigation deterministic by adding a specific file and serving
it from the embedded_test_server

[commit-bot: I haz the power, 2017-03-02 00:16:12]

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

[commit-bot: I haz the power, 2017-03-02 00:16:13]

Dry run: This issue passed the CQ dry run.

[skym, 2017-03-02 19:56:11]

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:58:
EXPECT_GE(sample_count, expected_inclusive_lower_bound);
When trying to always put the expected value on the left, goes wrong. Thanks for
fixing my mistake.

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:212:
GURL url = embedded_test_server()->GetURL("/fragment.html");
Per our discussion offline, lets use the data: URI thing.

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:216:
embedded_test_server();
Should this line be removed?

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:228:
ASSERT_TRUE(OpenTab(0, first_url));
Since this is apparently flaky, can we completely remove the usage of
non-resolving urls from sync integ tests? Also, lets comment on it somewhere,
maybe in the helper utils file.

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:350:
TriggerSyncForModelTypes(0, syncer::ModelTypeSet(syncer::SESSIONS));
Why the change? Reading this code, it isn't clear what's being done here.

[Patrick Noland, 2017-03-02 20:39:47]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 20:40:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 21:54:10]

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

[commit-bot: I haz the power, 2017-03-02 21:54:11]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-03-02 21:57:45]

PTAL

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:58:
EXPECT_GE(sample_count, expected_inclusive_lower_bound);
On 2017/03/02 19:56:11, skym wrote:
> When trying to always put the expected value on the left, goes wrong. Thanks
for
> fixing my mistake.

Acknowledged.

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:212:
GURL url = embedded_test_server()->GetURL("/fragment.html");
On 2017/03/02 19:56:11, skym wrote:
> Per our discussion offline, lets use the data: URI thing.

Done.

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:216:
embedded_test_server();
On 2017/03/02 19:56:11, skym wrote:
> Should this line be removed?

Done.

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:228:
ASSERT_TRUE(OpenTab(0, first_url));
On 2017/03/02 19:56:11, skym wrote:
> Since this is apparently flaky, can we completely remove the usage of
> non-resolving urls from sync integ tests? Also, lets comment on it somewhere,
> maybe in the helper utils file.

Done.

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:350:
TriggerSyncForModelTypes(0, syncer::ModelTypeSet(syncer::SESSIONS));
On 2017/03/02 19:56:11, skym wrote:
> Why the change? Reading this code, it isn't clear what's being done here.

WaitForURLOnServer, which was used here previously, only waited until the
FakeServer saw a window with the given URL. For this test, we don't care about
that; we want to wait until a sync cycle completes, since that's the event that
triggers the histogram emission.

[skym, 2017-03-02 22:59:43]

lgtm

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:350:
TriggerSyncForModelTypes(0, syncer::ModelTypeSet(syncer::SESSIONS));
On 2017/03/02 21:57:45, Patrick Noland wrote:
> On 2017/03/02 19:56:11, skym wrote:
> > Why the change? Reading this code, it isn't clear what's being done here.
> 
> WaitForURLOnServer, which was used here previously, only waited until the
> FakeServer saw a window with the given URL. For this test, we don't care about
> that; we want to wait until a sync cycle completes, since that's the event
that
> triggers the histogram emission. 

Discussed this offline, I would still prefer the WaitForURLOnServer approach.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:97:
static const char* kURL1 = "data:text/html,<html><title>Test</title></html>";
Can you make these the first thing that's declared in the anonymous namespace?

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:178: //
We want a URL that doesn't 404 and has a non-empty title.
I don't think we want to repeat this every time we use a data: url.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:203: //
We want a URL that doesn't 404 and has a valid fragment.
see above

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:205:
"data:text/html,<html><title>Test</title><body><div "
constant? Also, like what is this thing really testing? Is there special
fragment logic that it exercises? Cause it sure doesn't expect any different
outward behavior than the other tests. You navigate to a different url and see
it show up on the server. I would kind of like this test to be deleted.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:211:
"data:text/html,<html><title>Test</title><body><div "
you get a constant, you get a constant, everyone gets a constant!

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:51: static
const char* kURL1 = "data:text/html,<html><title>Test</title></html>";
Same, move up please.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:68:
base::StringPrintf("data:text/html,<html><title>Test%s</title></html>",
How about this literal is a constant as well?

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:84:
base::StringPrintf("data:text/html,<html><title>Test%s</title></html>",
constant

https://codereview.chromium.org/2725813003/diff/30001/chrome/test/data/sync/f...
File chrome/test/data/sync/fragment.html (right):

https://codereview.chromium.org/2725813003/diff/30001/chrome/test/data/sync/f...
chrome/test/data/sync/fragment.html:1: <html>
Were you going to delete this file now?

[Patrick Noland, 2017-03-02 23:31:20]

Patchset #4 (id:50001) has been deleted

[Patrick Noland, 2017-03-02 23:32:48]

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2725813003/diff/10001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:350:
TriggerSyncForModelTypes(0, syncer::ModelTypeSet(syncer::SESSIONS));
On 2017/03/02 22:59:43, skym wrote:
> On 2017/03/02 21:57:45, Patrick Noland wrote:
> > On 2017/03/02 19:56:11, skym wrote:
> > > Why the change? Reading this code, it isn't clear what's being done here.
> > 
> > WaitForURLOnServer, which was used here previously, only waited until the
> > FakeServer saw a window with the given URL. For this test, we don't care
about
> > that; we want to wait until a sync cycle completes, since that's the event
> that
> > triggers the histogram emission. 
> 
> Discussed this offline, I would still prefer the WaitForURLOnServer approach.

Done.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:97:
static const char* kURL1 = "data:text/html,<html><title>Test</title></html>";
On 2017/03/02 22:59:43, skym wrote:
> Can you make these the first thing that's declared in the anonymous namespace?

Done.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:178: //
We want a URL that doesn't 404 and has a non-empty title.
On 2017/03/02 22:59:43, skym wrote:
> I don't think we want to repeat this every time we use a data: url.

Done.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:203: //
We want a URL that doesn't 404 and has a valid fragment.
On 2017/03/02 22:59:43, skym wrote:
> see above

Done.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:205:
"data:text/html,<html><title>Test</title><body><div "
On 2017/03/02 22:59:43, skym wrote:
> constant? Also, like what is this thing really testing? Is there special
> fragment logic that it exercises? Cause it sure doesn't expect any different
> outward behavior than the other tests. You navigate to a different url and see
> it show up on the server. I would kind of like this test to be deleted.

I think it's fair to check this. Fragments are processed entirely client side
and aren't included in the GET request to the server. This test ensures that
despite this specialness, the entire URL is still synced.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:211:
"data:text/html,<html><title>Test</title><body><div "
On 2017/03/02 22:59:43, skym wrote:
> you get a constant, you get a constant, everyone gets a constant!

Done.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
(right):

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:68:
base::StringPrintf("data:text/html,<html><title>Test%s</title></html>",
On 2017/03/02 22:59:43, skym wrote:
> How about this literal is a constant as well?

Done.

https://codereview.chromium.org/2725813003/diff/30001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:84:
base::StringPrintf("data:text/html,<html><title>Test%s</title></html>",
On 2017/03/02 22:59:43, skym wrote:
> constant

Done.

[Patrick Noland, 2017-03-02 23:32:55]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2017-03-02 23:32:55]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2725813003/#ps70001
(title: "Switch back to WaitForURLOnServer, make everything a constant")

[commit-bot: I haz the power, 2017-03-02 23:33:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 02:52:03]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1488497575056210,
"parent_rev": "fed96bfa74e4035dbfacf50cf7cf7e020283a154", "commit_rev":
"1a3b9e6a70c04989ec277114f50417164fc9f32f"}

[commit-bot: I haz the power, 2017-03-03 02:52:41]

Description was changed from

==========
reland of [sync] Add Sessions integration tests

Add integration tests for a number of new scenarios:
* Multi window, multi tab
* Navigation hierarchy changes
* Tab movement

Add a new checker for waiting to see a SessionsHierarchy on
the FakeServer. Refactor some existing tests to use this instead of
waiting for sync cycle completion then manually verifying a match.

Add several new helper methods to SessionsHelper, and refactor some
existing ones.

Add the ability to open a new Browser from an existing Profile
to SyncTest.

R=zea@chromium.org, skym@chromium.org

BUG=695241
==========

to

==========
reland of [sync] Add Sessions integration tests

Add integration tests for a number of new scenarios:
* Multi window, multi tab
* Navigation hierarchy changes
* Tab movement

Add a new checker for waiting to see a SessionsHierarchy on
the FakeServer. Refactor some existing tests to use this instead of
waiting for sync cycle completion then manually verifying a match.

Add several new helper methods to SessionsHelper, and refactor some
existing ones.

Add the ability to open a new Browser from an existing Profile
to SyncTest.

R=zea@chromium.org, skym@chromium.org

BUG=695241

Review-Url: https://codereview.chromium.org/2725813003
Cr-Commit-Position: refs/heads/master@{#454481}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a3b9e6a70c04989ec277114f504...
==========

[commit-bot: I haz the power, 2017-03-03 02:52:42]

Committed patchset #4 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/1a3b9e6a70c04989ec277114f504...