Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2279223002:
Add OfflinePageSuggestionsProviderTest (Closed)
|
Created:
4 years, 3 months ago by Philipp Keck Modified:
4 years, 3 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd OfflinePageSuggestionsProviderTest
BUG=628198
Committed: https://crrev.com/5fe00b8dfc81a113eb3d02641cf71daff59b5fa0
Cr-Commit-Position: refs/heads/master@{#415211}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Move MockDismissedSuggestionsCallback into test class #Patch Set 3 : Marc's comments #
Total comments: 8
Patch Set 4 : Marc's new comments #Patch Set 5 : Marc's new new comments #Patch Set 6 : Add missing dependency to components/ntp_snippets:unit_tests #Patch Set 7 : Add another missing dependency #Patch Set 8 : Fix Windows compiler error #Patch Set 9 : Maybe now fix the Windows compiler error #Patch Set 10 : Maybe this time fix the Windows compiler error #
Dependent Patchsets: Messages
Total messages: 42 (15 generated)
pke@google.com changed reviewers: + treib@chromium.org
PTAL
Nice! Just a bunch of nits below. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:93: class MockDismissedSuggestionsCallback I find this pattern pretty weird - why not just make a MOCK_METHOD on the test class? Is it mostly to circumvent the lack of move support in gtest? https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:112: ~OfflinePageSuggestionsProviderTest() override { provider_.reset(); } I don't think this is necessary? https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:114: void SetUp() override { CreateProvider(true, true, true); } Can this just happen in the ctor instead? https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:169: protected: What's the difference between public and protected here? https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:180: // Last so that the dependencies are deleted after the provider. I think this will actually not be the case if you manually delete the provider in the dtor. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:193: *(observer()), nit: parens not necessary https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:203: EXPECT_CALL(*(observer()), also here (and a bunch more cases below) https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:212: RecreateProvider(true, true, true); Why do you need to recreate the provider here? Wouldn't a "FireModelChanged" or something like that be better? https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:233: // Enable recent tabs, dsiable downloads. typo https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:273: EXPECT_THAT( EXPECT_FALSE? https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:321: Call(ElementsAre(Property(&ContentSuggestion::url, Unordered?
https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:93: class MockDismissedSuggestionsCallback On 2016/08/26 12:14:50, Marc Treib wrote: > I find this pattern pretty weird - why not just make a MOCK_METHOD on the test > class? Is it mostly to circumvent the lack of move support in gtest? Normally, a testing::MockFunction would do the job, but yes, here it doesn't work because of the non-movable type. So we need a MOCK_METHOD and a helper implementation of the real method. Moved the two methods into the main test class (and renamed them). https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:112: ~OfflinePageSuggestionsProviderTest() override { provider_.reset(); } On 2016/08/26 12:14:50, Marc Treib wrote: > I don't think this is necessary? Done. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:114: void SetUp() override { CreateProvider(true, true, true); } On 2016/08/26 12:14:50, Marc Treib wrote: > Can this just happen in the ctor instead? Done. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:169: protected: On 2016/08/26 12:14:50, Marc Treib wrote: > What's the difference between public and protected here? None. Moved all to public. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:180: // Last so that the dependencies are deleted after the provider. On 2016/08/26 12:14:50, Marc Treib wrote: > I think this will actually not be the case if you manually delete the provider > in the dtor. Removed the comment. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:193: *(observer()), On 2016/08/26 12:14:50, Marc Treib wrote: > nit: parens not necessary Done. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:203: EXPECT_CALL(*(observer()), On 2016/08/26 12:14:50, Marc Treib wrote: > also here (and a bunch more cases below) Done. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:212: RecreateProvider(true, true, true); On 2016/08/26 12:14:50, Marc Treib wrote: > Why do you need to recreate the provider here? Wouldn't a "FireModelChanged" or > something like that be better? Done. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:233: // Enable recent tabs, dsiable downloads. On 2016/08/26 12:14:50, Marc Treib wrote: > typo Done. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:273: EXPECT_THAT( On 2016/08/26 12:14:50, Marc Treib wrote: > EXPECT_FALSE? Done. https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:321: Call(ElementsAre(Property(&ContentSuggestion::url, On 2016/08/26 12:14:50, Marc Treib wrote: > Unordered? Done.
lgtm LGTM, thanks! https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:180: // Last so that the dependencies are deleted after the provider. On 2016/08/26 12:41:45, Philipp Keck wrote: > On 2016/08/26 12:14:50, Marc Treib wrote: > > I think this will actually not be the case if you manually delete the provider > > in the dtor. > > Removed the comment. Now that the dtor is gone, the comment might actually be relevant again? You probably want to provider to be destroyed before the model. https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: MOCK_METHOD1( nit: Add a comment explaining the workaround? It might also be good to give the two methods more distinguishable names.
https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:180: // Last so that the dependencies are deleted after the provider. On 2016/08/26 12:52:03, Marc Treib wrote: > On 2016/08/26 12:41:45, Philipp Keck wrote: > > On 2016/08/26 12:14:50, Marc Treib wrote: > > > I think this will actually not be the case if you manually delete the > provider > > > in the dtor. > > > > Removed the comment. > > Now that the dtor is gone, the comment might actually be relevant again? You > probably want to provider to be destroyed before the model. The comment applies now. But is it important? I'm fine putting it back in, but it doesn't really help. Like many other things you commented on in this CL, it was only here because it's in NTPSnippetsServiceTest. https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: MOCK_METHOD1( On 2016/08/26 12:52:03, Marc Treib wrote: > nit: Add a comment explaining the workaround? Done. > It might also be good to give the two methods more distinguishable names. Now they have distinguishable names, but I'm not perfectly happy with either of the names, so let me know if you have better ideas.
lgtm https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:180: // Last so that the dependencies are deleted after the provider. On 2016/08/26 17:19:55, Philipp Keck wrote: > On 2016/08/26 12:52:03, Marc Treib wrote: > > On 2016/08/26 12:41:45, Philipp Keck wrote: > > > On 2016/08/26 12:14:50, Marc Treib wrote: > > > > I think this will actually not be the case if you manually delete the > > provider > > > > in the dtor. > > > > > > Removed the comment. > > > > Now that the dtor is gone, the comment might actually be relevant again? You > > probably want to provider to be destroyed before the model. > > The comment applies now. But is it important? I'm fine putting it back in, but > it doesn't really help. Like many other things you commented on in this CL, it > was only here because it's in NTPSnippetsServiceTest. Yes, it is important: The provider calls model_->RemoveObserver in its dtor, which is illegal if the model has already been destroyed. https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: MOCK_METHOD1( On 2016/08/26 17:19:55, Philipp Keck wrote: > On 2016/08/26 12:52:03, Marc Treib wrote: > > nit: Add a comment explaining the workaround? > Done. > > > It might also be good to give the two methods more distinguishable names. > > Now they have distinguishable names, but I'm not perfectly happy with either of > the names, so let me know if you have better ideas. No better ideas, no... One suggestion Tim had in a similar situation: Use the same name, but a different type (std::list instead of std::vector) in the second method (the helper will have to copy stuff from the vector into the list). That way, the test body looks less confusing (same method name), and matchers will still work the same. And if gmock ever supports movable types, we just remove the helper and that's it. But I'm fine with it either way - your call.
https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/1/components/ntp_snippets/off... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:180: // Last so that the dependencies are deleted after the provider. On 2016/08/29 09:28:48, Marc Treib wrote: > On 2016/08/26 17:19:55, Philipp Keck wrote: > > On 2016/08/26 12:52:03, Marc Treib wrote: > > > On 2016/08/26 12:41:45, Philipp Keck wrote: > > > > On 2016/08/26 12:14:50, Marc Treib wrote: > > > > > I think this will actually not be the case if you manually delete the > > > provider > > > > > in the dtor. > > > > > > > > Removed the comment. > > > > > > Now that the dtor is gone, the comment might actually be relevant again? You > > > probably want to provider to be destroyed before the model. > > > > The comment applies now. But is it important? I'm fine putting it back in, but > > it doesn't really help. Like many other things you commented on in this CL, it > > was only here because it's in NTPSnippetsServiceTest. > > Yes, it is important: The provider calls model_->RemoveObserver in its dtor, > which is illegal if the model has already been destroyed. Done. https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: MOCK_METHOD1( On 2016/08/29 09:28:48, Marc Treib wrote: > On 2016/08/26 17:19:55, Philipp Keck wrote: > > On 2016/08/26 12:52:03, Marc Treib wrote: > > > nit: Add a comment explaining the workaround? > > Done. > > > > > It might also be good to give the two methods more distinguishable names. > > > > Now they have distinguishable names, but I'm not perfectly happy with either > of > > the names, so let me know if you have better ideas. > > No better ideas, no... > One suggestion Tim had in a similar situation: Use the same name, but a > different type (std::list instead of std::vector) in the second method (the > helper will have to copy stuff from the vector into the list). That way, the > test body looks less confusing (same method name), and matchers will still work > the same. And if gmock ever supports movable types, we just remove the helper > and that's it. > But I'm fine with it either way - your call. I tried, but this std::list workaround is not possible. The std::Bind call "couldn't infer template argument 'Functor'".
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2279223002/#ps80001 (title: "Marc's new new 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Added missing dependencies, PTAL: https://codereview.chromium.org/2279223002/diff2/80001:120001/components/ntp_...
On 2016/08/29 11:57:00, Philipp Keck wrote: > Added missing dependencies, PTAL: > https://codereview.chromium.org/2279223002/diff2/80001:120001/components/ntp_... Still LGTM
The CQ bit was checked by pke@google.com
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: MOCK_METHOD1( On 2016/08/29 11:32:42, Philipp Keck wrote: > On 2016/08/29 09:28:48, Marc Treib wrote: > > On 2016/08/26 17:19:55, Philipp Keck wrote: > > > On 2016/08/26 12:52:03, Marc Treib wrote: > > > > nit: Add a comment explaining the workaround? > > > Done. > > > > > > > It might also be good to give the two methods more distinguishable names. > > > > > > Now they have distinguishable names, but I'm not perfectly happy with either > > of > > > the names, so let me know if you have better ideas. > > > > No better ideas, no... > > One suggestion Tim had in a similar situation: Use the same name, but a > > different type (std::list instead of std::vector) in the second method (the > > helper will have to copy stuff from the vector into the list). That way, the > > test body looks less confusing (same method name), and matchers will still > work > > the same. And if gmock ever supports movable types, we just remove the helper > > and that's it. > > But I'm fine with it either way - your call. > > I tried, but this std::list workaround is not possible. The std::Bind call > "couldn't infer template argument 'Functor'". in this particular case, I wouldn't use the MOCK macro anyways as you're not implementing a mock but much more a fake. The mock is for verifying behavior, the fake is more concerned about before/after states. You could simply capture the state and after the test verify the proper data is stored. Would be cleaner IMO as no other methods are mocked out.
https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: MOCK_METHOD1( On 2016/08/29 12:45:14, tschumann wrote: > On 2016/08/29 11:32:42, Philipp Keck wrote: > > On 2016/08/29 09:28:48, Marc Treib wrote: > > > On 2016/08/26 17:19:55, Philipp Keck wrote: > > > > On 2016/08/26 12:52:03, Marc Treib wrote: > > > > > nit: Add a comment explaining the workaround? > > > > Done. > > > > > > > > > It might also be good to give the two methods more distinguishable > names. > > > > > > > > Now they have distinguishable names, but I'm not perfectly happy with > either > > > of > > > > the names, so let me know if you have better ideas. > > > > > > No better ideas, no... > > > One suggestion Tim had in a similar situation: Use the same name, but a > > > different type (std::list instead of std::vector) in the second method (the > > > helper will have to copy stuff from the vector into the list). That way, the > > > test body looks less confusing (same method name), and matchers will still > > work > > > the same. And if gmock ever supports movable types, we just remove the > helper > > > and that's it. > > > But I'm fine with it either way - your call. > > > > I tried, but this std::list workaround is not possible. The std::Bind call > > "couldn't infer template argument 'Functor'". > > in this particular case, I wouldn't use the MOCK macro anyways as you're not > implementing a mock but much more a fake. > The mock is for verifying behavior, the fake is more concerned about > before/after states. > You could simply capture the state and after the test verify the proper data is > stored. Would be cleaner IMO as no other methods are mocked out. I disagree: I find a mock method and EXPECT_CALLs much easier to follow than manually storing and checking some state. With EXPECT_CALL, all the expectations are right in the test body; otherwise, I have to look up into the test class to see what happens.
https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: MOCK_METHOD1( On 2016/08/29 13:01:51, Marc Treib wrote: > On 2016/08/29 12:45:14, tschumann wrote: > > On 2016/08/29 11:32:42, Philipp Keck wrote: > > > On 2016/08/29 09:28:48, Marc Treib wrote: > > > > On 2016/08/26 17:19:55, Philipp Keck wrote: > > > > > On 2016/08/26 12:52:03, Marc Treib wrote: > > > > > > nit: Add a comment explaining the workaround? > > > > > Done. > > > > > > > > > > > It might also be good to give the two methods more distinguishable > > names. > > > > > > > > > > Now they have distinguishable names, but I'm not perfectly happy with > > either > > > > of > > > > > the names, so let me know if you have better ideas. > > > > > > > > No better ideas, no... > > > > One suggestion Tim had in a similar situation: Use the same name, but a > > > > different type (std::list instead of std::vector) in the second method > (the > > > > helper will have to copy stuff from the vector into the list). That way, > the > > > > test body looks less confusing (same method name), and matchers will still > > > work > > > > the same. And if gmock ever supports movable types, we just remove the > > helper > > > > and that's it. > > > > But I'm fine with it either way - your call. > > > > > > I tried, but this std::list workaround is not possible. The std::Bind call > > > "couldn't infer template argument 'Functor'". > > > > in this particular case, I wouldn't use the MOCK macro anyways as you're not > > implementing a mock but much more a fake. > > The mock is for verifying behavior, the fake is more concerned about > > before/after states. > > You could simply capture the state and after the test verify the proper data > is > > stored. Would be cleaner IMO as no other methods are mocked out. > > I disagree: I find a mock method and EXPECT_CALLs much easier to follow than > manually storing and checking some state. With EXPECT_CALL, all the expectations > are right in the test body; otherwise, I have to look up into the test class to > see what happens. @Tim: Yes, currently, there are no other methods mocked out. But there will be, for example when we want to use the ImageFetchedCallback once the image fetching is supported by the provider. It might something like this: EXPECT_CALL(*this, ReceivedSuggestionImage(AnyNonEmptyImageMatcher())); and there are other, similar cases in other unit tests where the same reasoning applies (but we currently use MOCK and EXPECT_CALL). Implementing a fake class to store away the data to be checked later is more work and more code to read. And I also see Marc's point that it would make the test harder to understand (more data flows), even though I agree that the testing situation here is somewhat a corner case between mock and fake.
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2279223002/#ps180001 (title: "Maybe this time fix the Windows compiler error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: MOCK_METHOD1( On 2016/08/29 13:35:06, Philipp Keck wrote: > On 2016/08/29 13:01:51, Marc Treib wrote: > > On 2016/08/29 12:45:14, tschumann wrote: > > > On 2016/08/29 11:32:42, Philipp Keck wrote: > > > > On 2016/08/29 09:28:48, Marc Treib wrote: > > > > > On 2016/08/26 17:19:55, Philipp Keck wrote: > > > > > > On 2016/08/26 12:52:03, Marc Treib wrote: > > > > > > > nit: Add a comment explaining the workaround? > > > > > > Done. > > > > > > > > > > > > > It might also be good to give the two methods more distinguishable > > > names. > > > > > > > > > > > > Now they have distinguishable names, but I'm not perfectly happy with > > > either > > > > > of > > > > > > the names, so let me know if you have better ideas. > > > > > > > > > > No better ideas, no... > > > > > One suggestion Tim had in a similar situation: Use the same name, but a > > > > > different type (std::list instead of std::vector) in the second method > > (the > > > > > helper will have to copy stuff from the vector into the list). That way, > > the > > > > > test body looks less confusing (same method name), and matchers will > still > > > > work > > > > > the same. And if gmock ever supports movable types, we just remove the > > > helper > > > > > and that's it. > > > > > But I'm fine with it either way - your call. > > > > > > > > I tried, but this std::list workaround is not possible. The std::Bind call > > > > "couldn't infer template argument 'Functor'". > > > > > > in this particular case, I wouldn't use the MOCK macro anyways as you're not > > > implementing a mock but much more a fake. > > > The mock is for verifying behavior, the fake is more concerned about > > > before/after states. > > > You could simply capture the state and after the test verify the proper data > > is > > > stored. Would be cleaner IMO as no other methods are mocked out. > > > > I disagree: I find a mock method and EXPECT_CALLs much easier to follow than > > manually storing and checking some state. With EXPECT_CALL, all the > expectations > > are right in the test body; otherwise, I have to look up into the test class > to > > see what happens. > > @Tim: Yes, currently, there are no other methods mocked out. But there will be, > for example when we want to use the ImageFetchedCallback once the image fetching > is supported by the provider. It might something like this: > EXPECT_CALL(*this, ReceivedSuggestionImage(AnyNonEmptyImageMatcher())); > and there are other, similar cases in other unit tests where the same reasoning > applies (but we currently use MOCK and EXPECT_CALL). Implementing a fake class > to store away the data to be checked later is more work and more code to read. > > And I also see Marc's point that it would make the test harder to understand > (more data flows), even though I agree that the testing situation here is > somewhat a corner case between mock and fake. IMO, adding MOCK methods on a test is abusing the mock framework and methodology. For example, you use this function and a mock because all you need is a way to store some results. Let's look at one of the usages (from below): 305 // And appear in the dismissed suggestions for the right category. 306 EXPECT_CALL(*this, ReceivedDismissedSuggestions(UnorderedElementsAre( 307 Property(&ContentSuggestion::url, 308 GURL("file:///some/folder/test2.mhtml")), 309 Property(&ContentSuggestion::url, 310 GURL("file:///some/folder/test3.mhtml"))))); 311 provider()->GetDismissedSuggestionsForDebugging( 312 recent_tabs_category(), 313 base::Bind( 314 &OfflinePageSuggestionsProviderTest::ReceiveDismissedSuggestions, 315 base::Unretained(this))); 316 Mock::VerifyAndClearExpectations(this); 317 What you want to verify is, that the callback you're passing into the function get's called with a certain set of parameters. If you want to make sure the callback gets only called once, you should pass in a MockFunction. If you just want to verify the state, then you should do something like namespace { void CaptureSuggestionUrls(std::vector<std::string>>* captured_urls, std::vector<ContentSuggestion> suggestions) { //... copy urls to output parameter } } // namespace. and have a test like: std::vector<std::string>> captured_urls; provider()->GetDismissedSuggestionsForDebugging( recent_tabs_category(), base::Bind( &CaptureSuggestionUrls, &captured_urls)); EXPECT_THAT(captured_urls, UnorderedElementsAre(...)); Which makes the test self-contained and easy to follow.
On 2016/08/29 14:13:32, tschumann wrote: > https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... > File > components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc > (right): > > https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... > components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: > MOCK_METHOD1( > On 2016/08/29 13:35:06, Philipp Keck wrote: > > On 2016/08/29 13:01:51, Marc Treib wrote: > > > On 2016/08/29 12:45:14, tschumann wrote: > > > > On 2016/08/29 11:32:42, Philipp Keck wrote: > > > > > On 2016/08/29 09:28:48, Marc Treib wrote: > > > > > > On 2016/08/26 17:19:55, Philipp Keck wrote: > > > > > > > On 2016/08/26 12:52:03, Marc Treib wrote: > > > > > > > > nit: Add a comment explaining the workaround? > > > > > > > Done. > > > > > > > > > > > > > > > It might also be good to give the two methods more distinguishable > > > > names. > > > > > > > > > > > > > > Now they have distinguishable names, but I'm not perfectly happy > with > > > > either > > > > > > of > > > > > > > the names, so let me know if you have better ideas. > > > > > > > > > > > > No better ideas, no... > > > > > > One suggestion Tim had in a similar situation: Use the same name, but > a > > > > > > different type (std::list instead of std::vector) in the second method > > > (the > > > > > > helper will have to copy stuff from the vector into the list). That > way, > > > the > > > > > > test body looks less confusing (same method name), and matchers will > > still > > > > > work > > > > > > the same. And if gmock ever supports movable types, we just remove the > > > > helper > > > > > > and that's it. > > > > > > But I'm fine with it either way - your call. > > > > > > > > > > I tried, but this std::list workaround is not possible. The std::Bind > call > > > > > "couldn't infer template argument 'Functor'". > > > > > > > > in this particular case, I wouldn't use the MOCK macro anyways as you're > not > > > > implementing a mock but much more a fake. > > > > The mock is for verifying behavior, the fake is more concerned about > > > > before/after states. > > > > You could simply capture the state and after the test verify the proper > data > > > is > > > > stored. Would be cleaner IMO as no other methods are mocked out. > > > > > > I disagree: I find a mock method and EXPECT_CALLs much easier to follow than > > > manually storing and checking some state. With EXPECT_CALL, all the > > expectations > > > are right in the test body; otherwise, I have to look up into the test class > > to > > > see what happens. > > > > @Tim: Yes, currently, there are no other methods mocked out. But there will > be, > > for example when we want to use the ImageFetchedCallback once the image > fetching > > is supported by the provider. It might something like this: > > EXPECT_CALL(*this, ReceivedSuggestionImage(AnyNonEmptyImageMatcher())); > > and there are other, similar cases in other unit tests where the same > reasoning > > applies (but we currently use MOCK and EXPECT_CALL). Implementing a fake class > > to store away the data to be checked later is more work and more code to read. > > > > And I also see Marc's point that it would make the test harder to understand > > (more data flows), even though I agree that the testing situation here is > > somewhat a corner case between mock and fake. > > IMO, adding MOCK methods on a test is abusing the mock framework and > methodology. > > For example, you use this function and a mock because all you need is a way to > store some results. > Let's look at one of the usages (from below): > 305 // And appear in the dismissed suggestions for the right category. > 306 EXPECT_CALL(*this, ReceivedDismissedSuggestions(UnorderedElementsAre( > 307 Property(&ContentSuggestion::url, > 308 GURL("file:///some/folder/test2.mhtml")), > 309 Property(&ContentSuggestion::url, > 310 > GURL("file:///some/folder/test3.mhtml"))))); > 311 provider()->GetDismissedSuggestionsForDebugging( > 312 recent_tabs_category(), > 313 base::Bind( > 314 &OfflinePageSuggestionsProviderTest::ReceiveDismissedSuggestions, > 315 base::Unretained(this))); > 316 Mock::VerifyAndClearExpectations(this); > 317 > > What you want to verify is, that the callback you're passing into the function > get's called with a certain set of parameters. > If you want to make sure the callback gets only called once, you should pass in > a MockFunction. > If you just want to verify the state, then you should do something like > > namespace { > > void CaptureSuggestionUrls(std::vector<std::string>>* captured_urls, > std::vector<ContentSuggestion> suggestions) { > //... copy urls to output parameter > } > > } // namespace. > > and have a test like: > > std::vector<std::string>> captured_urls; > provider()->GetDismissedSuggestionsForDebugging( > recent_tabs_category(), > base::Bind( > &CaptureSuggestionUrls, &captured_urls)); > EXPECT_THAT(captured_urls, UnorderedElementsAre(...)); > > Which makes the test self-contained and easy to follow. That's a neat pattern, I'm going to try and implement this. To me, it feels more like a stub/mock with SaveArgs. I was looking for something like an ArgumentCaptor from Mockito, but I only found SaveArgs and that doesn't support non-copyable types. Your suggested code pretty much implements a clever SaveArgs and works around the copying by actually moving the data. It still feels different from a "fake" in that there is no class that fakes the behavior of the SnippetsInternals (receiving the dismissed suggestions and storing them in some local data structure). Instead, the data is stored on the stack of the test method.
On 2016/08/29 14:22:19, Philipp Keck wrote: > On 2016/08/29 14:13:32, tschumann wrote: > > > https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... > > File > > > components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2279223002/diff/40001/components/ntp_snippets... > > > components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:159: > > MOCK_METHOD1( > > On 2016/08/29 13:35:06, Philipp Keck wrote: > > > On 2016/08/29 13:01:51, Marc Treib wrote: > > > > On 2016/08/29 12:45:14, tschumann wrote: > > > > > On 2016/08/29 11:32:42, Philipp Keck wrote: > > > > > > On 2016/08/29 09:28:48, Marc Treib wrote: > > > > > > > On 2016/08/26 17:19:55, Philipp Keck wrote: > > > > > > > > On 2016/08/26 12:52:03, Marc Treib wrote: > > > > > > > > > nit: Add a comment explaining the workaround? > > > > > > > > Done. > > > > > > > > > > > > > > > > > It might also be good to give the two methods more > distinguishable > > > > > names. > > > > > > > > > > > > > > > > Now they have distinguishable names, but I'm not perfectly happy > > with > > > > > either > > > > > > > of > > > > > > > > the names, so let me know if you have better ideas. > > > > > > > > > > > > > > No better ideas, no... > > > > > > > One suggestion Tim had in a similar situation: Use the same name, > but > > a > > > > > > > different type (std::list instead of std::vector) in the second > method > > > > (the > > > > > > > helper will have to copy stuff from the vector into the list). That > > way, > > > > the > > > > > > > test body looks less confusing (same method name), and matchers will > > > still > > > > > > work > > > > > > > the same. And if gmock ever supports movable types, we just remove > the > > > > > helper > > > > > > > and that's it. > > > > > > > But I'm fine with it either way - your call. > > > > > > > > > > > > I tried, but this std::list workaround is not possible. The std::Bind > > call > > > > > > "couldn't infer template argument 'Functor'". > > > > > > > > > > in this particular case, I wouldn't use the MOCK macro anyways as you're > > not > > > > > implementing a mock but much more a fake. > > > > > The mock is for verifying behavior, the fake is more concerned about > > > > > before/after states. > > > > > You could simply capture the state and after the test verify the proper > > data > > > > is > > > > > stored. Would be cleaner IMO as no other methods are mocked out. > > > > > > > > I disagree: I find a mock method and EXPECT_CALLs much easier to follow > than > > > > manually storing and checking some state. With EXPECT_CALL, all the > > > expectations > > > > are right in the test body; otherwise, I have to look up into the test > class > > > to > > > > see what happens. > > > > > > @Tim: Yes, currently, there are no other methods mocked out. But there will > > be, > > > for example when we want to use the ImageFetchedCallback once the image > > fetching > > > is supported by the provider. It might something like this: > > > EXPECT_CALL(*this, ReceivedSuggestionImage(AnyNonEmptyImageMatcher())); > > > and there are other, similar cases in other unit tests where the same > > reasoning > > > applies (but we currently use MOCK and EXPECT_CALL). Implementing a fake > class > > > to store away the data to be checked later is more work and more code to > read. > > > > > > And I also see Marc's point that it would make the test harder to understand > > > (more data flows), even though I agree that the testing situation here is > > > somewhat a corner case between mock and fake. > > > > IMO, adding MOCK methods on a test is abusing the mock framework and > > methodology. > > > > For example, you use this function and a mock because all you need is a way to > > store some results. > > Let's look at one of the usages (from below): > > 305 // And appear in the dismissed suggestions for the right category. > > 306 EXPECT_CALL(*this, ReceivedDismissedSuggestions(UnorderedElementsAre( > > 307 Property(&ContentSuggestion::url, > > 308 > GURL("file:///some/folder/test2.mhtml")), > > 309 Property(&ContentSuggestion::url, > > 310 > > GURL("file:///some/folder/test3.mhtml"))))); > > 311 provider()->GetDismissedSuggestionsForDebugging( > > 312 recent_tabs_category(), > > 313 base::Bind( > > 314 > &OfflinePageSuggestionsProviderTest::ReceiveDismissedSuggestions, > > 315 base::Unretained(this))); > > 316 Mock::VerifyAndClearExpectations(this); > > 317 > > > > What you want to verify is, that the callback you're passing into the function > > get's called with a certain set of parameters. > > If you want to make sure the callback gets only called once, you should pass > in > > a MockFunction. > > If you just want to verify the state, then you should do something like > > > > namespace { > > > > void CaptureSuggestionUrls(std::vector<std::string>>* captured_urls, > > std::vector<ContentSuggestion> suggestions) { > > //... copy urls to output parameter > > } > > > > } // namespace. > > > > and have a test like: > > > > std::vector<std::string>> captured_urls; > > provider()->GetDismissedSuggestionsForDebugging( > > recent_tabs_category(), > > base::Bind( > > &CaptureSuggestionUrls, &captured_urls)); > > EXPECT_THAT(captured_urls, UnorderedElementsAre(...)); > > > > Which makes the test self-contained and easy to follow. > > That's a neat pattern, I'm going to try and implement this. > > To me, it feels more like a stub/mock with SaveArgs. I was looking for something > like an ArgumentCaptor from Mockito, but I only found SaveArgs and that doesn't > support non-copyable types. Your suggested code pretty much implements a clever > SaveArgs and works around the copying by actually moving the data. > It still feels different from a "fake" in that there is no class that fakes the > behavior of the SnippetsInternals (receiving the dismissed suggestions and > storing them in some local data structure). Instead, the data is stored on the > stack of the test method. See this new CL: https://codereview.chromium.org/2291593002
The CQ bit was unchecked by commit-bot@chromium.org
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 pke@google.com
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 dsansome@chromium.org
dsansome@chromium.org changed reviewers: + dsansome@chromium.org
(I unset the commit bit on this change to try fix a stuck CQ - https://bugs.chromium.org/p/chromium/issues/detail?id=642077. The issue is fixed now so I'm sending this to CQ again for you)
The CQ bit was checked by dsansome@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 #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add OfflinePageSuggestionsProviderTest BUG=628198 ========== to ========== Add OfflinePageSuggestionsProviderTest BUG=628198 Committed: https://crrev.com/5fe00b8dfc81a113eb3d02641cf71daff59b5fa0 Cr-Commit-Position: refs/heads/master@{#415211} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5fe00b8dfc81a113eb3d02641cf71daff59b5fa0 Cr-Commit-Position: refs/heads/master@{#415211}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2297493002/ by perkj@chromium.org. The reason for reverting is: Fails on Chrome os. https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
