[fuchsia] Add webrunner unit tests, browsertests to Fuchsia bots.

* Fixed webrunner_unittests (tests were not being exercised, so the
  test code drifted from the impl)
* Added webrunner_unittests, webrunner_browsertests to Fuchsia test
  bot list.


Bug: 871594
Change-Id: I3f86f2f1d76534c51236a3dea43dc898d996b121
Reviewed-on: https://chromium-review.googlesource.com/1174868
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584669}

testing/buildbot/chromium.fyi.json

@@ -3129,6 +3129,18 @@
           "can_use_on_swarming_builders": true
         },
         "test": "ui_base_unittests"
+      },
+      {
+        "swarming": {
+          "can_use_on_swarming_builders": true
+        },
+        "test": "webrunner_browsertests"
+      },
+      {
+        "swarming": {
+          "can_use_on_swarming_builders": true
+        },
+        "test": "webrunner_unittests"
       }
     ]
   },
@@ -3280,6 +3292,28 @@
           ]
         },
         "test": "ui_base_unittests"
+      },
+      {
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "webrunner_browsertests"
+      },
+      {
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "webrunner_unittests"
       }
     ]
   },
@@ -3446,6 +3480,28 @@
           ]
         },
         "test": "ui_base_unittests"
+      },
+      {
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "webrunner_browsertests"
+      },
+      {
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "webrunner_unittests"
       }
     ]
   },

testing/buildbot/chromium.linux.json

@@ -786,6 +786,28 @@
           ]
         },
         "test": "ui_base_unittests"
+      },
+      {
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "webrunner_browsertests"
+      },
+      {
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "webrunner_unittests"
       }
     ]
   },

testing/buildbot/gn_isolate_map.pyl

@@ -1351,6 +1351,14 @@
       "../../third_party/blink/tools/run_blinkpy_tests.py"
     ]
   },
+  "webrunner_browsertests": {
+    "label": "//webrunner:webrunner_browsertests",
+    "type": "console_test_launcher",
+  },
+  "webrunner_unittests": {
+    "label": "//webrunner:webrunner_unittests",
+    "type": "console_test_launcher",
+  },
   "webkit_unit_tests": {
     "label": "//third_party/blink/renderer/controller:webkit_unit_tests",
     "type": "console_test_launcher",

testing/buildbot/test_suites.pyl

@@ -981,6 +981,8 @@
         '--test-launcher-filter-file=../../testing/buildbot/filters/fuchsia.ui_base_unittests.filter',
       ],
     },
