Test SuggestionsServiceImpl without friending anything

components/suggestions/suggestions_service_impl_unittest.cc

 // Copyright 2014 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.
 
 #include "components/suggestions/suggestions_service_impl.h"
 
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
@@ skipping 28 matching lines @@
 using testing::AnyNumber;
 using testing::DoAll;
 using testing::Eq;
 using testing::Return;
 using testing::SetArgPointee;
 using testing::StrictMock;
 
 namespace {
 
 const char kAccountId[] = "account";
+const char kSuggestionsUrlPath[] = "/chromesuggestions";
+const char kBlacklistUrlPath[] = "/chromesuggestions/blacklist";
+const char kBlacklistClearUrlPath[] = "/chromesuggestions/blacklist/clear";
 const char kTestTitle[] = "a title";
 const char kTestUrl[] = "http://go.com";
 const char kTestFaviconUrl[] =
     "https://s2.googleusercontent.com/s2/favicons?domain_url="
     "http://go.com&alt=s&sz=32";
 const char kBlacklistedUrl[] = "http://blacklist.com";
-const char kBlacklistedUrlAlt[] = "http://blacklist-atl.com";
-const int64_t kTestDefaultExpiry = 1402200000000000;
-const int64_t kTestSetExpiry = 1404792000000000;
-
-std::unique_ptr<net::FakeURLFetcher> CreateURLFetcher(
-    const GURL& url,
-    net::URLFetcherDelegate* delegate,
-    const std::string& response_data,
-    net::HttpStatusCode response_code,
-    net::URLRequestStatus::Status status) {
-  std::unique_ptr<net::FakeURLFetcher> fetcher(new net::FakeURLFetcher(
-      url, delegate, response_data, response_code, status));
-
-  if (response_code == net::HTTP_OK) {
-    scoped_refptr<net::HttpResponseHeaders> download_headers(
-        new net::HttpResponseHeaders(""));
-    download_headers->AddHeader("Content-Type: text/html");
-    fetcher->set_response_headers(download_headers);
-  }
-  return fetcher;
-}
+const int64_t kTestSetExpiry = 12121212;  // This timestamp lies in the past.
 
 // GMock matcher for protobuf equality.
 MATCHER_P(EqualsProto, message, "") {
   // This implementation assumes protobuf serialization is deterministic, which
   // is true in practice but technically not something that code is supposed
   // to rely on.  However, it vastly simplifies the implementation.
   std::string expected_serialized, actual_serialized;
   message.SerializeToString(&expected_serialized);
   arg.SerializeToString(&actual_serialized);
   return expected_serialized == actual_serialized;
 }
 
 }  // namespace
 
 namespace suggestions {
 
 SuggestionsProfile CreateSuggestionsProfile() {
   SuggestionsProfile profile;
   profile.set_timestamp(123);
   ChromeSuggestion* suggestion = profile.add_suggestions();
   suggestion->set_title(kTestTitle);
   suggestion->set_url(kTestUrl);
-  suggestion->set_expiry_ts(kTestSetExpiry);
   return profile;
 }
 
-// Creates one suggestion with expiry timestamp and one without.
-SuggestionsProfile CreateSuggestionsProfileWithExpiryTimestamps() {
-  SuggestionsProfile profile;
-  profile.set_timestamp(123);
-  ChromeSuggestion* suggestion = profile.add_suggestions();
-  suggestion->set_title(kTestTitle);
-  suggestion->set_url(kTestUrl);
-  suggestion->set_expiry_ts(kTestSetExpiry);
-
-  suggestion = profile.add_suggestions();
-  suggestion->set_title(kTestTitle);
-  suggestion->set_url(kTestUrl);
-
-  return profile;
-}
-
 class MockSyncService : public syncer::FakeSyncService {
  public:
   MockSyncService() {}
   virtual ~MockSyncService() {}
   MOCK_CONST_METHOD0(CanSyncStart, bool());
   MOCK_CONST_METHOD0(IsSyncActive, bool());
   MOCK_CONST_METHOD0(ConfigurationDone, bool());
   MOCK_CONST_METHOD0(GetActiveDataTypes, syncer::ModelTypeSet());
 };
 
@@ skipping 36 matching lines @@
   MOCK_METHOD1(GetCandidateForUpload, bool(GURL*));
   MOCK_METHOD1(RemoveUrl, bool(const GURL&));
   MOCK_METHOD1(FilterSuggestions, void(SuggestionsProfile*));
 };
 
 class SuggestionsServiceTest : public testing::Test {
  protected:
   SuggestionsServiceTest()
       : signin_client_(&pref_service_),
         signin_manager_(&signin_client_, &account_tracker_),
-        factory_(nullptr, base::Bind(&CreateURLFetcher)),
+        request_context_(new net::TestURLRequestContextGetter(
+            io_message_loop_.task_runner())),
         mock_thumbnail_manager_(nullptr),
         mock_blacklist_store_(nullptr),
         test_suggestions_store_(nullptr) {
     SigninManagerBase::RegisterProfilePrefs(pref_service_.registry());
     SigninManagerBase::RegisterPrefs(pref_service_.registry());
 
     signin_manager_.SignIn(kAccountId);
     token_service_.UpdateCredentials(kAccountId, "refresh_token");
     token_service_.set_auto_post_fetch_response_on_message_loop(true);
   }
 
   ~SuggestionsServiceTest() override {}
 
   void SetUp() override {
-    request_context_ =
-        new net::TestURLRequestContextGetter(io_message_loop_.task_runner());
+    IgnoreRepeatedMockCalls();
+    CreateSuggestionsServiceWithMocks();
+  }
 
-    EXPECT_CALL(mock_sync_service_, CanSyncStart())
+  void TearDown() override {
+    testing::Mock::VerifyAndClearExpectations(thumbnail_manager());
+    testing::Mock::VerifyAndClearExpectations(blacklist_store());
+    testing::Mock::VerifyAndClearExpectations(sync_service());

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

I don't think this is necessary. Doesn't that happen at destruction anyway?

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

Gone.

+  }
+
+  GURL CurrentlyQueriedUrl() {

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

GetCurrentlyQueriedUrl?

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

Done.

+    net::TestURLFetcher* fetcher = factory_.GetFetcherByID(0);
+    if (!fetcher) {
+      return GURL();
+    }
+    return fetcher->GetOriginalURL();
+  }
+
+  void RespondToFetch(const std::string& response_body,
+                      net::HttpStatusCode response_code,
+                      net::URLRequestStatus status) {
+    net::TestURLFetcher* fetcher = factory_.GetFetcherByID(0);
+    ASSERT_TRUE(fetcher) << "Tried to respond to fetch that is not ongoing!";
+    fetcher->SetResponseString(response_body);
+    fetcher->set_response_code(response_code);
+    fetcher->set_status(status);
+    fetcher->delegate()->OnURLFetchComplete(fetcher);
+  }
+
+  void RespondToFetchWithProfile(const SuggestionsProfile& profile) {

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

nit: RespondToFetchWithSuggestions? I find the "profile" name quite confusing;
"profile" usually has a different meaning in Chrome.

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

Done.

+    RespondToFetch(
+        profile.SerializeAsString(), net::HTTP_OK,
+        net::URLRequestStatus(net::URLRequestStatus::SUCCESS, net::OK));
+  }
+
+  void NotifySuggestionsService() {

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

Notify of what?
TBH, I'd leave this inlined into all the tests that need it. You can make it a
one-liner by inlining the cast. (Or by making the overridden method public,
which people seem to prefer anyway. Not a fan of that, but oh well.)

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

Inlined with cast.
(I am a little bit split here. Making the already publicly accessible method
public
seems like a good move)

[Marc Treib, 2017/05/10 12:16:24]

We had that discussion before. I prefer keeping it private, but I'm in the
minority :)
Anyway, either's fine.

[fhorschig, 2017/05/10 12:38:15]

It's not a big deal. (My reasoning was: we don't approve private
inheritance, so why would you make single methods private ... it's the
same outcome with different use of keywords.)

