Populate EEP estimate in NQE

net/base/network_quality_estimator_unittest.cc

Index: net/base/network_quality_estimator_unittest.cc
diff --git a/net/base/network_quality_estimator_unittest.cc b/net/base/network_quality_estimator_unittest.cc
index b1ffdebaeb58e378ad6806ba6c2028f26ae2a33a..c6634a18ffd29b925b21dd22d95b1af06e674897 100644
--- a/net/base/network_quality_estimator_unittest.cc
+++ b/net/base/network_quality_estimator_unittest.cc
@@ -32,12 +32,19 @@ namespace {
 class TestNetworkQualityEstimator : public net::NetworkQualityEstimator {
  public:
   TestNetworkQualityEstimator(
-      const std::map<std::string, std::string>& variation_params)
-      : NetworkQualityEstimator(scoped_ptr<net::ExternalEstimateProvider>(),
+      const std::map<std::string, std::string>& variation_params,
+      scoped_ptr<net::ExternalEstimateProvider> external_estimate_provider)
+      : NetworkQualityEstimator(external_estimate_provider.Pass(),
                                 variation_params,
                                 true,
                                 true) {}
 
+  explicit TestNetworkQualityEstimator(
+      const std::map<std::string, std::string>& variation_params)
+      : TestNetworkQualityEstimator(
+            variation_params,
+            scoped_ptr<net::ExternalEstimateProvider>()) {}
+
   ~TestNetworkQualityEstimator() override {}
 
   // Overrides the current network type and id.
@@ -62,6 +69,8 @@ class TestNetworkQualityEstimator : public net::NetworkQualityEstimator {
 
   net::NetworkChangeNotifier::ConnectionType current_network_type_;
   std::string current_network_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestNetworkQualityEstimator);
 };
 
 }  // namespace
@@ -654,4 +663,194 @@ TEST(NetworkQualityEstimatorTest, TestGetMedianRTTSince) {
   EXPECT_EQ(100, downstream_throughput_kbps);
 }
 
+class TestExternalEstimateProvider : public ExternalEstimateProvider {
+ public:
+  TestExternalEstimateProvider()
+      : time_since_last_update_(base::TimeDelta::FromSeconds(1)),
+        time_since_last_update_count_(0),
+        rtt_count_(0),
+        downstream_throughput_kbps_count_(0),
+        update_count_(0) {}
+  ~TestExternalEstimateProvider() override {}
+
+  // ExternalEstimateProvider implementation:
+  bool GetRTT(base::TimeDelta* rtt) const override {
+    *rtt = base::TimeDelta::FromMilliseconds(1);
+    rtt_count_++;
+    return true;
+  }
+
+  // ExternalEstimateProvider implementation:
+  bool GetDownstreamThroughputKbps(
+      int32_t* downstream_throughput_kbps) const override {
+    *downstream_throughput_kbps = 100;
+    downstream_throughput_kbps_count_++;
+    return true;

[mmenke, 2015/09/15 20:09:09]

Maybe also have another test with an ExternalEstimateProvider where these both
return false, and make sure we don't get any observations from it?

[tbansal1, 2015/09/15 22:33:07]

Done.

+  }
+
+  // ExternalEstimateProvider implementation:
+  bool GetUpstreamThroughputKbps(
+      int32_t* upstream_throughput_kbps) const override {
+    // NetworkQualityEstimator does not support upstream throughput.
+    ADD_FAILURE();
+    return false;
+  }
+
+  // ExternalEstimateProvider implementation:
+  bool GetTimeSinceLastUpdate(
+      base::TimeDelta* time_since_last_update) const override {
+    *time_since_last_update = time_since_last_update_;
+    time_since_last_update_count_++;
+    return true;
+  }
+
+  // ExternalEstimateProvider implementation:
+  void SetUpdatedEstimateDelegate(UpdatedEstimateDelegate* delegate) override {}
+
+  // ExternalEstimateProvider implementation:
+  void Update() const override { update_count_++; }
+
+  void set_time_since_last_update(base::TimeDelta time_since_last_update) {
+    time_since_last_update_ = time_since_last_update;
+  }
+
+  size_t get_time_since_last_update_count() const {

[mmenke, 2015/09/15 20:09:09]

-get.  The get in get_rtt_count is because it's the number of times *Get*RTT was
called, not because we're getting the rtt_count (We're not, in fact, getting a
count of RTTs).

[tbansal1, 2015/09/15 22:33:08]

I am confused.
This returns the number of times *Get*TimeSinceLastUpdate() was called. So, this
should have get too.

+    return time_since_last_update_count_;
+  }
+  size_t get_rtt_count() const { return rtt_count_; }

