ntp_tiles: Refactor existing MostVisitedSites tests

components/ntp_tiles/most_visited_sites.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_NTP_TILES_MOST_VISITED_SITES_H_
 #define COMPONENTS_NTP_TILES_MOST_VISITED_SITES_H_
 
 #include <stddef.h>
 
 #include <memory>
@@ skipping 101 matching lines @@
   void Refresh();
 
   void AddOrRemoveBlacklistedUrl(const GURL& url, bool add_url);
   void ClearBlacklistedUrls();
 
   // MostVisitedSitesSupervisor::Observer implementation.
   void OnBlockedSitesChanged() override;
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
+  // 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:
-  friend class MostVisitedSitesTest;
-
   void BuildCurrentTiles();
 
   // Initialize the query to Top Sites. Called if the SuggestionsService
   // returned no data.
   void InitiateTopSitesQuery();
 
   // If there's a whitelist entry point for the URL, return the large icon path.
   base::FilePath GetWhitelistLargeIconPath(const GURL& url);
 
   // Callback for when data is available from TopSites.
@@ skipping 11 matching lines @@
 
   // Takes the personal and whitelist tiles and creates popular tiles if
   // necessary.
   NTPTilesVector CreatePopularSitesTiles(const NTPTilesVector& personal_tiles,
                                          const NTPTilesVector& whitelist_tiles);
 
   // Takes the personal tiles, creates and merges in whitelist and popular tiles
   // 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();
 
   void OnPopularSitesAvailable(bool success);
 
   void OnIconMadeAvailable(const GURL& site_url, bool newly_available);
 
   // history::TopSitesObserver implementation.
   void TopSitesLoaded(history::TopSites* top_sites) override;
@@ skipping 26 matching lines @@
 
   // For callbacks may be run after destruction.
   base::WeakPtrFactory<MostVisitedSites> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MostVisitedSites);
 };
 
 }  // namespace ntp_tiles
 
 #endif  // COMPONENTS_NTP_TILES_MOST_VISITED_SITES_H_