+    // The implementation of |OnStateChanged| is private in the
+    // SuggestionsService, but public for every SyncServiceObserver.
+    syncer::SyncServiceObserver* observer = suggestions_service();
+    observer->OnStateChanged(sync_service());
+  }
+
+  void IgnoreRepeatedMockCalls() {

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

The point isn't to ignore the calls, but to return appropriate defaults.
(Arguably these shouldn't be mocks at all, but fakes. But let's leave that for a
possible followup.)
So "Set up default responses" or something? (Or just inline it back into SetUp?
If not, it should be private.)

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

Inlined.

+    EXPECT_CALL(*sync_service(), CanSyncStart())
         .Times(AnyNumber())
         .WillRepeatedly(Return(true));
-    EXPECT_CALL(mock_sync_service_, IsSyncActive())
+    EXPECT_CALL(*sync_service(), IsSyncActive())
         .Times(AnyNumber())
         .WillRepeatedly(Return(true));
-    EXPECT_CALL(mock_sync_service_, ConfigurationDone())
+    EXPECT_CALL(*sync_service(), ConfigurationDone())
         .Times(AnyNumber())
         .WillRepeatedly(Return(true));
-    EXPECT_CALL(mock_sync_service_, GetActiveDataTypes())
+    EXPECT_CALL(*sync_service(), GetActiveDataTypes())
         .Times(AnyNumber())
         .WillRepeatedly(
             Return(syncer::ModelTypeSet(syncer::HISTORY_DELETE_DIRECTIVES)));
+  }
 
+  MockBlacklistStore* blacklist_store() { return mock_blacklist_store_; }
+
+  SuggestionsServiceImpl* suggestions_service() {
+    return suggestions_service_.get();
+  }
+
+  TestSuggestionsStore* suggestions_store() { return test_suggestions_store_; }
+
+  MockSyncService* sync_service() { return &mock_sync_service_; }
+
+  MockImageManager* thumbnail_manager() { return mock_thumbnail_manager_; }
+
+  FakeProfileOAuth2TokenService* token_service() { return &token_service_; }

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

nit: The order of the accessors seems a bit arbitrary. Match the order of the
fields?

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

I don't consider alphabetic ordering very arbitrary.
But order of fields make more sense. Changed.

[Marc Treib, 2017/05/10 12:16:24]

Hah, I didn't notice it was alphabetical :)
Still, the new order is better IMO.

[fhorschig, 2017/05/10 12:38:15]

Agreed.

+
+ private:
+  void CreateSuggestionsServiceWithMocks() {

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

Any reason this is a separate method?

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

Only Historic reasons. Inlined into setup.

     // These objects are owned by the SuggestionsService, but we keep the
     // pointers around for testing.
     test_suggestions_store_ = new TestSuggestionsStore();
     mock_thumbnail_manager_ = new StrictMock<MockImageManager>();
     mock_blacklist_store_ = new StrictMock<MockBlacklistStore>();
     suggestions_service_ = base::MakeUnique<SuggestionsServiceImpl>(
         &signin_manager_, &token_service_, &mock_sync_service_,
         request_context_.get(), base::WrapUnique(test_suggestions_store_),
         base::WrapUnique(mock_thumbnail_manager_),
         base::WrapUnique(mock_blacklist_store_));
   }
 
-  bool HasPendingSuggestionsRequest() const {
-    return !!suggestions_service_->pending_request_.get();
-  }
-
- protected:
   base::MessageLoopForIO io_message_loop_;
   TestingPrefServiceSyncable pref_service_;
   AccountTrackerService account_tracker_;
   TestSigninClient signin_client_;
   FakeSigninManagerBase signin_manager_;
-  net::FakeURLFetcherFactory factory_;
+  net::TestURLFetcherFactory factory_;
   FakeProfileOAuth2TokenService token_service_;
   MockSyncService mock_sync_service_;
   scoped_refptr<net::TestURLRequestContextGetter> request_context_;
   // Owned by the SuggestionsService.
   MockImageManager* mock_thumbnail_manager_;
   MockBlacklistStore* mock_blacklist_store_;
   TestSuggestionsStore* test_suggestions_store_;
 
   std::unique_ptr<SuggestionsServiceImpl> suggestions_service_;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(SuggestionsServiceTest);
 };
 
 TEST_F(SuggestionsServiceTest, FetchSuggestionsData) {
   base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
-  // Set up net::FakeURLFetcherFactory.
-  factory_.SetFakeResponse(SuggestionsServiceImpl::BuildSuggestionsURL(),
-                           CreateSuggestionsProfile().SerializeAsString(),
-                           net::HTTP_OK, net::URLRequestStatus::SUCCESS);
-
-  EXPECT_CALL(*mock_thumbnail_manager_, Initialize(_));
-  EXPECT_CALL(*mock_blacklist_store_, FilterSuggestions(_));
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
+  EXPECT_CALL(*thumbnail_manager(), Initialize(_));
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_));
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
       .WillOnce(Return(false));
 
   // Send the request. The data should be returned to the callback.
-  suggestions_service_->FetchSuggestionsData();
+  suggestions_service()->FetchSuggestionsData();
 
   EXPECT_CALL(callback, Run(_));
 
-  // Let the network request run.
   base::RunLoop().RunUntilIdle();

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

Is the RunLoop still needed? (Lots more instances of this below)

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

Sadly yes. The request is preceeded by a task from the token fetcher.

+  EXPECT_FALSE(CurrentlyQueriedUrl().is_empty());
+  EXPECT_TRUE(CurrentlyQueriedUrl().is_valid());

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

