Allow BackoffEntry to be serialized and deserialized.

net/base/backoff_entry_serializer_unittest.cc

Index: net/base/backoff_entry_serializer_unittest.cc
diff --git a/net/base/backoff_entry_serializer_unittest.cc b/net/base/backoff_entry_serializer_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f6d5e09e2f58f077dfbc28a9d3bfb7bc8d1b0cac
--- /dev/null
+++ b/net/base/backoff_entry_serializer_unittest.cc
@@ -0,0 +1,141 @@
+// 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/backoff_entry.h"
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/time/tick_clock.h"
+#include "base/values.h"
+#include "net/base/backoff_entry_serializer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+using base::Time;
+using base::TimeDelta;
+using base::TimeTicks;
+
+BackoffEntry::Policy base_policy = {
+  0 /* num_errors_to_ignore */,
+  1000 /* initial_delay_ms */,
+  2.0 /* multiply_factor */,
+  0.0 /* jitter_factor */,
+  20000 /* maximum_backoff_ms */,
+  2000 /* entry_lifetime_ms */,
+  false /* always_use_initial_delay */
+};
+
+class TestTickClock : public base::TickClock {
+ public:
+  TestTickClock() {}
+  ~TestTickClock() override {}
+
+  TimeTicks NowTicks() override { return now_ticks_; }
+  void set_now(TimeTicks now) { now_ticks_ = now; }
+
+ private:
+  TimeTicks now_ticks_;
+  DISALLOW_COPY_AND_ASSIGN(TestTickClock);

[mmenke, 2015/05/06 17:34:48]

nit:  blank line before DISALLOW_COPY_AND_ASSIGN

[johnme, 2015/05/06 20:38:09]

Done.

+};
+
+TEST(BackoffEntrySerializerTest, SerializeTimeOffsets) {
+  TestTickClock now_ticks;
+  Time now_time = Time::FromJsTime(1430907555111.0);  // May 2015
+  BackoffEntry original(&base_policy, &now_ticks);
+  // 2 errors.
+  original.InformOfRequest(false);
+  original.InformOfRequest(false);
+  scoped_ptr<base::ListValue> serialized =
+      BackoffEntrySerializer::SerializeToValue(original, now_time);
+
+  {
+    // Test that immediate deserialization round-trips.
+    scoped_ptr<BackoffEntry> deserialized =
+        BackoffEntrySerializer::DeserializeFromValue(*serialized, &base_policy,
+                                                     &now_ticks, now_time);
+    ASSERT_TRUE(deserialized.get());
+    EXPECT_EQ(original.failure_count(), deserialized->failure_count());
+    EXPECT_EQ(original.GetReleaseTime(), deserialized->GetReleaseTime());
+  }
+
+  {
+    // Test deserialization when wall clock has advanced but TimeTicks haven't
+    // (e.g. device was rebooted).
+    Time later = now_time + TimeDelta::FromDays(1);

[mmenke, 2015/05/06 17:34:48]

I find these time names pretty confusing - we're deserializing when "now" is
"later", which seems counter-intuitive to me.  Maybe, in variable names:

now -> original (Matching the corresponding BackoffEntry)
later -> deserialization, also matching the corresponding BackoffEntry (Less
concerned about this one).

[johnme, 2015/05/06 20:38:09]

Ok, I went with original_time, original_ticks, later_time, later_ticks,
earlier_time.

[mmenke, 2015/05/06 20:48:43]

SGTM

+    scoped_ptr<BackoffEntry> deserialized =
+        BackoffEntrySerializer::DeserializeFromValue(*serialized, &base_policy,
+                                                     &now_ticks, later);
+    ASSERT_TRUE(deserialized.get());
+    EXPECT_EQ(original.failure_count(), deserialized->failure_count());
+    EXPECT_EQ(original.GetReleaseTime() - TimeDelta::FromDays(1),
+              deserialized->GetReleaseTime());

[mmenke, 2015/05/06 17:34:48]

This is a negative TimeTicks value, which seems weird.  Do we allow negative
TimeTicks values elsewhere?  Should we have a minimum of 0?

[mmenke, 2015/05/06 17:34:48]

I think this is weird enough to be worth a comment.  Unclear from context that
"GetReleaseTime" returns TimeTicks, and since the day has advanced, then the
TimeTicks of the release time should have gone backwards.

[johnme, 2015/05/06 20:38:09]

Why is negative TimeTicks weird? If we assume that zero is when the OS booted up
(not sure if that's actually true - it's probably platform-specific), it doesn't
seem unreasonable to be able to represent times before that.

[johnme, 2015/05/06 20:38:09]

Done, I added details comments to all these cases.

+  }
+
+  {
+    // Test deserialization when TimeTicks have advanced but wall clock hasn't
+    // (e.g. it's an hour later, but a DST change cancelled that out).
+    TestTickClock later_ticks;
+    later_ticks.set_now(TimeTicks() + TimeDelta::FromDays(1));
+    scoped_ptr<BackoffEntry> deserialized =
+        BackoffEntrySerializer::DeserializeFromValue(*serialized, &base_policy,
+                                                     &later_ticks, now_time);
+    ASSERT_TRUE(deserialized.get());
+    EXPECT_EQ(original.failure_count(), deserialized->failure_count());
+    EXPECT_EQ(original.GetReleaseTime() + TimeDelta::FromDays(1),
+              deserialized->GetReleaseTime());
+  }
+
+  {
+    // Test deserialization when both wall clock and TimeTicks have advanced
+    // (e.g. it's just later than it's used to be).
+    TestTickClock later_ticks;
+    later_ticks.set_now(TimeTicks() + TimeDelta::FromDays(1));
+    Time later = now_time + TimeDelta::FromDays(1);
+    scoped_ptr<BackoffEntry> deserialized =
+        BackoffEntrySerializer::DeserializeFromValue(*serialized, &base_policy,
+                                                     &later_ticks, later);
+    ASSERT_TRUE(deserialized.get());
+    EXPECT_EQ(original.failure_count(), deserialized->failure_count());
+    EXPECT_EQ(original.GetReleaseTime(), deserialized->GetReleaseTime());

[mmenke, 2015/05/06 17:34:48]

Again, think this requires an explanation.

[johnme, 2015/05/06 20:38:09]

Done.

+  }
+
+  {
+    // Test deserialization when wall clock has gone backwards but TimeTicks
+    // haven't (e.g. the system clock was fast but they fixed it).
+    EXPECT_LT(TimeDelta::FromSeconds(1), original.GetTimeUntilRelease());
+    Time earlier = now_time - TimeDelta::FromSeconds(1);
+    scoped_ptr<BackoffEntry> deserialized =
+        BackoffEntrySerializer::DeserializeFromValue(*serialized, &base_policy,
+                                                     &now_ticks, earlier);
+    ASSERT_TRUE(deserialized.get());
+    EXPECT_EQ(original.failure_count(), deserialized->failure_count());
+    // Deserialization should not exceed original time_until_release in such a
+    // situation.
+    EXPECT_EQ(original.GetTimeUntilRelease(),
+              deserialized->GetTimeUntilRelease());
+    // Since TimeTicks are equal, the release times will be too.
+    EXPECT_EQ(original.GetReleaseTime(), deserialized->GetReleaseTime());
+  }
+}
+
+TEST(BackoffEntrySerializerTest, SerializeNoFailures) {

[mmenke, 2015/05/06 17:34:48]

Suggest this test first.  Think it's nice to have the more basic tests go before
the tests that "depend" on them passing.  Not as useful as it used to be, when
tests were run in order, but still think it makes things clearer.

[mmenke, 2015/05/06 17:34:48]

This is pretty much identical to the first block in SerializeTimeOffsets, except
we're using a "time" of 0.  Maybe run it using the current time and time ticks,
to make it more interesting?  Or could consider getting rid of it.

[johnme, 2015/05/06 20:38:09]

Done. It now covers the case where a) there are no failures, and b)
serialization times are both realistic (non-zero).

[johnme, 2015/05/06 20:38:09]

Done.

+  TestTickClock now_ticks;
+  BackoffEntry original(&base_policy, &now_ticks);
+  scoped_ptr<base::ListValue> serialized =
+      BackoffEntrySerializer::SerializeToValue(original, Time());
+
+  scoped_ptr<BackoffEntry> deserialized =
+      BackoffEntrySerializer::DeserializeFromValue(*serialized, &base_policy,
+                                                   &now_ticks, Time());
+  ASSERT_TRUE(deserialized.get());
+  EXPECT_EQ(original.failure_count(), deserialized->failure_count());
+  EXPECT_EQ(original.GetReleaseTime(), deserialized->GetReleaseTime());
+}
+
+}  // namespace
+
+}  // namespace net