ntp_tiles: Refactor existing MostVisitedSites tests

ntp_tiles: Refactor existing MostVisitedSites tests

Expectations are now more explicitly inlined in tests without an
intermediate Check() function. Among other things, this allows better
failure messages including line numbers.

This is a preceding step before adding new tests, which require a few
other patches submitted first.

BUG=619584
Committed: https://crrev.com/8dc93c5196fbf8d6309f065276f4766ba5e00abf
Cr-Commit-Position: refs/heads/master@{#438824}

[Marc Treib, 2016-12-13 15:50:25]

Did you mean to send this for review? If so, you should add a reviewer :)

On Tue, Dec 13, 2016 at 4:24 PM <mastiz@chromium.org> wrote:

> Reviewers:
> CL: https://codereview.chromium.org/2567333003/
>
> Description:
> ntp_tiles: Refactor existing MostVisitedSites tests
>
> Expectations are now more explicitly inlined in tests without an
> intermediate Check() function. Among other things, this allows better
> failure messages including line numbers.
>
> This is a preceding step before adding new tests, which require a few
> other patches submitted first.
>
> BUG=619584
>
> Affected files (+95, -99 lines):
> M components/ntp_tiles/most_visited_sites.h
> M components/ntp_tiles/most_visited_sites_unittest.cc
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "New Tab Page development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ntp-dev+unsubscribe@chromium.org.
> To post to this group, send email to ntp-dev@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a1141a400b6dd2605...
>
<https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a1141a400b6dd2605...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mastiz, 2016-12-13 15:59:31]

This is the second that happens the same to me today, not sure which button I
misclicked. Anyway, your reply automatically made you a reviewer, thanks! :-)

[mastiz, 2016-12-13 15:59:55]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 16:00:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 16:37:01]

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

[commit-bot: I haz the power, 2016-12-13 16:37:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2016-12-13 16:44:15]

lgtm

LGTM with some optional suggestions

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.h:126: NTPTilesVector popular_tiles);
I generally prefer "friend class ...Test" over exposing methods for tests. Do
you have a different opinion on that?

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites_unittest.cc (right):

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites_unittest.cc:21: // Defined for
googletest. Must be defined in the same namespace.
I think it's also possible to define an "operator<<" instead, which to me feels
a bit less magic. WDYT?

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites_unittest.cc:80:
HasUrl("https://www.site1.com/")),
Instead of checking three separate properties, could we define an operator== for
NTPTile?

[mastiz, 2016-12-14 08:54:23]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[mastiz, 2016-12-14 08:54:27]

PTAL.

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.h:126: NTPTilesVector popular_tiles);
On 2016/12/13 16:44:15, Marc Treib wrote:
> I generally prefer "friend class ...Test" over exposing methods for tests. Do
> you have a different opinion on that?

Yes, my preferences are: public API if not too ugly (most notably for static
methods); friend tests; friend classes. Rationale: exposing less internals is
better, for readability of tests and maintenance burden.

In this particular case, the additional reason why I think this is a good idea
is https://codereview.chromium.org/2568133005/ where this is moved to
SuggestionsServiceImpl.

WDYT?

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites_unittest.cc (right):

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites_unittest.cc:21: // Defined for
googletest. Must be defined in the same namespace.
On 2016/12/13 16:44:15, Marc Treib wrote:
> I think it's also possible to define an "operator<<" instead, which to me
feels
> a bit less magic. WDYT?

I *think*, for test-oriented printing, which is the case because we don't even
print all fields, PrintTo is preferred. It also seems quite widely used in
chromium, although operator<< is harder to search for.

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites_unittest.cc:80:
HasUrl("https://www.site1.com/")),
On 2016/12/13 16:44:15, Marc Treib wrote:
> Instead of checking three separate properties, could we define an operator==
for
> NTPTile?

Done.

[commit-bot: I haz the power, 2016-12-14 08:54:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2016-12-14 09:27:34]

Still LGTM

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.h:126: NTPTilesVector popular_tiles);
On 2016/12/14 08:54:27, mastiz wrote:
> On 2016/12/13 16:44:15, Marc Treib wrote:
> > I generally prefer "friend class ...Test" over exposing methods for tests.
Do
> > you have a different opinion on that?
> 
> Yes, my preferences are: public API if not too ugly (most notably for static
> methods); friend tests; friend classes. Rationale: exposing less internals is
> better, for readability of tests and maintenance burden.

Well, this exposes less internals to the test, but more to actual non-test code.
I've seen several cases where comments say "exposed for testing", but the
functions ended up being called in prod code. So my preferences are:
- FRIEND_TEST or friend TestClass (don't really have a preference for one over
the other)
- DoTheThingForTesting (at least we have presubmit hooks that check it isn't
called in prod code)
- // public for testing, only as a last resort.

