Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713913002:
[sync] Add Sessions integration tests (Closed)
|
Created:
3 years, 10 months ago by Patrick Noland Modified:
3 years, 9 months ago CC:
chromium-reviews, sync-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[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/2713913002
Cr-Commit-Position: refs/heads/master@{#453687}
Committed: https://chromium.googlesource.com/chromium/src/+/dd0356002ee91f25daa99dd147c57913517e9aab
Patch Set 1 : Update commit message #
Total comments: 68
Patch Set 2 : Response to comments #
Total comments: 20
Patch Set 3 : Response to further comments #Patch Set 4 : Fix TwoClient tests #Messages
Total messages: 49 (32 generated)
Description was changed from ========== [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. FREEZE.unindexed [autofill] Add rogerm@ to OWNERS for autofill components, browser and renderer Also add sebsg to autofill renderer OWNERS BUG= Review-Url: https://codereview.chromium.org/2703613005 Cr-Commit-Position: refs/heads/master@{#451389} ========== to ========== [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. ==========
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== [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. ========== to ========== [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 ==========
pnoland@chromium.org changed reviewers: + skym@chromium.org, zea@chromium.org
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Nicolas, Sky, PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Love the new cases. Having to deal with multiple kinds of indices is unfortunate. So it goes, I suppose. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sessions_helper.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.cc:144: tab_contents->GetController().GoBack(); ->GetController().GoBack(); vs content::WebContents* tab_contents = By assigning a local variable, you're making the line longer. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.cc:253: if (!OpenTab(profile_index, url)) { So you're passing a profile_index in as a browser_index here. If you don't want them to be interchangeable, maybe some of these functions need to take both in as arguments. Or is one way conversions okay, you can go from profile_index -> browser_index, but not necessarily the other way around? If this is the case, you need to document it. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:91: ASSERT_TRUE(GetLocalWindows(browser_index, &windows)); GetLocalWindows takes a profile_index. This doesn't seem valid. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:165: ASSERT_TRUE(WaitForTabsToLoad(0, urls)); Inline the whole thing! https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:316: std::multiset<std::string>({base_url.spec(), new_tab_url.spec()})); Do you need to specify std::multiset<std::string> ? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:336: new_hierarchy.AddWindow({base_url.spec()}); Why the {}? It seems like it should do the same thing. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:364: SessionsHierarchy changed_hierarchy; Doesn't this make you want to have a constructor for SessionsHierarchy that could take an initializer list of std::strings and/or std::multiset<std::string>s? It could all get inlined! https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:455: browsers_.resize(browsers_.size() + 1); why not browsers_.push_back(new Browser(...)); ? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:459: Browser* browser = GetBrowser(index); But. What? You just made this thing, why fetch it again? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:460: EXPECT_NE(nullptr, browser) << "Could not create Browser " << index; So when does this happen? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:461: profiles_[index] = profile; I'm confused, is browsers_.size() allowed to != profiles_.size() or not? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:141: Browser* GetBrowser(int index) WARN_UNUSED_RESULT; Why are all these methods annotated with WARN_UNUSED_RESULT? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:146: // done so that the profile associated with the new browser can be found at What does it mean for there to be two Browsers that share a Profile? Is this the same or different from when I have two windows open for a single profile? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:38: std::vector<ScopedWindowMap> expected_windows(1); Wow, you really seem to like reserving space. What about using an initializer list instead? EXPECT_TRUE(ForeignSessionsMatchChecker(profile_index, {std::move(windows)})); https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:40: ASSERT_TRUE( EXPECT_TRUE? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:157: ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows)); This pattern seems fairly awkward. It shows up over and over here. ScopedWindowMap client0_windows; ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows)); WaitForWindowsInForeignSession(1, std::move(client0_windows)); I'd prefer to see something like EXPECT_TRUE(OpenTab(0, GURL(kURL2)); WaitForForeignSessionsToSync({0, 1}, FROM_HERE); Don't feel like you have to do this though. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:162: WaitForWindowsInForeignSession(0, std::move(client0_windows)); You're moving client0_windows twice. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:236: EXPECT_EQ(client0_windows.size(), 2u); https://github.com/google/googletest/blob/master/googletest/docs/Primer.md "Historical note: Before February 2016 *_EQ had a convention of calling it as ASSERT_EQ(expected, actual), so lots of existing code uses this order. Now *_EQ treats both parameters in the same way." I guess they don't care anymore, I liked it the way things were!
https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sessions_helper.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.cc:253: if (!OpenTab(profile_index, url)) { On 2017/02/24 00:14:09, skym wrote: > So you're passing a profile_index in as a browser_index here. If you don't want > them to be interchangeable, maybe some of these functions need to take both in > as arguments. > > Or is one way conversions okay, you can go from profile_index -> browser_index, > but not necessarily the other way around? If this is the case, you need to > document it. This is kind of ugly. SessionsHelper deeply assumes the exact correspondence between browser index and profile index. That's why every function used to just take a generic "index" parameter. Adding multiple browsers per profile creates the potential to violate this assumption. To mitigate this, when you add a new browser with AddBrowser, it copies the corresponding profile pointer into the same position as the new browser. So if you create a new browser using profile 0, there will be two distinct browsers: one in spot 0 and spot 1. There will be two profile pointers pointing to a single distinct profile, one in spot 0 and one in spot 1. So specifying 1 as a profile index will work fine. This is confusing, since there's only one distinct profile. I've tried to insulate callers from these confusing details a little bit by having some functions take in a profile index and others take in a browser index. Functions that take in a profile index operate on whichever browser happens to live in the corresponding spot. Functions that take in a browser index operate on the specified browser. In practice this is fine since the only tests that create multiple browsers per profile operate using browser indexes. I will think about how to make all this more clear to future users of these classes. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:91: ASSERT_TRUE(GetLocalWindows(browser_index, &windows)); On 2017/02/24 00:14:09, skym wrote: > GetLocalWindows takes a profile_index. This doesn't seem valid. Good catch. This works in practice for reasons explained above, but the name should be updated. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:316: std::multiset<std::string>({base_url.spec(), new_tab_url.spec()})); On 2017/02/24 00:14:09, skym wrote: > Do you need to specify std::multiset<std::string> ? The compiler complains if you don't :/ https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:364: SessionsHierarchy changed_hierarchy; On 2017/02/24 00:14:09, skym wrote: > Doesn't this make you want to have a constructor for SessionsHierarchy that > could take an initializer list of std::strings and/or > std::multiset<std::string>s? It could all get inlined! Yes it does, I'll just add that https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:455: browsers_.resize(browsers_.size() + 1); On 2017/02/24 00:14:09, skym wrote: > why not browsers_.push_back(new Browser(...)); ? Because I'm copying the bizarre pattern used elsewhere in this file. I'll fix it. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:459: Browser* browser = GetBrowser(index); On 2017/02/24 00:14:09, skym wrote: > But. What? You just made this thing, why fetch it again? Because I'm copying the bizarre pattern used elsewhere in this file. I'll fix it. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:460: EXPECT_NE(nullptr, browser) << "Could not create Browser " << index; On 2017/02/24 00:14:09, skym wrote: > So when does this happen? Good question. The old code seemed to think it was possible so I'm loath to remove it. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:461: profiles_[index] = profile; On 2017/02/24 00:14:09, skym wrote: > I'm confused, is browsers_.size() allowed to != profiles_.size() or not? No, they should always match. There's a longwinded explanation above, but the tldr is we copy the profile pointer so that the correspondence in size and position is maintained. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:141: Browser* GetBrowser(int index) WARN_UNUSED_RESULT; On 2017/02/24 00:14:09, skym wrote: > Why are all these methods annotated with WARN_UNUSED_RESULT? Good question, I'll check into that. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:146: // done so that the profile associated with the new browser can be found at On 2017/02/24 00:14:09, skym wrote: > What does it mean for there to be two Browsers that share a Profile? Is this the > same or different from when I have two windows open for a single profile? It's having two windows open for the same profile.
https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sessions_helper.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.cc:253: if (!OpenTab(profile_index, url)) { On 2017/02/24 01:00:40, Patrick Noland wrote: > On 2017/02/24 00:14:09, skym wrote: > > So you're passing a profile_index in as a browser_index here. If you don't > want > > them to be interchangeable, maybe some of these functions need to take both in > > as arguments. > > > > Or is one way conversions okay, you can go from profile_index -> > browser_index, > > but not necessarily the other way around? If this is the case, you need to > > document it. > > This is kind of ugly. SessionsHelper deeply assumes the exact correspondence > between browser index and profile index. That's why every function used to just > take a generic "index" parameter. Adding multiple browsers per profile creates > the potential to violate this assumption. > > To mitigate this, when you add a new browser with AddBrowser, it copies the > corresponding profile pointer into the same position as the new browser. So if > you create a new browser using profile 0, there will be two distinct browsers: > one in spot 0 and spot 1. There will be two profile pointers pointing to a > single distinct profile, one in spot 0 and one in spot 1. So specifying 1 as a > profile index will work fine. This is confusing, since there's only one distinct > profile. > > I've tried to insulate callers from these confusing details a little bit by > having some functions take in a profile index and others take in a browser > index. Functions that take in a profile index operate on whichever browser > happens to live in the corresponding spot. Functions that take in a browser > index operate on the specified browser. In practice this is fine since the only > tests that create multiple browsers per profile operate using browser indexes. > > I will think about how to make all this more clear to future users of these > classes. When you have some functions take a profile index and others take a browser index, you're doing the exact opposite of insulating callers from this complexity. You're making it their problem, and forcing them to understand this. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:316: std::multiset<std::string>({base_url.spec(), new_tab_url.spec()})); On 2017/02/24 01:00:40, Patrick Noland wrote: > On 2017/02/24 00:14:09, skym wrote: > > Do you need to specify std::multiset<std::string> ? > > The compiler complains if you don't :/ But, why? Also, I don't think you need () and {} even if you are forced to specify multiset https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:364: SessionsHierarchy changed_hierarchy; On 2017/02/24 01:00:40, Patrick Noland wrote: > On 2017/02/24 00:14:09, skym wrote: > > Doesn't this make you want to have a constructor for SessionsHierarchy that > > could take an initializer list of std::strings and/or > > std::multiset<std::string>s? It could all get inlined! > > Yes it does, I'll just add that Bonus points for making a fancy one that can take a combo of strings and multisets https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:423: LOG(FATAL) << "SetupClients() has not yet been called."; Can these be EXPECT_* and not LOG(FATAL)? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:426: return profiles_[index]; What would happen if you checked for nullptr here? Instead of at every call site. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:445: return browsers_[index]; Same, can we check for nullptr? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:459: Browser* browser = GetBrowser(index); On 2017/02/24 01:00:40, Patrick Noland wrote: > On 2017/02/24 00:14:09, skym wrote: > > But. What? You just made this thing, why fetch it again? > > Because I'm copying the bizarre pattern used elsewhere in this file. I'll fix > it. ...Is this a memory leak? Who owns this Browser object? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:460: EXPECT_NE(nullptr, browser) << "Could not create Browser " << index; On 2017/02/24 01:00:40, Patrick Noland wrote: > On 2017/02/24 00:14:09, skym wrote: > > So when does this happen? > > Good question. The old code seemed to think it was possible so I'm loath to > remove it. This is test code, worst case a test fails. Remove it!
Some initial comments https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sessions_helper.h (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:39: // Copies the local session windows of profileat |profile_index| to nit: profileat https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:103: bool OpenTab(int browser_index, const GURL& url); I think window_index might be clearer than browser_index? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:105: // See OpenTab, except that the tab is opened in position |tab_index|. Comment about behavior if tab_index is greater than the number of tabs? (same with the next function) https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:168: class SessionHierarchyMatchChecker : public SingleClientStatusChangeChecker { nit: prefer having this in its own file. It's confusing to see a class like this used elsewhere, only to have to grep to find out which file its in. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:172: const fake_server::SessionsHierarchy& sessions_hierarchy, nit: prefer having const inputs at start, and mutable inputs after (following the inputs first, outputs last convention) https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:180: const fake_server::SessionsHierarchy& sessions_hierarchy_; Is sessions_hierarchy_ guaranteed to outlive this class? Assuming that seems brittle. Why not make a copy?
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Patchset #2 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sessions_helper.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.cc:144: tab_contents->GetController().GoBack(); On 2017/02/24 00:14:09, skym wrote: > ->GetController().GoBack(); > vs > content::WebContents* tab_contents = > > By assigning a local variable, you're making the line longer. Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.cc:253: if (!OpenTab(profile_index, url)) { On 2017/02/24 16:20:17, skym wrote: > On 2017/02/24 01:00:40, Patrick Noland wrote: > > On 2017/02/24 00:14:09, skym wrote: > > > So you're passing a profile_index in as a browser_index here. If you don't > > want > > > them to be interchangeable, maybe some of these functions need to take both > in > > > as arguments. > > > > > > Or is one way conversions okay, you can go from profile_index -> > > browser_index, > > > but not necessarily the other way around? If this is the case, you need to > > > document it. > > > > This is kind of ugly. SessionsHelper deeply assumes the exact correspondence > > between browser index and profile index. That's why every function used to > just > > take a generic "index" parameter. Adding multiple browsers per profile creates > > the potential to violate this assumption. > > > > To mitigate this, when you add a new browser with AddBrowser, it copies the > > corresponding profile pointer into the same position as the new browser. So if > > you create a new browser using profile 0, there will be two distinct browsers: > > one in spot 0 and spot 1. There will be two profile pointers pointing to a > > single distinct profile, one in spot 0 and one in spot 1. So specifying 1 as a > > profile index will work fine. This is confusing, since there's only one > distinct > > profile. > > > > I've tried to insulate callers from these confusing details a little bit by > > having some functions take in a profile index and others take in a browser > > index. Functions that take in a profile index operate on whichever browser > > happens to live in the corresponding spot. Functions that take in a browser > > index operate on the specified browser. In practice this is fine since the > only > > tests that create multiple browsers per profile operate using browser indexes. > > > > > I will think about how to make all this more clear to future users of these > > classes. > > When you have some functions take a profile index and others take a browser > index, you're doing the exact opposite of insulating callers from this > complexity. You're making it their problem, and forcing them to understand this. Discussed offline, I went back to just naming everything index. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sessions_helper.h (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:39: // Copies the local session windows of profileat |profile_index| to On 2017/02/24 20:25:30, Nicolas Zea wrote: > nit: profileat Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:103: bool OpenTab(int browser_index, const GURL& url); On 2017/02/24 20:25:29, Nicolas Zea wrote: > I think window_index might be clearer than browser_index? After discussing with sky, we think just "index" is fine. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:105: // See OpenTab, except that the tab is opened in position |tab_index|. On 2017/02/24 20:25:30, Nicolas Zea wrote: > Comment about behavior if tab_index is greater than the number of tabs? (same > with the next function) Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:168: class SessionHierarchyMatchChecker : public SingleClientStatusChangeChecker { On 2017/02/24 20:25:30, Nicolas Zea wrote: > nit: prefer having this in its own file. It's confusing to see a class like this > used elsewhere, only to have to grep to find out which file its in. Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:172: const fake_server::SessionsHierarchy& sessions_hierarchy, On 2017/02/24 20:25:30, Nicolas Zea wrote: > nit: prefer having const inputs at start, and mutable inputs after (following > the inputs first, outputs last convention) Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sessions_helper.h:180: const fake_server::SessionsHierarchy& sessions_hierarchy_; On 2017/02/24 20:25:29, Nicolas Zea wrote: > Is sessions_hierarchy_ guaranteed to outlive this class? Assuming that seems > brittle. Why not make a copy? Done, although this requires an out of line copy constructor. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:91: ASSERT_TRUE(GetLocalWindows(browser_index, &windows)); On 2017/02/24 00:14:09, skym wrote: > GetLocalWindows takes a profile_index. This doesn't seem valid. I ended up just removing this parameter, which is pointless for a single client test to supply. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:165: ASSERT_TRUE(WaitForTabsToLoad(0, urls)); On 2017/02/24 00:14:09, skym wrote: > Inline the whole thing! Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:316: std::multiset<std::string>({base_url.spec(), new_tab_url.spec()})); On 2017/02/24 16:20:17, skym wrote: > On 2017/02/24 01:00:40, Patrick Noland wrote: > > On 2017/02/24 00:14:09, skym wrote: > > > Do you need to specify std::multiset<std::string> ? > > > > The compiler complains if you don't :/ > > But, why? Also, I don't think you need () and {} even if you are forced to > specify multiset This is replaced with the initializer_list now, although the braces are required for that https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:336: new_hierarchy.AddWindow({base_url.spec()}); On 2017/02/24 00:14:09, skym wrote: > Why the {}? It seems like it should do the same thing. Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:364: SessionsHierarchy changed_hierarchy; On 2017/02/24 16:20:17, skym wrote: > On 2017/02/24 01:00:40, Patrick Noland wrote: > > On 2017/02/24 00:14:09, skym wrote: > > > Doesn't this make you want to have a constructor for SessionsHierarchy that > > > could take an initializer list of std::strings and/or > > > std::multiset<std::string>s? It could all get inlined! > > > > Yes it does, I'll just add that > > Bonus points for making a fancy one that can take a combo of strings and > multisets That requires a variadic template, which should be used judiciously, so I'm going to stick to a regular initializer_list https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:423: LOG(FATAL) << "SetupClients() has not yet been called."; On 2017/02/24 16:20:17, skym wrote: > Can these be EXPECT_* and not LOG(FATAL)? Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:426: return profiles_[index]; On 2017/02/24 16:20:17, skym wrote: > What would happen if you checked for nullptr here? Instead of at every call > site. Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:445: return browsers_[index]; On 2017/02/24 16:20:17, skym wrote: > Same, can we check for nullptr? Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:459: Browser* browser = GetBrowser(index); On 2017/02/24 16:20:17, skym wrote: > On 2017/02/24 01:00:40, Patrick Noland wrote: > > On 2017/02/24 00:14:09, skym wrote: > > > But. What? You just made this thing, why fetch it again? > > > > Because I'm copying the bizarre pattern used elsewhere in this file. I'll fix > > it. > > ...Is this a memory leak? Who owns this Browser object? According to sync_test.h "Browser object lifetime is managed by BrowserList, so we don't use a ScopedVector here." https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:141: Browser* GetBrowser(int index) WARN_UNUSED_RESULT; On 2017/02/24 00:14:09, skym wrote: > Why are all these methods annotated with WARN_UNUSED_RESULT? I removed them all and it still compiles. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:38: std::vector<ScopedWindowMap> expected_windows(1); On 2017/02/24 00:14:09, skym wrote: > Wow, you really seem to like reserving space. What about using an initializer > list instead? > > EXPECT_TRUE(ForeignSessionsMatchChecker(profile_index, {std::move(windows)})); I actually tried this. You can't (trivially) put unique_ptrs into initializer lists like that unfortunately. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:40: ASSERT_TRUE( On 2017/02/24 00:14:09, skym wrote: > EXPECT_TRUE? Done. https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:157: ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows)); On 2017/02/24 00:14:09, skym wrote: > This pattern seems fairly awkward. It shows up over and over here. > > ScopedWindowMap client0_windows; > ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows)); > WaitForWindowsInForeignSession(1, std::move(client0_windows)); > > I'd prefer to see something like > > EXPECT_TRUE(OpenTab(0, GURL(kURL2)); > WaitForForeignSessionsToSync({0, 1}, FROM_HERE); > > Don't feel like you have to do this though. I've gone ahead and added a helper to do this, although I'm not sure what the FOR_HERE is for? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:162: WaitForWindowsInForeignSession(0, std::move(client0_windows)); On 2017/02/24 00:14:09, skym wrote: > You're moving client0_windows twice. Done.
https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:38: std::vector<ScopedWindowMap> expected_windows(1); On 2017/02/27 18:53:26, Patrick Noland wrote: > On 2017/02/24 00:14:09, skym wrote: > > Wow, you really seem to like reserving space. What about using an initializer > > list instead? > > > > EXPECT_TRUE(ForeignSessionsMatchChecker(profile_index, {std::move(windows)})); > > I actually tried this. You can't (trivially) put unique_ptrs into initializer > lists like that unfortunately. Why? https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:157: ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows)); On 2017/02/27 18:53:26, Patrick Noland wrote: > On 2017/02/24 00:14:09, skym wrote: > > This pattern seems fairly awkward. It shows up over and over here. > > > > ScopedWindowMap client0_windows; > > ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows)); > > WaitForWindowsInForeignSession(1, std::move(client0_windows)); > > > > I'd prefer to see something like > > > > EXPECT_TRUE(OpenTab(0, GURL(kURL2)); > > WaitForForeignSessionsToSync({0, 1}, FROM_HERE); > > > > Don't feel like you have to do this though. > > I've gone ahead and added a helper to do this, although I'm not sure what the > FOR_HERE is for? So, we have a problem with our integ test checkers, in that when you fail a build, often there isn't enough information to figure out what went wrong. I actually made this problem a bunch worse when I moved the critical portions of waiting into a base class in https://codereview.chromium.org/2379433002/ . I'd really like test failures to contain exactly which wait they were, and my thoughts were that FROM_HERE would facilitate that. It's still a half baked idea, I haven't tried any of this yet. Feel free to ignore for now.
lgtm https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:38: std::vector<ScopedWindowMap> expected_windows(1); On 2017/02/27 19:12:27, skym wrote: > On 2017/02/27 18:53:26, Patrick Noland wrote: > > On 2017/02/24 00:14:09, skym wrote: > > > Wow, you really seem to like reserving space. What about using an > initializer > > > list instead? > > > > > > EXPECT_TRUE(ForeignSessionsMatchChecker(profile_index, > {std::move(windows)})); > > > > I actually tried this. You can't (trivially) put unique_ptrs into initializer > > lists like that unfortunately. > > Why? Okay, you're right, and things get confusing really fast. http://stackoverflow.com/questions/8468774/can-i-list-initialize-a-vector-of-... http://stackoverflow.com/questions/7231351/initializer-list-constructing-a-ve... https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/sessions_helper.h (right): https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sessions_helper.h:31: // |local_windows|. Returns true if successful. This now wraps too early. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sessions_helper.h:85: // If |tab_index| is -1 or greater than the number of tabs, the tab will be Do we need to supposed -1 or greater? Can we just crash bad tests that do this? Oh it looks like this is just the default behavior of chrome::AddTabAt? Does it make sense to document this here? Do we use this? https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sessions_helper.h:92: void MoveTab(int from_index, int to_index, int tab_index); Do all these navigate methods block? https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/sync_test.h (left): https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sync_test.h:172: virtual bool SetupClients() WARN_UNUSED_RESULT; I think WARN_UNUSED_RESULT is good here. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sync_test.h:175: virtual bool SetupSync() WARN_UNUSED_RESULT; Same. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:36: std::vector<ScopedWindowMap> expected_windows(1); I think still push_back is more clean than this pattern. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:51: static const char* kURL1 = "http://127.0.0.1/bubba1"; kinda odd how the one client test doesn't use constants for these. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:213: ASSERT_TRUE(OpenTabAtIndex(2, 2, GURL(kURL4))); Still feels like a lot of assert that could be expect, through this whole file. https://codereview.chromium.org/2713913002/diff/100001/components/sync/test/f... File components/sync/test/fake_server/sessions_hierarchy.cc (right): https://codereview.chromium.org/2713913002/diff/100001/components/sync/test/f... components/sync/test/fake_server/sessions_hierarchy.cc:22: SessionsHierarchy::Window window; This could be re-written with initializer lists, right? :)
https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:38: std::vector<ScopedWindowMap> expected_windows(1); On 2017/02/27 19:12:27, skym wrote: > On 2017/02/27 18:53:26, Patrick Noland wrote: > > On 2017/02/24 00:14:09, skym wrote: > > > Wow, you really seem to like reserving space. What about using an > initializer > > > list instead? > > > > > > EXPECT_TRUE(ForeignSessionsMatchChecker(profile_index, > {std::move(windows)})); > > > > I actually tried this. You can't (trivially) put unique_ptrs into initializer > > lists like that unfortunately. > > Why? initializer lists don't work with move only types. They provide read only sequences, and the proposal explicitly for them explicitly states the desire to share identical initializer_list sequences by reusing the same instance. As far as I can tell this isn't an intentional feature, just the result of designing initializer lists well before move semantics were formalized. There is some background here: http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4166.pdf
Just one nit, otherwise LGTM with Skys' comment's addressed. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:89: void ExpectNavigationChain(const std::vector<GURL> urls) { nit: pass by const ref
https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/sessions_helper.h (right): https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sessions_helper.h:31: // |local_windows|. Returns true if successful. On 2017/02/27 19:42:50, skym wrote: > This now wraps too early. Done. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sessions_helper.h:85: // If |tab_index| is -1 or greater than the number of tabs, the tab will be On 2017/02/27 19:42:50, skym wrote: > Do we need to supposed -1 or greater? Can we just crash bad tests that do this? > Oh it looks like this is just the default behavior of chrome::AddTabAt? Does it > make sense to document this here? Do we use this? We don't use it but I wanted to honestly document the behavior. It's not inconceivable someone would want to use it, is it? https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sessions_helper.h:92: void MoveTab(int from_index, int to_index, int tab_index); On 2017/02/27 19:42:50, skym wrote: > Do all these navigate methods block? No, of the Navigate* functions, only NavigateTab blocks. I'll document that. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:89: void ExpectNavigationChain(const std::vector<GURL> urls) { On 2017/02/27 23:20:06, Nicolas Zea wrote: > nit: pass by const ref Done. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/sync_test.h (left): https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sync_test.h:172: virtual bool SetupClients() WARN_UNUSED_RESULT; On 2017/02/27 19:42:50, skym wrote: > I think WARN_UNUSED_RESULT is good here. Done. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/sync_test.h:175: virtual bool SetupSync() WARN_UNUSED_RESULT; On 2017/02/27 19:42:50, skym wrote: > Same. Done. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc (right): https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:36: std::vector<ScopedWindowMap> expected_windows(1); On 2017/02/27 19:42:50, skym wrote: > I think still push_back is more clean than this pattern. Done. https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:51: static const char* kURL1 = "http://127.0.0.1/bubba1"; On 2017/02/27 19:42:50, skym wrote: > kinda odd how the one client test doesn't use constants for these. Agreed https://codereview.chromium.org/2713913002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:213: ASSERT_TRUE(OpenTabAtIndex(2, 2, GURL(kURL4))); On 2017/02/27 19:42:50, skym wrote: > Still feels like a lot of assert that could be expect, through this whole file. Sometimes yes, although there are cases where if the operation fails you don't want to keep going; the subsequent operations will either crash or their results will be completely meaningless. I'll convert the asserts in this test to expects, but it can't be applied universally. https://codereview.chromium.org/2713913002/diff/100001/components/sync/test/f... File components/sync/test/fake_server/sessions_hierarchy.cc (right): https://codereview.chromium.org/2713913002/diff/100001/components/sync/test/f... components/sync/test/fake_server/sessions_hierarchy.cc:22: SessionsHierarchy::Window window; On 2017/02/27 19:42:50, skym wrote: > This could be re-written with initializer lists, right? :) Done.
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org, skym@chromium.org Link to the patchset: https://codereview.chromium.org/2713913002/#ps120001 (title: "Response to further comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org, skym@chromium.org Link to the patchset: https://codereview.chromium.org/2713913002/#ps140001 (title: "Fix TwoClient tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488309153865460,
"parent_rev": "dbe75656dc50037306c8a3b6968a5592f4b17106", "commit_rev":
"dd0356002ee91f25daa99dd147c57913517e9aab"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== [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/2713913002 Cr-Commit-Position: refs/heads/master@{#453687} Committed: https://chromium.googlesource.com/chromium/src/+/dd0356002ee91f25daa99dd147c5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/dd0356002ee91f25daa99dd147c5...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:140001) has been created in https://codereview.chromium.org/2722953002/ by sergeyu@chromium.org. The reason for reverting is: SingleClientSessionsSyncTest.CookieJarMismatch fails on mac: https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/37124.
Message was sent while issue was closed.
note also FragmentURLNavigation was flaky, i.e. see https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel... https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel... |
