cros: DBUS client to interact with fingerprint DBUS API.

chromeos/dbus/biod/biod_client_unittest.cc

Index: chromeos/dbus/biod/biod_client_unittest.cc
diff --git a/chromeos/dbus/biod/biod_client_unittest.cc b/chromeos/dbus/biod/biod_client_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2246fccec249ddbeeea7c2fe3063e126112c2aba
--- /dev/null
+++ b/chromeos/dbus/biod/biod_client_unittest.cc
@@ -0,0 +1,389 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.

[Daniel Erat, 2017/03/31 04:30:10]

2017

[sammiequon, 2017/03/31 22:52:13]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/dbus/biod/biod_client.h"
+
+#include <map>
+#include <string>
+
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "dbus/mock_bus.h"
+#include "dbus/mock_object_proxy.h"
+#include "dbus/object_path.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::Invoke;
+using ::testing::Return;
+
+namespace chromeos {
+
+namespace {
+
+// Shorthand for a commonly-used constant.
+const char* kInterface = biod::kBiometricsManagerInterface;
+
+void CheckObjectPathCallback(const dbus::ObjectPath& expected_object_path,
+                             const dbus::ObjectPath& object_path) {
+  EXPECT_EQ(expected_object_path, object_path);
+}
+
+void CheckObjectPathArrayCallback(
+    const std::vector<dbus::ObjectPath>& expected_object_paths,
+    const std::vector<dbus::ObjectPath>& object_paths) {
+  CHECK_EQ(expected_object_paths.size(), object_paths.size());

[Daniel Erat, 2017/03/31 04:30:10]

ASSERT_EQ?

[sammiequon, 2017/03/31 22:52:13]

Done.

+  for (size_t i = 0; i < expected_object_paths.size(); ++i)
+    EXPECT_EQ(expected_object_paths[i], object_paths[i]);
+}
+
+// Matcher that verifies that a dbus::Message has member |name|.
+MATCHER_P(HasMember, name, "") {
+  if (arg->GetMember() != name) {
+    *result_listener << "has member " << arg->GetMember();
+    return false;
+  }
+  return true;
+}
+
+// Runs |callback| with |response|. Needed due to ResponseCallback expecting a
+// bare pointer rather than an std::unique_ptr.
+void RunResponseCallback(dbus::ObjectProxy::ResponseCallback callback,
+                         std::unique_ptr<dbus::Response> response) {
+  callback.Run(response.get());
+}
+
+// Implementation of BiodClient::Observer.
+class TestBiodObserver : public BiodClient::Observer {
+ public:
+  TestBiodObserver() {}
+  ~TestBiodObserver() override {}
+
+  int num_enroll_scan_received() const { return num_enroll_scan_received_; }

[Daniel Erat, 2017/03/31 04:30:10]

s/scan/scans/

[sammiequon, 2017/03/31 22:52:13]

Done.

+  int num_auth_scan_received() const { return num_auth_scan_received_; }

[Daniel Erat, 2017/03/31 04:30:10]

s/scan/scans/

[sammiequon, 2017/03/31 22:52:13]

Done.

+  int num_failure_received() const { return num_failure_received_; }

[Daniel Erat, 2017/03/31 04:30:11]

s/failure/failures/

[sammiequon, 2017/03/31 22:52:13]

Done.

+
+  // BiodClient::Observer:
+  void BiodServiceRestarted() override {}
+
+  void BiodEnrollScanDoneReceived(biod::ScanResult scan_result,
+                                  bool enroll_sesion_complete) override {
+    num_enroll_scan_received_++;
+  }
+
+  void BiodAuthScanDoneReceived(biod::ScanResult scan_result,
+                                const AuthScanMatches& matches) override {
+    num_auth_scan_received_++;
+  }
+
+  void BiodSessionFailedReceived() override { num_failure_received_++; }
+
+ private:
+  int num_enroll_scan_received_ = 0;
+  int num_auth_scan_received_ = 0;
+  int num_failure_received_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TestBiodObserver);
+};
+
+}  // namespace
+
+class BiodClientTest : public testing::Test {
+ public:
+  BiodClientTest() {}
+  ~BiodClientTest() override {}
+
+  void SetUp() override {
+    dbus::Bus::Options options;
+    options.bus_type = dbus::Bus::SYSTEM;
+    bus_ = new dbus::MockBus(options);
+
+    proxy_ =
+        new dbus::MockObjectProxy(bus_.get(), biod::kBiodServiceName,
+                                  dbus::ObjectPath(biod::kBiodServicePath));
+
+    // |client_|'s Init() method should request a proxy for communicating with
+    // biometrics api.
+    EXPECT_CALL(*bus_.get(),
+                GetObjectProxy(biod::kBiodServiceName,
+                               dbus::ObjectPath(biod::kBiodServicePath)))
+        .WillRepeatedly(Return(proxy_.get()));
+
+    // Save |client_|'s signal callback.
+    EXPECT_CALL(*proxy_.get(), ConnectToSignal(kInterface, _, _, _))
+        .WillRepeatedly(Invoke(this, &BiodClientTest::ConnectToSignal));
+
+    // |client_|'s Init() method should register methods.
+    EXPECT_CALL(
+        *proxy_.get(),
+        CallMethod(HasMember(biod::kBiometricsManagerStartEnrollSessionMethod),
+                   _, _))
+        .WillRepeatedly(Invoke(this, &BiodClientTest::OnCallMethod));
+    EXPECT_CALL(
+        *proxy_.get(),
+        CallMethod(HasMember(biod::kBiometricsManagerGetRecordsForUserMethod),
+                   _, _))
+        .WillRepeatedly(Invoke(this, &BiodClientTest::OnCallMethod));
+    EXPECT_CALL(
+        *proxy_.get(),
+        CallMethod(HasMember(biod::kBiometricsManagerDestroyAllRecordsMethod),
+                   _, _))
+        .WillRepeatedly(Invoke(this, &BiodClientTest::OnCallMethod));
+    EXPECT_CALL(
+        *proxy_.get(),
+        CallMethod(HasMember(biod::kBiometricsManagerStartAuthSessionMethod), _,
+                   _))
+        .WillRepeatedly(Invoke(this, &BiodClientTest::OnCallMethod));

[Daniel Erat, 2017/03/31 04:30:10]

adding a bunch of expectations like this doesn't improve anything. they all
invoke the same method, so this code will have the same problem that i described
earlier of not being able to e.g. handle multiple method calls.

it would be better to add an expectation within each test listing only the
method that you expect to be called, and to verify that it is called exactly
once.

googlemock doesn't integrate at all with base::Bind(), which makes using it in
chromium extremely painful.

i think your best bet due to the limitations of googlemock may be something like
this:

std::map<std::string, std::unique_ptr<dbus::Response>> pending_method_calls_; 
// private member

void AddMethodExpectation(
    const std::string& method_name,
    std::unique_ptr<dbus::Response> response) {
  ASSERT_FALSE(pending_method_calls_.count(method_name));
  pending_method_calls_[method_name] = std::move(response);
  EXPECT_CALL(
      *proxy_.get(),
      CallMethod(HasMember(method_name), _, _)),
      .WillOnce(Invoke(this, &BiodClientTest::OnCallMethod));
}

void OnCallMethod(...) {
   // assert that the called method is in the map
   // remove the response from the map
   // post the response to the callback
}

then make tests call AddMethodExpectation as needed.

[sammiequon, 2017/03/31 22:52:13]

Done.

+
+    client_.reset(BiodClient::Create(REAL_DBUS_CLIENT_IMPLEMENTATION));
+    client_->Init(bus_.get());
+
+    // Execute callbacks posted by Init().
+    base::RunLoop().RunUntilIdle();
+  }
+
+ protected:
+  // Set the response a method is expected to have when a D-Bus method is
+  // called.
+  void SetExpectedResponse(std::unique_ptr<dbus::Response> response) {
+    expected_response_ = std::move(response);
+  }
+
+  // Synchronously passes |signal| to |client_|'s handler, simulating the signal
+  // from biometrics.
+  void EmitSignal(dbus::Signal* signal) {
+    const std::string signal_name = signal->GetMember();
+    const auto it = signal_callbacks_.find(signal_name);
+    ASSERT_TRUE(it != signal_callbacks_.end())
+        << "Client didn't register for signal " << signal_name;
+    it->second.Run(signal);
+  }
+
+  // Passes a enroll scan done signal to |client_|.
+  void EmitEnrollScanDoneSignal(biod::ScanResult scan_result,
+                                bool enroll_session_complete) {
+    dbus::Signal signal(kInterface,
+                        biod::kBiometricsManagerEnrollScanDoneSignal);
+    dbus::MessageWriter writer(&signal);
+    writer.AppendUint32(uint32_t{scan_result});
+    writer.AppendBool(enroll_session_complete);
+    EmitSignal(&signal);
+  }
+
+  // Passes a auth scan done signal to |client_|.
+  void EmitAuthScanDoneSignal(biod::ScanResult scan_result,
+                              const AuthScanMatches& matches) {
+    dbus::Signal signal(kInterface, biod::kBiometricsManagerAuthScanDoneSignal);
+    dbus::MessageWriter writer(&signal);
+    writer.AppendUint32(uint32_t{scan_result});
+
+    dbus::MessageWriter array_writer(nullptr);
+    writer.OpenArray("{sx}", &array_writer);
+    for (auto& match : matches) {
+      dbus::MessageWriter entry_writer(nullptr);
+      array_writer.OpenDictEntry(&entry_writer);
+      entry_writer.AppendString(match.first);
+      entry_writer.AppendArrayOfStrings(match.second);
+      array_writer.CloseContainer(&entry_writer);
+    }
+    writer.CloseContainer(&array_writer);
+    EmitSignal(&signal);
+  }
+
+  // Passes a scan failed signal to |client_|.
+  void EmitScanFailedSignal() {

[Daniel Erat, 2017/03/31 04:30:11]

i don't see this getting called

[sammiequon, 2017/03/31 22:52:13]

Done.

+    dbus::Signal signal(kInterface,
+                        biod::kBiometricsManagerSessionFailedSignal);
+    EmitSignal(&signal);
+  }
+
+  std::unique_ptr<dbus::Response> expected_response_ = nullptr;
+
+  base::MessageLoop message_loop_;
+
+  // Mock bus and proxy for simulating calls.
+  scoped_refptr<dbus::MockBus> bus_;
+  scoped_refptr<dbus::MockObjectProxy> proxy_;
+
+  std::unique_ptr<BiodClient> client_;
+
+  // Maps from biod signal name to the corresponding callback provided by
+  // |client_|.
+  std::map<std::string, dbus::ObjectProxy::SignalCallback> signal_callbacks_;
+
+ private:
+  // Handles calls to |proxy_|'s ConnectToSignal() method.
+  void ConnectToSignal(
+      const std::string& interface_name,
+      const std::string& signal_name,
+      dbus::ObjectProxy::SignalCallback signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
+    EXPECT_EQ(interface_name, kInterface);
+    signal_callbacks_[signal_name] = signal_callback;
+    message_loop_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(on_connected_callback, interface_name,
+                              signal_name, true /* success */));
+  }
+
+  // Handles calls to |proxy_|'s CallMethod().
+  void OnCallMethod(dbus::MethodCall* method_call,
+                    int timeout_ms,
+                    const dbus::ObjectProxy::ResponseCallback& callback) {
+    message_loop_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(&RunResponseCallback, callback,
+                              base::Passed(&expected_response_)));
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(BiodClientTest);
+};
+
+TEST_F(BiodClientTest, TestStartEnrollSession) {
+  const std::string fake_id("fakeId");

[Daniel Erat, 2017/03/31 04:30:11]

kFakeId, kFakeLabel, etc.

https://google.github.io/styleguide/cppguide.html#Constant_Names

[sammiequon, 2017/03/31 22:52:13]

Done.

+  const std::string fake_label("fakeLabel");
+  const dbus::ObjectPath fake_object_path(std::string("/fake/object/path"));
+
+  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(response.get());
+  writer.AppendObjectPath(fake_object_path);
+
+  // Create a fake response with a fake object path. The start enroll
+  // call should return this object path.
+  SetExpectedResponse(std::move(response));
+  client_->StartEnrollSession(
+      fake_id, fake_label,
+      base::Bind(&CheckObjectPathCallback, fake_object_path));

[Daniel Erat, 2017/03/31 04:30:11]

how about making the callback instead save the path to a passed-in pointer? then
you can put the expectation within the test, and also catch the case where the
callback doesn't get called at all (which i don't think the current code
covers).

