chromebox_for_meeting: Improve tests for CfmBrowserService

This cl fixes minor issues with tests and defines a good way to test
CfM Chromim Services.

Bug:b:175079076
Test:'autoninja -C out/Default/ chrome/test:unit_tests
chromeos:chromeos_unittests && xvfb-run ./out/Default/unit_tests
--gtest_filter=CfmBrowserServiceTest.* &&
./out/Default/chromeos_unittests --dbus-stub
--gtest_filter=CfmHotlineClientTest.*'

Change-Id: I3193e611818af08060f305d37799c9b79d97e2b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2577733
Commit-Queue: Kyle Williams <kdgwill@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Auto-Submit: Kyle Williams <kdgwill@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835548}

chrome/browser/chromeos/BUILD.gn

@@ -4118,8 +4118,7 @@ source_set("unit_tests") {
 
   if (is_cfm) {
     sources += [
-      "chromebox_for_meetings/fake_service_context.cc",
-      "chromebox_for_meetings/fake_service_context.h",
+      "chromebox_for_meetings/browser/cfm_browser_service_unittest.cc",
       "chromebox_for_meetings/service_adaptor_unittest.cc",
     ]
     deps +=

chrome/browser/chromeos/chromebox_for_meetings/browser/cfm_browser_service_unittest.cc

+// Copyright 2020 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 "chrome/browser/chromeos/chromebox_for_meetings/browser/cfm_browser_service.h"
+
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/macros.h"
+#include "base/optional.h"
+#include "base/run_loop.h"
+#include "base/test/bind.h"
+#include "base/test/mock_callback.h"
+#include "base/test/task_environment.h"
+#include "chromeos/dbus/chromebox_for_meetings/fake_cfm_hotline_client.h"
+#include "chromeos/services/chromebox_for_meetings/public/cpp/fake_service_connection.h"
+#include "chromeos/services/chromebox_for_meetings/public/cpp/fake_service_context.h"
+#include "chromeos/services/chromebox_for_meetings/public/cpp/service_connection.h"
+#include "chromeos/services/chromebox_for_meetings/public/mojom/cfm_browser.mojom.h"
+#include "chromeos/services/chromebox_for_meetings/public/mojom/cfm_service_manager.mojom.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/remote.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace chromeos {
+namespace cfm {
+namespace {
+
+class CfmBrowserServiceTest : public testing::Test {
+ public:
+  CfmBrowserServiceTest() = default;
+  CfmBrowserServiceTest(const CfmBrowserServiceTest&) = delete;
+  CfmBrowserServiceTest& operator=(const CfmBrowserServiceTest&) = delete;
+
+  void SetUp() override {
+    CfmHotlineClient::InitializeFake();
+    ServiceConnection::UseFakeServiceConnectionForTesting(
+        &fake_service_connection_);
+    CfmBrowserService::Initialize();
+  }
+
+  void TearDown() override {
+    CfmBrowserService::Shutdown();
+    CfmHotlineClient::Shutdown();
+  }
+
+  FakeCfmHotlineClient* GetClient() {
+    return static_cast<FakeCfmHotlineClient*>(CfmHotlineClient::Get());
+  }
+
+  // Returns a mojo::Remote for the mojom::CfmBrowser by faking the way the cfm
+  // mojom binder daemon would request it through chrome.
+  mojo::Remote<mojom::CfmBrowser> GetBrowserRemote() {
+    base::RunLoop run_loop;
+
+    auto* interface_name = mojom::CfmBrowser::Name_;
+
+    // Fake out CfmServiceContext
+    FakeCfmServiceContext context;
+    mojo::Receiver<mojom::CfmServiceContext> context_receiver(&context);
+    fake_service_connection_.SetCallback(base::BindLambdaForTesting(
+        [&](mojo::PendingReceiver<mojom::CfmServiceContext> pending_receiver,
+            bool success) {
+          ASSERT_TRUE(success);
+          context_receiver.Bind(std::move(pending_receiver));
+        }));
+
+    mojo::Remote<mojom::CfmServiceAdaptor> adaptor_remote;
+    context.SetFakeProvideAdaptorCallback(base::BindLambdaForTesting(
+        [&](const std::string& service_id,
+            mojo::PendingRemote<mojom::CfmServiceAdaptor>
+                adaptor_pending_remote,
+            mojom::CfmServiceContext::ProvideAdaptorCallback callback) {
+          ASSERT_EQ(interface_name, service_id);
+          adaptor_remote.Bind(std::move(adaptor_pending_remote));
+          std::move(callback).Run(true);
+        }));
+
+    EXPECT_TRUE(GetClient()->FakeEmitSignal(interface_name));
+    run_loop.RunUntilIdle();
+
+    EXPECT_TRUE(context_receiver.is_bound());
+    EXPECT_TRUE(adaptor_remote.is_connected());
+
+    mojo::Remote<mojom::CfmBrowser> browser_remote;
+    adaptor_remote->OnBindService(
+        browser_remote.BindNewPipeAndPassReceiver().PassPipe());
+    EXPECT_TRUE(browser_remote.is_connected());
+
+    return browser_remote;
+  }
+
+ protected:
+  base::test::SingleThreadTaskEnvironment task_environment_;
+  FakeServiceConnectionImpl fake_service_connection_;
+};
+
+// This test ensures that the CfmBrowserService is discoverable by its mojom
+// name by sending a signal received by CfmHotlineClient.
+TEST_F(CfmBrowserServiceTest, BrowserServiceAvailable) {
+  ASSERT_TRUE(GetClient()->FakeEmitSignal(mojom::CfmBrowser::Name_));
+}
+
+// This test ensures that the CfmBrowserService correctly registers itself for
+// discovery by the cfm mojom binder daemon and correctly returns a working
+// mojom remote.
+TEST_F(CfmBrowserServiceTest, GetBrowserRemote) {
+  ASSERT_TRUE(GetBrowserRemote().is_connected());
+}
+
+}  // namespace
+}  // namespace cfm
+}  // namespace chromeos

chrome/browser/chromeos/chromebox_for_meetings/service_adaptor_unittest.cc

@@ -14,9 +14,9 @@
 #include "base/test/bind.h"
 #include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
-#include "chrome/browser/chromeos/chromebox_for_meetings/fake_service_context.h"
 #include "chromeos/dbus/chromebox_for_meetings/cfm_hotline_client.h"
 #include "chromeos/services/chromebox_for_meetings/public/cpp/fake_service_connection.h"
+#include "chromeos/services/chromebox_for_meetings/public/cpp/fake_service_context.h"
 #include "chromeos/services/chromebox_for_meetings/public/cpp/service_connection.h"
 #include "chromeos/services/chromebox_for_meetings/public/mojom/cfm_service_manager.mojom.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"

chromeos/dbus/chromebox_for_meetings/cfm_hotline_client_unittest.cc

@@ -12,6 +12,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/callback_forward.h"
 #include "base/files/file.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
@@ -24,6 +25,7 @@
 #include "dbus/object_path.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/cros_system_api/dbus/cfm/dbus-constants.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 using ::testing::_;
@@ -32,6 +34,19 @@ using ::testing::Return;
 
 namespace chromeos {
 
+class FakeCfmObserver : public cfm::CfmObserver {
+ public:
+  FakeCfmObserver() { CfmHotlineClient::Get()->AddObserver(this); }
+  ~FakeCfmObserver() override { CfmHotlineClient::Get()->RemoveObserver(this); }
+
+  bool ServiceRequestReceived(const std::string& service_id) override {
+    request_service_id_ = std::move(service_id);
+    return true;
+  }
+
+  std::string request_service_id_;
+};
+
 class CfmHotlineClientTest : public testing::Test {
  public:
   CfmHotlineClientTest() {
@@ -57,6 +72,11 @@ class CfmHotlineClientTest : public testing::Test {
                                dbus::ObjectPath(::cfm::broker::kServicePath)))
         .WillOnce(Return(mock_proxy_.get()));
 
+    EXPECT_CALL(
+        *mock_proxy_.get(),
+        DoConnectToSignal(::cfm::broker::kServiceInterfaceName, _, _, _))
+        .WillRepeatedly(Invoke(this, &CfmHotlineClientTest::ConnectToSignal));
+
     CfmHotlineClient::Initialize(mock_bus_.get());
     client_ = CfmHotlineClient::Get();
 
@@ -85,7 +105,35 @@ class CfmHotlineClientTest : public testing::Test {
         FROM_HERE, base::BindOnce(std::move(*callback), response));
   }
 
+  void EmitServiceRequestedSignal(const std::string& interface_name) {
+    dbus::Signal signal(::cfm::broker::kServiceInterfaceName,
+                        ::cfm::broker::kMojoServiceRequestedSignal);
+    dbus::MessageWriter writer(&signal);
+    writer.AppendString(interface_name);
+
+    // simulate 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);
+  }
+
  protected:
+  // 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, ::cfm::broker::kServiceInterfaceName);
+    signal_callbacks_[signal_name] = signal_callback;
+    task_environment_.GetMainThreadTaskRunner()->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(*on_connected_callback), interface_name,
+                       signal_name, true /* success */));
+  }
+
   base::test::SingleThreadTaskEnvironment task_environment_;
   CfmHotlineClient* client_;
   scoped_refptr<dbus::MockBus> mock_bus_;
