HttpServerPropertiesManager and tests cleanup

net/http/http_server_properties_manager_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 "net/http/http_server_properties_manager.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/json/json_reader.h"
@@ skipping 67 matching lines @@
 };
 
 class TestingHttpServerPropertiesManager : public HttpServerPropertiesManager {
  public:
   TestingHttpServerPropertiesManager(
       HttpServerPropertiesManager::PrefDelegate* pref_delegate,
       scoped_refptr<base::SingleThreadTaskRunner> pref_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> net_task_runner)
       : HttpServerPropertiesManager(pref_delegate,
                                     pref_task_runner,
-                                    net_task_runner),
-        pref_update_delay_(base::TimeDelta()),
-        cache_update_delay_(base::TimeDelta()) {
+                                    net_task_runner) {
     InitializeOnNetworkThread();
   }
 
   ~TestingHttpServerPropertiesManager() override {}
 
   // Make these methods public for testing.
   using HttpServerPropertiesManager::ScheduleUpdateCacheOnPrefThread;
 
-  // Post tasks without a delay during tests.
-  void StartPrefsUpdateTimerOnNetworkThread(base::TimeDelta delay) override {
-    HttpServerPropertiesManager::StartPrefsUpdateTimerOnNetworkThread(
-        pref_update_delay_);
-  }
-
   void UpdateCacheFromPrefsOnUIConcrete() {
     HttpServerPropertiesManager::UpdateCacheFromPrefsOnPrefThread();
   }
 
-  // Post tasks without a delay during tests.
-  void StartCacheUpdateTimerOnPrefThread(base::TimeDelta delay) override {
-    HttpServerPropertiesManager::StartCacheUpdateTimerOnPrefThread(
-        cache_update_delay_);
-  }
-
   void UpdatePrefsFromCacheOnNetworkThreadConcrete(
       const base::Closure& callback) {
     HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkThread(callback);
   }
 
   void ScheduleUpdatePrefsOnNetworkThreadConcrete(Location location) {
     HttpServerPropertiesManager::ScheduleUpdatePrefsOnNetworkThread(location);
   }
 
   void ScheduleUpdatePrefsOnNetworkThread() {
     // Picked a random Location as caller.
     HttpServerPropertiesManager::ScheduleUpdatePrefsOnNetworkThread(
         DETECTED_CORRUPTED_PREFS);
   }
 
-  void set_pref_update_delay(base::TimeDelta delay) {
-    pref_update_delay_ = delay;
-  }
-  void set_cache_update_delay(base::TimeDelta delay) {
-    cache_update_delay_ = delay;
-  }
   MOCK_METHOD0(UpdateCacheFromPrefsOnPrefThread, void());
   MOCK_METHOD1(UpdatePrefsFromCacheOnNetworkThread, void(const base::Closure&));
   MOCK_METHOD1(ScheduleUpdatePrefsOnNetworkThread, void(Location location));
   MOCK_METHOD6(UpdateCacheFromPrefsOnNetworkThread,
                void(std::vector<std::string>* spdy_servers,
                     AlternativeServiceMap* alternative_service_map,
                     IPAddress* last_quic_address,
                     ServerNetworkStatsMap* server_network_stats_map,
                     QuicServerInfoMap* quic_server_info_map,
                     bool detected_corrupted_prefs));
   MOCK_METHOD6(UpdatePrefsOnPrefThread,
                void(base::ListValue* spdy_server_list,
                     AlternativeServiceMap* alternative_service_map,
                     IPAddress* last_quic_address,
                     ServerNetworkStatsMap* server_network_stats_map,
                     QuicServerInfoMap* quic_server_info_map,
                     const base::Closure& completion));
 
  private:
-  // Time delays used in test for posting tasks. Default to zero.
-  base::TimeDelta pref_update_delay_;
-  base::TimeDelta cache_update_delay_;
   DISALLOW_COPY_AND_ASSIGN(TestingHttpServerPropertiesManager);
 };
 
 }  // namespace
 
 // TODO(rtenneti): After we stop supporting version 3 and everyone has migrated
 // to version 4, delete the following code.
 static const int kHttpServerPropertiesVersions[] = {3, 4, 5};
 
 class HttpServerPropertiesManagerTest : public testing::TestWithParam<int> {
@@ skipping 89 matching lines @@
   std::unique_ptr<TestingHttpServerPropertiesManager>
       http_server_props_manager_;
   base::Time one_day_from_now_;
   scoped_refptr<TestMockTimeTaskRunner> pref_test_task_runner_;
   scoped_refptr<TestMockTimeTaskRunner> net_test_task_runner_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(HttpServerPropertiesManagerTest);
 };
 
