[Doodle] Automatically expire Doodles after their TTL

components/doodle/doodle_service_unittest.cc

Index: components/doodle/doodle_service_unittest.cc
diff --git a/components/doodle/doodle_service_unittest.cc b/components/doodle/doodle_service_unittest.cc
index 3f6b09c71fe7e4f8779f96b04db603125d7bcd46..53dfabf51bb1b187c8b5b5f96de26b6e62ab27e6 100644
--- a/components/doodle/doodle_service_unittest.cc
+++ b/components/doodle/doodle_service_unittest.cc
@@ -10,6 +10,9 @@
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/test/simple_test_tick_clock.h"
+#include "base/test/test_simple_task_runner.h"
 #include "base/time/time.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -56,19 +59,38 @@ class MockDoodleObserver : public DoodleService::Observer {
 class DoodleServiceTest : public testing::Test {
  public:
   DoodleServiceTest() : fetcher_(nullptr) {

[fhorschig, 2017/03/02 20:12:59]

expiry_timer_(nullptr)?

[Marc Treib, 2017/03/03 10:06:58]

Indeed, thanks!

+    tick_clock_.SetNowTicks(base::TimeTicks::UnixEpoch() +
+                            base::TimeDelta::FromDays(5000));
+    task_runner_ = new base::TestSimpleTaskRunner();

[fhorschig, 2017/03/02 20:12:59]

any reason you don't initialize this earlier?
(initializer-list)

[Marc Treib, 2017/03/03 10:06:57]

Nope. Done.

+
+    auto expiry_timer = base::MakeUnique<base::OneShotTimer>(&tick_clock_);
+    expiry_timer->SetTaskRunner(task_runner_);
+    expiry_timer_ = expiry_timer.get();
+
     auto fetcher = base::MakeUnique<FakeDoodleFetcher>();
     fetcher_ = fetcher.get();
-    service_ = base::MakeUnique<DoodleService>(std::move(fetcher));
+
+    service_ = base::MakeUnique<DoodleService>(std::move(fetcher),
+                                               std::move(expiry_timer));
   }
 
   DoodleService* service() { return service_.get(); }
   FakeDoodleFetcher* fetcher() { return fetcher_; }
 
-  base::TimeDelta some_time() const { return base::TimeDelta::FromHours(1); }
+  base::TestSimpleTaskRunner* task_runner() { return task_runner_.get(); }
+
+  base::TimeDelta SomeTime() const { return base::TimeDelta::FromHours(1); }

[fhorschig, 2017/03/02 20:12:59]

When it was lower_case, it used to be pretty clear that it's only a get.
Maybe call it |AnHour()| so it's clearer it's const.
... can we make it an implementation detail?

[Marc Treib, 2017/03/03 10:06:58]

I don't know what you mean by "implementation detail". Anyway, it's gone now.

 
  private:
-  std::unique_ptr<DoodleService> service_;
+  // Weak, owned by the service.
   FakeDoodleFetcher* fetcher_;
+
+  base::SimpleTestTickClock tick_clock_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  // Weak, owned by the service.
+  base::OneShotTimer* expiry_timer_;

[fhorschig, 2017/03/02 20:12:59]

nitty nit: Would be nice to visually group the weak references.
(moving up or down)

[Marc Treib, 2017/03/03 10:06:58]

To me, the "logical" grouping (clock + task_runner + timer belong together) is
more important than the "weak" property.

+
+  std::unique_ptr<DoodleService> service_;
 };
 
 TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) {
@@ -82,7 +104,7 @@ TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) {
   // Serve it (with an arbitrary config).
   DoodleConfig config;
   config.doodle_type = DoodleType::SIMPLE;
-  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, some_time(), config);
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(), config);
 
   // The config should be available.
   EXPECT_THAT(service()->config(), Eq(config));
@@ -96,7 +118,7 @@ TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) {
   DoodleConfig other_config;
   other_config.doodle_type = DoodleType::SLIDESHOW;
   DCHECK(config != other_config);
-  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, some_time(),
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(),
                                other_config);
 
   // The config should have been updated.
@@ -118,7 +140,7 @@ TEST_F(DoodleServiceTest, CallsObserverOnConfigReceived) {
   DoodleConfig config;
   config.doodle_type = DoodleType::SIMPLE;
   EXPECT_CALL(observer, OnDoodleConfigUpdated(Eq(config)));
-  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, some_time(), config);
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(), config);
 
   // Remove the observer before the service gets destroyed.
   service()->RemoveObserver(&observer);
@@ -129,7 +151,7 @@ TEST_F(DoodleServiceTest, CallsObserverOnConfigRemoved) {
   service()->Refresh();
   DoodleConfig config;
   config.doodle_type = DoodleType::SIMPLE;
-  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, some_time(), config);
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(), config);
   ASSERT_THAT(service()->config(), Eq(config));
 
   // Register an observer and request a refresh.
