Add in-memory caching of network quality estimates across network changes.

net/base/network_quality_estimator_unittest.cc

 // Copyright 2015 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/base/network_quality_estimator.h"
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
+#include "base/strings/safe_sprintf.h"
 #include "base/test/histogram_tester.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/network_quality.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ skipping 43 matching lines @@
   }
 
   // With large transfer, both |fastest_rtt| and |peak_throughput_kbps| will be
   // updated.
   estimator.NotifyDataReceived(*(request.get()), min_transfer_size_in_bytes);
   {
     NetworkQuality network_quality = estimator.GetEstimate();
     EXPECT_GT(network_quality.fastest_rtt_confidence, 0);
     EXPECT_GT(network_quality.peak_throughput_kbps_confidence, 0);
     EXPECT_GE(network_quality.fastest_rtt, request_duration);
-    EXPECT_GT(network_quality.peak_throughput_kbps, uint32_t(0));
+    EXPECT_GT(network_quality.peak_throughput_kbps, 0U);
     EXPECT_LE(
         network_quality.peak_throughput_kbps,
         min_transfer_size_in_bytes * 8.0 / request_duration.InMilliseconds());
   }
   EXPECT_EQ(estimator.bytes_read_since_last_connection_change_, true);
 
   // Check UMA histograms.
   base::HistogramTester histogram_tester;
   histogram_tester.ExpectTotalCount("NQE.PeakKbps.Unknown", 0);
   histogram_tester.ExpectTotalCount("NQE.FastestRTT.Unknown", 0);
 
   estimator.OnConnectionTypeChanged(
       NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI);
   histogram_tester.ExpectTotalCount("NQE.PeakKbps.Unknown", 1);
   histogram_tester.ExpectTotalCount("NQE.FastestRTT.Unknown", 1);
   {
     NetworkQuality network_quality = estimator.GetEstimate();
-    EXPECT_EQ(estimator.current_connection_type_,
+    EXPECT_EQ(estimator.current_network_id_.type,
               NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI);
     EXPECT_EQ(network_quality.fastest_rtt_confidence, 0);
     EXPECT_EQ(network_quality.peak_throughput_kbps_confidence, 0);
   }
 }
 #endif  // !defined(OS_IOS)
 
+// Helps in setting the current network name.
+class TestNetworkQualityEstimator : public NetworkQualityEstimator {

[mmenke, 2015/06/09 19:48:07]

This should be in an anonymous namespace.  May be able to put all the test in it
as well.

[tbansal1, 2015/06/10 00:23:29]

Moved to anon namespace. Not sure what test can I put there?

+ public:
+  TestNetworkQualityEstimator() : current_network_name_(std::string()) {}

[mmenke, 2015/06/09 19:48:07]

nit:  No need to explicitly initialize current_network_name_

[tbansal1, 2015/06/10 00:23:29]

Done.

+
+  ~TestNetworkQualityEstimator() override {}
+
+  // Overrides the current network name.
+  void SetCurrentNetworkName(std::string current_network_name) {
+    current_network_name_ = current_network_name;
+  }
+
+  using NetworkQualityEstimator::GetCacheSizeForTests;
+  using NetworkQualityEstimator::OnConnectionTypeChanged;
+  using NetworkQualityEstimator::ReadCachedNetworkQualityEstimate;
+
+ private:
+  // NetworkQualityEstimator implementation that returns the overriden network

[mmenke, 2015/06/09 19:48:08]

overriden -> overridden

[tbansal1, 2015/06/10 00:23:29]

Done.

+  // name (instead of invoking platform APIs).
+  std::string GetCurrentNetworkName() const override {
+    return current_network_name_;
+  }
+
+  std::string current_network_name_;
+};
+
+// Test if the network estimates are cached when network change notification
+// is invoked.
+TEST(NetworkQualityEstimatorTest, TestCaching) {
+  TestNetworkQualityEstimator estimator;
+  EXPECT_EQ(0U, estimator.GetCacheSizeForTests());
+
+  size_t cache_size = 1;

[mmenke, 2015/06/09 19:48:07]

expected_cache_size

[tbansal1, 2015/06/10 00:23:29]

Done.

+  estimator.SetCurrentNetworkName("test-1");
+  estimator.OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType::CONNECTION_2G);

[mmenke, 2015/06/09 19:48:07]

Think this test would be much clearer as a single method.  Something like:

estimator.SimulateNetworkChangeTo(type, name);

And get rid of SetCurrentNetworkName.

[tbansal1, 2015/06/10 00:23:29]

Thanks, this helped a lot.

+  // Cache entry will be added for (NONE, "")