-INSTANTIATE_TEST_CASE_P(Tests,
+INSTANTIATE_TEST_CASE_P(/* no prefix */,
                         HttpServerPropertiesManagerTest,
                         ::testing::ValuesIn(kHttpServerPropertiesVersions));
 
 TEST_P(HttpServerPropertiesManagerTest,
        SingleUpdateForTwoSpdyServerPrefChanges) {
   ExpectCacheUpdate();
 
   // Set up the prefs for https://www.google.com and https://mail.google.com and
   // then set it twice. Only expect a single cache update.
 
@@ skipping 180 matching lines @@
     EXPECT_EQ(kProtoHTTP2, map_it->second[0].alternative_service.protocol);
     EXPECT_TRUE(map_it->second[0].alternative_service.host.empty());
     EXPECT_EQ(444, map_it->second[0].alternative_service.port);
   }
 
   // Verify SupportsQuic.
   IPAddress last_address;
   EXPECT_TRUE(http_server_props_manager_->GetSupportsQuic(&last_address));
   EXPECT_EQ("127.0.0.1", last_address.ToString());
 
-  /*
   // Verify ServerNetworkStats.
   const ServerNetworkStats* stats2 =
       http_server_props_manager_->GetServerNetworkStats(google_server);
   EXPECT_EQ(10, stats2->srtt.ToInternalValue());
   const ServerNetworkStats* stats3 =
       http_server_props_manager_->GetServerNetworkStats(mail_server);
   EXPECT_EQ(20, stats3->srtt.ToInternalValue());
 
   // Verify QuicServerInfo.
   EXPECT_EQ(quic_server_info1, *http_server_props_manager_->GetQuicServerInfo(
                                    google_quic_server_id));
   EXPECT_EQ(quic_server_info2, *http_server_props_manager_->GetQuicServerInfo(
                                    mail_quic_server_id));
   EXPECT_EQ(quic_server_info3, *http_server_props_manager_->GetQuicServerInfo(
                                    play_quic_server_id));
 
   // Verify the MRU order.
   http_server_props_manager_->SetMaxServerConfigsStoredInProperties(2);
   EXPECT_EQ(nullptr, http_server_props_manager_->GetQuicServerInfo(
                          google_quic_server_id));
   EXPECT_EQ(quic_server_info2, *http_server_props_manager_->GetQuicServerInfo(
                                    mail_quic_server_id));
   EXPECT_EQ(quic_server_info3, *http_server_props_manager_->GetQuicServerInfo(
                                    play_quic_server_id));
-  */
 }
 
 TEST_P(HttpServerPropertiesManagerTest, BadCachedHostPortPair) {
   ExpectCacheUpdate();
   // The prefs are automatically updated in the case corruption is detected.
   ExpectPrefsUpdate(1);
   ExpectScheduleUpdatePrefsOnNetworkThread();
 
   base::DictionaryValue* server_pref_dict = new base::DictionaryValue;
 
@@ skipping 169 matching lines @@
   EXPECT_TRUE(http_server_props_manager_->SupportsRequestPriority(spdy_server));
   Mock::VerifyAndClearExpectations(http_server_props_manager_.get());
 }
 
 // Regression test for crbug.com/670519. Test that there is only one pref update
 // scheduled if multiple updates happen in a given time period. Subsequent pref
 // update could also be scheduled once the previous scheduled update is
 // completed.
 TEST_P(HttpServerPropertiesManagerTest,
        SinglePrefUpdateForTwoSpdyServerCacheChanges) {
-  http_server_props_manager_->set_pref_update_delay(
-      base::TimeDelta::FromMilliseconds(60));
+  // Keep this in sync with http_server_properties_manager.cc
+  const int64_t kUpdatePrefsDelayMs = 60000;

[Zhongyi Shi, 2017/02/10 18:01:38]

Just to confirm, looks like all the tests are now running with the default delay
as .cc file when update pref/cache, right? 

Here we need to declare a local kUpdatePrefsDelayMs as we want to control the
timer and verify the behavior of ther net thread. In those tests we don't need
verify, we just fast forward until no task remain.

[xunjieli, 2017/02/10 20:01:21]

Yes.
Yes, that's my understanding too.

   ExpectPrefsUpdate(2);
   ExpectScheduleUpdatePrefsOnNetworkThreadRepeatedly(3);
 
   // Post an update task to the network thread. SetSupportsSpdy calls
   // ScheduleUpdatePrefsOnNetworkThread with a delay of 60ms.
   url::SchemeHostPort spdy_server("https", "mail.google.com", 443);
   EXPECT_FALSE(
       http_server_props_manager_->SupportsRequestPriority(spdy_server));
   http_server_props_manager_->SetSupportsSpdy(spdy_server, true);
   // The pref update task should be scheduled to network thread.
   EXPECT_EQ(1u, net_test_task_runner_->GetPendingTaskCount());
 
-  // Move forward the task runner with 20ms.
-  net_test_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(20));
+  // Move forward the task runner with kUpdatePrefsDelayMs/2.
+  net_test_task_runner_->FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kUpdatePrefsDelayMs / 2));
 
   // Set another spdy server to trigger another call to
   // ScheduleUpdatePrefsOnNetworkThread. There should be no new update posted to
   // the network thread.
   url::SchemeHostPort spdy_server2("https", "drive.google.com", 443);
   http_server_props_manager_->SetSupportsSpdy(spdy_server2, true);
   EXPECT_EQ(1u, net_test_task_runner_->GetPendingTaskCount());
   EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
 
