Implement throttling logic for fetching affiliation information.

components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc

Index: components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc
diff --git a/components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc b/components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..39d333c3e6839ed768167dc76770ac1cd8717b8e
--- /dev/null
+++ b/components/password_manager/core/browser/affiliation_fetch_throttler_unittest.cc
@@ -0,0 +1,231 @@
+// 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 "components/password_manager/core/browser/affiliation_fetch_throttler.h"
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/test/test_mock_time_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/time/tick_clock.h"
+#include "base/time/time.h"
+#include "components/password_manager/core/browser/affiliation_fetch_throttler_delegate.h"
+#include "net/base/network_change_notifier.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace password_manager {
+namespace {
+
+class MockAffiliationFetchThrottlerDelegate
+    : public testing::StrictMock<AffiliationFetchThrottlerDelegate> {
+ public:
+  MockAffiliationFetchThrottlerDelegate() {}
+
+  MOCK_METHOD0(OnCanSendNetworkRequest, bool());

[mmenke, 2015/01/21 17:18:52]

It's generally recommended you don't use GMOCK, to limit the amount of code
people need to be familiar with to start hacking on Chrome.  Doesn't seem to
save us all that much here.  Suggest just keeping a count of times called, and
asserting on that.

[mmenke, 2015/01/21 17:18:53]

In no tests does OnCanSendNetworkRequest return false.

[engedy, 2015/01/23 22:09:43]

While the password_manager component uses GMock extensively, you are correct
that there is no need for it in this case, so I have stopped using it.

[engedy, 2015/01/23 22:09:43]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockAffiliationFetchThrottlerDelegate);
+};
+
+class MockNetworkChangeNotifier : public net::NetworkChangeNotifier {
+ public:
+  MockNetworkChangeNotifier() : connection_type_(CONNECTION_UNKNOWN) {}
+
+  void set_connection_type(ConnectionType connection_type) {
+    connection_type_ = connection_type;
+    NotifyObserversOfConnectionTypeChangeForTests(connection_type_);
+  }
+
+  ConnectionType GetCurrentConnectionType() const override {
+    return connection_type_;
+  }
+
+ private:
+  ConnectionType connection_type_;
+};
+
+}  // namespace
+
+class AffiliationFetchThrottlerTest : public testing::Test {
+ public:
+  AffiliationFetchThrottlerTest()
+      : network_change_notifier_(new MockNetworkChangeNotifier),

[mmenke, 2015/01/21 17:18:53]

Any reason this needs to be a scoped_ptr?

[engedy, 2015/01/23 22:09:43]

Nope. Done.

+        task_runner_(new base::TestMockTimeTaskRunner) {}
+  ~AffiliationFetchThrottlerTest() override {}
+
+  void SetUp() override {
+    policy_.reset(new AffiliationFetchThrottler(
+        &mock_delegate_, task_runner_, task_runner_->GetMockTickClock()));

[mmenke, 2015/01/21 17:18:52]

Can this be done in the constructor, preferable in the initializer list?

[engedy, 2015/01/23 22:09:44]

N/A. The instance under test is created in the test methods now.

+  }
+
+  void SimulateHasNetworkConnectivity(bool has_connectivity) {
+    network_change_notifier_->set_connection_type(
+        has_connectivity ? net::NetworkChangeNotifier::CONNECTION_UNKNOWN
+                         : net::NetworkChangeNotifier::CONNECTION_NONE);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  // Forwards time, and given that MockAffiliationFetchThrottlerDelegate is a
+  // strict mock, also ensures OnCanSendNetworkRequest() is not called back
+  // during this period.
+  void FastForwardTimeBySecs(double secs) {
+    task_runner_->FastForwardBy(base::TimeDelta::FromSecondsD(secs));
+  }
+
+  void FastForwardUntilNoTasksRemain() {
+    task_runner_->FastForwardUntilNoTasksRemain();
+  }
+
+  size_t GetPendingTaskCount() { return task_runner_->GetPendingTaskCount(); }

[mmenke, 2015/01/21 17:18:52]

const

[engedy, 2015/01/23 22:09:43]

Done.

+
+  // Forwards time until the next OnCanSendNetworkRequest() callback, and
+  // verifies that it occurs between |min_delay_sec| and |max_delay_sec| seconds
+  // from now, inclusive.
+  void AssertReleaseBetweenSecs(double min_delay_sec, double max_delay_sec) {
+    base::TimeTicks ticks_at_start = task_runner_->GetCurrentMockTime();
+    base::TimeDelta min_delay = base::TimeDelta::FromSecondsD(min_delay_sec);
+    if (min_delay.ToInternalValue() > 0) {
+      task_runner_->FastForwardBy(min_delay -
+                                  base::TimeDelta::FromInternalValue(1));
+    }
+    EXPECT_CALL(mock_delegate_, OnCanSendNetworkRequest())
+        .Times(1)
+        .WillRepeatedly(testing::Return(true));
+    task_runner_->FastForwardUntilNoTasksRemain();
+    ASSERT_TRUE(testing::Mock::VerifyAndClearExpectations(&mock_delegate_));
+    ASSERT_GE(base::TimeDelta::FromSecondsD(max_delay_sec),
+              task_runner_->GetCurrentMockTime() - ticks_at_start);
+  }
+
+  AffiliationFetchThrottler& policy() { return *policy_.get(); }

[mmenke, 2015/01/21 17:18:53]

Seems like this should be returning an AffiliationFetchThrottler* - generally
doesn't seem a great idea to use non-const references.

[engedy, 2015/01/23 22:09:44]

Done.

+
+ private:
+  // Needed because NetworkChangeNotifier uses ObserverList, which notifies
+  // observers on the MessageLoop that belongs to the thread from which they
+  // have registered.
+  base::MessageLoop message_loop_;
+  scoped_ptr<MockNetworkChangeNotifier> network_change_notifier_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
+  MockAffiliationFetchThrottlerDelegate mock_delegate_;
+  scoped_ptr<AffiliationFetchThrottler> policy_;

[mmenke, 2015/01/21 17:18:53]

Calling an "AffiliationFetchThrottler" policy_ is very confusing.  Should call
it throttler_, at least.

[engedy, 2015/01/23 22:09:44]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(AffiliationFetchThrottlerTest);
+};
+
+TEST_F(AffiliationFetchThrottlerTest, SuccessfulRequests) {
+  SimulateHasNetworkConnectivity(true);
+
+  policy().SignalNetworkRequestNeeded();
+  AssertReleaseBetweenSecs(0, 0);
+
+  // Signal while request is in flight should be ignored.

[mmenke, 2015/01/21 17:18:53]

We should make sure that it's ignored here.  Is the mock class doing so?  I
guess the "StrictMock" makes sure of that.  Regardless, see earlier comment
about avoiding gmock.

[engedy, 2015/01/23 22:09:44]

Yes, the strict mock ensured this, but I have made this more explicit.

+  policy().SignalNetworkRequestNeeded();
+  FastForwardUntilNoTasksRemain();
+  policy().InformOfNetworkRequestComplete(true);
+  FastForwardUntilNoTasksRemain();
+
+  // Duplicate the second signal 3 times: still only 1 callback should arrive.
+  policy().SignalNetworkRequestNeeded();
+  policy().SignalNetworkRequestNeeded();
+  policy().SignalNetworkRequestNeeded();
+  AssertReleaseBetweenSecs(0, 0);
+}
+
+TEST_F(AffiliationFetchThrottlerTest, FailedRequests) {
+  SimulateHasNetworkConnectivity(true);
+
+  policy().SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(0, 0));
+  policy().InformOfNetworkRequestComplete(false);
+
+  // Request after first failure should be delayed by 10 seconds with 50%
+  // fuzzing factor.
+  policy().SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10));