Valid implies not empty, so the above check is redundant.
Also, this should maybe be an ASSERT?

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

Absolutely.

+  EXPECT_EQ(CurrentlyQueriedUrl().path(), kSuggestionsUrlPath);
+  RespondToFetchWithProfile(CreateSuggestionsProfile());
 
-  SuggestionsProfile suggestions;
-  test_suggestions_store_->LoadSuggestions(&suggestions);
-  ASSERT_EQ(1, suggestions.suggestions_size());
-  EXPECT_EQ(kTestTitle, suggestions.suggestions(0).title());
-  EXPECT_EQ(kTestUrl, suggestions.suggestions(0).url());
-  EXPECT_EQ(kTestFaviconUrl, suggestions.suggestions(0).favicon_url());
+  SuggestionsProfile suggestions_profile;

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

nit: Any reason for putting the "profile" back? (more instances below)

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

Rerenamed all instances.
(Technically, it _is_ a profile that contains a list of suggestions)

[Marc Treib, 2017/05/10 12:16:24]

Some places called this "suggestions_profile", some just "suggestions". I
decided to make it "suggestions" everywhere for consistency, since I'm not a fan
of the "Profile" name anyway.

[fhorschig, 2017/05/10 12:38:15]

Acknowledged.

+  suggestions_store()->LoadSuggestions(&suggestions_profile);
+  ASSERT_EQ(1, suggestions_profile.suggestions_size());
+  EXPECT_EQ(kTestTitle, suggestions_profile.suggestions(0).title());
+  EXPECT_EQ(kTestUrl, suggestions_profile.suggestions(0).url());
+  EXPECT_EQ(kTestFaviconUrl, suggestions_profile.suggestions(0).favicon_url());
 }
 
 TEST_F(SuggestionsServiceTest, IgnoresNoopSyncChange) {
   base::MockCallback<SuggestionsService::ResponseCallback> callback;
   EXPECT_CALL(callback, Run(_)).Times(0);
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  factory_.SetFakeResponse(SuggestionsServiceImpl::BuildSuggestionsURL(),
-                           CreateSuggestionsProfile().SerializeAsString(),
-                           net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
   // An no-op change should not result in a suggestions refresh.
-  suggestions_service_->OnStateChanged(&mock_sync_service_);
+  NotifySuggestionsService();
 
   // Let any network request run (there shouldn't be one).

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

If the RunLoop is still needed, this comment needs to be updated.

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

Done.

   base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(suggestions_service()->has_pending_request_for_testing());
 }
 
 TEST_F(SuggestionsServiceTest, IgnoresUninterestingSyncChange) {
   base::MockCallback<SuggestionsService::ResponseCallback> callback;
   EXPECT_CALL(callback, Run(_)).Times(0);
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  factory_.SetFakeResponse(SuggestionsServiceImpl::BuildSuggestionsURL(),
-                           CreateSuggestionsProfile().SerializeAsString(),
-                           net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
   // An uninteresting change should not result in a network request (the
   // SyncState is INITIALIZED_ENABLED_HISTORY before and after).
-  EXPECT_CALL(mock_sync_service_, GetActiveDataTypes())
+  EXPECT_CALL(*sync_service(), GetActiveDataTypes())
       .Times(AnyNumber())
       .WillRepeatedly(Return(syncer::ModelTypeSet(
           syncer::HISTORY_DELETE_DIRECTIVES, syncer::BOOKMARKS)));
-  suggestions_service_->OnStateChanged(&mock_sync_service_);
+  NotifySuggestionsService();
 
   // Let any network request run (there shouldn't be one).
   base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(suggestions_service()->has_pending_request_for_testing());
 }
 
 // During startup, the state changes from NOT_INITIALIZED_ENABLED to
 // INITIALIZED_ENABLED_HISTORY (for a signed-in user with history sync enabled).
 // This should *not* result in an automatic fetch.
 TEST_F(SuggestionsServiceTest, DoesNotFetchOnStartup) {
   // The sync service starts out inactive.
-  EXPECT_CALL(mock_sync_service_, IsSyncActive()).WillRepeatedly(Return(false));
-  suggestions_service_->OnStateChanged(&mock_sync_service_);
-
-  ASSERT_EQ(SuggestionsServiceImpl::NOT_INITIALIZED_ENABLED,
-            suggestions_service_->ComputeSyncState());
+  EXPECT_CALL(*sync_service(), IsSyncActive()).WillRepeatedly(Return(false));
+  NotifySuggestionsService();
+
+  base::RunLoop().RunUntilIdle();
+  ASSERT_FALSE(suggestions_service()->has_pending_request_for_testing());
+
+  // Sync getting enabled should not result in a fetch.
+  EXPECT_CALL(*sync_service(), IsSyncActive()).WillRepeatedly(Return(true));
+  NotifySuggestionsService();
+
+  // Let any network request run (there shouldn't be one).
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(suggestions_service()->has_pending_request_for_testing());
+}
+
+TEST_F(SuggestionsServiceTest, FetchSuggestionsDataSyncNotInitializedEnabled) {
+  EXPECT_CALL(*sync_service(), IsSyncActive()).WillRepeatedly(Return(false));
+  NotifySuggestionsService();
 
   base::MockCallback<SuggestionsService::ResponseCallback> callback;
   EXPECT_CALL(callback, Run(_)).Times(0);
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  factory_.SetFakeResponse(SuggestionsServiceImpl::BuildSuggestionsURL(),
-                           CreateSuggestionsProfile().SerializeAsString(),
-                           net::HTTP_OK, net::URLRequestStatus::SUCCESS);
-
-  // Sync getting enabled should not result in a fetch.
-  EXPECT_CALL(mock_sync_service_, IsSyncActive()).WillRepeatedly(Return(true));
-  suggestions_service_->OnStateChanged(&mock_sync_service_);
-
-  ASSERT_EQ(SuggestionsServiceImpl::INITIALIZED_ENABLED_HISTORY,
-            suggestions_service_->ComputeSyncState());
-
-  // Let any network request run (there shouldn't be one).
-  base::RunLoop().RunUntilIdle();
-}
-
-TEST_F(SuggestionsServiceTest, FetchSuggestionsDataSyncNotInitializedEnabled) {
-  EXPECT_CALL(mock_sync_service_, IsSyncActive()).WillRepeatedly(Return(false));
-  suggestions_service_->OnStateChanged(&mock_sync_service_);
-
-  base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  EXPECT_CALL(callback, Run(_)).Times(0);
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
   // Try to fetch suggestions. Since sync is not active, no network request
   // should be sent.
-  suggestions_service_->FetchSuggestionsData();
+  suggestions_service()->FetchSuggestionsData();
 
   // Let any network request run (there shouldn't be one).
   base::RunLoop().RunUntilIdle();
-
-  // |test_suggestions_store_| should still contain the default values.
+  EXPECT_FALSE(suggestions_service()->has_pending_request_for_testing());
+
+  // |suggestions_store()| should still contain the default values.
   SuggestionsProfile suggestions;
-  test_suggestions_store_->LoadSuggestions(&suggestions);
+  suggestions_store()->LoadSuggestions(&suggestions);
   EXPECT_THAT(suggestions, EqualsProto(CreateSuggestionsProfile()));
 }
 
 TEST_F(SuggestionsServiceTest, FetchSuggestionsDataSyncDisabled) {
-  EXPECT_CALL(mock_sync_service_, CanSyncStart()).WillRepeatedly(Return(false));
-
-  base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
+  EXPECT_CALL(*sync_service(), CanSyncStart()).WillRepeatedly(Return(false));
+
+  base::MockCallback<SuggestionsService::ResponseCallback> callback;
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
   // Tell SuggestionsService that the sync state changed. The cache should be
   // cleared and empty data returned to the callback.
   EXPECT_CALL(callback, Run(EqualsProto(SuggestionsProfile())));
-  suggestions_service_->OnStateChanged(&mock_sync_service_);
+  NotifySuggestionsService();
 
   // Try to fetch suggestions. Since sync is not active, no network request
   // should be sent.
-  suggestions_service_->FetchSuggestionsData();
+  suggestions_service()->FetchSuggestionsData();
 
   // Let any network request run.
   base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(suggestions_service()->has_pending_request_for_testing());
 }
 
 TEST_F(SuggestionsServiceTest, FetchSuggestionsDataNoAccessToken) {
-  token_service_.set_auto_post_fetch_response_on_message_loop(false);
+  token_service()->set_auto_post_fetch_response_on_message_loop(false);
 
   base::MockCallback<SuggestionsService::ResponseCallback> callback;
   EXPECT_CALL(callback, Run(_)).Times(0);
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
-      .WillOnce(Return(false));
-
-  suggestions_service_->FetchSuggestionsData();
-
-  token_service_.IssueErrorForAllPendingRequests(GoogleServiceAuthError(
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
+
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
+      .WillOnce(Return(false));
+
+  suggestions_service()->FetchSuggestionsData();
+
+  token_service()->IssueErrorForAllPendingRequests(GoogleServiceAuthError(
       GoogleServiceAuthError::State::INVALID_GAIA_CREDENTIALS));
 
   // No network request should be sent.
   base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(HasPendingSuggestionsRequest());
-}
-
-TEST_F(SuggestionsServiceTest, IssueRequestIfNoneOngoingError) {
-  // Fake a request error.
-  factory_.SetFakeResponse(SuggestionsServiceImpl::BuildSuggestionsURL(),
-                           "irrelevant", net::HTTP_OK,
-                           net::URLRequestStatus::FAILED);
-
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
+  EXPECT_FALSE(suggestions_service()->has_pending_request_for_testing());
+}
+
+TEST_F(SuggestionsServiceTest, FetchingSuggestionsIgnoresRequestFailure) {
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
+      .WillOnce(Return(false));
+
+  suggestions_service()->FetchSuggestionsData();
+
+  base::RunLoop().RunUntilIdle();
+  RespondToFetch("irrelevant", net::HTTP_OK,
+                 net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                       net::ERR_INVALID_RESPONSE));

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

What exactly does this test?

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

The original idea was probably to just have the code path running and look if
it crashes.
The only expectation is |GetTimeUntilReadyForUpload| which seems to be a hacky
proxy to check whether |ScheduleBlacklistUpload| has been called.

(I did not want to write a replacement in this CL. It's already too large)

[Marc Treib, 2017/05/10 12:16:24]

Acknowledged.

[fhorschig, 2017/05/10 12:38:15]

Acknowledged.

+}
+
+TEST_F(SuggestionsServiceTest, FetchingSuggestionsClearsStoreIfResponseNotOK) {
+  suggestions_store()->StoreSuggestions(CreateSuggestionsProfile());
+
+  // Expect that an upload to the blacklist is scheduled.
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
       .WillOnce(Return(false));
 
   // Send the request. Empty data will be returned to the callback.
-  suggestions_service_->IssueRequestIfNoneOngoing(
-      SuggestionsServiceImpl::BuildSuggestionsURL());
-
-  // (Testing only) wait until suggestion fetch is complete.
-  base::RunLoop().RunUntilIdle();
-}
-
-TEST_F(SuggestionsServiceTest, IssueRequestIfNoneOngoingResponseNotOK) {
-  // Fake a non-200 response code.
-  factory_.SetFakeResponse(SuggestionsServiceImpl::BuildSuggestionsURL(),
-                           "irrelevant", net::HTTP_BAD_REQUEST,
-                           net::URLRequestStatus::SUCCESS);
-
-  // Expect that an upload to the blacklist is scheduled.
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
-      .WillOnce(Return(false));
-
-  // Send the request. Empty data will be returned to the callback.
-  suggestions_service_->IssueRequestIfNoneOngoing(
-      SuggestionsServiceImpl::BuildSuggestionsURL());
-
-  // (Testing only) wait until suggestion fetch is complete.
-  base::RunLoop().RunUntilIdle();
-
-  // Expect no suggestions in the cache.
+  suggestions_service()->FetchSuggestionsData();
+
+  base::RunLoop().RunUntilIdle();
+  RespondToFetch(
+      "irrelevant", net::HTTP_BAD_REQUEST,
+      net::URLRequestStatus(net::URLRequestStatus::SUCCESS, net::OK));
+
   SuggestionsProfile empty_suggestions;
-  EXPECT_FALSE(test_suggestions_store_->LoadSuggestions(&empty_suggestions));
+  EXPECT_FALSE(suggestions_store()->LoadSuggestions(&empty_suggestions));
 }
 
 TEST_F(SuggestionsServiceTest, BlacklistURL) {
+  // Calling RunUntilIdle on the MessageLoop only works when the task is not

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

nit: It's a RunLoop, not a MessageLoop.

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

Thanks! I mindlessly copied that comment.

+  // posted for the future.
   const base::TimeDelta no_delay = base::TimeDelta::FromSeconds(0);
-  suggestions_service_->set_blacklist_delay(no_delay);
-
-  base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  const GURL blacklisted_url(kBlacklistedUrl);
-  const GURL request_url(
-      SuggestionsServiceImpl::BuildSuggestionsBlacklistURL(blacklisted_url));
-  factory_.SetFakeResponse(request_url,
-                           CreateSuggestionsProfile().SerializeAsString(),
-                           net::HTTP_OK, net::URLRequestStatus::SUCCESS);
-  EXPECT_CALL(*mock_thumbnail_manager_, Initialize(_)).Times(2);
-
-  // Expected calls to the blacklist store.
-  EXPECT_CALL(*mock_blacklist_store_, BlacklistUrl(Eq(blacklisted_url)))
-      .WillOnce(Return(true));
-  EXPECT_CALL(*mock_blacklist_store_, FilterSuggestions(_)).Times(2);
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
+  suggestions_service()->set_blacklist_delay_for_testing(no_delay);
+
+  base::MockCallback<SuggestionsService::ResponseCallback> callback;
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
+
+  EXPECT_CALL(*thumbnail_manager(), Initialize(_)).Times(2);
+  EXPECT_CALL(*blacklist_store(), BlacklistUrl(Eq(GURL(kBlacklistedUrl))))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_)).Times(2);
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
       .WillOnce(DoAll(SetArgPointee<0>(no_delay), Return(true)))
       .WillOnce(Return(false));
-  EXPECT_CALL(*mock_blacklist_store_, GetCandidateForUpload(_))
-      .WillOnce(DoAll(SetArgPointee<0>(blacklisted_url), Return(true)));
-  EXPECT_CALL(*mock_blacklist_store_, RemoveUrl(Eq(blacklisted_url)))
+  EXPECT_CALL(*blacklist_store(), GetCandidateForUpload(_))
+      .WillOnce(DoAll(SetArgPointee<0>(GURL(kBlacklistedUrl)), Return(true)));
+  EXPECT_CALL(*blacklist_store(), RemoveUrl(Eq(GURL(kBlacklistedUrl))))
       .WillOnce(Return(true));
 
   EXPECT_CALL(callback, Run(_)).Times(2);
 
-  EXPECT_TRUE(suggestions_service_->BlacklistURL(blacklisted_url));
+  EXPECT_TRUE(suggestions_service()->BlacklistURL(GURL(kBlacklistedUrl)));
 
   // Wait on the upload task, the blacklist request and the next blacklist
-  // scheduling task. This only works when the scheduling task is not for future
-  // execution (note how both the SuggestionsService's scheduling delay and the
-  // BlacklistStore's candidacy delay are zero).

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

I'd leave the explanation here.

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

Reverted.

-  base::RunLoop().RunUntilIdle();
-
-  SuggestionsProfile suggestions;
-  test_suggestions_store_->LoadSuggestions(&suggestions);
-  ASSERT_EQ(1, suggestions.suggestions_size());
-  EXPECT_EQ(kTestTitle, suggestions.suggestions(0).title());
-  EXPECT_EQ(kTestUrl, suggestions.suggestions(0).url());
-  EXPECT_EQ(kTestFaviconUrl, suggestions.suggestions(0).favicon_url());
+  // scheduling task.
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(CurrentlyQueriedUrl().path(), kBlacklistUrlPath);
+  RespondToFetchWithProfile(CreateSuggestionsProfile());
+
+  SuggestionsProfile suggestions_profile;
+  suggestions_store()->LoadSuggestions(&suggestions_profile);
+  ASSERT_EQ(1, suggestions_profile.suggestions_size());
+  EXPECT_EQ(kTestTitle, suggestions_profile.suggestions(0).title());
+  EXPECT_EQ(kTestUrl, suggestions_profile.suggestions(0).url());
+  EXPECT_EQ(kTestFaviconUrl, suggestions_profile.suggestions(0).favicon_url());
 }
 
 TEST_F(SuggestionsServiceTest, BlacklistURLFails) {
   base::MockCallback<SuggestionsService::ResponseCallback> callback;
   EXPECT_CALL(callback, Run(_)).Times(0);
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  const GURL blacklisted_url(kBlacklistedUrl);
-  EXPECT_CALL(*mock_blacklist_store_, BlacklistUrl(Eq(blacklisted_url)))
-      .WillOnce(Return(false));
-  EXPECT_FALSE(suggestions_service_->BlacklistURL(blacklisted_url));
-}
-
-// Initial blacklist request fails, triggering a second which succeeds.
-TEST_F(SuggestionsServiceTest, BlacklistURLRequestFails) {
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
+
+  EXPECT_CALL(*blacklist_store(), BlacklistUrl(Eq(GURL(kBlacklistedUrl))))
+      .WillOnce(Return(false));
+
+  EXPECT_FALSE(suggestions_service()->BlacklistURL(GURL(kBlacklistedUrl)));
+}
+
+TEST_F(SuggestionsServiceTest, RetryBlacklistURLRequestAfterFailure) {
+  // Calling RunUntilIdle on the MessageLoop only works when the task is not
+  // posted for the future.
   const base::TimeDelta no_delay = base::TimeDelta::FromSeconds(0);
-  suggestions_service_->set_blacklist_delay(no_delay);
-
-  base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  const GURL blacklisted_url(kBlacklistedUrl);
-  const GURL request_url(
-      SuggestionsServiceImpl::BuildSuggestionsBlacklistURL(blacklisted_url));
-  const GURL blacklisted_url_alt(kBlacklistedUrlAlt);
-  const GURL request_url_alt(
-      SuggestionsServiceImpl::BuildSuggestionsBlacklistURL(
-          blacklisted_url_alt));
-
-  // Note: we want to set the response for the blacklist URL to first
-  // succeed, then fail. This doesn't seem possible. For simplicity of testing,
-  // we'll pretend the URL changed in the BlacklistStore between the first and
-  // the second request, and adjust expectations accordingly.
-  factory_.SetFakeResponse(request_url, "irrelevant", net::HTTP_OK,
-                           net::URLRequestStatus::FAILED);
-  factory_.SetFakeResponse(request_url_alt,
-                           CreateSuggestionsProfile().SerializeAsString(),
-                           net::HTTP_OK, net::URLRequestStatus::SUCCESS);
-
-  // Expectations.
-  EXPECT_CALL(*mock_thumbnail_manager_, Initialize(_)).Times(2);
-  EXPECT_CALL(*mock_blacklist_store_, BlacklistUrl(Eq(blacklisted_url)))
-      .WillOnce(Return(true));
-  EXPECT_CALL(*mock_blacklist_store_, FilterSuggestions(_)).Times(2);
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
+  suggestions_service()->set_blacklist_delay_for_testing(no_delay);
+
+  base::MockCallback<SuggestionsService::ResponseCallback> callback;
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
+
+  EXPECT_CALL(*thumbnail_manager(), Initialize(_)).Times(2);
+  EXPECT_CALL(*blacklist_store(), BlacklistUrl(Eq(GURL(kBlacklistedUrl))))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_)).Times(2);
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))

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