-  // Move forward another 40ms. The pref update should be executed.
-  net_test_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(40));
+  // Forward another kUpdatePrefsDelayMs/2. The pref update should be executed.
+  net_test_task_runner_->FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kUpdatePrefsDelayMs / 2));
   EXPECT_FALSE(net_test_task_runner_->HasPendingTask());
   EXPECT_TRUE(pref_test_task_runner_->HasPendingTask());
   pref_test_task_runner_->FastForwardUntilNoTasksRemain();
   EXPECT_FALSE(net_test_task_runner_->HasPendingTask());
   EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
 
   EXPECT_TRUE(http_server_props_manager_->SupportsRequestPriority(spdy_server));
   EXPECT_TRUE(
       http_server_props_manager_->SupportsRequestPriority(spdy_server2));
   // Set the third spdy server to trigger one more call to
@@ skipping 401 matching lines @@
     EXPECT_EQ(kProtoQUIC, alternative_service_vector[0].protocol);
     EXPECT_EQ(i, alternative_service_vector[0].port);
   }
 
   // Verify SupportsQuic.
   IPAddress address;
   ASSERT_TRUE(http_server_props_manager_->GetSupportsQuic(&address));
   EXPECT_EQ("127.0.0.1", address.ToString());
 }
 
-TEST_P(HttpServerPropertiesManagerTest, UpdateCacheWithPrefs) {
+TEST_P(HttpServerPropertiesManagerTest, UpdatePrefsWithCache) {
   ExpectScheduleUpdatePrefsOnNetworkThreadRepeatedly(5);
 
-  const url::SchemeHostPort server_www("http", "www.google.com", 80);
-  const url::SchemeHostPort server_mail("http", "mail.google.com", 80);
+  const url::SchemeHostPort server_www("https", "www.google.com", 80);
+  const url::SchemeHostPort server_mail("https", "mail.google.com", 80);
 
-  // Set alternate protocol.
+  // #1 & #2: Set alternate protocol.
   AlternativeServiceInfoVector alternative_service_info_vector;
   AlternativeService www_alternative_service1(kProtoHTTP2, "", 443);
   base::Time expiration1;
   ASSERT_TRUE(base::Time::FromUTCString("2036-12-01 10:00:00", &expiration1));
   alternative_service_info_vector.push_back(
       AlternativeServiceInfo(www_alternative_service1, expiration1));
   AlternativeService www_alternative_service2(kProtoHTTP2, "www.google.com",
                                               1234);
   base::Time expiration2;
   ASSERT_TRUE(base::Time::FromUTCString("2036-12-31 10:00:00", &expiration2));
   alternative_service_info_vector.push_back(
       AlternativeServiceInfo(www_alternative_service2, expiration2));
-  http_server_props_manager_->SetAlternativeServices(
-      server_www, alternative_service_info_vector);
+  ASSERT_TRUE(http_server_props_manager_->SetAlternativeServices(
+      server_www, alternative_service_info_vector));
 
   AlternativeService mail_alternative_service(kProtoHTTP2, "foo.google.com",
                                               444);
   base::Time expiration3 = base::Time::Max();
-  http_server_props_manager_->SetAlternativeService(
-      server_mail, mail_alternative_service, expiration3);
+  ASSERT_TRUE(http_server_props_manager_->SetAlternativeService(
+      server_mail, mail_alternative_service, expiration3));
 
-  // Set ServerNetworkStats.
+  // #3: Set ServerNetworkStats.
   ServerNetworkStats stats;
   stats.srtt = base::TimeDelta::FromInternalValue(42);
   http_server_props_manager_->SetServerNetworkStats(server_mail, stats);
 
-  // Set quic_server_info string.
+  // #4: Set quic_server_info string.
   QuicServerId mail_quic_server_id("mail.google.com", 80);
   std::string quic_server_info1("quic_server_info1");
   http_server_props_manager_->SetQuicServerInfo(mail_quic_server_id,
                                                 quic_server_info1);
 
-  // Set SupportsQuic.
+  // #5: Set SupportsQuic.
   IPAddress actual_address(127, 0, 0, 1);
   http_server_props_manager_->SetSupportsQuic(true, actual_address);
 
-  // Update cache.
+  // Update Prefs.
   ExpectPrefsUpdate(1);
-  ExpectCacheUpdate();
-  http_server_props_manager_->ScheduleUpdateCacheOnPrefThread();
-

[Zhongyi Shi, 2017/02/10 18:01:38]

Hm, I thought we had UpdateCacheWithPref and UpdatePrefsWithCache in the tests
before your change. 

The other way to make this test indeed a UpdateCacheWithPrefs would be instead
of call ScheduleUpdateCacheOnPrefThread directly, call
OnHttpServerPropertiesChanged. Does that make sense? Your fix looks good to me,
maybe we should add one more test. How do you think? 

https://cs.chromium.org/chromium/src/net/http/http_server_properties_manager....

[xunjieli, 2017/02/10 20:01:21]

Talked to Cherie offline. 
There are other 4 tests that test the other direction:
BadSupportsQuic, SingleUpdateForTwoSpdyServerPrefChanges, BadCachedHostPortPair,
BadCachedAltProtocolPort

   EXPECT_TRUE(net_test_task_runner_->HasPendingTask());
+  EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
+  net_test_task_runner_->FastForwardUntilNoTasksRemain();
   EXPECT_TRUE(pref_test_task_runner_->HasPendingTask());
-  net_test_task_runner_->FastForwardUntilNoTasksRemain();
   pref_test_task_runner_->FastForwardUntilNoTasksRemain();
   EXPECT_FALSE(net_test_task_runner_->HasPendingTask());
   EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
 
   // Verify preferences.
   const char expected_json[] =
       "{\"quic_servers\":{\"https://"
       "mail.google.com:80\":{\"server_info\":\"quic_server_info1\"}},"
       "\"servers\":["
-      "{\"http://www.google.com\":{"
+      "{\"https://www.google.com:80\":{"
       "\"alternative_service\":[{\"expiration\":\"13756212000000000\","
       "\"port\":443,\"protocol_str\":\"h2\"},"
       "{\"expiration\":\"13758804000000000\",\"host\":\"www.google.com\","
       "\"port\":1234,\"protocol_str\":\"h2\"}]}},"
-      "{\"http://mail.google.com\":{\"alternative_service\":[{"
+      "{\"https://mail.google.com:80\":{\"alternative_service\":[{"
       "\"expiration\":\"9223372036854775807\",\"host\":\"foo.google.com\","
       "\"port\":444,\"protocol_str\":\"h2\"}],"
       "\"network_stats\":{\"srtt\":42}}}"
       "],"
       "\"supports_quic\":{\"address\":\"127.0.0.1\",\"used_quic\":true},"
       "\"version\":5}";
 
   const base::Value* http_server_properties =
       &pref_delegate_->GetServerProperties();
   std::string preferences_json;
   EXPECT_TRUE(
       base::JSONWriter::Write(*http_server_properties, &preferences_json));
   EXPECT_EQ(expected_json, preferences_json);
 }
 
 TEST_P(HttpServerPropertiesManagerTest,
        SingleCacheUpdateForMultipleUpdatesScheduled) {
-  http_server_props_manager_->set_cache_update_delay(
-      base::TimeDelta::FromMilliseconds(60));
-
+  // Keep this in sync with http_server_properties_manager.cc
+  const int64_t kUpdateCacheDelayMs = 1000;
   // Update cache.
   ExpectCacheUpdate();
 
   EXPECT_EQ(0u, pref_test_task_runner_->GetPendingTaskCount());
   // Update cache.
   http_server_props_manager_->ScheduleUpdateCacheOnPrefThread();
   EXPECT_EQ(1u, pref_test_task_runner_->GetPendingTaskCount());
 
-  // Move forward the task runner 20ms.
-  pref_test_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(20));
+  // Move forward the task runner kUpdateCacheDelayMs/2.
+  pref_test_task_runner_->FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kUpdateCacheDelayMs / 2));
   // Schedule a new cache update within the time window should be a no-op.
   http_server_props_manager_->ScheduleUpdateCacheOnPrefThread();
   EXPECT_EQ(1u, pref_test_task_runner_->GetPendingTaskCount());
 