[mmenke, 2015/09/15 20:09:09]

nit:  Rename rtt_count_, too.

[tbansal1, 2015/09/15 22:33:07]

Done.

+  size_t get_downstream_throughput_kbps_count() const {

[mmenke, 2015/09/15 20:09:09]

-get

[tbansal1, 2015/09/15 22:33:07]

same here. this is counting number of times GetDownstreamThroughputKbps() was
called.

+    return downstream_throughput_kbps_count_;
+  }
+  size_t update_count() const { return update_count_; }
+
+ private:
+  base::TimeDelta time_since_last_update_;
+
+  mutable size_t time_since_last_update_count_;
+  mutable size_t rtt_count_;
+  mutable size_t downstream_throughput_kbps_count_;
+  mutable size_t update_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestExternalEstimateProvider);
+};
+
+// Tests if the external estimate provider is called in the constructor and
+// on network change notification.
+TEST(NetworkQualityEstimatorTest, TestExternalEstimateProvider) {
+  TestExternalEstimateProvider* test_external_estimate_provider =
+      new TestExternalEstimateProvider();
+  scoped_ptr<ExternalEstimateProvider> external_estimate_provider(
+      test_external_estimate_provider);
+  std::map<std::string, std::string> variation_params;
+  TestNetworkQualityEstimator estimator(variation_params,
+                                        external_estimate_provider.Pass());
+
+  base::TimeDelta rtt;
+  int32_t kbps;
+  EXPECT_TRUE(estimator.GetRTTEstimate(&rtt));
+  EXPECT_TRUE(estimator.GetDownlinkThroughputKbpsEstimate(&kbps));
+
+  EXPECT_EQ(
+      1U, test_external_estimate_provider->get_time_since_last_update_count());
+  EXPECT_EQ(1U, test_external_estimate_provider->get_rtt_count());
+  EXPECT_EQ(
+      1U,
+      test_external_estimate_provider->get_downstream_throughput_kbps_count());
+
+  // Change network type to WiFi. Number of queries to External estimate
+  // provider must increment.
+  estimator.SimulateNetworkChangeTo(
+      NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI, "test-1");
+  EXPECT_TRUE(estimator.GetRTTEstimate(&rtt));
+  EXPECT_TRUE(estimator.GetDownlinkThroughputKbpsEstimate(&kbps));
+  EXPECT_EQ(
+      2U, test_external_estimate_provider->get_time_since_last_update_count());
+  EXPECT_EQ(2U, test_external_estimate_provider->get_rtt_count());
+  EXPECT_EQ(
+      2U,
+      test_external_estimate_provider->get_downstream_throughput_kbps_count());
+
+  // Change network type to 2G. Number of queries to External estimate provider
+  // must increment.
+  estimator.SimulateNetworkChangeTo(
+      NetworkChangeNotifier::ConnectionType::CONNECTION_2G, "test-1");
+  EXPECT_EQ(
+      3U, test_external_estimate_provider->get_time_since_last_update_count());
+  EXPECT_EQ(3U, test_external_estimate_provider->get_rtt_count());
+  EXPECT_EQ(
+      3U,
+      test_external_estimate_provider->get_downstream_throughput_kbps_count());
+
+  // Set the external estimate as old. Network Quality estimator should request
+  // an update on connection type change.
+  EXPECT_EQ(0U, test_external_estimate_provider->update_count());
+  test_external_estimate_provider->set_time_since_last_update(
+      base::TimeDelta::Max());
+
+  estimator.SimulateNetworkChangeTo(
+      NetworkChangeNotifier::ConnectionType::CONNECTION_2G, "test-2");
+  EXPECT_EQ(
+      4U, test_external_estimate_provider->get_time_since_last_update_count());
+  EXPECT_EQ(3U, test_external_estimate_provider->get_rtt_count());
+  EXPECT_EQ(
+      3U,
+      test_external_estimate_provider->get_downstream_throughput_kbps_count());
+  EXPECT_EQ(1U, test_external_estimate_provider->update_count());
+
+  // Estimates are unavailable because external estimate provider never
+  // notifies network quality estimator of the updated estimates.
+  EXPECT_FALSE(estimator.GetRTTEstimate(&rtt));
+  EXPECT_FALSE(estimator.GetDownlinkThroughputKbpsEstimate(&kbps));
+}
+
+// Tests if the estimate from the external estimate provider is merged with the
+// observations collected from the HTTP requests.
+TEST(NetworkQualityEstimatorTest, TestExternalEstimateProviderMergeEstimates) {
+  TestExternalEstimateProvider* test_external_estimate_provider =
+      new TestExternalEstimateProvider();
+  scoped_ptr<ExternalEstimateProvider> external_estimate_provider(
+      test_external_estimate_provider);
+  std::map<std::string, std::string> variation_params;
+  TestNetworkQualityEstimator estimator(variation_params,
+                                        external_estimate_provider.Pass());
+
+  base::TimeDelta rtt;
+  int32_t kbps;
+  // Estimate provided by network quality estimator should match the estimate
+  // provided by external estimate provider.
+  EXPECT_TRUE(estimator.GetRTTEstimate(&rtt));
+  base::TimeDelta external_estimate_provider_rtt;

[mmenke, 2015/09/15 20:09:09]

Think it's much cleaner to make this a constant (1 millisecond) and check that
rtt is exactly what we expect.

[tbansal1, 2015/09/15 22:33:08]

Done.

+  EXPECT_TRUE(
+      test_external_estimate_provider->GetRTT(&external_estimate_provider_rtt));
+  EXPECT_EQ(external_estimate_provider_rtt, rtt);
+
+  EXPECT_TRUE(estimator.GetDownlinkThroughputKbpsEstimate(&kbps));
+  int32_t external_estimate_provider_downstream_throughput;
+  EXPECT_TRUE(test_external_estimate_provider->GetDownstreamThroughputKbps(
+      &external_estimate_provider_downstream_throughput));
+  EXPECT_EQ(external_estimate_provider_downstream_throughput, kbps);

[mmenke, 2015/09/15 20:09:09]

As above, should be comparing these with constants.

[tbansal1, 2015/09/15 22:33:08]

Done.

+  EXPECT_EQ(1U, estimator.downstream_throughput_kbps_observations_.Size());
+
+  // Send a HTTP request.
+  net::test_server::EmbeddedTestServer embedded_test_server;
+  embedded_test_server.ServeFilesFromDirectory(
+      base::FilePath(FILE_PATH_LITERAL("net/data/url_request_unittest")));

[mmenke, 2015/09/15 20:09:09]

include FilePath header.

[tbansal1, 2015/09/15 22:33:08]

Done.

+  ASSERT_TRUE(embedded_test_server.InitializeAndWaitUntilReady());
+
+  TestDelegate test_delegate;
+  TestURLRequestContext context(false);
+
+  scoped_ptr<URLRequest> request(
+      context.CreateRequest(embedded_test_server.GetURL("/echo.html"),
+                            DEFAULT_PRIORITY, &test_delegate));
+  request->SetLoadFlags(request->load_flags() | LOAD_MAIN_FRAME);

[mmenke, 2015/09/15 20:09:09]

I don't think we care about the load flags?  Can just remove this line.  Also
want to get rid of the load flag, eventually.  Don't want more dependencies on
it.

[tbansal1, 2015/09/15 22:33:08]

Done.

+  request->Start();
+  base::RunLoop().Run();
+
+  // Both RTT and downstream throughput should be updated.
+  estimator.NotifyHeadersReceived(*request);
+  estimator.NotifyRequestCompleted(*request);

[mmenke, 2015/09/15 20:09:09]

Why are we doing this manually?  Can't we just set the estimator on the
URLRequestContext?  Initialize it with true instead of false, set its estimator,
and then call init on it, and you shouldn't need these two lines.

[tbansal1, 2015/09/15 22:33:08]

Done. That's what I was doing in URLRequestTestHTTP.NetworkQualityEstimator
test.

+  EXPECT_EQ(2U, estimator.downstream_throughput_kbps_observations_.Size());
+}
+
 }  // namespace net