[mmenke, 2015/01/21 17:18:53]

Relying on exact values for doubles just seems like a really bad idea.

[engedy, 2015/01/23 22:09:44]

Not that we are not relying on *exact* value: this gets converted to a
base::TimeDelta, which is integer-based with a microsecond granularity. Now that
I have added rounding, as long as the floating point error is less than 0.0005
milliseconds, we are fine. In most cases, this is fulfilled, by a large margin.

The only exception is the AffiliationFetchThrottlerTest.FailedRequests test
where the |maximum_delay_ms| is so large that its precision is pretty close to
this margin. I will investigate that.

[mmenke, 2015/01/23 22:39:53]

If we get a 5 or a 10 in the random calculation (Assuming we can get a 5 or 10),
you're relying on a range including an exact match [5, 10] vs (5, 10).  If we
can't get a 5 or a 10 in the random calculation, you're depending on that, which
also doesn't seem like a good idea.  That makes me very nervous.

[engedy, 2015/01/24 00:35:38]

Not sure I understand what you mean by this. Except for that one test I
mentioned, the current solution is correct in that it accepts the boundary
values.

The production code being non-deterministic, and so having to resort to checking
ranges in the tests makes me a whole lot more nervous.

+  policy().InformOfNetworkRequestComplete(false);
+
+  // Request after first failure should be delayed by 40 seconds with 50%
+  // fuzzing factor, applied twice.

[mmenke, 2015/01/21 17:18:52]

This isn't true.  Jitter factor is applied independently for each retry.  Jitty
for a request is:

initial_backoff * multiply_factor^(effective_failure_count - 1) * Uniform(1 -
jitter_factor, 1]

[engedy, 2015/01/23 22:09:44]

Right. Done.

+  policy().SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(10, 40));
+  policy().InformOfNetworkRequestComplete(false);
+}
+
+TEST_F(AffiliationFetchThrottlerTest, GracePeriodAfterConnectivityIsRestored) {
+  SimulateHasNetworkConnectivity(false);
+
+  // After connectivity is re-established, the first request should be delayed
+  // by 10 seconds with 50% fuzzing factor.
+  policy().SignalNetworkRequestNeeded();

[mmenke, 2015/01/21 17:18:52]

FastForwardUntilNoTasksRemain();?  (And same goes for right after all sets of
events that should not trigger a request, just before even event that actually
will)

[engedy, 2015/01/23 22:09:43]

I have added it here, and verified elsewhere, please let me know if I have
missed a place.

+  SimulateHasNetworkConnectivity(true);
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10));

