Move ws and ime services off ServiceTest framework

This migrates ws and related IME service tests away from the deprecated
ServiceTest framework in favor of regular unit tests using newer
Service Manager test helpers.

Since this is the final set of tests which was using ServiceTest in
services_unittests, the target is also replaced with a plain old gtest
suite and the global catalog configuration junk is removed.

Bug: 906239
Change-Id: Ic85c7be164fde2b6a9883d0af547600c71f446b5
Reviewed-on: https://chromium-review.googlesource.com/c/1352695
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612187}

ash/shell/content/client/shell_main_delegate.cc

@@ -44,8 +44,9 @@ std::unique_ptr<service_manager::Service> CreateTapVisualizer() {
   return std::make_unique<tap_visualizer::TapVisualizerApp>();
 }
 
-std::unique_ptr<service_manager::Service> CreateTestImeDriver() {
-  return std::make_unique<ws::test::TestIMEApplication>();
+std::unique_ptr<service_manager::Service> CreateTestImeDriver(
+    service_manager::mojom::ServiceRequest request) {
+  return std::make_unique<ws::test::TestIMEApplication>(std::move(request));
 }
 
 class ShellContentUtilityClient : public content::ContentUtilityClient {
@@ -65,11 +66,6 @@ class ShellContentUtilityClient : public content::ContentUtilityClient {
       info.factory = base::BindRepeating(&CreateTapVisualizer);
       (*services)[tap_visualizer::mojom::kServiceName] = info;
     }
-    {
-      service_manager::EmbeddedServiceInfo info;
-      info.factory = base::BindRepeating(&CreateTestImeDriver);
-      (*services)[test_ime_driver::mojom::kServiceName] = info;
-    }
   }
 
   std::unique_ptr<service_manager::Service> HandleServiceRequest(
@@ -77,6 +73,8 @@ class ShellContentUtilityClient : public content::ContentUtilityClient {
       service_manager::mojom::ServiceRequest request) override {
     if (service_name == quick_launch::mojom::kServiceName)
       return CreateQuickLaunch(std::move(request));
+    if (service_name == test_ime_driver::mojom::kServiceName)
+      return CreateTestImeDriver(std::move(request));
 
     return nullptr;
   }

services/BUILD.gn

@@ -3,8 +3,6 @@
 # found in the LICENSE file.
 
 import("//build/config/ui.gni")
-import("//services/catalog/public/tools/catalog.gni")
-import("//services/service_manager/public/tools/test/service_test.gni")
 import("//testing/test.gni")
 import("//tools/v8_context_snapshot/v8_context_snapshot.gni")
 
@@ -12,18 +10,8 @@ import("//tools/v8_context_snapshot/v8_context_snapshot.gni")
 # to avoid having to maintain a separate test binary for every service.
 #
 # To add tests for a new service, please define a "tests" source_set in the
-# service subdirectory and add it as a dependency here. If your unit tests
-# use the ServiceTest framework, you must also include corresponding catalog
-# entries in the "services_unittests_catalog" target below.
-#
-# TODO(https://crbug.com/906239): Move all platforms off of service_test.
-if (is_ios) {
-  services_unittests_target_type = "test"
-} else {
-  services_unittests_target_type = "service_test"
-}
-
-target(services_unittests_target_type, "services_unittests") {
+# service subdirectory and add it as a dependency here.
+test("services_unittests") {
   # If your service does not run on iOS, its tests should go in the !iOS
   # section below. If you are unsure, contact blundell@chromium.org.
   deps = [
@@ -32,6 +20,7 @@ target(services_unittests_target_type, "services_unittests") {
     "//services/metrics/public/cpp:tests",
     "//services/network:tests",
     "//services/network/public/cpp:tests",
+    "//services/test:run_all_unittests",
   ]
 
   if (!is_ios) {
@@ -85,38 +74,14 @@ target(services_unittests_target_type, "services_unittests") {
     # standalone services must be added here.
     deps += [ "//services/video_capture:tests" ]
   }
-
-  if (is_ios) {
-    deps += [ "//mojo/core/test:run_all_unittests" ]
-  } else {
-    test_runner = "//services/test:run_all_service_tests"
-    catalog = ":services_unittests_catalog"
-  }
 }
 
 if (!is_ios) {
-  catalog("services_unittests_catalog") {
-    testonly = true
-
-    catalog_deps = [ "//services/viz:tests_catalog" ]
-
-    if (use_aura) {
-      catalog_deps += [
-        "//services/ws/ime:tests_catalog",
-        "//services/ws:tests_catalog",
-      ]
-    }
-  }
-
-  service_test("services_perftests") {
+  test("services_perftests") {
     deps = [
+      "//services/test:run_all_unittests",
       "//services/viz/public/cpp/compositing:perftests",
     ]
-    catalog = ":services_perftests_catalog"
-  }
-
-  catalog("services_perftests_catalog") {
-    testonly = true
   }
 }
 

services/test/BUILD.gn

@@ -4,7 +4,7 @@
 
 import("//build/config/ui.gni")
 
-source_set("run_all_service_tests") {
+source_set("run_all_unittests") {
   # This target is in a separate directory because we want to expand DEPS so
   # that it not all of //services can access //ui.
   visibility = [ "//services:*" ]
@@ -12,13 +12,13 @@ source_set("run_all_service_tests") {
   testonly = true
 
   sources = [
-    "run_all_service_tests.cc",
+    "run_all_unittests.cc",
   ]
 
   deps = [
     "//base",
     "//base/test:test_support",
-    "//services/service_manager/public/cpp/test:common_initialization",
+    "//mojo/core/embedder",
     "//ui/base",
   ]
 
@@ -41,7 +41,7 @@ source_set("run_all_service_tests") {
 }
 
 bundle_data("tests_bundle_data") {
-  visibility = [ ":run_all_service_tests" ]
+  visibility = [ ":run_all_unittests" ]
   testonly = true
   sources = [
     "//services/test/data/content-sniffer-test0.html",

services/test/run_all_service_tests.cc → services/test/run_all_unittests.cc

-// Copyright 2017 The Chromium Authors. All rights reserved.
+// Copyright 2018 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.
 
@@ -6,15 +6,26 @@
 #include "base/files/file.h"
 #include "base/i18n/icu_util.h"
 #include "base/macros.h"
+#include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/test/launcher/unit_test_launcher.h"
 #include "base/test/test_suite.h"
+#include "base/threading/thread.h"
 #include "build/build_config.h"
-#include "services/service_manager/public/cpp/test/common_initialization.h"
+#include "mojo/core/embedder/embedder.h"
+#include "mojo/core/embedder/scoped_ipc_support.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/resource/scale_factor.h"
 #include "ui/base/ui_base_paths.h"
 
+#if defined(OS_ANDROID)
+#include "base/android/jni_android.h"
+#endif
+
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+#include "mojo/core/embedder/default_mach_broker.h"
+#endif
+
 namespace {
 
 class ServiceTestSuite : public base::TestSuite {
@@ -25,6 +36,8 @@ class ServiceTestSuite : public base::TestSuite {
  protected:
   void Initialize() override {
     base::TestSuite::Initialize();
+
+#if !defined(OS_IOS)
     ui::RegisterPathProvider();
 
     base::FilePath ui_test_pak_path;
@@ -41,13 +54,17 @@ class ServiceTestSuite : public base::TestSuite {
         path.Append(FILE_PATH_LITERAL("bluetooth_test_strings.pak"));
     ui::ResourceBundle::GetSharedInstance().AddDataPackFromPath(
         bluetooth_test_strings, ui::SCALE_FACTOR_NONE);
+#endif  // !defined(OS_IOS)
 
     // base::TestSuite and ViewsInit both try to load icu. That's ok for tests.
     base::i18n::AllowMultipleInitializeCallsForTesting();
   }
 
   void Shutdown() override {
+#if !defined(OS_IOS)
     ui::ResourceBundle::CleanupSharedInstance();
+#endif
+
     base::TestSuite::Shutdown();
   }
 
@@ -60,7 +77,21 @@ class ServiceTestSuite : public base::TestSuite {
 int main(int argc, char** argv) {
   ServiceTestSuite test_suite(argc, argv);
 
-  return service_manager::InitializeAndLaunchUnitTests(
+  mojo::core::Init();
+
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  mojo::core::SetMachPortProvider(
+      mojo::core::DefaultMachBroker::Get()->port_provider());
+#endif
+
+  base::Thread ipc_thread("IPC thread");
+  ipc_thread.StartWithOptions(
+      base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
+  mojo::core::ScopedIPCSupport ipc_support(
+      ipc_thread.task_runner(),
+      mojo::core::ScopedIPCSupport::ShutdownPolicy::CLEAN);
+
+  return base::LaunchUnitTests(
       argc, argv,
-      base::Bind(&ServiceTestSuite::Run, base::Unretained(&test_suite)));
+      base::BindOnce(&ServiceTestSuite::Run, base::Unretained(&test_suite)));
 }

services/ws/BUILD.gn

@@ -4,11 +4,11 @@
 
 import("//build/config/ui.gni")
 import("//mojo/public/tools/bindings/mojom.gni")
-import("//testing/test.gni")
 import("//services/catalog/public/tools/catalog.gni")
 import("//services/service_manager/public/cpp/service.gni")
 import("//services/service_manager/public/service_manifest.gni")
 import("//services/service_manager/public/tools/test/service_test.gni")
+import("//testing/test.gni")
 
 component("lib") {
   friend = [
@@ -158,12 +158,13 @@ static_library("test_support") {
 
   deps = [
     ":lib",
+    ":tests_catalog_source",
     "//base",
     "//base/test:test_support",
     "//components/viz/test:test_support",
     "//mojo/public/cpp/bindings:bindings",
     "//services/service_manager/public/cpp",
-    "//services/service_manager/public/cpp:service_test_support",
+    "//services/service_manager/public/cpp/test:test_support",
     "//services/ws/common",
     "//services/ws/public/cpp",
     "//services/ws/public/cpp/host",
@@ -196,6 +197,12 @@ catalog("tests_catalog") {
   standalone_services = [ "//services/ws/test_ws:manifest" ]
 }
 
+catalog_cpp_source("tests_catalog_source") {
+  testonly = true
+  catalog = ":tests_catalog"
+  generated_function_name = "ws::test::CreateTestCatalog"
+}
+
 source_set("tests") {
   testonly = true
 

services/ws/ime/BUILD.gn

@@ -5,7 +5,6 @@
 import("//services/catalog/public/tools/catalog.gni")
 import("//services/service_manager/public/cpp/service.gni")
 import("//services/service_manager/public/service_manifest.gni")
-import("//services/service_manager/public/tools/test/service_test.gni")
 import("//testing/test.gni")
 
 source_set("lib") {
@@ -32,11 +31,14 @@ source_set("tests") {
   ]
 
   deps = [
+    ":tests_catalog_source",
     "//base",
+    "//base/test:test_support",
     "//services/service_manager/public/cpp",
-    "//services/service_manager/public/cpp:service_test_support",
+    "//services/service_manager/public/cpp/test:test_support",
     "//services/ws/ime/test_ime_driver/public/mojom",
     "//services/ws/public/mojom",
+    "//testing/gtest",
   ]
 
   data_deps = [
@@ -53,5 +55,14 @@ service_manifest("unittest_manifest") {
 catalog("tests_catalog") {
   testonly = true
   embedded_services = [ ":unittest_manifest" ]
-  standalone_services = [ "//services/ws/ime/test_ime_driver:manifest" ]
+  standalone_services = [
+    "//services/ws/ime/test_ime_driver:manifest",
+    "//services/ws/test_ws:manifest",
+  ]
+}
+
+catalog_cpp_source("tests_catalog_source") {
+  testonly = true
+  catalog = ":tests_catalog"
+  generated_function_name = "ws::test::CreateImeTestCatalog"
 }

services/ws/ime/ime_unittest.cc

@@ -7,12 +7,16 @@
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
-#include "services/service_manager/public/cpp/service_context.h"
-#include "services/service_manager/public/cpp/service_test.h"
+#include "services/service_manager/public/cpp/service.h"
+#include "services/service_manager/public/cpp/service_binding.h"
+#include "services/service_manager/public/cpp/test/test_service_manager.h"
 #include "services/ws/ime/test_ime_driver/public/mojom/constants.mojom.h"
+#include "services/ws/ime/tests_catalog_source.h"
 #include "services/ws/public/mojom/constants.mojom.h"
 #include "services/ws/public/mojom/ime/ime.mojom.h"
+#include "testing/gtest/include/gtest/gtest.h"
 #include "ui/events/event.h"
 #include "ui/events/keycodes/dom/dom_code.h"
 
@@ -54,14 +58,13 @@ class TestTextInputClient : public ws::mojom::TextInputClient {
   DISALLOW_COPY_AND_ASSIGN(TestTextInputClient);
 };
 
-class IMEAppTest : public service_manager::test::ServiceTest {
+class IMEAppTest : public testing::Test {
  public:
-  IMEAppTest() : ServiceTest("ime_unittests") {}
-  ~IMEAppTest() override {}
-
-  // service_manager::test::ServiceTest:
-  void SetUp() override {
-    ServiceTest::SetUp();
+  IMEAppTest()
+      : test_service_manager_(ws::test::CreateImeTestCatalog()),
+        test_service_binding_(
+            &test_service_,
+            test_service_manager_.RegisterTestInstance("ime_unittests")) {
     // test_ime_driver will register itself as the current IMEDriver.
     // TODO(https://crbug.com/904148): This should not use |WarmService()|.
     connector()->WarmService(service_manager::ServiceFilter::ByName(
@@ -69,6 +72,12 @@ class IMEAppTest : public service_manager::test::ServiceTest {
     connector()->BindInterface(ws::mojom::kServiceName, &ime_driver_);
   }
 
+  ~IMEAppTest() override {}
+
+  service_manager::Connector* connector() {
+    return test_service_binding_.GetConnector();
+  }
+
   bool ProcessKeyEvent(ws::mojom::InputMethodPtr* input_method,
                        std::unique_ptr<ui::Event> event) {
     (*input_method)
@@ -89,6 +98,11 @@ class IMEAppTest : public service_manager::test::ServiceTest {
     run_loop_->Quit();
   }
 
+  base::test::ScopedTaskEnvironment task_environment_;
+  service_manager::TestServiceManager test_service_manager_;
+  service_manager::Service test_service_;
+  service_manager::ServiceBinding test_service_binding_;
+
   ws::mojom::IMEDriverPtr ime_driver_;
   std::unique_ptr<base::RunLoop> run_loop_;
   bool handled_;

services/ws/ime/test_ime_driver/main.cc

@@ -3,10 +3,18 @@
 // found in the LICENSE file.
 
 #include "services/service_manager/public/c/main.h"
-#include "services/service_manager/public/cpp/service_runner.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "mojo/public/cpp/system/message_pipe.h"
+#include "services/service_manager/public/mojom/service.mojom.h"
 #include "services/ws/ime/test_ime_driver/test_ime_application.h"
 
 MojoResult ServiceMain(MojoHandle service_request_handle) {
-  service_manager::ServiceRunner runner(new ws::test::TestIMEApplication);
-  return runner.Run(service_request_handle);
+  base::MessageLoop message_loop;
+  base::RunLoop run_loop;
+  ws::test::TestIMEApplication app{service_manager::mojom::ServiceRequest(
+      mojo::MakeScopedHandle(mojo::MessagePipeHandle(service_request_handle)))};
+  app.set_termination_closure(run_loop.QuitClosure());
+  run_loop.Run();
+  return MOJO_RESULT_OK;
 }

services/ws/ime/test_ime_driver/test_ime_application.cc

@@ -6,7 +6,6 @@
 
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/service_manager/public/cpp/connector.h"
-#include "services/service_manager/public/cpp/service_context.h"
 #include "services/ws/ime/test_ime_driver/test_ime_driver.h"
 #include "services/ws/public/mojom/constants.mojom.h"
 #include "services/ws/public/mojom/ime/ime.mojom.h"
@@ -14,9 +13,11 @@
 namespace ws {
 namespace test {
 
-TestIMEApplication::TestIMEApplication() {}
+TestIMEApplication::TestIMEApplication(
+    service_manager::mojom::ServiceRequest request)
+    : service_binding_(this, std::move(request)) {}
 
-TestIMEApplication::~TestIMEApplication() {}
+TestIMEApplication::~TestIMEApplication() = default;
 
 void TestIMEApplication::OnStart() {
   mojom::IMEDriverPtr ime_driver_ptr;
@@ -24,7 +25,8 @@ void TestIMEApplication::OnStart() {
                           MakeRequest(&ime_driver_ptr));
 
   mojom::IMERegistrarPtr ime_registrar;
-  context()->connector()->BindInterface(mojom::kServiceName, &ime_registrar);
+  service_binding_.GetConnector()->BindInterface(mojom::kServiceName,
+                                                 &ime_registrar);
   ime_registrar->RegisterDriver(std::move(ime_driver_ptr));
 }
 

services/ws/ime/test_ime_driver/test_ime_application.h

@@ -7,19 +7,23 @@
 
 #include "base/macros.h"
 #include "services/service_manager/public/cpp/service.h"
+#include "services/service_manager/public/cpp/service_binding.h"
+#include "services/service_manager/public/mojom/service.mojom.h"
 
 namespace ws {
 namespace test {
 
 class TestIMEApplication : public service_manager::Service {
  public:
-  TestIMEApplication();
+  explicit TestIMEApplication(service_manager::mojom::ServiceRequest request);
   ~TestIMEApplication() override;
 
  private:
   // service_manager::Service:
   void OnStart() override;
 
+  service_manager::ServiceBinding service_binding_;
+
   DISALLOW_COPY_AND_ASSIGN(TestIMEApplication);
 };
 

services/ws/window_server_service_test_base.cc

@@ -7,11 +7,9 @@
 #include <memory>
 
 #include "base/command_line.h"
-#include "base/macros.h"
 #include "base/memory/ptr_util.h"
-#include "services/service_manager/public/cpp/service.h"
-#include "services/service_manager/public/cpp/service_test.h"
 #include "services/ws/common/switches.h"
+#include "services/ws/tests_catalog_source.h"
 #include "ui/gl/gl_switches.h"
 
 namespace ws {
@@ -20,27 +18,6 @@ namespace {
 
 const char kTestAppName[] = "ui_ws2_service_unittests";
 
-class WindowServerServiceTestClient
-    : public service_manager::test::ServiceTestClient {
- public:
-  explicit WindowServerServiceTestClient(WindowServerServiceTestBase* test)
-      : ServiceTestClient(test), test_(test) {}
-  ~WindowServerServiceTestClient() override {}
-
- private:
-  // service_manager::test::ServiceTestClient:
-  void OnBindInterface(const service_manager::BindSourceInfo& source_info,
-                       const std::string& interface_name,
-                       mojo::ScopedMessagePipeHandle interface_pipe) override {
-    test_->OnBindInterface(source_info, interface_name,
-                           std::move(interface_pipe));
-  }
-
-  WindowServerServiceTestBase* test_;
-
-  DISALLOW_COPY_AND_ASSIGN(WindowServerServiceTestClient);
-};
-
 void EnsureCommandLineSwitch(const std::string& name) {
   base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
   if (!cmd_line->HasSwitch(name))
@@ -50,16 +27,18 @@ void EnsureCommandLineSwitch(const std::string& name) {
 }  // namespace
 
 WindowServerServiceTestBase::WindowServerServiceTestBase()
-    : ServiceTest(kTestAppName) {
+    : test_service_manager_(test::CreateTestCatalog()),
+      test_service_binding_(
+          this,
+          test_service_manager_.RegisterTestInstance(kTestAppName)) {
   EnsureCommandLineSwitch(switches::kUseTestConfig);
   EnsureCommandLineSwitch(::switches::kOverrideUseSoftwareGLForTests);
 }
 
-WindowServerServiceTestBase::~WindowServerServiceTestBase() {}
+WindowServerServiceTestBase::~WindowServerServiceTestBase() = default;
 
-std::unique_ptr<service_manager::Service>
-WindowServerServiceTestBase::CreateService() {
-  return std::make_unique<WindowServerServiceTestClient>(this);
+const char* WindowServerServiceTestBase::test_name() const {
+  return kTestAppName;
 }
 
 }  // namespace ws

services/ws/window_server_service_test_base.h

@@ -6,24 +6,36 @@
 #define SERVICES_WS_WINDOW_SERVER_SERVICE_TEST_BASE_H_
 
 #include "base/macros.h"
-#include "services/service_manager/public/cpp/service_test.h"
+#include "base/test/scoped_task_environment.h"
+#include "services/service_manager/public/cpp/connector.h"
+#include "services/service_manager/public/cpp/service.h"
+#include "services/service_manager/public/cpp/service_binding.h"
+#include "services/service_manager/public/cpp/test/test_service_manager.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace ws {
 
-// Base class for all window manager ServiceTests to perform some common setup.
-class WindowServerServiceTestBase : public service_manager::test::ServiceTest {
+// Base class for all window manager service tests to perform some common setup.
+// This fixture brings up a test Service Manager and acts as a service instance
+// which identifies as the service named "ui_ws2_service_unittests". Subclasses
+// can implement |OnBindInterface()| to handle interface requests targeting that
+// instance in tests.
+class WindowServerServiceTestBase : public testing::Test,
+                                    public service_manager::Service {
  public:
   WindowServerServiceTestBase();
   ~WindowServerServiceTestBase() override;
 
-  virtual void OnBindInterface(
-      const service_manager::BindSourceInfo& source_info,
-      const std::string& interface_name,
-      mojo::ScopedMessagePipeHandle interface_pipe) = 0;
+  service_manager::Connector* connector() {
+    return test_service_binding_.GetConnector();
+  }
+
+  const char* test_name() const;
 
  private:
-  // service_manager::test::ServiceTest:
-  std::unique_ptr<service_manager::Service> CreateService() override;
+  base::test::ScopedTaskEnvironment task_environment_;
+  service_manager::TestServiceManager test_service_manager_;
+  service_manager::ServiceBinding test_service_binding_;
 
   DISALLOW_COPY_AND_ASSIGN(WindowServerServiceTestBase);
 };

services/ws/window_tree_client_unittest.cc

@@ -13,7 +13,6 @@
 #include "mojo/public/cpp/bindings/associated_binding.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "services/service_manager/public/cpp/binder_registry.h"
-#include "services/service_manager/public/cpp/service_test.h"
 #include "services/ws/common/util.h"
 #include "services/ws/ids.h"
 #include "services/ws/public/mojom/constants.mojom.h"