I just dislike making prod code less readable (e.g. by exposing things that
don't need to be exposed), only for testing reasons.

> In this particular case, the additional reason why I think this is a good idea
> is https://codereview.chromium.org/2568133005/ where this is moved to
> SuggestionsServiceImpl.
> 
> WDYT?

In this particular case (static method), I'm fine with this. :)

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites_unittest.cc (right):

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites_unittest.cc:21: // Defined for
googletest. Must be defined in the same namespace.
On 2016/12/14 08:54:27, mastiz wrote:
> On 2016/12/13 16:44:15, Marc Treib wrote:
> > I think it's also possible to define an "operator<<" instead, which to me
> feels
> > a bit less magic. WDYT?
> 
> I *think*, for test-oriented printing, which is the case because we don't even
> print all fields, PrintTo is preferred. It also seems quite widely used in
> chromium, although operator<< is harder to search for.

I guess it's just me being unfamiliar with the testing framework, but I wonder
why PrintTo even exists, if there is also a standard way of achieving the same
thing.
Anyway, it's not a big deal.

[commit-bot: I haz the power, 2016-12-14 09:37:46]

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

[commit-bot: I haz the power, 2016-12-14 09:37:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mastiz, 2016-12-14 09:40:29]

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2567333003/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.h:126: NTPTilesVector popular_tiles);
On 2016/12/14 09:27:34, Marc Treib wrote:
> On 2016/12/14 08:54:27, mastiz wrote:
> > On 2016/12/13 16:44:15, Marc Treib wrote:
> > > I generally prefer "friend class ...Test" over exposing methods for tests.
> Do
> > > you have a different opinion on that?
> > 
> > Yes, my preferences are: public API if not too ugly (most notably for static
> > methods); friend tests; friend classes. Rationale: exposing less internals
is
> > better, for readability of tests and maintenance burden.
> 
> Well, this exposes less internals to the test, but more to actual non-test
code.
> I've seen several cases where comments say "exposed for testing", but the
> functions ended up being called in prod code. So my preferences are:
> - FRIEND_TEST or friend TestClass (don't really have a preference for one over
> the other)
> - DoTheThingForTesting (at least we have presubmit hooks that check it isn't
> called in prod code)
> - // public for testing, only as a last resort.
> 
> I just dislike making prod code less readable (e.g. by exposing things that
> don't need to be exposed), only for testing reasons.
> 
> > In this particular case, the additional reason why I think this is a good
idea
> > is https://codereview.chromium.org/2568133005/ where this is moved to
> > SuggestionsServiceImpl.
> > 
> > WDYT?
> 
> In this particular case (static method), I'm fine with this. :)

This is a good topic to bring up to our testing discussion. I'd definitely like
to argue for more public exposure and less friends :-)

[mastiz, 2016-12-15 13:12:23]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-12-15 13:12:23]

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/2567333003/#ps40001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-12-15 13:12:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 14:01:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481807543030870,
"parent_rev": "2c1727ba259ede92321be7862249663410018e25", "commit_rev":
"2fe7f0723f73da1b730b76b9c9427536dcabb1d9"}

[commit-bot: I haz the power, 2016-12-15 14:02:28]

Description was changed from

==========
ntp_tiles: Refactor existing MostVisitedSites tests

Expectations are now more explicitly inlined in tests without an
intermediate Check() function. Among other things, this allows better
failure messages including line numbers.

This is a preceding step before adding new tests, which require a few
other patches submitted first.

BUG=619584
==========

to

==========
ntp_tiles: Refactor existing MostVisitedSites tests

Expectations are now more explicitly inlined in tests without an
intermediate Check() function. Among other things, this allows better
failure messages including line numbers.

This is a preceding step before adding new tests, which require a few
other patches submitted first.

BUG=619584

Review-Url: https://codereview.chromium.org/2567333003
Committed:
https://chromium.googlesource.com/chromium/src/+/2fe7f0723f73da1b730b76b9c942...
==========

[commit-bot: I haz the power, 2016-12-15 14:02:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2fe7f0723f73da1b730b76b9c942...

[commit-bot: I haz the power, 2016-12-15 14:04:17]

Description was changed from

==========
ntp_tiles: Refactor existing MostVisitedSites tests

Expectations are now more explicitly inlined in tests without an
intermediate Check() function. Among other things, this allows better
failure messages including line numbers.

This is a preceding step before adding new tests, which require a few
other patches submitted first.

BUG=619584

Review-Url: https://codereview.chromium.org/2567333003
Committed:
https://chromium.googlesource.com/chromium/src/+/2fe7f0723f73da1b730b76b9c942...
==========

to

==========
ntp_tiles: Refactor existing MostVisitedSites tests

Expectations are now more explicitly inlined in tests without an
intermediate Check() function. Among other things, this allows better
failure messages including line numbers.

This is a preceding step before adding new tests, which require a few
other patches submitted first.

BUG=619584

Committed: https://crrev.com/8dc93c5196fbf8d6309f065276f4766ba5e00abf
Cr-Commit-Position: refs/heads/master@{#438824}
==========

[commit-bot: I haz the power, 2016-12-15 14:04:18]

Patchset 3 (id:??) landed as
https://crrev.com/8dc93c5196fbf8d6309f065276f4766ba5e00abf
Cr-Commit-Position: refs/heads/master@{#438824}