cros: DBUS client to interact with fingerprint DBUS API.

chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc

Index: chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc
diff --git a/chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc b/chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c6204076288a33227145774bbb6e220948bfb274
--- /dev/null
+++ b/chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc
@@ -0,0 +1,379 @@
+// Copyright 2016 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 "chromeos/dbus/biod/biod_biometrics_manager_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) {
+  EXPECT_EQ(expected_object_paths.size(), object_paths.size());

[Daniel Erat, 2017/03/28 00:14:35]

you need ASSERT_EQ or CHECK_EQ here. otherwise you'll segfault if the sizes are
different.

[sammiequon, 2017/03/28 01:31:06]

Done.

+  for (size_t i = 0; i < expected_object_paths.size(); ++i)
+    EXPECT_EQ(expected_object_paths[i], object_paths[i]);
+}
+
+// Implementation of BiodBiometricsManagerClient::Observer.
+class TestBiometricsObserver : public BiodBiometricsManagerClient::Observer {
+ public:
+  TestBiometricsObserver() {}
+  ~TestBiometricsObserver() override {}
+
+  int num_enroll_scan_received() const { return num_enroll_scan_received_; }
+  int num_auth_scan_received() const { return num_auth_scan_received_; }
+  int num_failure_received() const { return num_failure_received_; }
+
+  // BiodBiometricsManagerClient::Observer:
+  void BiodServiceRestarted() override {}
+
+  void BiodEnrollScanDoneReceived(biod::ScanResult scan_result,
+                                  bool is_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(TestBiometricsObserver);
+};
+
+}  // namespace
+
+class BiodBiometricsManagerClientTest : public testing::Test {
+ public:
+  BiodBiometricsManagerClientTest() {}
+  ~BiodBiometricsManagerClientTest() 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, &BiodBiometricsManagerClientTest::ConnectToSignal));
+
+    // |client_|'s Init() method should register methods.
+    EXPECT_CALL(*proxy_.get(), CallMethod(_, _, _))
+        .WillRepeatedly(
+            Invoke(this, &BiodBiometricsManagerClientTest::OnCallMethod));
+
+    client_.reset(
+        BiodBiometricsManagerClient::Create(REAL_DBUS_CLIENT_IMPLEMENTATION));
+    client_->Init(bus_.get());
+
+    // Execute callbacks posted by Init().
+    base::RunLoop().RunUntilIdle();
+  }
+
+ protected:
+  // Set the values a method is expected to have when a dbus method is called.
+  void SetExpectedMethodArguments(const std::string& expected_interface,
+                                  const std::string& expected_name,
+                                  dbus::Response* response) {

[Daniel Erat, 2017/03/28 00:14:35]

document memory ownership/lifetime when passing raw pointers (here and in
EmitSignal). for example, i think that ownership of |response| isn't passed and
that the pointed-to response must remain valid until OnCallMethod() is called.

[sammiequon, 2017/03/28 01:31:05]

I changed this to use smart pointer is that ok? If so, done.

+    expected_interface_ = expected_interface;
+    expected_name_ = expected_name;
+    expected_response_ = 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 is_complete) {
+    dbus::Signal signal(kInterface,
+                        biod::kBiometricsManagerEnrollScanDoneSignal);
+    dbus::MessageWriter writer(&signal);
+    writer.AppendUint32(uint32_t{scan_result});
+    writer.AppendBool(is_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(NULL);

[Daniel Erat, 2017/03/28 00:14:35]

s/NULL/nullptr/ throughout this file

[sammiequon, 2017/03/28 01:31:05]

Done.

+    writer.OpenArray("{sx}", &array_writer);
+    for (auto& match : matches) {
+      dbus::MessageWriter entry_writer(NULL);
+      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() {
+    dbus::Signal signal(kInterface,
+                        biod::kBiometricsManagerSessionFailedSignal);
+    EmitSignal(&signal);
+  }
+
+  std::string expected_interface_;
+  std::string expected_name_;
+  dbus::Response* expected_response_;

[Daniel Erat, 2017/03/28 00:14:35]

this memory looks like it's currently left uninitialized until the first call to
SetExpectedMethodArguments(). please initialize it to nullptr here.

[sammiequon, 2017/03/28 01:31:05]

Done.

+
+  base::MessageLoop message_loop_;
+
+  // Mock bus and proxy for simulating calls.
+  scoped_refptr<dbus::MockBus> bus_;
+  scoped_refptr<dbus::MockObjectProxy> proxy_;
+
+  std::unique_ptr<BiodBiometricsManagerClient> client_;
+
+  // Maps from powerd signal name to the corresponding callback provided by

[Daniel Erat, 2017/03/28 00:14:35]

i see where this code came from. :-P please fix the comment.

[sammiequon, 2017/03/28 01:31:05]

Oops. Done.

+  // |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) {
+    EXPECT_EQ(expected_interface_, method_call->GetInterface());
+    EXPECT_EQ(expected_name_, method_call->GetMember());

[Daniel Erat, 2017/03/28 00:14:35]

this is fragile and will create a lot of future work for someone if a method is
added later that needs to call multiple d-bus methods in the future.

please add expectations for the proxy calls that you expect using EXPECT_CALL
instead. you can see examples of doing this in e.g.
power_manager_client_unittest.cc.

[sammiequon, 2017/03/28 01:31:05]

You mean consolidating all of this the d-bus calls testing into this one
OnCallMethod (with the SetExpectedStuff). Do you wish for me to add seperate
methods for each call i.e OnStartEnrollSessionMethod?

[Daniel Erat, 2017/03/28 01:44:30]

i mean setting expectations similar to this (from the PowerManagerClient tests):

    EXPECT_CALL(
        *proxy_.get(),
        CallMethod(HasMember(power_manager::kRegisterSuspendDelayMethod), _, _))
        .WillRepeatedly(
            Invoke(this, &PowerManagerClientTest::RegisterSuspendDelay));

that is, let googlemock do the matching. this will scale to calling multiple
methods, verifying that a particular method is called exactly once, etc.

[sammiequon, 2017/03/28 03:17:54]

I gave it a shot.

+
+    message_loop_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(callback, expected_response_));
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(BiodBiometricsManagerClientTest);
+};
+
+TEST_F(BiodBiometricsManagerClientTest, TestStartEnrollSession) {
+  const std::string& fake_id("fakeId");

[Daniel Erat, 2017/03/28 00:14:35]

uh, const reference? string constants should be:

const char kFakeId = "fakeId";

please update the rest of the file.

[sammiequon, 2017/03/28 01:31:05]

I've believed I've seen this notation used in many tests (minus the reference).
Just skimming the dbus unittests it seems to be half/half.

[Daniel Erat, 2017/03/28 01:44:30]

yes, a const std::string is okay; it's the reference that's strange (since
there's no existing variable that the reference points towards, although i
believe that the c++ spec says that a temporary is created and persists), along
with not using kFakeId naming.

[sammiequon, 2017/03/28 03:17:54]

So this means i should use the kFoo naming instead of the underscore seperated
naming here? Because I saw both instances in other tests as well, but I'm not
strongly opposed to either.

+  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.
+  SetExpectedMethodArguments(kInterface,
+                             biod::kBiometricsManagerStartEnrollSessionMethod,
+                             response.get());
+  client_->StartEnrollSession(
+      fake_id, fake_label,
+      base::Bind(&CheckObjectPathCallback, fake_object_path));
+
+  // Verify that by sending a empty reponse or a improperly formatted one, the
+  // response is an empty object path. Also, logs will get printed.
+  SetExpectedMethodArguments(
+      kInterface, biod::kBiometricsManagerStartEnrollSessionMethod, nullptr);
+  client_->StartEnrollSession(
+      fake_id, fake_label,
+      base::Bind(&CheckObjectPathCallback, dbus::ObjectPath()));
+
+  std::unique_ptr<dbus::Response> bad_response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter bad_writer(bad_response.get());
+  bad_writer.AppendString("");
+  SetExpectedMethodArguments(kInterface,
+                             biod::kBiometricsManagerStartEnrollSessionMethod,
+                             bad_response.get());
+  client_->StartEnrollSession(
+      fake_id, fake_label,
+      base::Bind(&CheckObjectPathCallback, dbus::ObjectPath()));
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(BiodBiometricsManagerClientTest, 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 fake object paths. The get enrollments

[Daniel Erat, 2017/03/28 00:14:35]

nit: "array of"

[sammiequon, 2017/03/28 01:31:05]

Done.

+  // call should return this array of object paths.
+  SetExpectedMethodArguments(kInterface,
+                             biod::kBiometricsManagerGetRecordsForUserMethod,
+                             response.get());
+  client_->GetRecordsForUser(
+      fake_id, base::Bind(&CheckObjectPathArrayCallback, fake_object_paths));
+
+  // Verify that by sending a empty reponse the response is an empty array of
+  // object paths. Also, logs will get printed.
+  SetExpectedMethodArguments(
+      kInterface, biod::kBiometricsManagerGetRecordsForUserMethod, nullptr);
+  client_->GetRecordsForUser(fake_id,
+                             base::Bind(&CheckObjectPathArrayCallback,
+                                        std::vector<dbus::ObjectPath>()));
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(BiodBiometricsManagerClientTest, TestDestroyAllRecords) {
+  std::unique_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+
+  SetExpectedMethodArguments(kInterface,
+                             biod::kBiometricsManagerDestroyAllRecordsMethod,
+                             response.get());
+  client_->DestroyAllRecords();
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(BiodBiometricsManagerClientTest, 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);
+
+  SetExpectedMethodArguments(kInterface,
+                             biod::kBiometricsManagerStartAuthSessionMethod,
+                             response.get());
+  client_->StartAuthSession(
+      base::Bind(&CheckObjectPathCallback, fake_object_path));
+
+  // Verify that by sending a empty reponse or a improperly formatted one, the
+  // response is an empty object path. Also, logs will get printed.
+  SetExpectedMethodArguments(
+      kInterface, biod::kBiometricsManagerStartAuthSessionMethod, nullptr);
+  client_->StartAuthSession(
+      base::Bind(&CheckObjectPathCallback, dbus::ObjectPath()));
+
+  std::unique_ptr<dbus::Response> bad_response(dbus::Response::CreateEmpty());
+  dbus::MessageWriter bad_writer(bad_response.get());
+  bad_writer.AppendString("");
+  SetExpectedMethodArguments(kInterface,
+                             biod::kBiometricsManagerStartAuthSessionMethod,
+                             bad_response.get());
+  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(BiodBiometricsManagerClientTest, TestEnrollScanDoneObserver) {
+  TestBiometricsObserver observer1;
+  TestBiometricsObserver observer2;
+  client_->AddObserver(&observer1);
+
+  const biod::ScanResult scan_signal = biod::ScanResult::SCAN_RESULT_SUCCESS;
+  const bool is_complete = false;
+  EmitEnrollScanDoneSignal(scan_signal, is_complete);
+  EXPECT_EQ(1, observer1.num_enroll_scan_received());
+  EXPECT_EQ(0, observer2.num_enroll_scan_received());
+
+  client_->AddObserver(&observer2);
+  EmitEnrollScanDoneSignal(scan_signal, is_complete);
+  EXPECT_EQ(2, observer1.num_enroll_scan_received());
+  EXPECT_EQ(1, observer2.num_enroll_scan_received());
+
+  client_->RemoveObserver(&observer1);
+  EmitEnrollScanDoneSignal(scan_signal, is_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(BiodBiometricsManagerClientTest, TestAuthScanDoneObserver) {
+  TestBiometricsObserver observer1;
+  TestBiometricsObserver 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);
+  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