[sammiequon, 2017/03/31 22:52:13]

I tried using a unique ptr but I couldn't bind as reference or bind like
&(callbackresult.get()) so I went with the normal pointer.

+  base::RunLoop().RunUntilIdle();
+
+  // Verify that by sending a empty reponse or a improperly formatted one, the
+  // response is an empty object path. Also, logs will get printed.
+  SetExpectedResponse(nullptr);
+  client_->StartEnrollSession(
+      fake_id, fake_label,
+      base::Bind(&CheckObjectPathCallback, dbus::ObjectPath()));
+  base::RunLoop().RunUntilIdle();
+
+  std::unique_ptr<dbus::Response> bad_response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter bad_writer(bad_response.get());
+  bad_writer.AppendString("");
+  SetExpectedResponse(std::move(bad_response));
+  client_->StartEnrollSession(
+      fake_id, fake_label,
+      base::Bind(&CheckObjectPathCallback, dbus::ObjectPath()));
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(BiodClientTest, TestGetRecordsForUser) {
+  const std::string fake_id("fakeId");
+  const dbus::ObjectPath fake_object_path(std::string("/fake/object/path"));
+  const dbus::ObjectPath fake_object_path2(std::string("/fake/object/path2"));
+  const std::vector<dbus::ObjectPath> fake_object_paths = {fake_object_path,
+                                                           fake_object_path2};
+
+  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(response.get());
+  writer.AppendArrayOfObjectPaths(fake_object_paths);
+
+  // Create a fake response with an array of fake object paths. The get
+  // enrollments call should return this array of object paths.
+  SetExpectedResponse(std::move(response));
+  client_->GetRecordsForUser(
+      fake_id, base::Bind(&CheckObjectPathArrayCallback, fake_object_paths));
+  base::RunLoop().RunUntilIdle();
+
+  // Verify that by sending a empty reponse the response is an empty array of
+  // object paths. Also, logs will get printed.
+  SetExpectedResponse(nullptr);
+  client_->GetRecordsForUser(fake_id,
+                             base::Bind(&CheckObjectPathArrayCallback,
+                                        std::vector<dbus::ObjectPath>()));
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(BiodClientTest, TestDestroyAllRecords) {
+  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+
+  SetExpectedResponse(std::move(response));
+  client_->DestroyAllRecords();
+  base::RunLoop().RunUntilIdle();

[Daniel Erat, 2017/03/31 04:30:10]

this isn't verifying anything right now, i don't think.

[sammiequon, 2017/03/31 22:52:13]

Done.

+}
+
+TEST_F(BiodClientTest, TestStartAuthentication) {
+  const dbus::ObjectPath fake_object_path(std::string("/fake/object/path"));
+
+  // Create a fake response with a fake object path. The start authentication
+  // call should return this object path.
+  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(response.get());
+  writer.AppendObjectPath(fake_object_path);
+
+  SetExpectedResponse(std::move(response));
+  client_->StartAuthSession(
+      base::Bind(&CheckObjectPathCallback, fake_object_path));
+  base::RunLoop().RunUntilIdle();
+
+  // Verify that by sending a empty reponse or a improperly formatted one, the
+  // response is an empty object path. Also, logs will get printed.
+  SetExpectedResponse(nullptr);
+  client_->StartAuthSession(
+      base::Bind(&CheckObjectPathCallback, dbus::ObjectPath()));
+  base::RunLoop().RunUntilIdle();
+
+  std::unique_ptr<dbus::Response> bad_response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter bad_writer(bad_response.get());
+  bad_writer.AppendString("");
+  SetExpectedResponse(std::move(bad_response));
+  client_->StartAuthSession(
+      base::Bind(&CheckObjectPathCallback, dbus::ObjectPath()));
+  base::RunLoop().RunUntilIdle();
+}
+
+// Verify when enroll scan done signals are mocked, observer(s) catch the
+// signals as expected.
+TEST_F(BiodClientTest, TestEnrollScanDoneObserver) {
+  TestBiodObserver observer1;
+  TestBiodObserver observer2;
+  client_->AddObserver(&observer1);
+
+  const biod::ScanResult scan_signal = biod::ScanResult::SCAN_RESULT_SUCCESS;
+  const bool enroll_session_complete = false;
+  EmitEnrollScanDoneSignal(scan_signal, enroll_session_complete);
+  EXPECT_EQ(1, observer1.num_enroll_scan_received());
+  EXPECT_EQ(0, observer2.num_enroll_scan_received());
+
+  client_->AddObserver(&observer2);
+  EmitEnrollScanDoneSignal(scan_signal, enroll_session_complete);
+  EXPECT_EQ(2, observer1.num_enroll_scan_received());
+  EXPECT_EQ(1, observer2.num_enroll_scan_received());
+
+  client_->RemoveObserver(&observer1);
+  EmitEnrollScanDoneSignal(scan_signal, enroll_session_complete);
+  EXPECT_EQ(2, observer1.num_enroll_scan_received());
+  EXPECT_EQ(2, observer2.num_enroll_scan_received());
+}
+
+// Verify when auth scan done signals are mocked, observer(s) catch the signals
+// as expected.
+TEST_F(BiodClientTest, TestAuthScanDoneObserver) {
+  TestBiodObserver observer1;
+  TestBiodObserver observer2;
+  client_->AddObserver(&observer1);
+
+  const biod::ScanResult scan_signal = biod::ScanResult::SCAN_RESULT_SUCCESS;
+  const AuthScanMatches test_attempt;
+  EmitAuthScanDoneSignal(scan_signal, test_attempt);
+  EXPECT_EQ(1, observer1.num_auth_scan_received());
+  EXPECT_EQ(0, observer2.num_auth_scan_received());
+
+  client_->AddObserver(&observer2);

[Daniel Erat, 2017/03/31 04:30:11]

this second observer doesn't seem useful. how would the class being tested even
be able to call an observer that isn't registered with it?

i'd combine all of this into a much-shorter single test that just adds a single
observer and verifies that emitting the enroll scan and auth scan done signals
once increment its received count as expected.

[sammiequon, 2017/03/31 22:52:13]

Done.

+  EmitAuthScanDoneSignal(scan_signal, test_attempt);
+  EXPECT_EQ(2, observer1.num_auth_scan_received());
+  EXPECT_EQ(1, observer2.num_auth_scan_received());
+
+  client_->RemoveObserver(&observer1);
+  EmitAuthScanDoneSignal(scan_signal, test_attempt);
+  EXPECT_EQ(2, observer1.num_auth_scan_received());
+  EXPECT_EQ(2, observer2.num_auth_scan_received());
+}
+}  // namespace chromeos