[mmenke, 2015/01/21 17:18:53]

These numbers should not be hardcoded.  Should use constants from the
AffiliationFetchThrottler (Both for the max time, and for the jitter).  Could
even just declare the backoff params as private a member of the class, and make
the test fixture a friend.  (Or could make the params a public member).

[engedy, 2015/01/23 22:09:44]

Done.

+  policy().InformOfNetworkRequestComplete(true);
+
+  // The next request should not be delayed.
+  policy().SignalNetworkRequestNeeded();
+  AssertReleaseBetweenSecs(0, 0);
+}
+
+// Same as GracePeriodAfterConnectivityIsRestored, but the network comes back
+// just before SignalNetworkRequestNeeded() is called.
+TEST_F(AffiliationFetchThrottlerTest, GracePeriodAfterConnectivityIsRestored2) {
+  SimulateHasNetworkConnectivity(false);
+
+  SimulateHasNetworkConnectivity(true);
+  policy().SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10));

[mmenke, 2015/01/21 17:18:53]

You may want to add a way to EXPECT on the actual delay used in addition or
instead of these checks.  Could either create a subclass of the TaskRunner that
records the times, or a subclass of AffiliationFetchThrottlerTest that has
access to them (And have the subclass call into the TaskRunner or not).  Anyhow,
up to you, I just think if you output the actual delays on failure, it makes
life much easier in the case of regression (Or, say, flaky tests...).

[engedy, 2015/01/23 22:09:44]

Done.

+  policy().InformOfNetworkRequestComplete(true);
+
+  policy().SignalNetworkRequestNeeded();
+  AssertReleaseBetweenSecs(0, 0);
+}
+
+// TODO(engedy): This test is flaky. Investigate.
+TEST_F(AffiliationFetchThrottlerTest, ConnectivityLostDuringBackoff) {
+  SimulateHasNetworkConnectivity(true);
+
+  policy().SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(0, 0));
+  policy().InformOfNetworkRequestComplete(false);
+
+  policy().SignalNetworkRequestNeeded();
+  SimulateHasNetworkConnectivity(false);
+
+  // Let the exponential backoff delay expire, and verify nothing happens.
+  FastForwardUntilNoTasksRemain();
+
+  // Verify that the request is, however, sent after 10 seconds with 50% fuzzing
+  // factor.
+  SimulateHasNetworkConnectivity(true);
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10));
+}
+
+TEST_F(AffiliationFetchThrottlerTest,
+       ConnectivityLostAndRestoredDuringBackoff) {
+  SimulateHasNetworkConnectivity(true);
+
+  policy().SignalNetworkRequestNeeded();
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(0, 0));
+  policy().InformOfNetworkRequestComplete(false);
+
+  policy().SignalNetworkRequestNeeded();
+
+  // Also verify that a flaky connection will not flood the task queue with lots
+  // of tasks.

[mmenke, 2015/01/21 17:18:52]

I don't think this is needed...  The NCN itself already has protection against
this - delaying and de-duping network change messages, with platform-dependent
delays varying from 500 ms to 4 seconds.  Delaying when a connection is
re-established may be worth doing anyways, to prevent a flurry of activity when
online, but I'm not sure the 5 second delay gets us anything the ~1 second delay
elsewhere doesn't get us.

[engedy, 2015/01/23 22:09:43]

This mostly test what you have pointed out earlier: that we do not flood the
task runner with lots of future tasks even if the connection state changes
often.

+  for (size_t t = 1; t <= 5; ++t) {
+    SimulateHasNetworkConnectivity(false);
+    SimulateHasNetworkConnectivity(true);
+    FastForwardTimeBySecs(1);
+  }
+  EXPECT_EQ(1u, GetPendingTaskCount());
+
+  ASSERT_NO_FATAL_FAILURE(AssertReleaseBetweenSecs(5, 10));
+}

[mmenke, 2015/01/21 17:18:53]

Do we have a test where connectivity is lost and restored during a request
(Particularly one that ends up failing)?

[engedy, 2015/01/23 22:09:44]

I have added two, though I wasn't sure if the URLFetcher would fail immediately
after connection is deemed to have been lost, or later, or only when connection
is restored, or at an unrelated time? I guess the tests should not rely on this
anyway, so we might want to add a 3rd test here.

[mmenke, 2015/01/23 22:39:53]

You shouldn't depend on what it does - it depends on what it's doing, whether
we're using SPDY or HTTP, whether we're in proxy resolution phase or DNS
resolution, reading from the cache (Guess we're probably not doing that here),
etc.

[engedy, 2015/01/26 19:39:34]

I have added an additional test.

+
+}  // namespace password_manager