@@ -153,7 +175,7 @@ TEST_F(DoodleServiceTest, CallsObserverOnConfigUpdated) {
   service()->Refresh();
   DoodleConfig config;
   config.doodle_type = DoodleType::SIMPLE;
-  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, some_time(), config);
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(), config);
   ASSERT_THAT(service()->config(), Eq(config));
 
   // Register an observer and request a refresh.
@@ -168,19 +190,19 @@ TEST_F(DoodleServiceTest, CallsObserverOnConfigUpdated) {
   other_config.doodle_type = DoodleType::SLIDESHOW;
   DCHECK(config != other_config);
   EXPECT_CALL(observer, OnDoodleConfigUpdated(Eq(other_config)));
-  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, some_time(),
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(),
                                other_config);
 
   // Remove the observer before the service gets destroyed.
   service()->RemoveObserver(&observer);
 }
 
-TEST_F(DoodleServiceTest, DoesNotCallObserverWhenConfigEquivalent) {
+TEST_F(DoodleServiceTest, DoesNotCallObserverIfConfigEquivalent) {
   // Load some doodle config.
   service()->Refresh();
   DoodleConfig config;
   config.doodle_type = DoodleType::SIMPLE;
-  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, some_time(), config);
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(), config);
   ASSERT_THAT(service()->config(), Eq(config));
 
   // Register an observer and request a refresh.
@@ -194,11 +216,67 @@ TEST_F(DoodleServiceTest, DoesNotCallObserverWhenConfigEquivalent) {
   DoodleConfig equivalent_config;
   equivalent_config.doodle_type = DoodleType::SIMPLE;
   DCHECK(config == equivalent_config);
-  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, some_time(),
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(),
                                equivalent_config);
 
   // Remove the observer before the service gets destroyed.
   service()->RemoveObserver(&observer);
 }
 
+TEST_F(DoodleServiceTest, CallsObserverWhenConfigExpires) {
+  // Load some doodle config.
+  service()->Refresh();
+  DoodleConfig config;
+  config.doodle_type = DoodleType::SIMPLE;
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(), config);
+  ASSERT_THAT(service()->config(), Eq(config));
+
+  // Make sure the task arrived at the timer's task runner.
+  ASSERT_THAT(task_runner()->NumPendingTasks(), Eq(1u));
+  EXPECT_THAT(task_runner()->NextPendingTaskDelay(), Eq(SomeTime()));
+
+  // Register an observer.
+  StrictMock<MockDoodleObserver> observer;
+  service()->AddObserver(&observer);
+
+  // Run the expiry task. The observer should get notified that there's no
+  // config anymore.
+  EXPECT_CALL(observer, OnDoodleConfigUpdated(Eq(base::nullopt)));
+  task_runner()->RunPendingTasks();

[Marc Treib, 2017/03/02 18:51:02]

So, this is the famous and amazing TestSimpleTaskRunner in action. Admittedly
it's less than optimal; it ignores time completely. But the only alternative I
can see would be using a full MessageLoop with its MessageLoopTaskRunner. But
that seems messy and hard to control, e.g. there's no way to inject a Clock as
far as I can tell.

[fhorschig, 2017/03/02 20:12:59]

What about TestMockTimeTaskRunner. As far as I know, it's the same but it
allows to fast forward:
https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?t...

This even comes with a free Mock{Tick,}Clock.
We used it once already:
https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/remote_su...

[Marc Treib, 2017/03/03 10:06:58]

D'oh, forgot about that one again. Thanks for the pointer!
Done.

+
+  // Remove the observer before the service gets destroyed.
+  service()->RemoveObserver(&observer);
+}
+
+TEST_F(DoodleServiceTest, DisregardsAlreadyExpiredConfigs) {
+  StrictMock<MockDoodleObserver> observer;
+  service()->AddObserver(&observer);
+
+  ASSERT_THAT(service()->config(), Eq(base::nullopt));
+
+  // Load an already-expired config. This should have no effect; in particular
+  // no call to the observer.
+  service()->Refresh();
+  DoodleConfig config;
+  config.doodle_type = DoodleType::SIMPLE;
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
+                               base::TimeDelta::FromSeconds(0), config);
+  EXPECT_THAT(service()->config(), Eq(base::nullopt));
+
+  // Load a doodle config as usual. Nothing to see here.
+  service()->Refresh();
+  EXPECT_CALL(observer, OnDoodleConfigUpdated(Eq(config)));
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, SomeTime(), config);
+  ASSERT_THAT(service()->config(), Eq(config));
+
+  // Now load an expired config again. The cached one should go away.
+  service()->Refresh();
+  EXPECT_CALL(observer, OnDoodleConfigUpdated(Eq(base::nullopt)));
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
+                               base::TimeDelta::FromSeconds(0), config);
+
+  // Remove the observer before the service gets destroyed.
+  service()->RemoveObserver(&observer);
+}
+
 }  // namespace doodle