NQE: Cleanup observation buffer code

net/nqe/observation_buffer_unittest.cc

Index: net/nqe/observation_buffer_unittest.cc
diff --git a/net/nqe/network_quality_observation_unittest.cc b/net/nqe/observation_buffer_unittest.cc
similarity index 76%
rename from net/nqe/network_quality_observation_unittest.cc
rename to net/nqe/observation_buffer_unittest.cc
index f8215a23f4e74c635bd1c3c20520d772af859896..f5861647540483046175a696896bf139143640e6 100644
--- a/net/nqe/network_quality_observation_unittest.cc
+++ b/net/nqe/observation_buffer_unittest.cc
@@ -2,30 +2,79 @@
 // 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());
@@ -47,9 +96,9 @@ TEST(NetworkQualityObservationTest, PercentileSameTimestamps) {
   // 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(
@@ -63,9 +112,9 @@ TEST(NetworkQualityObservationTest, PercentileSameTimestamps) {
   }
 
   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(
@@ -118,9 +167,9 @@ TEST(NetworkQualityObservationTest, PercentileSameTimestamps) {
 // 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);
 
@@ -137,18 +186,18 @@ TEST(NetworkQualityObservationTest, PercentileDifferentTimestamps) {
 
   // 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));
   }
@@ -181,9 +230,9 @@ TEST(NetworkQualityObservationTest, PercentileDifferentTimestamps) {
 
 // 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;
@@ -200,9 +249,9 @@ TEST(NetworkQualityObservationTest, DisallowedObservationSources) {
   // 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));
   }
@@ -210,22 +259,22 @@ TEST(NetworkQualityObservationTest, DisallowedObservationSources) {
   // 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));
   }
@@ -269,23 +318,23 @@ TEST(NetworkQualityObservationTest, DisallowedObservationSources) {
   }
 }
 
-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));
 
@@ -327,6 +376,8 @@ TEST(NetworkQualityObservationTest, TestGetMedianRTTSince) {
 
 }  // namespace
 
+}  // namespace internal
+
 }  // namespace nqe
 
 }  // namespace net