@@ -96,6 +144,9 @@ class CfmHotlineClientTest : public testing::Test {
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
   std::deque<std::unique_ptr<dbus::Response>> used_responses_;
+  // Maps from biod signal name to the corresponding callback provided by
+  // |client_|.
+  std::map<std::string, dbus::ObjectProxy::SignalCallback> signal_callbacks_;
 };
 
 TEST_F(CfmHotlineClientTest, BootstrapMojoSuccessTest) {
@@ -129,4 +180,15 @@ TEST_F(CfmHotlineClientTest, BootstrapMojoFailureTest) {
   base::RunLoop().RunUntilIdle();
 }
 
+TEST_F(CfmHotlineClientTest, EmitMojoServiceRequestedSignal) {
+  base::RunLoop run_loop;
+  const std::string interface_name = "Foo";
+
+  FakeCfmObserver observer;
+  EmitServiceRequestedSignal(interface_name);
+
+  run_loop.RunUntilIdle();
+  EXPECT_EQ(observer.request_service_id_, interface_name);
+}
+
 }  // namespace chromeos

chromeos/dbus/chromebox_for_meetings/fake_cfm_hotline_client.cc

@@ -36,4 +36,13 @@ void FakeCfmHotlineClient::RemoveObserver(cfm::CfmObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+bool FakeCfmHotlineClient::FakeEmitSignal(const std::string& interface_name) {
+  for (auto& observer : observer_list_) {
+    if (observer.ServiceRequestReceived(interface_name)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace chromeos

chromeos/dbus/chromebox_for_meetings/fake_cfm_hotline_client.h

@@ -14,7 +14,8 @@
 namespace chromeos {
 
 // Fake implementation of CfmHotlineClient. This is currently a no-op fake.
-class FakeCfmHotlineClient : public CfmHotlineClient {
+class COMPONENT_EXPORT(CFM_HOTLINE_CLIENT) FakeCfmHotlineClient
+    : public CfmHotlineClient {
  public:
   FakeCfmHotlineClient();
   FakeCfmHotlineClient(const FakeCfmHotlineClient&) = delete;
@@ -30,6 +31,9 @@ class FakeCfmHotlineClient : public CfmHotlineClient {
   void AddObserver(cfm::CfmObserver* observer) override;
   void RemoveObserver(cfm::CfmObserver* observer) override;
 
+  // Fake a ::cfm::broker::kMojoServiceRequestedSignal signal event
+  bool FakeEmitSignal(const std::string& interface_name);
+
  private:
   // A list of observers that are listening on state changes, etc.
   cfm::CfmObserverList observer_list_;

chromeos/services/chromebox_for_meetings/public/cpp/BUILD.gn

@@ -30,6 +30,8 @@ source_set("test_support") {
   sources = [
     "fake_service_connection.cc",
     "fake_service_connection.h",
+    "fake_service_context.cc",
+    "fake_service_context.h",
   ]
   deps = [
     ":cpp",

chrome/browser/chromeos/chromebox_for_meetings/fake_service_context.cc → chromeos/services/chromebox_for_meetings/public/cpp/fake_service_context.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/chromeos/chromebox_for_meetings/fake_service_context.h"
+#include "chromeos/services/chromebox_for_meetings/public/cpp/fake_service_context.h"
 
 namespace chromeos {
 namespace cfm {

chrome/browser/chromeos/chromebox_for_meetings/fake_service_context.h → chromeos/services/chromebox_for_meetings/public/cpp/fake_service_context.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_CHROMEOS_CHROMEBOX_FOR_MEETINGS_FAKE_SERVICE_CONTEXT_H_
-#define CHROME_BROWSER_CHROMEOS_CHROMEBOX_FOR_MEETINGS_FAKE_SERVICE_CONTEXT_H_
+#ifndef CHROMEOS_SERVICES_CHROMEBOX_FOR_MEETINGS_PUBLIC_CPP_FAKE_SERVICE_CONTEXT_H_
+#define CHROMEOS_SERVICES_CHROMEBOX_FOR_MEETINGS_PUBLIC_CPP_FAKE_SERVICE_CONTEXT_H_
 
 #include "base/bind.h"
 #include "mojo/public/cpp/bindings/receiver.h"
@@ -53,4 +53,4 @@ class FakeCfmServiceContext : public mojom::CfmServiceContext {
 }  // namespace cfm
 }  // namespace chromeos
 
-#endif  // CHROME_BROWSER_CHROMEOS_CHROMEBOX_FOR_MEETINGS_FAKE_SERVICE_CONTEXT_H_
+#endif  // CHROMEOS_SERVICES_CHROMEBOX_FOR_MEETINGS_PUBLIC_CPP_FAKE_SERVICE_CONTEXT_H_

chromeos/services/chromebox_for_meetings/public/cpp/service_connection.cc

@@ -44,7 +44,6 @@ class ServiceConnectionImpl : public ServiceConnection,
   // Response callback for CfmHotlineClient::BootstrapMojoConnection.
   void OnBootstrapMojoConnectionResponse(
       mojo::PendingReceiver<mojom::CfmServiceContext> receiver,
-      mojo::PlatformChannel channel,
       mojo::ScopedMessagePipeHandle context_remote_pipe,
       bool success);
 
@@ -121,12 +120,11 @@ void ServiceConnectionImpl::CfMContextServiceStarted(
       channel.TakeRemoteEndpoint().TakePlatformHandle().TakeFD(),
       base::BindOnce(&ServiceConnectionImpl::OnBootstrapMojoConnectionResponse,
                      weak_factory_.GetWeakPtr(), std::move(receiver),
-                     std::move(channel), std::move(pipe)));
+                     std::move(pipe)));
 }
 
 void ServiceConnectionImpl::OnBootstrapMojoConnectionResponse(
     mojo::PendingReceiver<mojom::CfmServiceContext> receiver,
-    mojo::PlatformChannel channel,
     mojo::ScopedMessagePipeHandle context_remote_pipe,
     bool success) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

chromeos/services/chromebox_for_meetings/public/cpp/service_connection_unittest.cc

@@ -40,25 +40,26 @@ class CfmServiceConnectionTest : public testing::Test {
   void SetUp() override {
     CfmHotlineClient::InitializeFake();
     ServiceConnection::UseFakeServiceConnectionForTesting(
-        &fake_service_connection);
+        &fake_service_connection_);
   }
 
   void TearDown() override { CfmHotlineClient::Shutdown(); }
 
-  void SetCallback(FakeServiceConnectionImpl::FakeBootstrapCallback callback) {
-    fake_service_connection.SetCallback(std::move(callback));
+  void SetBootstrapCallback(
+      FakeServiceConnectionImpl::FakeBootstrapCallback callback) {
+    fake_service_connection_.SetCallback(std::move(callback));
   }
 
  private:
   base::test::TaskEnvironment task_environment_;
-  FakeServiceConnectionImpl fake_service_connection;
+  FakeServiceConnectionImpl fake_service_connection_;
 };
 
-TEST_F(CfmServiceConnectionTest, BindServiceContext) {
+TEST_F(CfmServiceConnectionTest, FakeBindServiceContext) {
   base::RunLoop run_loop;
 
   bool test_success = false;
-  SetCallback(base::BindLambdaForTesting(
+  SetBootstrapCallback(base::BindLambdaForTesting(
       [&](mojo::PendingReceiver<mojom::CfmServiceContext>, bool success) {
         test_success = success;
         run_loop.QuitClosure().Run();