[Doodle] Introduce a DoodleService

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
new file mode 100644
index 0000000000000000000000000000000000000000..1b937c3364aff83d563206695caee21bd5343a62
--- /dev/null
+++ b/components/doodle/doodle_service_unittest.cc
@@ -0,0 +1,171 @@
+// Copyright 2017 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/doodle/doodle_service.h"
+
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::Eq;
+using testing::StrictMock;
+
+namespace doodle {
+
+namespace {
+
+class FakeDoodleFetcher : public DoodleFetcher {
+ public:
+  FakeDoodleFetcher() = default;
+  ~FakeDoodleFetcher() override = default;
+
+  void FetchDoodle(FinishedCallback callback) override {
+    callbacks_.push_back(std::move(callback));
+  }
+
+  size_t num_pending_callbacks() const { return callbacks_.size(); }
+
+  void ServeAllCallbacks(DoodleState state,
+                         const base::Optional<DoodleConfig>& config) {
+    for (auto& callback : callbacks_) {
+      std::move(callback).Run(state, config);
+    }
+    callbacks_.clear();
+  }
+
+ private:
+  std::vector<FinishedCallback> callbacks_;
+};
+
+class MockDoodleObserver : public DoodleService::Observer {
+ public:
+  MOCK_METHOD1(OnDoodleConfigUpdated,
+               void(const base::Optional<DoodleConfig>&));
+};
+
+}  // namespace
+
+// Equality operator for DoodleConfigs, for use by testing::Eq.
+// Note: This must be outside of the anonymous namespace.
+bool operator==(const DoodleConfig& lhs, const DoodleConfig& rhs) {
+  return lhs.IsEquivalent(rhs);
+}
+
+class DoodleServiceTest : public testing::Test {
+ public:
+  DoodleServiceTest() : fetcher_(nullptr) {
+    auto fetcher = base::MakeUnique<FakeDoodleFetcher>();
+    fetcher_ = fetcher.get();
+    service_ = base::MakeUnique<DoodleService>(std::move(fetcher));
+  }
+
+  DoodleService* service() { return service_.get(); }
+  FakeDoodleFetcher* fetcher() { return fetcher_; }
+
+ private:
+  std::unique_ptr<DoodleService> service_;
+  FakeDoodleFetcher* fetcher_;
+};
+
+TEST_F(DoodleServiceTest, CallsObserverOnConfigReceived) {
+  StrictMock<MockDoodleObserver> observer;
+  service()->AddObserver(&observer);
+
+  ASSERT_THAT(service()->config(), Eq(base::nullopt));

[fhorschig, 2017/02/27 17:43:53]

Why not EXPECT_THAT? the code wouldn't break and we would get more
information about other failures.
(It's debatable ... if you want to express a strong precondition,
keep it and ignore the reoccurring remarks)

[Marc Treib, 2017/02/28 10:25:48]

The pattern is: ASSERT for preconditions, EXPECT for the actual tested things.

Here, if the service somehow already had a config, then the test below wouldn't
test what it's supposed to test anymore.

+
+  // Request a refresh of the doodle config.
+  service()->Refresh();
+  // The request should have arrived at the fetcher.
+  EXPECT_THAT(fetcher()->num_pending_callbacks(), Eq(1u));
+
+  // Serve it (with an arbitrary config). The observer should get notified.
+  DoodleConfig config;
+  config.doodle_type = DoodleType::SIMPLE;
+  EXPECT_CALL(observer, OnDoodleConfigUpdated(Eq(config)));
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, config);
+
+  // Remove the observer before the service gets destroyed.
+  service()->RemoveObserver(&observer);
+}
+
+TEST_F(DoodleServiceTest, CallsObserverOnConfigRemoved) {
+  // Load some doodle config.
+  service()->Refresh();
+  DoodleConfig config;
+  config.doodle_type = DoodleType::SIMPLE;
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, config);
+  ASSERT_THAT(service()->config(), Eq(config));

[fhorschig, 2017/02/27 17:43:53]

Same as above, I think an EXPECT is enough to break the test and
we will still receive sth. like an inconsistent fetcher state.

[Marc Treib, 2017/02/28 10:25:49]

Precondition

+
+  // Register an observer and request a refresh.
+  StrictMock<MockDoodleObserver> observer;
+  service()->AddObserver(&observer);
+  service()->Refresh();
+  ASSERT_THAT(fetcher()->num_pending_callbacks(), Eq(1u));

[fhorschig, 2017/02/27 17:43:59]

