[NTP Snippets] Add unit tests for NTPSnippetsFetcher

[NTP Snippets] Add unit tests for NTPSnippetsFetcher

The class had almost no coverage given that, besides not having unit tests, the ones for NTPSnippetsService don't actually exercise the fetcher.

The proposed tests make use of FakeURLFetcherFactory to exercise basic success and failure cases and verify how callbacks are triggered. FakeURLFetcherFactory doesn't however support verifying features like the uploaded POST data for selected hosts which is left outside the scope of this patch.

BUG=584428
Committed: https://crrev.com/302f9f70265f86097aa3bc4c7695fe99d067b47c
Cr-Commit-Position: refs/heads/master@{#390950}

[mastiz, 2016-05-02 10:09:19]

mastiz@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-05-02 11:12:01]

Very nice! LGTM with some small comments below.

Also +jkrcal FYI

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/BUILD.gn:42: "ntp_snippets_fetcher_unittest.cc",
You should also add this file to components/components_tests.gyp (the joys of
having two build systems in parallel...)

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (left):

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:89: // TODO(treib): What to do
if there's already a pending request?
Right now any pending request will just get implicitly canceled, right?
That might be worth a comment in the header :)

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:34:
/*default_factory=*/nullptr),
Should fit on the previous line?

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:78:
Run(/*snippets_json=*/json_str, /*status_message=*/""));
nit: We generally use std::string() to denote empty strings

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:113: // expected to be
called once.
I think you actually need to say ".Times(1)" after the EXPECT_CALL above,
otherwise the second call would just be an "uninteresting mock function call",
but not actually an error? (I haven't tried it out, but either way, IMO
explicitly stating the "times 1" would make things clearer)

[mastiz, 2016-05-02 12:34:29]

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/BUILD.gn:42: "ntp_snippets_fetcher_unittest.cc",
On 2016/05/02 11:12:00, Marc Treib wrote:
> You should also add this file to components/components_tests.gyp (the joys of
> having two build systems in parallel...)

Done.

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher.cc (left):

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher.cc:89: // TODO(treib): What to do
if there's already a pending request?
On 2016/05/02 11:12:00, Marc Treib wrote:
> Right now any pending request will just get implicitly canceled, right?
> That might be worth a comment in the header :)

Done.

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:34:
/*default_factory=*/nullptr),
On 2016/05/02 11:12:01, Marc Treib wrote:
> Should fit on the previous line?

Done.

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:78:
Run(/*snippets_json=*/json_str, /*status_message=*/""));
On 2016/05/02 11:12:00, Marc Treib wrote:
> nit: We generally use std::string() to denote empty strings

Done.

https://codereview.chromium.org/1940843002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_fetcher_unittest.cc:113: // expected to be
called once.
On 2016/05/02 11:12:00, Marc Treib wrote:
> I think you actually need to say ".Times(1)" after the EXPECT_CALL above,
> otherwise the second call would just be an "uninteresting mock function call",
> but not actually an error? (I haven't tried it out, but either way, IMO
> explicitly stating the "times 1" would make things clearer)
Done.

You however don't need to: "If neither WillOnce() nor WillRepeatedly() is in the
EXPECT_CALL(), the inferred cardinality is Times(1)."
(https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide)

I find the explicit variant rather verbose, but it seems to be popular enough in
the Chromium codebase.

[mastiz, 2016-05-02 12:34:54]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-05-02 12:34:54]

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/1940843002/#ps60001
(title: "Added Times(1) for clarity as suggested")

[commit-bot: I haz the power, 2016-05-02 12:35:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1940843002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1940843002/60001

[commit-bot: I haz the power, 2016-05-02 12:52:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-02 12:52:01]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[mastiz, 2016-05-02 13:37:16]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-05-02 13:37:16]

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/1940843002/#ps80001
(title: "Fixed build for linux_chromium_chromeos_compile_dbg_ng")

[commit-bot: I haz the power, 2016-05-02 13:37:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1940843002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1940843002/80001

[commit-bot: I haz the power, 2016-05-02 13:43:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-02 13:43:59]

Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)

[mastiz, 2016-05-02 13:57:14]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2016-05-02 13:57:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1940843002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1940843002/80001

[commit-bot: I haz the power, 2016-05-02 14:03:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-02 14:03:51]

Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)

[mastiz, 2016-05-02 14:20:28]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2016-05-02 14:20:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1940843002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1940843002/80001

[commit-bot: I haz the power, 2016-05-02 15:40:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-02 15:42:27]

Description was changed from

==========
[NTP Snippets] Add unit tests for NTPSnippetsFetcher

The class had almost no coverage given that, besides not having unit tests, the
ones for NTPSnippetsService don't actually exercise the fetcher.

The proposed tests make use of FakeURLFetcherFactory to exercise basic success
and failure cases and verify how callbacks are triggered. FakeURLFetcherFactory
doesn't however support verifying features like the uploaded POST data for
selected hosts which is left outside the scope of this patch.

BUG=584428
==========

to

==========
[NTP Snippets] Add unit tests for NTPSnippetsFetcher

The class had almost no coverage given that, besides not having unit tests, the
ones for NTPSnippetsService don't actually exercise the fetcher.

The proposed tests make use of FakeURLFetcherFactory to exercise basic success
and failure cases and verify how callbacks are triggered. FakeURLFetcherFactory
doesn't however support verifying features like the uploaded POST data for
selected hosts which is left outside the scope of this patch.

BUG=584428

Committed: https://crrev.com/302f9f70265f86097aa3bc4c7695fe99d067b47c
Cr-Commit-Position: refs/heads/master@{#390950}
==========

[commit-bot: I haz the power, 2016-05-02 15:42:28]

Patchset 5 (id:??) landed as
https://crrev.com/302f9f70265f86097aa3bc4c7695fe99d067b47c
Cr-Commit-Position: refs/heads/master@{#390950}

[Dirk Pranke, 2016-05-02 17:08:22]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1942913002/ by dpranke@chromium.org.

The reason for reverting is: I think this broke components_unittests on iOS:

https://build.chromium.org/p/chromium.mac/builders/iOS_Simulator_%28dbg%29/bu....