Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2384403005:
Fix flaky test ContentSerializedNavigationBuilderTest.ToNavigationEntry (Closed)
DescriptionFix flaky test ContentSerializedNavigationBuilderTest.ToNavigationEntry
The fix is to call RegisterExtendedInfoHandler only once in test.
BUG=653294
Committed: https://crrev.com/3cf12be57931ebc2a8a25af27059e799ecd10b0c
Cr-Commit-Position: refs/heads/master@{#425131}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address feedback #
Total comments: 8
Patch Set 3 : Address more feedback #
Messages
Total messages: 24 (15 generated)
jianli@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was checked by jianli@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.
https://codereview.chromium.org/2384403005/diff/1/components/sessions/content... File components/sessions/content/content_serialized_navigation_builder_unittest.cc (right): https://codereview.chromium.org/2384403005/diff/1/components/sessions/content... components/sessions/content/content_serialized_navigation_builder_unittest.cc:100: ContentSerializedNavigationDriver::GetInstance()-> While this works for your test, it means any other test that runs after yours gets state your test installed. I would prefer you add a reset function on ContentSerializedNavigationDriver that is private and friended by this test that clears the map on setup/teardown.
The CQ bit was checked by jianli@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...
https://codereview.chromium.org/2384403005/diff/1/components/sessions/content... File components/sessions/content/content_serialized_navigation_builder_unittest.cc (right): https://codereview.chromium.org/2384403005/diff/1/components/sessions/content... components/sessions/content/content_serialized_navigation_builder_unittest.cc:100: ContentSerializedNavigationDriver::GetInstance()-> On 2016/10/06 17:01:47, sky wrote: > While this works for your test, it means any other test that runs after yours > gets state your test installed. I would prefer you add a reset function on > ContentSerializedNavigationDriver that is private and friended by this test that > clears the map on setup/teardown. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder_unittest.cc (right): https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder_unittest.cc:100: ContentSerializedNavigationDriver::GetInstance()-> Make sure you run git cl format. Also, this is pretty unreadable. Maybe have a local for ContentSerializedNavigationDriver* ? https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder_unittest.cc:116: }; private: DISALLOW... https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... File components/sessions/content/content_serialized_navigation_driver.h (right): https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... components/sessions/content/content_serialized_navigation_driver.h:63: friend struct base::DefaultSingletonTraits<ContentSerializedNavigationDriver>; Move all the friends before the constructor, newline, then constructor. https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... components/sessions/content/content_serialized_navigation_driver.h:67: void RemoveAllExtendedInfoHandlersForTesting(); Can the test do extended_info_handler_map_.clear() directly so we're not exposing a function only for testing?
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... File components/sessions/content/content_serialized_navigation_builder_unittest.cc (right): https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder_unittest.cc:100: ContentSerializedNavigationDriver::GetInstance()-> On 2016/10/07 15:35:24, sky wrote: > Make sure you run git cl format. Also, this is pretty unreadable. Maybe have a > local for ContentSerializedNavigationDriver* ? Done. https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... components/sessions/content/content_serialized_navigation_builder_unittest.cc:116: }; On 2016/10/07 15:35:24, sky wrote: > private: DISALLOW... Done. https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... File components/sessions/content/content_serialized_navigation_driver.h (right): https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... components/sessions/content/content_serialized_navigation_driver.h:63: friend struct base::DefaultSingletonTraits<ContentSerializedNavigationDriver>; On 2016/10/07 15:35:25, sky wrote: > Move all the friends before the constructor, newline, then constructor. Done. https://codereview.chromium.org/2384403005/diff/20001/components/sessions/con... components/sessions/content/content_serialized_navigation_driver.h:67: void RemoveAllExtendedInfoHandlersForTesting(); On 2016/10/07 15:35:25, sky wrote: > Can the test do extended_info_handler_map_.clear() directly so we're not > exposing a function only for testing? Done.
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.
LGTM
The CQ bit was checked by jianli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix flaky test ContentSerializedNavigationBuilderTest.ToNavigationEntry The fix is to call RegisterExtendedInfoHandler only once in test. BUG=653294 ========== to ========== Fix flaky test ContentSerializedNavigationBuilderTest.ToNavigationEntry The fix is to call RegisterExtendedInfoHandler only once in test. BUG=653294 Committed: https://crrev.com/3cf12be57931ebc2a8a25af27059e799ecd10b0c Cr-Commit-Position: refs/heads/master@{#425131} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3cf12be57931ebc2a8a25af27059e799ecd10b0c Cr-Commit-Position: refs/heads/master@{#425131} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