-  // Move forward the task runner 40ms, now the cache update should be
-  // exectured.
-  pref_test_task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(40));
+  // Move forward the task runner kUpdateCaceDelayMs/2, now the cache update
+  // should be exectured.
+  pref_test_task_runner_->FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kUpdateCacheDelayMs / 2));
 
   // Since this test has no pref corruption, there shouldn't be any pref update.
   EXPECT_FALSE(net_test_task_runner_->HasPendingTask());
   EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
 
   // Schedule one more cache update. The task should be successfully scheduled
   // on pref task runner.
   ExpectCacheUpdate();
   http_server_props_manager_->ScheduleUpdateCacheOnPrefThread();
   EXPECT_EQ(1u, pref_test_task_runner_->GetPendingTaskCount());
@@ skipping 76 matching lines @@
   ExpectScheduleUpdatePrefsOnNetworkThreadRepeatedly(2);
 
   AlternativeServiceInfoVector alternative_service_info_vector;
 
   const AlternativeService broken_alternative_service(
       kProtoHTTP2, "broken.example.com", 443);
   const base::Time time_one_day_later =
       base::Time::Now() + base::TimeDelta::FromDays(1);
   alternative_service_info_vector.push_back(
       AlternativeServiceInfo(broken_alternative_service, time_one_day_later));
