ntp_tiles: Refactor existing MostVisitedSites tests

components/ntp_tiles/most_visited_sites.h

Index: components/ntp_tiles/most_visited_sites.h
diff --git a/components/ntp_tiles/most_visited_sites.h b/components/ntp_tiles/most_visited_sites.h
index d097b1bc469974c307cf27518061f3737470e98d..0967ca66895b46eb6eaaf95f41eb6fb82d12afb0 100644
--- a/components/ntp_tiles/most_visited_sites.h
+++ b/components/ntp_tiles/most_visited_sites.h
@@ -119,9 +119,13 @@ class MostVisitedSites : public history::TopSitesObserver,
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
- private:
-  friend class MostVisitedSitesTest;
+  // Workhorse for SaveNewTiles. Implemented as a separate static and public
+  // method for ease of testing.
+  static NTPTilesVector MergeTiles(NTPTilesVector personal_tiles,
+                                   NTPTilesVector whitelist_tiles,
+                                   NTPTilesVector popular_tiles);

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

I generally prefer "friend class ...Test" over exposing methods for tests. Do
you have a different opinion on that?

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

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?

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

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 (static method), I'm fine with this. :)

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

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 :-)

 
+ private:
   void BuildCurrentTiles();
 
   // Initialize the query to Top Sites. Called if the SuggestionsService
@@ -153,12 +157,6 @@ class MostVisitedSites : public history::TopSitesObserver,
   // if appropriate, and saves the new tiles.
   void SaveNewTiles(NTPTilesVector personal_tiles);
 
-  // Workhorse for SaveNewTiles above. Implemented as a separate static method
-  // for ease of testing.
-  static NTPTilesVector MergeTiles(NTPTilesVector personal_tiles,
-                                   NTPTilesVector whitelist_tiles,
-                                   NTPTilesVector popular_tiles);
-
   // Notifies the observer about the availability of tiles.
   // Also records impressions UMA if not done already.
   void NotifyMostVisitedURLsObserver();