Test SuggestionsServiceImpl without friending anything

components/suggestions/suggestions_service_impl.h

 // Copyright 2016 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_SUGGESTIONS_SUGGESTIONS_SERVICE_IMPL_H_
 #define COMPONENTS_SUGGESTIONS_SUGGESTIONS_SERVICE_IMPL_H_
 
 #include <stdint.h>
 
 #include <memory>
@@ skipping 60 matching lines @@
       const ResponseCallback& callback) override WARN_UNUSED_RESULT;
   void GetPageThumbnail(const GURL& url,
                         const BitmapCallback& callback) override;
   void GetPageThumbnailWithURL(const GURL& url,
                                const GURL& thumbnail_url,
                                const BitmapCallback& callback) override;
   bool BlacklistURL(const GURL& candidate_url) override;
   bool UndoBlacklistURL(const GURL& url) override;
   void ClearBlacklist() override;
 
+  base::TimeDelta blacklist_delay_for_testing() const {

[Marc Treib, 2017/05/10 09:53:44]

Out of curiosity: Do you feel that "for_testing" methods are better than
friends? I usually tend to prefer friending the test base class, since then you
don't have to clutter up the public interface (but I don't feel very strongly,
so this is fine too).

[fhorschig, 2017/05/10 11:54:44]

In this case, I am (obviously) absolutely for for_testing as the friending was
abused to test small parts of the implementation and not overall behaviors.
Also, the public interface that really is used is SuggestionsService, so in
order to abuse the public for_testing methods, you would have to put in quite 
some effort.
(2nd point in our guidelines: go/zine-testing#heading=h.ch8i4lz9s0f2)

+    return scheduling_delay_;
+  }
+  void set_blacklist_delay_for_testing(base::TimeDelta delay) {
+    scheduling_delay_ = delay;
+  }
+  bool has_pending_request_for_testing() const {
+    return !!pending_request_.get();
+  }
+
   // Determines which URL a blacklist request was for, irrespective of the
   // request's status. Returns false if |request| is not a blacklist request.
   static bool GetBlacklistedUrl(const net::URLFetcher& request, GURL* url);
 
   // Register SuggestionsService related prefs in the Profile prefs.
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
  private:
-  friend class SuggestionsServiceTest;
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, FetchSuggestionsData);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, IgnoresNoopSyncChange);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           IgnoresUninterestingSyncChange);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, DoesNotFetchOnStartup);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           FetchSuggestionsDataSyncNotInitializedEnabled);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           FetchSuggestionsDataSyncDisabled);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           FetchSuggestionsDataNoAccessToken);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           IssueRequestIfNoneOngoingError);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           IssueRequestIfNoneOngoingResponseNotOK);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, BlacklistURL);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, BlacklistURLRequestFails);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, ClearBlacklist);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, UndoBlacklistURL);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           UndoBlacklistURLFailsIfNotInBlacklist);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           UndoBlacklistURLFailsIfAlreadyCandidate);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest,
-                           GetBlacklistedUrlBlacklistRequest);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, UpdateBlacklistDelay);
-  FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, CheckDefaultTimeStamps);
-
   // Establishes the different sync states that matter to SuggestionsService.
   enum SyncState {
     // State: Sync service is not initialized, yet not disabled. History sync
     //     state is unknown (since not initialized).
     // Behavior: Do not issue server requests, but serve from cache if
     //     available.
     NOT_INITIALIZED_ENABLED,
 
     // State: Sync service is initialized, sync is enabled and history sync is
     //     enabled.
@@ skipping 65 matching lines @@
   // If the local blacklist isn't empty, picks a URL from it and issues a
   // blacklist request for it.
   void UploadOneFromBlacklist();
 
   // Updates |scheduling_delay_| based on the success of the last request.
   void UpdateBlacklistDelay(bool last_request_successful);
 
   // Adds extra data to suggestions profile.
   void PopulateExtraData(SuggestionsProfile* suggestions);
 
-  // Test seams.
-  base::TimeDelta blacklist_delay() const { return scheduling_delay_; }
-  void set_blacklist_delay(base::TimeDelta delay) { scheduling_delay_ = delay; }
-
   base::ThreadChecker thread_checker_;
 
   SigninManagerBase* signin_manager_;
   OAuth2TokenService* token_service_;
 
   syncer::SyncService* sync_service_;
   ScopedObserver<syncer::SyncService, syncer::SyncServiceObserver>
       sync_service_observer_;
 
   SyncState sync_state_;
@@ skipping 29 matching lines @@
 
   // For callbacks may be run after destruction.
   base::WeakPtrFactory<SuggestionsServiceImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SuggestionsServiceImpl);
 };
 
 }  // namespace suggestions
 
 #endif  // COMPONENTS_SUGGESTIONS_SUGGESTIONS_SERVICE_IMPL_H_