I think the stuff below might be worth a comment (along the lines of the "Note"
that was there before)

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

Done and split the expectations to see the the correlation to success/failure.

       .WillOnce(DoAll(SetArgPointee<0>(no_delay), Return(true)))
       .WillOnce(DoAll(SetArgPointee<0>(no_delay), Return(true)))
       .WillOnce(Return(false));
-  EXPECT_CALL(*mock_blacklist_store_, GetCandidateForUpload(_))
-      .WillOnce(DoAll(SetArgPointee<0>(blacklisted_url), Return(true)))
-      .WillOnce(DoAll(SetArgPointee<0>(blacklisted_url_alt), Return(true)));
-  EXPECT_CALL(*mock_blacklist_store_, RemoveUrl(Eq(blacklisted_url_alt)))
+  EXPECT_CALL(*blacklist_store(), GetCandidateForUpload(_))
+      .WillOnce(DoAll(SetArgPointee<0>(GURL(kBlacklistedUrl)), Return(true)))
+      .WillOnce(DoAll(SetArgPointee<0>(GURL(kBlacklistedUrl)), Return(true)));
+  EXPECT_CALL(*blacklist_store(), RemoveUrl(Eq(GURL(kBlacklistedUrl))))
       .WillOnce(Return(true));
 
   EXPECT_CALL(callback, Run(_)).Times(2);
 
   // Blacklist call, first request attempt.