How important is the state of the fetcher?
I see that this might be worth an assert but in the test above,
an expect was sufficient.

[Marc Treib, 2017/02/28 10:25:49]

Precondition

+
+  // Serve the request with an empty doodle config. The observer should get
+  // notified.
+  EXPECT_CALL(observer, OnDoodleConfigUpdated(Eq(base::nullopt)));
+  fetcher()->ServeAllCallbacks(DoodleState::NO_DOODLE, base::nullopt);
+
+  // Remove the observer before the service gets destroyed.
+  service()->RemoveObserver(&observer);
+}
+
+TEST_F(DoodleServiceTest, CallsObserverOnConfigUpdated) {
+  // Load some doodle config.
+  service()->Refresh();
+  DoodleConfig config;
+  config.doodle_type = DoodleType::SIMPLE;
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, config);
+  ASSERT_THAT(service()->config(), Eq(config));

[fhorschig, 2017/02/27 17:43:59]

Same as above, EXPECT_THAT?

[Marc Treib, 2017/02/28 10:25:49]

Precondition

+
+  // Register an observer and request a refresh.
+  StrictMock<MockDoodleObserver> observer;
+  service()->AddObserver(&observer);
+  service()->Refresh();
+  ASSERT_THAT(fetcher()->num_pending_callbacks(), Eq(1u));
+
+  // Serve the request with a different doodle config. The observer should get
+  // notified.
+  DoodleConfig config2;
+  config2.doodle_type = DoodleType::SLIDESHOW;
+  ASSERT_FALSE(config.IsEquivalent(config2));

[fhorschig, 2017/02/27 17:43:59]

This looks like an invariant of the test code as no code changes 
config or config2:

DoodleConfig config;
config.doodle_type = DoodleType::SIMPLE;
[...]  // Code not modifying |content|.
DoodleConfig config2;
config2.doodle_type = DoodleType::SLIDESHOW;

Why would you assert that? Have I missed a Config& parameter here?
If you want to express that this condition is crucial for the test,
I would DCHECK it. That way, you don't look for an error in your code
but you know that the test was modified and is broken.
--> go/zine-testing 4.I.A.

[Marc Treib, 2017/02/28 10:25:49]

Well, it's arguable if a change in the IsEquivalent implementation should make
this test fail or crash. But sure, since it won't make much of a difference in
practice either way, done.

+  EXPECT_CALL(observer, OnDoodleConfigUpdated(Eq(config2)));
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, config2);
+
+  // Remove the observer before the service gets destroyed.
+  service()->RemoveObserver(&observer);
+}
+
+TEST_F(DoodleServiceTest, DoesNotCallObserverWhenConfigEquivalent) {
+  // Load some doodle config.
+  service()->Refresh();
+  DoodleConfig config;
+  config.doodle_type = DoodleType::SIMPLE;
+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, config);
+  ASSERT_THAT(service()->config(), Eq(config));

[fhorschig, 2017/02/27 17:43:59]

Same as above, EXPECT_THAT?

[Marc Treib, 2017/02/28 10:25:49]

Precondition

+
+  // Register an observer and request a refresh.
+  StrictMock<MockDoodleObserver> observer;
+  service()->AddObserver(&observer);
+  service()->Refresh();
+  ASSERT_THAT(fetcher()->num_pending_callbacks(), Eq(1u));
+
+  // Serve the request with an equivalent doodle config. The observer should get
+  // notified.
+  DoodleConfig config2;
+  config2.doodle_type = DoodleType::SIMPLE;
+  ASSERT_TRUE(config.IsEquivalent(config2));

[fhorschig, 2017/02/27 17:43:59]

Same as above, DCHECK?

Also, is there a special reason you sometimes use ASSERT_THAT(,Eq()) and
sometimes ASSERT_TRUE(IsEquivalent())?

(if there is none, I would prefer a consistent _THAT)

[Marc Treib, 2017/02/28 10:25:48]

Done.
Equality != equivalence.
To use ASSERT_THAT with IsEquivalent, I'd have to write a custom matcher based
on IsEquivalent. IMO it's *much* easier to read with ASSERT_TRUE.

I get using ASSERT_THAT(.., Eq(..)) over ASSERT_EQ for consistency, but IMO
there's absolutely nothing wrong with ASSERT_TRUE/FALSE. Same reason you don't
write "if (var == true)".

+  fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE, config2);
+
+  // Remove the observer before the service gets destroyed.
+  service()->RemoveObserver(&observer);
+}
+
+}  // namespace doodle