+    'webrunner_browsertests': {},
+    'webrunner_unittests': {},
   },
 
   'goma_gtests': {

webrunner/BUILD.gn

@@ -190,8 +190,7 @@ test("webrunner_browsertests") {
 
 test("webrunner_unittests") {
   sources = [
-    # TODO(kmarshall): Uncomment this when the tests are fixed.
-    # "service/context_provider_impl_unittest.cc",
+    "service/context_provider_impl_unittest.cc",
   ]
   deps = [
     ":fidl",

webrunner/service/context_provider_impl.cc

@@ -5,6 +5,7 @@
 #include "webrunner/service/context_provider_impl.h"
 
 #include <fuchsia/sys/cpp/fidl.h>
+#include <lib/async/default.h>
 #include <lib/zx/job.h>
 #include <stdio.h>
 #include <zircon/processargs.h>
@@ -26,20 +27,36 @@ namespace webrunner {
 namespace {
 
 // Relaunches the current executable as a Context process.
-base::Process LaunchContextProcess(base::CommandLine launch_command,
+base::Process LaunchContextProcess(const base::CommandLine& launch_command,
                                    const base::LaunchOptions& launch_options) {
+  base::CommandLine launch_command_modified = launch_command;
+
   // TODO(crbug.com/867052): Remove this flag when GPU process works on Fuchsia.
-  launch_command.AppendSwitch(switches::kDisableGpu);
-  return base::LaunchProcess(launch_command, launch_options);
+  launch_command_modified.AppendSwitch(switches::kDisableGpu);
+  return base::LaunchProcess(launch_command_modified, launch_options);
 }
 
 }  // namespace
 
-ContextProviderImpl::ContextProviderImpl()
-    : launch_(base::BindRepeating(&LaunchContextProcess)) {}
+ContextProviderImpl::ContextProviderImpl() : ContextProviderImpl(false) {}
+
+ContextProviderImpl::ContextProviderImpl(bool use_shared_tmp)
+    : launch_(base::BindRepeating(&LaunchContextProcess)),
+      use_shared_tmp_(use_shared_tmp) {}
 
 ContextProviderImpl::~ContextProviderImpl() = default;
 
+// static
+std::unique_ptr<ContextProviderImpl> ContextProviderImpl::CreateForTest() {
+  // Bind the unique_ptr in a two step process.
+  // std::make_unique<> doesn't work well with private constructors,
+  // and the unique_ptr(raw_ptr*) constructor format isn't permitted as per
+  // PRESUBMIT.py policy.
+  std::unique_ptr<ContextProviderImpl> provider;
+  provider.reset(new ContextProviderImpl(true));
+  return provider;
+}
+
 void ContextProviderImpl::SetLaunchCallbackForTests(
     const LaunchContextProcessCallback& launch) {
   launch_ = launch;
@@ -57,9 +74,11 @@ void ContextProviderImpl::Create(
   // Clone job because the context needs to be able to spawn child processes.
   launch_options.spawn_flags = FDIO_SPAWN_CLONE_JOB;
 
-  // Clone stderr to get logs in system debug log.
+  // Clone stdout/stderr to get logs in system debug log.
   launch_options.fds_to_remap.push_back(
       std::make_pair(STDERR_FILENO, STDERR_FILENO));
+  launch_options.fds_to_remap.push_back(
+      std::make_pair(STDOUT_FILENO, STDOUT_FILENO));
 
   // Context and child processes need access to the read-only package files.
   launch_options.paths_to_clone.push_back(base::FilePath("/pkg"));
@@ -67,6 +86,9 @@ void ContextProviderImpl::Create(
   // Context needs access to the read-only SSL root certificates list.
   launch_options.paths_to_clone.push_back(base::FilePath("/config/ssl"));
 
+  if (use_shared_tmp_)
+    launch_options.paths_to_clone.push_back(base::FilePath("/tmp"));
+
   // Transfer the ContextRequest handle to a well-known location in the child
   // process' handle table.
   zx::channel context_handle(context_request.TakeChannel());
@@ -91,8 +113,10 @@ void ContextProviderImpl::Create(
   zx::job job;
   zx_status_t status = zx::job::create(*base::GetDefaultJob(), 0, &job);
   ZX_CHECK(status == ZX_OK, status) << "zx_job_create";
+  launch_options.job_handle = job.get();
 
   ignore_result(launch_.Run(std::move(launch_command), launch_options));
+
   ignore_result(context_handle.release());
   ignore_result(job.release());
 }

webrunner/service/context_provider_impl.h

@@ -6,6 +6,7 @@
 #define WEBRUNNER_SERVICE_CONTEXT_PROVIDER_IMPL_H_
 
 #include <lib/fidl/cpp/binding_set.h>
+#include <memory>
 
 #include "base/callback.h"
 #include "base/macros.h"
@@ -26,6 +27,11 @@ class WEBRUNNER_EXPORT ContextProviderImpl
   ContextProviderImpl();
   ~ContextProviderImpl() override;
 
+  // Creates a ContextProviderImpl that shares its /tmp directory with its child
+  // processes. This is useful for GTest processes, which depend on a shared
+  // tmpdir for storing startup flags and retrieving test result files.
+  static std::unique_ptr<ContextProviderImpl> CreateForTest();
+
   // Binds |this| object instance to |request|.
   // The service will persist and continue to serve other channels in the event
   // that a bound channel is dropped.
@@ -38,11 +44,13 @@ class WEBRUNNER_EXPORT ContextProviderImpl
 
  private:
   using LaunchContextProcessCallback = base::RepeatingCallback<base::Process(
-      base::CommandLine command,
+      const base::CommandLine& command,
       const base::LaunchOptions& options)>;
 
   friend class ContextProviderImplTest;
 
+  explicit ContextProviderImpl(bool use_shared_tmp);
+
   // Overrides the default child process launching logic to call |launch|
   // instead.
   void SetLaunchCallbackForTests(const LaunchContextProcessCallback& launch);
@@ -50,6 +58,9 @@ class WEBRUNNER_EXPORT ContextProviderImpl
   // Spawns a Context child process.
   LaunchContextProcessCallback launch_;
 
+  // If set, then the ContextProvider will share /tmp with its child processes.
+  bool use_shared_tmp_ = true;
+
   fidl::BindingSet<chromium::web::ContextProvider> bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(ContextProviderImpl);

webrunner/service/context_provider_impl_unittest.cc

@@ -8,6 +8,7 @@
 #include <lib/fidl/cpp/binding.h>
 #include <zircon/processargs.h>
 
+#include <functional>
 #include <utility>
 #include <vector>
 
@@ -19,6 +20,8 @@
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/fuchsia/file_utils.h"
+#include "base/fuchsia/fuchsia_logging.h"
+#include "base/fuchsia/service_directory.h"
 #include "base/message_loop/message_loop.h"
 #include "base/test/multiprocess_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -30,42 +33,61 @@ namespace {
 
 constexpr char kTestDataFileIn[] = "DataFileIn";
 constexpr char kTestDataFileOut[] = "DataFileOut";
+constexpr char kUrl[] = "chrome://:emorhc";
+constexpr char kTitle[] = "Palindrome";
 
-class TestFrameObserver : public chromium::web::FrameObserver {
+// A fake Frame implementation that manages its own lifetime.
+class FakeFrame : public chromium::web::Frame {
  public:
-  void OnNavigationStateChanged(
-      chromium::web::NavigationStateChangeDetails change,
-      OnNavigationStateChangedCallback callback) override {
-    last_change_ = change;
-    if (change_callback_)
-      std::move(change_callback_).Run();
-    callback();
+  explicit FakeFrame(fidl::InterfaceRequest<chromium::web::Frame> request)
+      : binding_(this, std::move(request)) {
+    binding_.set_error_handler([this]() { delete this; });
   }
 
-  void SetNextChangeCallback(base::OnceClosure callback) {
-    change_callback_ = std::move(callback);
-  }
+  ~FakeFrame() override = default;
+
+  fidl::Binding<chromium::web::Frame>& binding() { return binding_; }
 
-  chromium::web::NavigationStateChangeDetails last_change() {
-    return last_change_;
+  void CreateView(
+      fidl::InterfaceRequest<fuchsia::ui::viewsv1token::ViewOwner> view_owner,
+      fidl::InterfaceRequest<fuchsia::sys::ServiceProvider> services) override {
   }
 
+  void GetNavigationController(
+      fidl::InterfaceRequest<chromium::web::NavigationController> controller)
+      override {}
+
  private:
-  chromium::web::NavigationStateChangeDetails last_change_ = {};
-  base::OnceClosure change_callback_;
+  fidl::Binding<chromium::web::Frame> binding_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeFrame);
 };
 
+// An implementation of Context that creates and binds FakeFrames.
 class FakeContext : public chromium::web::Context {
  public:
+  using CreateFrameCallback = base::RepeatingCallback<void(FakeFrame*)>;
+
+  FakeContext() = default;
+  ~FakeContext() override = default;
+
+  // Sets a callback that is invoked whenever new Frames are bound.
+  void set_on_create_frame_callback(CreateFrameCallback callback) {
+    on_create_frame_callback_ = callback;
+  }
+
   void CreateFrame(
-      fidl::InterfaceHandle<chromium::web::FrameObserver> observer,
-      fidl::InterfaceRequest<chromium::web::Frame> frame) override {
-    chromium::web::NavigationStateChangeDetails details;
-    details.url_changed = true;
-    details.entry.url = "";
-    details.entry.title = "";
-    observer.Bind()->OnNavigationStateChanged(details, []() {});
+      fidl::InterfaceRequest<chromium::web::Frame> frame_request) override {
+    FakeFrame* new_frame = new FakeFrame(std::move(frame_request));
+    on_create_frame_callback_.Run(new_frame);
+
+    // |new_frame| owns itself, so we intentionally leak the pointer.
   }
+
+ private:
+  CreateFrameCallback on_create_frame_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeContext);
 };
 
 MULTIPROCESS_TEST_MAIN(SpawnContextServer) {
@@ -79,18 +101,29 @@ MULTIPROCESS_TEST_MAIN(SpawnContextServer) {
     EXPECT_EQ(base::WriteFile(out_file, nullptr, 0), 0);
   }
 
-  FakeContext fake_context;
-  zx::channel context_handle{zx_take_startup_handle(kContextRequestHandleId)};
+  zx::channel context_handle(zx_take_startup_handle(kContextRequestHandleId));
   CHECK(context_handle);
-  fidl::Binding<chromium::web::Context> binding(&fake_context,
-                                                std::move(context_handle));
 
-  base::RunLoop run_loop;
+  FakeContext context;
+  fidl::Binding<chromium::web::Context> context_binding(
+      &context, fidl::InterfaceRequest<chromium::web::Context>(
+                    std::move(context_handle)));
+
+  // When a Frame is bound, immediately broadcast a navigation event to its
+  // event listeners.
+  context.set_on_create_frame_callback(
+      base::BindRepeating([](FakeFrame* frame) {
+        chromium::web::NavigationStateChangeDetails details;
+        details.url = kUrl;
+        details.title = kTitle;
+        frame->binding().events().OnNavigationStateChanged(std::move(details));
+      }));
 
   // Quit the process when the context is destroyed.
-  binding.set_error_handler([&run_loop]() { run_loop.Quit(); });
-
+  base::RunLoop run_loop;
+  context_binding.set_error_handler([&run_loop]() { run_loop.Quit(); });
   run_loop.Run();
+
   return 0;
 }
 
@@ -98,10 +131,10 @@ MULTIPROCESS_TEST_MAIN(SpawnContextServer) {
 
 class ContextProviderImplTest : public base::MultiProcessTest {
  public:
-  ContextProviderImplTest() {
-    provider_.SetLaunchCallbackForTests(base::BindRepeating(
-        &ContextProviderImplTest::LaunchProcess, base::Unretained(this)));
-    provider_.Bind(provider_ptr_.NewRequest());
+  ContextProviderImplTest() : provider_(ContextProviderImpl::CreateForTest()) {
+    provider_->SetLaunchCallbackForTests(base::BindRepeating(
+        &base::SpawnMultiProcessTestChild, "SpawnContextServer"));
+    provider_->Bind(provider_ptr_.NewRequest());
   }
 
   ~ContextProviderImplTest() override {
@@ -109,55 +142,55 @@ class ContextProviderImplTest : public base::MultiProcessTest {
     base::RunLoop().RunUntilIdle();
   }
 
-  // Start a new child process whose main function is defined in
-  // MULTIPROCESSING_TEST_MAIN(SpawnContextServer).
-  base::Process LaunchProcess(base::CommandLine command,
-                              const base::LaunchOptions& options) {
-    command.AppendSwitchASCII(switches::kTestChildProcess,
-                              "SpawnContextServer");
-    base::Process context_process = base::LaunchProcess(command, options);
-    EXPECT_TRUE(context_process.IsValid());
-    return context_process;
-  }
-
-  void CheckContext(fidl::InterfacePtr<chromium::web::Context>* context) {
+  // Check if a Context is responsive by creating a Frame from it and then
+  // listening for an event.
+  void CheckContextResponsive(
+      fidl::InterfacePtr<chromium::web::Context>* context) {
     // Call a Context method and wait for it to invoke an observer call.
     base::RunLoop run_loop;
-
     context->set_error_handler([&run_loop]() {
       ADD_FAILURE();
       run_loop.Quit();
     });
 
-    TestFrameObserver frame_observer;
-    fidl::Binding<chromium::web::FrameObserver> frame_observer_binding(
-        &frame_observer);
-    frame_observer_binding.set_error_handler([&run_loop]() {
-      ADD_FAILURE();
-      run_loop.Quit();
-    });
-    frame_observer.SetNextChangeCallback(run_loop.QuitClosure());
-
     chromium::web::FramePtr frame_ptr;
     frame_ptr.set_error_handler([&run_loop]() {
       ADD_FAILURE();
       run_loop.Quit();
     });
 
-    (*context)->CreateFrame(frame_observer_binding.NewBinding(),
-                            frame_ptr.NewRequest());
-
-
+    // Create a Frame and expect to see a navigation event.
+    chromium::web::NavigationStateChangeDetails nav_change_stored;
+    frame_ptr.events().OnNavigationStateChanged =
+        [&nav_change_stored,
+         &run_loop](chromium::web::NavigationStateChangeDetails nav_change) {
+          nav_change_stored = nav_change;
+          run_loop.Quit();
+        };
+    (*context)->CreateFrame(frame_ptr.NewRequest());
     run_loop.Run();
 
-    EXPECT_TRUE(frame_observer.last_change().url_changed ||
-                frame_observer.last_change().title_changed);
+    EXPECT_EQ(nav_change_stored.url, kUrl);
+    EXPECT_EQ(nav_change_stored.title, kTitle);
+  }
+
+  chromium::web::CreateContextParams BuildCreateContextParams() {
+    zx::channel client_channel;
+    zx::channel server_channel;
+    zx_status_t result =
+        zx::channel::create(0, &client_channel, &server_channel);
+    ZX_CHECK(result == ZX_OK, result) << "zx_channel_create()";
+    result = fdio_service_connect("/svc/.", server_channel.release());
+    ZX_CHECK(result == ZX_OK, result) << "Failed to open /svc";
+
+    chromium::web::CreateContextParams output;
+    output.service_directory = std::move(client_channel);
+    return output;
   }
 
  protected:
   base::MessageLoopForIO message_loop_;
-
-  ContextProviderImpl provider_;
+  std::unique_ptr<ContextProviderImpl> provider_;
   chromium::web::ContextProviderPtr provider_ptr_;
 
  private:
@@ -167,33 +200,32 @@ class ContextProviderImplTest : public base::MultiProcessTest {
 TEST_F(ContextProviderImplTest, LaunchContext) {
   // Connect to a new context process.
   fidl::InterfacePtr<chromium::web::Context> context;
-  chromium::web::CreateContextParams create_params;
+  chromium::web::CreateContextParams create_params = BuildCreateContextParams();
   provider_ptr_->Create(std::move(create_params), context.NewRequest());
-
-  CheckContext(&context);
+  CheckContextResponsive(&context);
 }
 
-// Verify that there can be more than one connection to the provider.
-TEST_F(ContextProviderImplTest, MultipleClients) {
+TEST_F(ContextProviderImplTest, MultipleConcurrentClients) {
+  // Bind a Provider connection, and create a Context from it.
   chromium::web::ContextProviderPtr provider_1_ptr;
-  provider_.Bind(provider_1_ptr.NewRequest());
+  provider_->Bind(provider_1_ptr.NewRequest());
+  chromium::web::ContextPtr context_1;
+  provider_1_ptr->Create(BuildCreateContextParams(), context_1.NewRequest());
 
-  // Connect a second client.
+  // Do the same on another Provider connection.
   chromium::web::ContextProviderPtr provider_2_ptr;
-  provider_.Bind(provider_2_ptr.NewRequest());
-  fidl::InterfacePtr<chromium::web::Context> context;
-  chromium::web::CreateContextParams create_params;
-  provider_2_ptr->Create(std::move(create_params), context.NewRequest());
-
-  TestFrameObserver frame_observer;
-  fidl::Binding<chromium::web::FrameObserver> frame_observer_binding(
-      &frame_observer);
-  base::RunLoop run_loop;
-  frame_observer.SetNextChangeCallback(run_loop.QuitClosure());
-  chromium::web::FramePtr frame_ptr;
-  context->CreateFrame(frame_observer_binding.NewBinding(),
-                       frame_ptr.NewRequest());
-  run_loop.Run();
+  provider_->Bind(provider_2_ptr.NewRequest());
+  chromium::web::ContextPtr context_2;
+  provider_2_ptr->Create(BuildCreateContextParams(), context_2.NewRequest());
+
+  CheckContextResponsive(&context_1);
+  CheckContextResponsive(&context_2);
+
+  // Ensure that the initial ContextProvider connection is still usable, by
+  // creating and verifying another Context from it.
+  chromium::web::ContextPtr context_3;
+  provider_2_ptr->Create(BuildCreateContextParams(), context_3.NewRequest());
+  CheckContextResponsive(&context_3);
 }
 
 TEST_F(ContextProviderImplTest, WithProfileDir) {
@@ -201,7 +233,7 @@ TEST_F(ContextProviderImplTest, WithProfileDir) {
 
   // Connect to a new context process.
   fidl::InterfacePtr<chromium::web::Context> context;
-  chromium::web::CreateContextParams create_params;
+  chromium::web::CreateContextParams create_params = BuildCreateContextParams();
 
   // Setup data dir.
   EXPECT_TRUE(profile_temp_dir.CreateUniqueTempDir());
@@ -219,7 +251,7 @@ TEST_F(ContextProviderImplTest, WithProfileDir) {
 
   provider_ptr_->Create(std::move(create_params), context.NewRequest());
 
-  CheckContext(&context);
+  CheckContextResponsive(&context);
 
   // Verify that the context process can write data to the out dir.
   EXPECT_TRUE(base::PathExists(