-  EXPECT_TRUE(suggestions_service_->BlacklistURL(blacklisted_url));
-
-  // Wait for the first scheduling, the first request, the second scheduling,
-  // second request and the third scheduling. Again, note that calling
-  // RunUntilIdle on the MessageLoop only works when the task is not posted for
-  // the future.
-  base::RunLoop().RunUntilIdle();
-
-  SuggestionsProfile suggestions;
-  test_suggestions_store_->LoadSuggestions(&suggestions);
-  ASSERT_EQ(1, suggestions.suggestions_size());
-  EXPECT_EQ(kTestTitle, suggestions.suggestions(0).title());
-  EXPECT_EQ(kTestUrl, suggestions.suggestions(0).url());
-  EXPECT_EQ(kTestFaviconUrl, suggestions.suggestions(0).favicon_url());
+  EXPECT_TRUE(suggestions_service()->BlacklistURL(GURL(kBlacklistedUrl)));
+
+  // Wait for the first scheduling receiving a failing response.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(CurrentlyQueriedUrl().path(), kBlacklistUrlPath);
+  RespondToFetch("irrelevant", net::HTTP_OK,
+                 net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                       net::ERR_INVALID_RESPONSE));
+
+  // Wait for the second scheduling followed by a successful response.
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(CurrentlyQueriedUrl().is_valid());

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