+  // #1: MarkAlternativeServiceBroken().
   http_server_props_manager_->MarkAlternativeServiceBroken(
       broken_alternative_service);
 
   const AlternativeService expired_alternative_service(
       kProtoHTTP2, "expired.example.com", 443);
   const base::Time time_one_day_ago =
       base::Time::Now() - base::TimeDelta::FromDays(1);
   alternative_service_info_vector.push_back(
       AlternativeServiceInfo(expired_alternative_service, time_one_day_ago));
 
   const AlternativeService valid_alternative_service(kProtoHTTP2,
                                                      "valid.example.com", 443);
   alternative_service_info_vector.push_back(
       AlternativeServiceInfo(valid_alternative_service, time_one_day_later));
 
   const url::SchemeHostPort server("https", "www.example.com", 443);
-  http_server_props_manager_->SetAlternativeServices(
-      server, alternative_service_info_vector);
+  // #2: SetAlternativeService().
+  ASSERT_TRUE(http_server_props_manager_->SetAlternativeServices(
+      server, alternative_service_info_vector));
 
   // Update cache.
   ExpectPrefsUpdate(1);
-  ExpectCacheUpdate();
-  http_server_props_manager_->ScheduleUpdateCacheOnPrefThread();
 
   EXPECT_TRUE(net_test_task_runner_->HasPendingTask());
+  EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
+  net_test_task_runner_->FastForwardUntilNoTasksRemain();
   EXPECT_TRUE(pref_test_task_runner_->HasPendingTask());
-  net_test_task_runner_->FastForwardUntilNoTasksRemain();
   pref_test_task_runner_->FastForwardUntilNoTasksRemain();
   EXPECT_FALSE(net_test_task_runner_->HasPendingTask());
   EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
 
   const base::DictionaryValue& pref_dict =
       pref_delegate_->GetServerProperties();
 
   const base::ListValue* servers_list = nullptr;
   ASSERT_TRUE(pref_dict.GetListWithoutPathExpansion("servers", &servers_list));
   base::ListValue::const_iterator it = servers_list->begin();
@@ skipping 158 matching lines @@
   pref_test_task_runner_->FastForwardUntilNoTasksRemain();
   EXPECT_FALSE(net_test_task_runner_->HasPendingTask());
   EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
   Mock::VerifyAndClearExpectations(http_server_props_manager_.get());
   http_server_props_manager_.reset();
   EXPECT_FALSE(net_test_task_runner_->HasPendingTask());
   EXPECT_FALSE(pref_test_task_runner_->HasPendingTask());
 }
 
 }  // namespace net