Refactor OfflinePageSuggestionsProviderTest

Refactor OfflinePageSuggestionsProviderTest

Instead of using a MOCK_METHOD and a helper method for verifying the
callback result of GetDismissedSuggestionsForDebugging, use a helper
method that captures the callback data and moves it to a vector on the
test method's stack.

BUG=628198

[Philipp Keck, 2016-08-29 14:49:34]

pke@google.com changed reviewers:
+ treib@chromium.org, tschumann@chromium.org

[Philipp Keck, 2016-08-29 14:49:35]

PTAL

[Marc Treib, 2016-08-29 14:54:29]

lgtm

https://codereview.chromium.org/2291593002/diff/1/components/ntp_snippets/off...
File
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2291593002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:156:
void CaptureDismissedSuggestions(
This could be a global function, or at least a static method. That'll make
Bind'ing easier.

[tschumann, 2016-08-29 15:19:11]

thanks!

https://codereview.chromium.org/2291593002/diff/1/components/ntp_snippets/off...
File
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2291593002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:156:
void CaptureDismissedSuggestions(
On 2016/08/29 14:54:29, Marc Treib wrote:
> This could be a global function, or at least a static method. That'll make
> Bind'ing easier.

Let's make this a function in an unnamed namespace, as it's independent from the
test fixture.

https://codereview.chromium.org/2291593002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:319:
Mock::VerifyAndClearExpectations(this);
you can drop this line here and in other places now.

You should only need that call rarely. Setting up multiple expectations creates
a stack and the first expectation that matches will be executed. 
And usually, the matchers of the expectation are precise enough to prevent this.
If you really need something like this, calling .RetiresOnSaturation(); at the
end of the EXPECT_CALL() statement makes sure the expectation gets removed once
saturated (but again, use that rarely).

(and if you really want to get everything verified at a specific place, consider
splitting up the test).

[Philipp Keck, 2016-08-29 15:27:05]

Thank you

https://codereview.chromium.org/2291593002/diff/1/components/ntp_snippets/off...
File
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2291593002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:156:
void CaptureDismissedSuggestions(
On 2016/08/29 15:19:11, tschumann wrote:
> On 2016/08/29 14:54:29, Marc Treib wrote:
> > This could be a global function, or at least a static method. That'll make
> > Bind'ing easier.
> 
> Let's make this a function in an unnamed namespace, as it's independent from
the
> test fixture.

Done.

https://codereview.chromium.org/2291593002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:319:
Mock::VerifyAndClearExpectations(this);
On 2016/08/29 15:19:11, tschumann wrote:
> you can drop this line here and in other places now.
> 
> You should only need that call rarely. Setting up multiple expectations
creates
> a stack and the first expectation that matches will be executed. 
> And usually, the matchers of the expectation are precise enough to prevent
this.
> If you really need something like this, calling .RetiresOnSaturation(); at the
> end of the EXPECT_CALL() statement makes sure the expectation gets removed
once
> saturated (but again, use that rarely).
> 
> (and if you really want to get everything verified at a specific place,
consider
> splitting up the test).

Done.

[tschumann, 2016-08-29 15:47:01]

lgtm

some more nits as we're already here. But overall looks really goods :)

https://codereview.chromium.org/2291593002/diff/20001/components/ntp_snippets...
File
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2291593002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:87:
class MockOfflinePageModel : public StubOfflinePageModel {
nit: while we're at it. This is more a fake than a Mock. Rename?

https://codereview.chromium.org/2291593002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:95:
std::vector<OfflinePageItem>* items() { return &items_; }
a proto-inspired convention is to call these functions mutable_items().
then you can have a const version of item() which returns a reference and can be
used when verifying data.

https://codereview.chromium.org/2291593002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:378:
AddItem(CreateDummyDownload(1));
for symmetry, I'd also call model() here and inline the convenience method. It
just adds a layer of indirection, and because you call clear() below, it doesn't
even provide abstraction.

[Philipp Keck, 2016-08-29 17:04:15]

https://codereview.chromium.org/2291593002/diff/20001/components/ntp_snippets...
File
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2291593002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:87:
class MockOfflinePageModel : public StubOfflinePageModel {
On 2016/08/29 15:47:00, tschumann wrote:
> nit: while we're at it. This is more a fake than a Mock. Rename?

Done.

https://codereview.chromium.org/2291593002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:95:
std::vector<OfflinePageItem>* items() { return &items_; }
On 2016/08/29 15:47:00, tschumann wrote:
> a proto-inspired convention is to call these functions mutable_items().
> then you can have a const version of item() which returns a reference and can
be
> used when verifying data.

Done.

https://codereview.chromium.org/2291593002/diff/20001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider_unittest.cc:378:
AddItem(CreateDummyDownload(1));
On 2016/08/29 15:47:00, tschumann wrote:
> for symmetry, I'd also call model() here and inline the convenience method. It
> just adds a layer of indirection, and because you call clear() below, it
doesn't
> even provide abstraction.

Done.

[tschumann, 2016-08-29 17:20:36]

lgtm