nit: Why? There isn't a similar assert above.

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

But there should be. Fixed.

+  EXPECT_EQ(CurrentlyQueriedUrl().path(), kBlacklistUrlPath);
+  RespondToFetchWithProfile(CreateSuggestionsProfile());
+
+  SuggestionsProfile suggestions_profile;
+  suggestions_store()->LoadSuggestions(&suggestions_profile);
+  ASSERT_EQ(1, suggestions_profile.suggestions_size());
+  EXPECT_EQ(kTestTitle, suggestions_profile.suggestions(0).title());
+  EXPECT_EQ(kTestUrl, suggestions_profile.suggestions(0).url());
+  EXPECT_EQ(kTestFaviconUrl, suggestions_profile.suggestions(0).favicon_url());
 }
 
 TEST_F(SuggestionsServiceTest, UndoBlacklistURL) {
   // Ensure scheduling the request doesn't happen before undo.
   const base::TimeDelta delay = base::TimeDelta::FromHours(1);
-  suggestions_service_->set_blacklist_delay(delay);
-
-  base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  const GURL blacklisted_url(kBlacklistedUrl);
+  suggestions_service()->set_blacklist_delay_for_testing(delay);
+
+  base::MockCallback<SuggestionsService::ResponseCallback> callback;
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
   // Blacklist expectations.
-  EXPECT_CALL(*mock_blacklist_store_, BlacklistUrl(Eq(blacklisted_url)))
-      .WillOnce(Return(true));
-  EXPECT_CALL(*mock_thumbnail_manager_,
+  EXPECT_CALL(*blacklist_store(), BlacklistUrl(Eq(GURL(kBlacklistedUrl))))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*thumbnail_manager(),
               Initialize(EqualsProto(CreateSuggestionsProfile())))
       .Times(AnyNumber());
-  EXPECT_CALL(*mock_blacklist_store_, FilterSuggestions(_)).Times(AnyNumber());
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_)).Times(AnyNumber());
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
       .WillOnce(DoAll(SetArgPointee<0>(delay), Return(true)));
   // Undo expectations.
-  EXPECT_CALL(*mock_blacklist_store_,
-              GetTimeUntilURLReadyForUpload(Eq(blacklisted_url), _))
+  EXPECT_CALL(*blacklist_store(),
+              GetTimeUntilURLReadyForUpload(Eq(GURL(kBlacklistedUrl)), _))
       .WillOnce(DoAll(SetArgPointee<1>(delay), Return(true)));
-  EXPECT_CALL(*mock_blacklist_store_, RemoveUrl(Eq(blacklisted_url)))
+  EXPECT_CALL(*blacklist_store(), RemoveUrl(Eq(GURL(kBlacklistedUrl))))
       .WillOnce(Return(true));
 
   EXPECT_CALL(callback, Run(_)).Times(2);
-  EXPECT_TRUE(suggestions_service_->BlacklistURL(blacklisted_url));
-  EXPECT_TRUE(suggestions_service_->UndoBlacklistURL(blacklisted_url));
+  EXPECT_TRUE(suggestions_service()->BlacklistURL(GURL(kBlacklistedUrl)));
+  EXPECT_TRUE(suggestions_service()->UndoBlacklistURL(GURL(kBlacklistedUrl)));
 }
 
 TEST_F(SuggestionsServiceTest, ClearBlacklist) {
   // Ensure scheduling the request doesn't happen before undo.

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

Pre-existing problem, but this comment seems out of place - there's no undo
involved?

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

Gone.

   const base::TimeDelta delay = base::TimeDelta::FromHours(1);
-  suggestions_service_->set_blacklist_delay(delay);
-
-  base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  const SuggestionsProfile suggestions_profile = CreateSuggestionsProfile();
-  const GURL blacklisted_url(kBlacklistedUrl);
-
-  factory_.SetFakeResponse(
-      SuggestionsServiceImpl::BuildSuggestionsBlacklistClearURL(),
-      suggestions_profile.SerializeAsString(), net::HTTP_OK,
-      net::URLRequestStatus::SUCCESS);
+  suggestions_service()->set_blacklist_delay_for_testing(delay);
+  base::MockCallback<SuggestionsService::ResponseCallback> callback;

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

nit: Keep the newline above

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

Done.

+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
   // Blacklist expectations.
-  EXPECT_CALL(*mock_blacklist_store_, BlacklistUrl(Eq(blacklisted_url)))
-      .WillOnce(Return(true));
-  EXPECT_CALL(*mock_thumbnail_manager_,
-              Initialize(EqualsProto(suggestions_profile)))
+  EXPECT_CALL(*blacklist_store(), BlacklistUrl(Eq(GURL(kBlacklistedUrl))))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*thumbnail_manager(),
+              Initialize(EqualsProto(CreateSuggestionsProfile())))
       .Times(AnyNumber());