[mmenke, 2015/06/09 19:48:07]

Suggest putting this above the line that actually adds a line.

[tbansal1, 2015/06/10 00:23:29]

Done.

+  EXPECT_EQ(cache_size, estimator.GetCacheSizeForTests());
+  for (size_t i = 1; i <= 10; ++i) {
+    // Calling the loop multiple times should have no effect on cache size as
+    // the same cache entry should get updated (because the network names do
+    // not change with multiple iterations of the loop).

[mmenke, 2015/06/09 19:48:08]

Not true - the second iteration adds an entry.

I think this test is sufficiently confusing that you should just unroll the
first two loop iterations.

[tbansal1, 2015/06/10 00:23:29]

Tried to make it simpler. Removed the for loop.

+    estimator.SetCurrentNetworkName("test-1");
+    EXPECT_EQ(cache_size, estimator.GetCacheSizeForTests()) << i;
+    estimator.OnConnectionTypeChanged(
+        NetworkChangeNotifier::ConnectionType::CONNECTION_2G);
+
+    // When i == 1, entry will be added for (2G, "test1")
+    // When i == 2, entry will be added for (2G, "test2")
+    if (i == 1 || i == 2)
+      ++cache_size;
+    EXPECT_EQ(cache_size, estimator.GetCacheSizeForTests()) << i;
+
+    estimator.SetCurrentNetworkName("test-2");
+    EXPECT_EQ(cache_size, estimator.GetCacheSizeForTests()) << i;
+    estimator.OnConnectionTypeChanged(
+        NetworkChangeNotifier::ConnectionType::CONNECTION_2G);

[mmenke, 2015/06/09 19:48:08]

Should also make a test-1 connection with type 3G, and make sure we correctly
distinguish the two.

[tbansal1, 2015/06/10 00:23:29]

Done.

+    EXPECT_EQ(cache_size, estimator.GetCacheSizeForTests()) << i;
+  }
+
+  // Reading quality of a networks seen before should return true.
+  estimator.SetCurrentNetworkName("test-1");

[mmenke, 2015/06/09 19:48:07]

BUG:  This doesn't actually change current_network_id_.  Need a network change
notification, too.

[tbansal1, 2015/06/10 00:23:29]

Fixed.

+  EXPECT_TRUE(estimator.ReadCachedNetworkQualityEstimate());
+  estimator.SetCurrentNetworkName("test-2");
+  EXPECT_TRUE(estimator.ReadCachedNetworkQualityEstimate());

[mmenke, 2015/06/09 19:48:07]

This seems like a very weak check that something was reached.  I'd really like
to store different information for test-1 and test-2, and see if we can retrieve
it.

[tbansal1, 2015/06/10 00:23:29]

Done.

+
+  // Reading quality of a network never seen should return false.
+  estimator.SetCurrentNetworkName("test-3");
+  estimator.OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType::CONNECTION_2G);
+  EXPECT_FALSE(estimator.ReadCachedNetworkQualityEstimate());
+}
+
+// Tests if the cache size remains bounded.
+TEST(NetworkQualityEstimatorTest, TestCacheMaximumSize) {
+  TestNetworkQualityEstimator estimator;
+  estimator.OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI);
+  EXPECT_EQ(1U, estimator.GetCacheSizeForTests());
+
+  char network_name[20];
+  const size_t network_count = 100;
+  for (size_t i = 1; i <= network_count; ++i) {
+    base::strings::SafeSPrintf(network_name, "%d", i);
+    estimator.SetCurrentNetworkName(network_name);
+    estimator.OnConnectionTypeChanged(
+        NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI);
+  }

[mmenke, 2015/06/09 19:48:07]

Suggest exposing NetworkQualityEstimator::kMaximumNetworkQualityCacheSize, and
using that explicitly.

[tbansal1, 2015/06/10 00:23:29]

Done.

+  EXPECT_LT(estimator.GetCacheSizeForTests(), network_count);
+  EXPECT_GT(estimator.GetCacheSizeForTests(), 0U);
+
+  // First network should be evicted from the cache.
+  base::strings::SafeSPrintf(network_name, "%d", 1);
+  estimator.SetCurrentNetworkName(network_name);
+  estimator.OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI);
+  EXPECT_FALSE(estimator.ReadCachedNetworkQualityEstimate());
+
+  // Last network should not be evicted from the cache.
+  base::strings::SafeSPrintf(network_name, "%d", network_count);
+  estimator.SetCurrentNetworkName(network_name);
+  estimator.OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI);
+  EXPECT_TRUE(estimator.ReadCachedNetworkQualityEstimate());
+}
+
 }  // namespace net