Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706343004:
[Sync] Refactor SessionsSyncManager unit tests (Closed)
|
Created:
3 years, 10 months ago by Nicolas Zea Modified:
3 years, 10 months ago Reviewers:
skym CC:
chromium-reviews, sync-reviews_chromium.org, Patrick Noland Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Refactor SessionsSyncManager unit tests
Tests no longer rely on BrowserWithTestTestWindowTest, and instead inject
all the relevant navigation events directly. This allows for much more control
over what gets tested, and in particular allows for testing situations with
multiple windows.
As part of this cleanup, various helper methods were added to make the tests
simpler and more readable. A couple outdated tests were also removed.
Lastly, because there are no longer any content dependencies, the test was
moved to components/sync_sessions, to live next to the code it's actually
testing (SessionsSyncManager).
BUG=639009, 671283
Review-Url: https://codereview.chromium.org/2706343004
Cr-Commit-Position: refs/heads/master@{#452320}
Committed: https://chromium.googlesource.com/chromium/src/+/5bdcd6fff79307e080a87ee417960f2e2141dc24
Patch Set 1 #Patch Set 2 : Move to components #
Total comments: 40
Patch Set 3 : Fix mobile compile #Patch Set 4 : Address comments #
Messages
Total messages: 19 (13 generated)
Description was changed from ========== [Sync] Refactor SessionsSyncManager unit tests Tests no longer rely on BrowserWithTestTestWindowTest, and instead inject all the relevant navigation events directly. This allows for much more control over what gets tested, and in particular allows for testing situations with multiple windows. As part of this cleanup, various helper methods were added to make the tests simpler and more readable. A couple outdated tests were also removed. BUG=639009 ========== to ========== [Sync] Refactor SessionsSyncManager unit tests Tests no longer rely on BrowserWithTestTestWindowTest, and instead inject all the relevant navigation events directly. This allows for much more control over what gets tested, and in particular allows for testing situations with multiple windows. As part of this cleanup, various helper methods were added to make the tests simpler and more readable. A couple outdated tests were also removed. Lastly, because there are no longer any content dependencies, the test was moved to components/sync_sessions, to live next to the code it's actually testing (SessionsSyncManager). BUG=639009 ==========
The CQ bit was checked by zea@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...
Description was changed from ========== [Sync] Refactor SessionsSyncManager unit tests Tests no longer rely on BrowserWithTestTestWindowTest, and instead inject all the relevant navigation events directly. This allows for much more control over what gets tested, and in particular allows for testing situations with multiple windows. As part of this cleanup, various helper methods were added to make the tests simpler and more readable. A couple outdated tests were also removed. Lastly, because there are no longer any content dependencies, the test was moved to components/sync_sessions, to live next to the code it's actually testing (SessionsSyncManager). BUG=639009 ========== to ========== [Sync] Refactor SessionsSyncManager unit tests Tests no longer rely on BrowserWithTestTestWindowTest, and instead inject all the relevant navigation events directly. This allows for much more control over what gets tested, and in particular allows for testing situations with multiple windows. As part of this cleanup, various helper methods were added to make the tests simpler and more readable. A couple outdated tests were also removed. Lastly, because there are no longer any content dependencies, the test was moved to components/sync_sessions, to live next to the code it's actually testing (SessionsSyncManager). BUG=639009, 671283 ==========
zea@chromium.org changed reviewers: + skym@chromium.org
PTAL! https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... File components/sync_sessions/sessions_sync_manager_unittest.cc (left): https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:955: TEST_F(SessionsSyncManagerTest, SwappedOutOnRestore) { This test was moved to the bottom of the file (along with the other restore related test). https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:2300: TEST_F(SessionsSyncManagerTest, CheckPrerenderedWebContentsSwap) { This was deleted as the SessionSyncManager no longer has any dependencies directly on the webcontents, so it's not actually testing any relevant logic.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by zea@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...
Awesome cleanup! lgtm https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... File components/sync_sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:58: const base::Time kTime0 = base::Time::FromInternalValue(100); Is there a reason this isn't 0,1,2,3,... ? Just to make it more obvious you hit timestamps if you print them out or something? https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:92: for (const SyncChange& change : changes) { What? No fancy lambda? Could use find_if :) https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:102: testing::AssertionResult ChangeTypeMatches( This method is kind of odd. I don't like that we're tying each test case to a specific ordering of changes. Something like map<url, vector<change type>> would be reasonable, though i guess it doesn't work for headers. multi_set<change type> would also make me feel better. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:106: if (types.size() != static_cast<size_t>(std::count_if( std::any_if with a negated version of the body would exit early. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:110: return change.change_type() == *types_iter++; Erm, if |types| is shorter than |changes|, bad things happen here, right? And |types| could have extra elements on the end and we wouldn't notice? https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:582: syncer::SyncError(FROM_HERE, syncer::SyncError::DATATYPE_ERROR, "Error", using syncer::SyncError? https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:595: ASSERT_EQ(manager()->current_machine_tag(), data.GetTag()); Lot of asserts, should some of these be expects? https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:726: std::unique_ptr<sessions::SerializedNavigationEntry> entry( What do you think of auto entry = base::MakeUnique<..>(..); https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:849: const int kNavs = 10; This naming seems odd to me. I always thought the kMixedCase naming was for static constants. Looking at our style guide https://google.github.io/styleguide/cppguide.html#Constant_Names it seems the kMixedCase is optional in this scenario. So free to keep. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:917: std::unique_ptr<sessions::SerializedNavigationEntry> entry3( MakeUnique https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1000: std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1), This happens over and over. Are we not allowed to have const vectors? https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1643: SyncDataList in = {CreateRemoteData(specifics)}; Might be able to inline this without causing a line wrap. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1765: for (size_t i = 0; i < kNumTabs; ++i) { I find all of this really confusing and difficult to follow. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1766: int index = kChangesPerTab * i; This really confuses me. So |out| is presumably of size 12, right? And we're about to go through two iterations of the above for loop. The first time through, |index| starts at 0. The second time, |index| starts at 2? Shouldn't it start at 6? https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1770: VerifyLocalHeaderChange(out[index++], (i == 0 ? 0 : 1), isn't i == 0 ? 0 : 1 the same as just i when i only goes from 0 to 1? https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1771: i); // No-op header update for parented tab. Is this comment talking about the i param or the entire line? Maybe it would be cleaner if this comment was above the code line instead of trailing. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1776: index++; // Ignore the tab add, which has no data. I think this would make more sense to put between this and the above scopes, or just in the first scope. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1867: sync_pb::SessionSpecifics meta( What do you think of using SessionSpecifics; ? Also, why is this variable name meta and meta2 below? I don't understand.
Comments addressed! https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... File components/sync_sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:58: const base::Time kTime0 = base::Time::FromInternalValue(100); On 2017/02/22 22:25:15, skym wrote: > Is there a reason this isn't 0,1,2,3,... ? Just to make it more obvious you hit > timestamps if you print them out or something? Exactly. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:92: for (const SyncChange& change : changes) { On 2017/02/22 22:25:11, skym wrote: > What? No fancy lambda? Could use find_if :) Good call! https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:102: testing::AssertionResult ChangeTypeMatches( On 2017/02/22 22:25:11, skym wrote: > This method is kind of odd. I don't like that we're tying each test case to a > specific ordering of changes. Something like map<url, vector<change type>> would > be reasonable, though i guess it doesn't work for headers. multi_set<change > type> would also make me feel better. But the ordering matters. If we for example do an update for something before adding it, the real change processor (GenericChangeProcessor) will trigger an error). https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:106: if (types.size() != static_cast<size_t>(std::count_if( On 2017/02/22 22:25:11, skym wrote: > std::any_if with a negated version of the body would exit early. Nice, done. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:110: return change.change_type() == *types_iter++; On 2017/02/22 22:25:12, skym wrote: > Erm, if |types| is shorter than |changes|, bad things happen here, right? And > |types| could have extra elements on the end and we wouldn't notice? Fixed by adding an extra size condition (coupled with the any_if) https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:582: syncer::SyncError(FROM_HERE, syncer::SyncError::DATATYPE_ERROR, "Error", On 2017/02/22 22:25:11, skym wrote: > using syncer::SyncError? Done. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:595: ASSERT_EQ(manager()->current_machine_tag(), data.GetTag()); On 2017/02/22 22:25:18, skym wrote: > Lot of asserts, should some of these be expects? Done. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:726: std::unique_ptr<sessions::SerializedNavigationEntry> entry( On 2017/02/22 22:25:13, skym wrote: > What do you think of > > auto entry = base::MakeUnique<..>(..); Done. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:849: const int kNavs = 10; On 2017/02/22 22:25:14, skym wrote: > This naming seems odd to me. I always thought the kMixedCase naming was for > static constants. Looking at our style guide > https://google.github.io/styleguide/cppguide.html#Constant_Names it seems the > kMixedCase is optional in this scenario. So free to keep. Acknowledged. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:917: std::unique_ptr<sessions::SerializedNavigationEntry> entry3( On 2017/02/22 22:25:20, skym wrote: > MakeUnique Done. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1000: std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1), On 2017/02/22 22:25:12, skym wrote: > This happens over and over. Are we not allowed to have const vectors? No, static consts must be POD. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1643: SyncDataList in = {CreateRemoteData(specifics)}; On 2017/02/22 22:25:11, skym wrote: > Might be able to inline this without causing a line wrap. Done. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1765: for (size_t i = 0; i < kNumTabs; ++i) { On 2017/02/22 22:25:11, skym wrote: > I find all of this really confusing and difficult to follow. I added some comments to try to clarify what this is verifying. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1766: int index = kChangesPerTab * i; On 2017/02/22 22:25:11, skym wrote: > This really confuses me. So |out| is presumably of size 12, right? And we're > about to go through two iterations of the above for loop. The first time > through, |index| starts at 0. The second time, |index| starts at 2? Shouldn't it > start at 6? It does start at 6. kChangesPerTab is 6 (2 + 4) https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1770: VerifyLocalHeaderChange(out[index++], (i == 0 ? 0 : 1), True, but that then is based on the assumption that kNumTabs will always be 2. In this case this should only be 0 on the first iteration, regardless of the total number of iterations. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1771: i); // No-op header update for parented tab. On 2017/02/22 22:25:12, skym wrote: > Is this comment talking about the i param or the entire line? Maybe it would be > cleaner if this comment was above the code line instead of trailing. No, the entire line. Moved the comments for this and the others. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1776: index++; // Ignore the tab add, which has no data. On 2017/02/22 22:25:19, skym wrote: > I think this would make more sense to put between this and the above scopes, or > just in the first scope. Done. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1867: sync_pb::SessionSpecifics meta( On 2017/02/22 22:25:17, skym wrote: > What do you think of using SessionSpecifics; ? Almost all of the others "using" are for syncer::. Given sync_pb is different, and implies proto specific semantics, I find it clearer to keep around. > > Also, why is this variable name meta and meta2 below? I don't understand. Just due to the old code. Updated to better names.
The CQ bit was checked by zea@chromium.org
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/2706343004/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... File components/sync_sessions/sessions_sync_manager_unittest.cc (right): https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:102: testing::AssertionResult ChangeTypeMatches( On 2017/02/22 22:58:41, Nicolas Zea wrote: > On 2017/02/22 22:25:11, skym wrote: > > This method is kind of odd. I don't like that we're tying each test case to a > > specific ordering of changes. Something like map<url, vector<change type>> > would > > be reasonable, though i guess it doesn't work for headers. multi_set<change > > type> would also make me feel better. > > But the ordering matters. If we for example do an update for something before > adding it, the real change processor (GenericChangeProcessor) will trigger an > error). Yes, the order matters for a specific tab. But the order between the tab and the window shouldn't matter. But I suppose this whole file is kind of making some assumptions about the underlying implementation. https://codereview.chromium.org/2706343004/diff/20001/components/sync_session... components/sync_sessions/sessions_sync_manager_unittest.cc:1766: int index = kChangesPerTab * i; On 2017/02/22 22:58:40, Nicolas Zea wrote: > On 2017/02/22 22:25:11, skym wrote: > > This really confuses me. So |out| is presumably of size 12, right? And we're > > about to go through two iterations of the above for loop. The first time > > through, |index| starts at 0. The second time, |index| starts at 2? Shouldn't > it > > start at 6? > > It does start at 6. kChangesPerTab is 6 (2 + 4) Oh, you're right, I kept reading the wrong line.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487805092364540,
"parent_rev": "169beb0367bc0ca03c4cb0baa18d183740f157d0", "commit_rev":
"5bdcd6fff79307e080a87ee417960f2e2141dc24"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Refactor SessionsSyncManager unit tests Tests no longer rely on BrowserWithTestTestWindowTest, and instead inject all the relevant navigation events directly. This allows for much more control over what gets tested, and in particular allows for testing situations with multiple windows. As part of this cleanup, various helper methods were added to make the tests simpler and more readable. A couple outdated tests were also removed. Lastly, because there are no longer any content dependencies, the test was moved to components/sync_sessions, to live next to the code it's actually testing (SessionsSyncManager). BUG=639009, 671283 ========== to ========== [Sync] Refactor SessionsSyncManager unit tests Tests no longer rely on BrowserWithTestTestWindowTest, and instead inject all the relevant navigation events directly. This allows for much more control over what gets tested, and in particular allows for testing situations with multiple windows. As part of this cleanup, various helper methods were added to make the tests simpler and more readable. A couple outdated tests were also removed. Lastly, because there are no longer any content dependencies, the test was moved to components/sync_sessions, to live next to the code it's actually testing (SessionsSyncManager). BUG=639009, 671283 Review-Url: https://codereview.chromium.org/2706343004 Cr-Commit-Position: refs/heads/master@{#452320} Committed: https://chromium.googlesource.com/chromium/src/+/5bdcd6fff79307e080a87ee41796... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/5bdcd6fff79307e080a87ee41796... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
