NQE: Cleanup observation buffer code

net/nqe/observation_buffer_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "net/nqe/network_quality_observation.h"
+#include "net/nqe/observation_buffer.h"
 
 #include <stddef.h>
 
+#include <memory>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/test/simple_test_tick_clock.h"
 #include "base/time/time.h"
 #include "net/nqe/network_quality_observation_source.h"

[RyanSturm, 2016/09/26 18:44:55]

#include "net/nqe/network_quality_observation.h"

[tbansal1, 2016/09/28 16:42:32]

Done.

-#include "net/nqe/observation_buffer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 
 namespace nqe {
 
+namespace internal {

[RyanSturm, 2016/09/26 18:44:55]

nit: Why did you move to internal? Is that really providing any benefit?

[tbansal1, 2016/09/28 16:42:31]

It avoids the need to prefix internal:: to some of the classes and data
structures, and makes code less verbose.

+
 namespace {
 
+// Verify that the buffer size is never exceeded.
+TEST(NetworkQualityObservationBufferTest, BoundedBuffer) {
+  ObservationBuffer<int32_t> observation_buffer(1.0);
+  const base::TimeTicks now =
+      base::TimeTicks() + base::TimeDelta::FromSeconds(1);
+  for (int i = 1; i <= 1000; ++i) {
+    observation_buffer.AddObservation(
+        Observation<int32_t>(i, now, NETWORK_QUALITY_OBSERVATION_SOURCE_TCP));
+    // The number of entries should be at most the maximum buffer size.
+    EXPECT_GE(300u, observation_buffer.Size());
+  }
+}
+
+// Verify that the percentiles are correctly computed when different

[RyanSturm, 2016/09/26 18:44:55]

"correctly" seems to imply that the values are decreasing at the correct rate.
It looks like you are checking that the values are decreasing, but not
necessarily at the right rate. Since you know this is geometric summing could
you consider adding a a case with rate .5 that checks that the most recent item
is worth more than half, and another case with rate .75 or 1 where the most
recent item is worth less than half.

If this is out of scope, just change the comment to be more accurate: "Verify
that the percentiles are monotonically non-decreasing when a weight is applied"

I also haven't looked at other tests in this file that might already check this
functionality.

[tbansal1, 2016/09/28 16:42:32]

Done.

+// observations have different weights.
+TEST(NetworkQualityObservationBufferTest, GetPercentileWithWeights) {
+  std::unique_ptr<base::SimpleTestTickClock> tick_clock(
+      new base::SimpleTestTickClock());
+  base::SimpleTestTickClock* tick_clock_ptr = tick_clock.get();
+
+  ObservationBuffer<int32_t> observation_buffer(0.005);
+  observation_buffer.SetTickClockForTesting(std::move(tick_clock));
+  const base::TimeTicks now = tick_clock_ptr->NowTicks();
+  for (int i = 1; i <= 100; ++i) {
+    observation_buffer.AddObservation(
+        Observation<int32_t>(i, now + base::TimeDelta::FromSeconds(i),
+                             NETWORK_QUALITY_OBSERVATION_SOURCE_TCP));
+  }
+  EXPECT_EQ(100U, observation_buffer.Size());
+
+  tick_clock_ptr->Advance(base::TimeDelta::FromSeconds(100));

[RyanSturm, 2016/09/26 18:44:55]

Can you call Advance in the for loop above and instead of now +
base::TimeDelta::FromSeconds(i) call NowTicks() every iteration?

[tbansal1, 2016/09/28 16:42:31]

Done.

+  for (int i = 1; i <= 100; ++i) {
+    // Verify that i'th percentile is more than i-1'th percentile.
+    int32_t result_i;
+    EXPECT_TRUE(observation_buffer.GetPercentile(
+        now, &result_i, i, std::vector<NetworkQualityObservationSource>()));
+
+    int32_t result_i_1;
+    EXPECT_TRUE(observation_buffer.GetPercentile(
+        now, &result_i_1, i - 1,
+        std::vector<NetworkQualityObservationSource>()));
+
+    EXPECT_LE(result_i_1, result_i);
+  }
+}
+
 // Verifies that the percentiles are correctly computed. All observations have
 // the same timestamp.
-TEST(NetworkQualityObservationTest, PercentileSameTimestamps) {
-  internal::ObservationBuffer<int32_t> int_buffer(0.5);
-  internal::ObservationBuffer<base::TimeDelta> time_delta_buffer(0.5);
+TEST(NetworkQualityObservationBufferTest, PercentileSameTimestamps) {
+  ObservationBuffer<int32_t> int_buffer(0.5);
+  ObservationBuffer<base::TimeDelta> time_delta_buffer(0.5);
   ASSERT_EQ(0u, int_buffer.Size());
   ASSERT_LT(0u, int_buffer.Capacity());
   ASSERT_EQ(0u, time_delta_buffer.Size());
   ASSERT_LT(0u, time_delta_buffer.Capacity());
 
   const base::TimeTicks now = base::TimeTicks::Now();
 
   int32_t result;
   base::TimeDelta time_delta_result;
 
   // Percentiles should be unavailable when no observations are available.
   EXPECT_FALSE(
       int_buffer.GetPercentile(base::TimeTicks(), &result, 50,
                                std::vector<NetworkQualityObservationSource>()));
   EXPECT_FALSE(time_delta_buffer.GetPercentile(
       base::TimeTicks(), &time_delta_result, 50,
       std::vector<NetworkQualityObservationSource>()));
 
   // Insert samples from {1,2,3,..., 100}. First insert odd samples, then even
   // samples. This helps in verifying that the order of samples does not matter.
   for (int i = 1; i <= 99; i += 2) {
-    int_buffer.AddObservation(internal::Observation<int32_t>(
+    int_buffer.AddObservation(Observation<int32_t>(
         i, now, NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
-    time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+    time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
         base::TimeDelta::FromMilliseconds(i), now,
         NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
     EXPECT_TRUE(int_buffer.GetPercentile(
         base::TimeTicks(), &result, 50,
         std::vector<NetworkQualityObservationSource>()));
     ASSERT_EQ(static_cast<size_t>(i / 2 + 1), int_buffer.Size());
     EXPECT_TRUE(time_delta_buffer.GetPercentile(
         base::TimeTicks(), &time_delta_result, 50,
         std::vector<NetworkQualityObservationSource>()));
     ASSERT_EQ(static_cast<size_t>(i / 2 + 1), time_delta_buffer.Size());
   }
 
   for (int i = 2; i <= 100; i += 2) {
-    int_buffer.AddObservation(internal::Observation<int32_t>(
+    int_buffer.AddObservation(Observation<int32_t>(
         i, now, NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
-    time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+    time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
         base::TimeDelta::FromMilliseconds(i), now,
         NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
     EXPECT_TRUE(int_buffer.GetPercentile(
         base::TimeTicks(), &result, 50,
         std::vector<NetworkQualityObservationSource>()));
     ASSERT_EQ(static_cast<size_t>(i / 2 + 50), int_buffer.Size());
     EXPECT_TRUE(time_delta_buffer.GetPercentile(
         base::TimeTicks(), &time_delta_result, 50,
         std::vector<NetworkQualityObservationSource>()));
     ASSERT_EQ(static_cast<size_t>(i / 2 + 50), time_delta_buffer.Size());
@@ skipping 32 matching lines @@
                                std::vector<NetworkQualityObservationSource>()));
   EXPECT_FALSE(time_delta_buffer.GetPercentile(
       base::TimeTicks(), &time_delta_result, 50,
       std::vector<NetworkQualityObservationSource>()));
 }
 
 // Verifies that the percentiles are correctly computed. Observations have
 // different timestamps with half the observations being very old and the rest
 // of them being very recent. Percentiles should factor in recent observations
 // much more heavily than older samples.
-TEST(NetworkQualityObservationTest, PercentileDifferentTimestamps) {
-  internal::ObservationBuffer<int32_t> int_buffer(0.5);
-  internal::ObservationBuffer<base::TimeDelta> time_delta_buffer(0.5);
+TEST(NetworkQualityObservationBufferTest, PercentileDifferentTimestamps) {
+  ObservationBuffer<int32_t> int_buffer(0.5);
+  ObservationBuffer<base::TimeDelta> time_delta_buffer(0.5);
   const base::TimeTicks now = base::TimeTicks::Now();
   const base::TimeTicks very_old = now - base::TimeDelta::FromDays(365);
 
   int32_t result;
   base::TimeDelta time_delta_result;
 
   // Network quality should be unavailable when no observations are available.
   EXPECT_FALSE(
       int_buffer.GetPercentile(base::TimeTicks(), &result, 50,
                                std::vector<NetworkQualityObservationSource>()));
   EXPECT_FALSE(time_delta_buffer.GetPercentile(
       base::TimeTicks(), &time_delta_result, 50,
       std::vector<NetworkQualityObservationSource>()));
 
   // First 50 samples have very old timestamp.
   for (int i = 1; i <= 50; ++i) {
-    int_buffer.AddObservation(internal::Observation<int32_t>(
+    int_buffer.AddObservation(Observation<int32_t>(
         i, very_old, NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
-    time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+    time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
         base::TimeDelta::FromMilliseconds(i), very_old,
         NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
   }
 
   // Next 50 (i.e., from 51 to 100) have recent timestamp.
   for (int i = 51; i <= 100; ++i) {
-    int_buffer.AddObservation(internal::Observation<int32_t>(
+    int_buffer.AddObservation(Observation<int32_t>(
         i, now, NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
-    time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+    time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
         base::TimeDelta::FromMilliseconds(i), now,
         NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
   }
 
   // Older samples have very little weight. So, all percentiles are >= 51
   // (lowest value among recent observations).
   for (int i = 1; i < 100; ++i) {
     // Checks if the difference between the two integers is less than 1. This is
     // required because computed percentiles may be slightly different from
     // what is expected due to floating point computation errors and integer
@@ skipping 12 matching lines @@
   EXPECT_FALSE(int_buffer.GetPercentile(
       now + base::TimeDelta::FromSeconds(1), &result, 50,
       std::vector<NetworkQualityObservationSource>()));
   EXPECT_FALSE(time_delta_buffer.GetPercentile(
       now + base::TimeDelta::FromSeconds(1), &time_delta_result, 50,
       std::vector<NetworkQualityObservationSource>()));
 }
 
 // Verifies that the percentiles are correctly computed when some of the
 // observation sources are disallowed. All observations have the same timestamp.
-TEST(NetworkQualityObservationTest, DisallowedObservationSources) {
-  internal::ObservationBuffer<int32_t> int_buffer(0.5);
-  internal::ObservationBuffer<base::TimeDelta> time_delta_buffer(0.5);
+TEST(NetworkQualityObservationBufferTest, DisallowedObservationSources) {
+  ObservationBuffer<int32_t> int_buffer(0.5);
+  ObservationBuffer<base::TimeDelta> time_delta_buffer(0.5);
   const base::TimeTicks now = base::TimeTicks::Now();
 
   int32_t result;
   base::TimeDelta time_delta_result;
 
   // Network quality should be unavailable when no observations are available.
   EXPECT_FALSE(
       int_buffer.GetPercentile(base::TimeTicks(), &result, 50,
                                std::vector<NetworkQualityObservationSource>()));
   EXPECT_FALSE(time_delta_buffer.GetPercentile(
       base::TimeTicks(), &time_delta_result, 50,
       std::vector<NetworkQualityObservationSource>()));
 
   // Insert samples from {1,2,3,..., 100}. First insert odd samples, then even
   // samples. This helps in verifying that the order of samples does not matter.
   for (int i = 1; i <= 99; i += 2) {
-    int_buffer.AddObservation(internal::Observation<int32_t>(
+    int_buffer.AddObservation(Observation<int32_t>(
         i, now, NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
-    time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+    time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
         base::TimeDelta::FromMilliseconds(i), now,
         NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
   }
 
   // Add samples for TCP and QUIC observations which should not be taken into
   // account when computing the percentile.
   for (int i = 1; i <= 99; i += 2) {
-    int_buffer.AddObservation(internal::Observation<int32_t>(
+    int_buffer.AddObservation(Observation<int32_t>(
         10000, now, NETWORK_QUALITY_OBSERVATION_SOURCE_TCP));
-    int_buffer.AddObservation(internal::Observation<int32_t>(
+    int_buffer.AddObservation(Observation<int32_t>(
         10000, now, NETWORK_QUALITY_OBSERVATION_SOURCE_QUIC));
-    time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+    time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
         base::TimeDelta::FromMilliseconds(10000), now,
         NETWORK_QUALITY_OBSERVATION_SOURCE_TCP));
-    time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+    time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
         base::TimeDelta::FromMilliseconds(10000), now,
         NETWORK_QUALITY_OBSERVATION_SOURCE_QUIC));
   }
 
   for (int i = 2; i <= 100; i += 2) {
-    int_buffer.AddObservation(internal::Observation<int32_t>(
+    int_buffer.AddObservation(Observation<int32_t>(
         i, now, NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
-    time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+    time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
         base::TimeDelta::FromMilliseconds(i), now,
         NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
   }
 
   std::vector<NetworkQualityObservationSource> disallowed_observation_sources;
   disallowed_observation_sources.push_back(
       NETWORK_QUALITY_OBSERVATION_SOURCE_TCP);
   disallowed_observation_sources.push_back(
       NETWORK_QUALITY_OBSERVATION_SOURCE_QUIC);
 
@@ skipping 23 matching lines @@
     EXPECT_TRUE(int_buffer.GetPercentile(base::TimeTicks(), &result, i,
                                          disallowed_observation_sources));
     EXPECT_NEAR(result, 10000, 1);
     EXPECT_TRUE(
         time_delta_buffer.GetPercentile(base::TimeTicks(), &time_delta_result,
                                         i, disallowed_observation_sources));
     EXPECT_NEAR(time_delta_result.InMilliseconds(), 10000, 1);
   }
 }
 
-TEST(NetworkQualityObservationTest, TestGetMedianRTTSince) {
-  internal::ObservationBuffer<int32_t> int_buffer(0.5);
-  internal::ObservationBuffer<base::TimeDelta> time_delta_buffer(0.5);
+TEST(NetworkQualityObservationBufferTest, TestGetMedianRTTSince) {
+  ObservationBuffer<int32_t> int_buffer(0.5);
+  ObservationBuffer<base::TimeDelta> time_delta_buffer(0.5);
   base::TimeTicks now = base::TimeTicks::Now();
   base::TimeTicks old = now - base::TimeDelta::FromMilliseconds(1);
   ASSERT_NE(old, now);
 
   // First sample has very old timestamp.
-  int_buffer.AddObservation(internal::Observation<int32_t>(
+  int_buffer.AddObservation(Observation<int32_t>(
       1, old, NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
-  time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+  time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
       base::TimeDelta::FromMilliseconds(1), old,
       NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
 
-  int_buffer.AddObservation(internal::Observation<int32_t>(
+  int_buffer.AddObservation(Observation<int32_t>(
       100, now, NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
-  time_delta_buffer.AddObservation(internal::Observation<base::TimeDelta>(
+  time_delta_buffer.AddObservation(Observation<base::TimeDelta>(
       base::TimeDelta::FromMilliseconds(100), now,
       NETWORK_QUALITY_OBSERVATION_SOURCE_URL_REQUEST));
 
   const struct {
     base::TimeTicks start_timestamp;
     bool expect_network_quality_available;
     base::TimeDelta expected_url_request_rtt;
     int32_t expected_downstream_throughput;
   } tests[] = {
       {now + base::TimeDelta::FromSeconds(10), false,
@@ skipping 21 matching lines @@
     if (test.expect_network_quality_available) {
       EXPECT_EQ(test.expected_url_request_rtt, url_request_rtt);
       EXPECT_EQ(test.expected_downstream_throughput,
                 downstream_throughput_kbps);
     }
   }
 }
 
 }  // namespace
 
+}  // namespace internal
+
 }  // namespace nqe
 
 }  // namespace net