-  EXPECT_CALL(*mock_blacklist_store_, FilterSuggestions(_)).Times(AnyNumber());
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_)).Times(AnyNumber());
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
       .WillOnce(DoAll(SetArgPointee<0>(delay), Return(true)));
-  EXPECT_CALL(*mock_blacklist_store_, ClearBlacklist());
+  EXPECT_CALL(*blacklist_store(), ClearBlacklist());
 
   EXPECT_CALL(callback, Run(_)).Times(2);
-  EXPECT_TRUE(suggestions_service_->BlacklistURL(blacklisted_url));
-  suggestions_service_->ClearBlacklist();
+  EXPECT_TRUE(suggestions_service()->BlacklistURL(GURL(kBlacklistedUrl)));
+  suggestions_service()->ClearBlacklist();
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(CurrentlyQueriedUrl().path(), kBlacklistClearUrlPath);
 }
 
 TEST_F(SuggestionsServiceTest, UndoBlacklistURLFailsIfNotInBlacklist) {
   // Ensure scheduling the request doesn't happen before undo.
   const base::TimeDelta delay = base::TimeDelta::FromHours(1);
-  suggestions_service_->set_blacklist_delay(delay);
-
-  base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  const GURL blacklisted_url(kBlacklistedUrl);
+  suggestions_service()->set_blacklist_delay_for_testing(delay);
+  base::MockCallback<SuggestionsService::ResponseCallback> callback;

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

Also here: newline above

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

Also done.

+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
   // Blacklist expectations.
-  EXPECT_CALL(*mock_blacklist_store_, BlacklistUrl(Eq(blacklisted_url)))
-      .WillOnce(Return(true));
-  EXPECT_CALL(*mock_thumbnail_manager_,
+  EXPECT_CALL(*blacklist_store(), BlacklistUrl(Eq(GURL(kBlacklistedUrl))))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*thumbnail_manager(),
               Initialize(EqualsProto(CreateSuggestionsProfile())));
-  EXPECT_CALL(*mock_blacklist_store_, FilterSuggestions(_));
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_));
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
       .WillOnce(DoAll(SetArgPointee<0>(delay), Return(true)));
-
+  // Undo expectations.
   // URL is not in local blacklist.
-  EXPECT_CALL(*mock_blacklist_store_,
-              GetTimeUntilURLReadyForUpload(Eq(blacklisted_url), _))
+  EXPECT_CALL(*blacklist_store(),
+              GetTimeUntilURLReadyForUpload(Eq(GURL(kBlacklistedUrl)), _))
       .WillOnce(Return(false));
 
   EXPECT_CALL(callback, Run(_));
   EXPECT_TRUE(suggestions_service_->BlacklistURL(blacklisted_url));
   EXPECT_FALSE(suggestions_service_->UndoBlacklistURL(blacklisted_url));

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

These two lines were replaced by the two below I think?

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

Gone.
(Yes, they even broke compilation)

+
+  EXPECT_TRUE(suggestions_service()->BlacklistURL(GURL(kBlacklistedUrl)));
+  EXPECT_FALSE(suggestions_service()->UndoBlacklistURL(GURL(kBlacklistedUrl)));
 }
 
 TEST_F(SuggestionsServiceTest, UndoBlacklistURLFailsIfAlreadyCandidate) {
   // Ensure scheduling the request doesn't happen before undo.
   const base::TimeDelta delay = base::TimeDelta::FromHours(1);
-  suggestions_service_->set_blacklist_delay(delay);
-
-  base::MockCallback<SuggestionsService::ResponseCallback> callback;
-  auto subscription = suggestions_service_->AddCallback(callback.Get());
-
-  const GURL blacklisted_url(kBlacklistedUrl);
+  suggestions_service()->set_blacklist_delay_for_testing(delay);
+
+  base::MockCallback<SuggestionsService::ResponseCallback> callback;
+  auto subscription = suggestions_service()->AddCallback(callback.Get());
 
   // Blacklist expectations.
-  EXPECT_CALL(*mock_blacklist_store_, BlacklistUrl(Eq(blacklisted_url)))
-      .WillOnce(Return(true));
-  EXPECT_CALL(*mock_thumbnail_manager_,
+  EXPECT_CALL(*blacklist_store(), BlacklistUrl(Eq(GURL(kBlacklistedUrl))))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*thumbnail_manager(),
               Initialize(EqualsProto(CreateSuggestionsProfile())));
-  EXPECT_CALL(*mock_blacklist_store_, FilterSuggestions(_));
-  EXPECT_CALL(*mock_blacklist_store_, GetTimeUntilReadyForUpload(_))
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_));
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
       .WillOnce(DoAll(SetArgPointee<0>(delay), Return(true)));
 
+  // Undo expectations.
   // URL is not yet candidate for upload.
-  base::TimeDelta negative_delay = base::TimeDelta::FromHours(-1);
-  EXPECT_CALL(*mock_blacklist_store_,
-              GetTimeUntilURLReadyForUpload(Eq(blacklisted_url), _))
+  const base::TimeDelta negative_delay = base::TimeDelta::FromHours(-1);
+  EXPECT_CALL(*blacklist_store(),
+              GetTimeUntilURLReadyForUpload(Eq(GURL(kBlacklistedUrl)), _))
       .WillOnce(DoAll(SetArgPointee<1>(negative_delay), Return(true)));
 
   EXPECT_CALL(callback, Run(_));
-  EXPECT_TRUE(suggestions_service_->BlacklistURL(blacklisted_url));
-  EXPECT_FALSE(suggestions_service_->UndoBlacklistURL(blacklisted_url));
-}
-
-TEST_F(SuggestionsServiceTest, GetBlacklistedUrlNotBlacklistRequest) {
-  // Not a blacklist request.
-  std::unique_ptr<net::FakeURLFetcher> fetcher(
-      CreateURLFetcher(GURL("http://not-blacklisting.com/a?b=c"), nullptr, "",
-                       net::HTTP_OK, net::URLRequestStatus::SUCCESS));
-  GURL retrieved_url;
-  EXPECT_FALSE(
-      SuggestionsServiceImpl::GetBlacklistedUrl(*fetcher, &retrieved_url));
-}
-
-TEST_F(SuggestionsServiceTest, GetBlacklistedUrlBlacklistRequest) {
-  // An actual blacklist request.
-  const GURL blacklisted_url("http://blacklisted.com/a?b=c&d=e");
-  const std::string encoded_blacklisted_url =
-      "http%3A%2F%2Fblacklisted.com%2Fa%3Fb%3Dc%26d%3De";
-  const std::string blacklist_request_prefix(
-      SuggestionsServiceImpl::BuildSuggestionsBlacklistURLPrefix());
-  std::unique_ptr<net::FakeURLFetcher> fetcher(CreateURLFetcher(
-      GURL(blacklist_request_prefix + encoded_blacklisted_url), nullptr, "",
-      net::HTTP_OK, net::URLRequestStatus::SUCCESS));
-  GURL retrieved_url;
-  EXPECT_TRUE(
-      SuggestionsServiceImpl::GetBlacklistedUrl(*fetcher, &retrieved_url));
-  EXPECT_EQ(blacklisted_url, retrieved_url);
-}
-
-TEST_F(SuggestionsServiceTest, UpdateBlacklistDelay) {
-  const base::TimeDelta initial_delay = suggestions_service_->blacklist_delay();
+
+  EXPECT_TRUE(suggestions_service()->BlacklistURL(GURL(kBlacklistedUrl)));
+  EXPECT_FALSE(suggestions_service()->UndoBlacklistURL(GURL(kBlacklistedUrl)));
+}
+
+TEST_F(SuggestionsServiceTest, TemporaryIncreasesBlacklistDelayOnFailure) {

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

s/Temporary/Temporarily/

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

Done.

+  EXPECT_CALL(*thumbnail_manager(), Initialize(_)).Times(AnyNumber());
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_)).Times(AnyNumber());
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(false));
+  const base::TimeDelta initial_delay =
+      suggestions_service()->blacklist_delay_for_testing();
 
   // Delay unchanged on success.
-  suggestions_service_->UpdateBlacklistDelay(true);
-  EXPECT_EQ(initial_delay, suggestions_service_->blacklist_delay());
+  suggestions_service()->FetchSuggestionsData();
+  base::RunLoop().RunUntilIdle();
+  RespondToFetchWithProfile(CreateSuggestionsProfile());
+  EXPECT_EQ(initial_delay,
+            suggestions_service()->blacklist_delay_for_testing());
 
   // Delay increases on failure.
-  suggestions_service_->UpdateBlacklistDelay(false);
-  EXPECT_GT(suggestions_service_->blacklist_delay(), initial_delay);
+  suggestions_service()->FetchSuggestionsData();
+  base::RunLoop().RunUntilIdle();
+  RespondToFetch(
+      "irrelevant", net::HTTP_BAD_REQUEST,
+      net::URLRequestStatus(net::URLRequestStatus::SUCCESS, net::OK));
+  EXPECT_GT(suggestions_service()->blacklist_delay_for_testing(),
+            initial_delay);
 
   // Delay resets on success.
-  suggestions_service_->UpdateBlacklistDelay(true);
-  EXPECT_EQ(initial_delay, suggestions_service_->blacklist_delay());
-}
-
-TEST_F(SuggestionsServiceTest, CheckDefaultTimeStamps) {
-  SuggestionsProfile suggestions =
-      CreateSuggestionsProfileWithExpiryTimestamps();
-  suggestions_service_->SetDefaultExpiryTimestamp(&suggestions,
-                                                  kTestDefaultExpiry);
-  EXPECT_EQ(kTestSetExpiry, suggestions.suggestions(0).expiry_ts());
-  EXPECT_EQ(kTestDefaultExpiry, suggestions.suggestions(1).expiry_ts());
+  suggestions_service()->FetchSuggestionsData();
+  base::RunLoop().RunUntilIdle();
+  RespondToFetchWithProfile(CreateSuggestionsProfile());
+  EXPECT_EQ(initial_delay,
+            suggestions_service()->blacklist_delay_for_testing());
+}
+
+TEST_F(SuggestionsServiceTest, DoesNotOverrideDefaultExpiryTime) {
+  EXPECT_CALL(*thumbnail_manager(), Initialize(_));
+  EXPECT_CALL(*blacklist_store(), FilterSuggestions(_));
+  EXPECT_CALL(*blacklist_store(), GetTimeUntilReadyForUpload(_))
+      .WillOnce(Return(false));
+
+  suggestions_service()->FetchSuggestionsData();
+
+  base::RunLoop().RunUntilIdle();
+  // Creates one suggestion without timestamp and adds a second with timestamp.
+  SuggestionsProfile profile = CreateSuggestionsProfile();
+  ChromeSuggestion* suggestion = profile.add_suggestions();
+  suggestion->set_title(kTestTitle);
+  suggestion->set_url(kTestUrl);
+  suggestion->set_expiry_ts(kTestSetExpiry);
+  RespondToFetchWithProfile(profile);
+
+  SuggestionsProfile suggestions_profile;
+  suggestions_store()->LoadSuggestions(&suggestions_profile);
+  ASSERT_EQ(2, suggestions_profile.suggestions_size());
+  // Suggestion[1] had a very old time stamp but should not be updated.
+  EXPECT_EQ(kTestSetExpiry, suggestions_profile.suggestions(1).expiry_ts());
+  // Suggestion[0] had no time stamp and should be ahead of the old suggestion.
+  EXPECT_LT(kTestSetExpiry, suggestions_profile.suggestions(0).expiry_ts());

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

nit: Check suggestions(0) first?

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

Done.

 }
 
 TEST_F(SuggestionsServiceTest, GetPageThumbnail) {
   const GURL test_url(kTestUrl);
   const GURL thumbnail_url("https://www.thumbnails.com/thumb.jpg");
   base::Callback<void(const GURL&, const gfx::Image&)> dummy_callback;
 
-  EXPECT_CALL(*mock_thumbnail_manager_, GetImageForURL(test_url, _));
-  suggestions_service_->GetPageThumbnail(test_url, dummy_callback);
+  EXPECT_CALL(*thumbnail_manager(), GetImageForURL(test_url, _));
+  suggestions_service()->GetPageThumbnail(test_url, dummy_callback);
 
-  EXPECT_CALL(*mock_thumbnail_manager_, AddImageURL(test_url, thumbnail_url));
-  EXPECT_CALL(*mock_thumbnail_manager_, GetImageForURL(test_url, _));
-  suggestions_service_->GetPageThumbnailWithURL(test_url, thumbnail_url,
-                                                dummy_callback);
+  EXPECT_CALL(*thumbnail_manager(), AddImageURL(test_url, thumbnail_url));
+  EXPECT_CALL(*thumbnail_manager(), GetImageForURL(test_url, _));
+  suggestions_service()->GetPageThumbnailWithURL(test_url, thumbnail_url,
+                                                 dummy_callback);
 }
 
 }  // namespace suggestions