network_diagnostics: Test NetworkDiagnosticsImpl

Validate the communication between NetworkDiagnosticsImpl and the
routines.

BUG=chromium:1096551
TEST=unit_tests --gtest_filter=NetworkDiagnosticsImplTest.*

Change-Id: Id0868b88791e4b3fe9ec18b6257b68ef64ed41b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2274149
Commit-Queue: Kartik Hegde <khegde@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789240}

chrome/browser/chromeos/BUILD.gn

@@ -3188,6 +3188,7 @@ source_set("unit_tests") {
     "net/network_diagnostics/gateway_can_be_pinged_routine_unittest.cc",
     "net/network_diagnostics/has_secure_wifi_connection_routine_unittest.cc",
     "net/network_diagnostics/lan_connectivity_routine_unittest.cc",
+    "net/network_diagnostics/network_diagnostics_impl_unittest.cc",
     "net/network_diagnostics/network_diagnostics_routine_unittest.cc",
     "net/network_diagnostics/signal_strength_routine_unittest.cc",
     "net/network_health/network_health_unittest.cc",

chrome/browser/chromeos/net/network_diagnostics/network_diagnostics_impl.cc

@@ -5,7 +5,9 @@
 #include "chrome/browser/chromeos/net/network_diagnostics/network_diagnostics_impl.h"
 
 #include <memory>
+#include <utility>
 
+#include "base/bind.h"
 #include "chrome/browser/chromeos/net/network_diagnostics/dns_latency_routine.h"
 #include "chrome/browser/chromeos/net/network_diagnostics/dns_resolution_routine.h"
 #include "chrome/browser/chromeos/net/network_diagnostics/dns_resolver_present_routine.h"
@@ -13,14 +15,19 @@
 #include "chrome/browser/chromeos/net/network_diagnostics/has_secure_wifi_connection_routine.h"
 #include "chrome/browser/chromeos/net/network_diagnostics/lan_connectivity_routine.h"
 #include "chrome/browser/chromeos/net/network_diagnostics/signal_strength_routine.h"
-#include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon/debug_daemon_client.h"
 #include "components/device_event_log/device_event_log.h"
 
 namespace chromeos {
 namespace network_diagnostics {
 
-NetworkDiagnosticsImpl::NetworkDiagnosticsImpl() {}
+NetworkDiagnosticsImpl::NetworkDiagnosticsImpl(
+    chromeos::DebugDaemonClient* debug_daemon_client) {
+  DCHECK(debug_daemon_client);
+  if (debug_daemon_client) {
+    debug_daemon_client_ = debug_daemon_client;
+  }
+}
 
 NetworkDiagnosticsImpl::~NetworkDiagnosticsImpl() {}
 
@@ -31,43 +38,104 @@ void NetworkDiagnosticsImpl::BindReceiver(
 }
 
 void NetworkDiagnosticsImpl::LanConnectivity(LanConnectivityCallback callback) {
-  LanConnectivityRoutine lan_connectivity_routine;
-  lan_connectivity_routine.RunRoutine(std::move(callback));
+  auto routine = std::make_unique<LanConnectivityRoutine>();
+  // RunRoutine() takes a lambda callback that takes ownership of the routine.
+  // This ensures that the routine stays alive when it makes asynchronous mojo
+  // calls. The routine will be destroyed when the lambda exits.
+  routine->RunRoutine(base::BindOnce(
+      [](std::unique_ptr<LanConnectivityRoutine> routine,
+         LanConnectivityCallback callback,
+         mojom::RoutineVerdict verdict) { std::move(callback).Run(verdict); },
+      std::move(routine), std::move(callback)));
 }
 
 void NetworkDiagnosticsImpl::SignalStrength(SignalStrengthCallback callback) {
-  SignalStrengthRoutine signal_strength_routine;
-  signal_strength_routine.RunRoutine(std::move(callback));
+  auto routine = std::make_unique<SignalStrengthRoutine>();
+  // RunRoutine() takes a lambda callback that takes ownership of the routine.
+  // This ensures that the routine stays alive when it makes asynchronous mojo
+  // calls. The routine will be destroyed when the lambda exits.
+  routine->RunRoutine(base::BindOnce(
+      [](std::unique_ptr<SignalStrengthRoutine> routine,
+         SignalStrengthCallback callback, mojom::RoutineVerdict verdict,
+         const std::vector<mojom::SignalStrengthProblem>& problems) {
+        std::move(callback).Run(verdict, std::move(problems));
+      },
+      std::move(routine), std::move(callback)));
 }
 
 void NetworkDiagnosticsImpl::GatewayCanBePinged(
     GatewayCanBePingedCallback callback) {
-  chromeos::DebugDaemonClient* client =
-      chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
-  GatewayCanBePingedRoutine gateway_can_be_pinged_routine(client);
-  gateway_can_be_pinged_routine.RunRoutine(std::move(callback));
+  auto routine =
+      std::make_unique<GatewayCanBePingedRoutine>(debug_daemon_client_);
+  // RunRoutine() takes a lambda callback that takes ownership of the routine.
+  // This ensures that the routine stays alive when it makes asynchronous mojo
+  // calls. The routine will be destroyed when the lambda exits.
+  routine->RunRoutine(base::BindOnce(
+      [](std::unique_ptr<GatewayCanBePingedRoutine> routine,
+         GatewayCanBePingedCallback callback, mojom::RoutineVerdict verdict,
+         const std::vector<mojom::GatewayCanBePingedProblem>& problems) {
+        std::move(callback).Run(verdict, std::move(problems));
+      },
+      std::move(routine), std::move(callback)));
 }
 
 void NetworkDiagnosticsImpl::HasSecureWiFiConnection(
     HasSecureWiFiConnectionCallback callback) {
-  HasSecureWiFiConnectionRoutine has_secure_wifi_connection_routine;
-  has_secure_wifi_connection_routine.RunRoutine(std::move(callback));
+  auto routine = std::make_unique<HasSecureWiFiConnectionRoutine>();
+  // RunRoutine() takes a lambda callback that takes ownership of the routine.
+  // This ensures that the routine stays alive when it makes asynchronous mojo
+  // calls. The routine will be destroyed when the lambda exits.
+  routine->RunRoutine(base::BindOnce(
+      [](std::unique_ptr<HasSecureWiFiConnectionRoutine> routine,
+         HasSecureWiFiConnectionCallback callback,
+         mojom::RoutineVerdict verdict,
+         const std::vector<mojom::HasSecureWiFiConnectionProblem>& problems) {
+        std::move(callback).Run(verdict, std::move(problems));
+      },
+      std::move(routine), std::move(callback)));
 }
 
 void NetworkDiagnosticsImpl::DnsResolverPresent(
     DnsResolverPresentCallback callback) {
-  DnsResolverPresentRoutine dns_resolver_present_routine;
-  dns_resolver_present_routine.RunRoutine(std::move(callback));
+  auto routine = std::make_unique<DnsResolverPresentRoutine>();
+  // RunRoutine() takes a lambda callback that takes ownership of the routine.
+  // This ensures that the routine stays alive when it makes asynchronous mojo
+  // calls. The routine will be destroyed when the lambda exits.
+  routine->RunRoutine(base::BindOnce(
+      [](std::unique_ptr<DnsResolverPresentRoutine> routine,
+         DnsResolverPresentCallback callback, mojom::RoutineVerdict verdict,
+         const std::vector<mojom::DnsResolverPresentProblem>& problems) {
+        std::move(callback).Run(verdict, std::move(problems));
+      },
+      std::move(routine), std::move(callback)));
 }
 
 void NetworkDiagnosticsImpl::DnsLatency(DnsLatencyCallback callback) {
-  DnsLatencyRoutine dns_latency_routine;
-  dns_latency_routine.RunRoutine(std::move(callback));
+  auto routine = std::make_unique<DnsLatencyRoutine>();
+  // RunRoutine() takes a lambda callback that takes ownership of the routine.
+  // This ensures that the routine stays alive when it makes asynchronous mojo
+  // calls. The routine will be destroyed when the lambda exits.
+  routine->RunRoutine(base::BindOnce(
+      [](std::unique_ptr<DnsLatencyRoutine> routine,
+         DnsLatencyCallback callback, mojom::RoutineVerdict verdict,
+         const std::vector<mojom::DnsLatencyProblem>& problems) {
+        std::move(callback).Run(verdict, std::move(problems));
+      },
+      std::move(routine), std::move(callback)));
 }
 
 void NetworkDiagnosticsImpl::DnsResolution(DnsResolutionCallback callback) {
-  DnsResolutionRoutine dns_resolution_routine;
-  dns_resolution_routine.RunRoutine(std::move(callback));
+  auto routine = std::make_unique<DnsResolutionRoutine>();
+  // RunRoutine() takes a lambda callback that takes ownership of the routine.
+  // This ensures that the routine stays alive when it makes asynchronous mojo
+  // calls. The routine will be destroyed when the lambda exits.
+  routine->RunRoutine(base::BindOnce(
+      [](std::unique_ptr<DnsResolutionRoutine> routine,
+         DnsResolutionCallback callback, mojom::RoutineVerdict verdict,
+         const std::vector<mojom::DnsResolutionProblem>& problems) {
+        std::move(callback).Run(verdict, std::move(problems));
+      },
+      std::move(routine), std::move(callback)));
 }
 
 }  // namespace network_diagnostics

chrome/browser/chromeos/net/network_diagnostics/network_diagnostics_impl.h

@@ -11,11 +11,14 @@
 #include "mojo/public/cpp/bindings/receiver_set.h"
 
 namespace chromeos {
+class DebugDaemonClient;
+
 namespace network_diagnostics {
 
 class NetworkDiagnosticsImpl : public mojom::NetworkDiagnosticsRoutines {
  public:
-  NetworkDiagnosticsImpl();
+  explicit NetworkDiagnosticsImpl(
+      chromeos::DebugDaemonClient* debug_daemon_client);
   NetworkDiagnosticsImpl(const NetworkDiagnosticsImpl&) = delete;
   NetworkDiagnosticsImpl& operator=(const NetworkDiagnosticsImpl&) = delete;
   ~NetworkDiagnosticsImpl() override;
@@ -38,6 +41,8 @@ class NetworkDiagnosticsImpl : public mojom::NetworkDiagnosticsRoutines {
 
  private:
   mojo::ReceiverSet<mojom::NetworkDiagnosticsRoutines> receivers_;
+  // An unowned pointer to the DebugDaemonClient instance.
+  chromeos::DebugDaemonClient* debug_daemon_client_;
   base::WeakPtrFactory<NetworkDiagnosticsImpl> weak_factory_{this};
 };
 

chrome/browser/chromeos/net/network_health/network_health_service.cc

@@ -5,11 +5,16 @@
 #include "chrome/browser/chromeos/net/network_health/network_health_service.h"
 
 #include "base/no_destructor.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
 
 namespace chromeos {
 namespace network_health {
 
-NetworkHealthService::NetworkHealthService() = default;
+NetworkHealthService::NetworkHealthService() {
+  network_diagnostics_ =
+      std::make_unique<network_diagnostics::NetworkDiagnosticsImpl>(
+          chromeos::DBusThreadManager::Get()->GetDebugDaemonClient());
+}
 
 void NetworkHealthService::BindRemote(
     mojo::PendingReceiver<mojom::NetworkHealthService> receiver) {
@@ -19,7 +24,7 @@ void NetworkHealthService::BindRemote(
 void NetworkHealthService::BindDiagnosticsRemote(
     mojo::PendingReceiver<
         network_diagnostics::mojom::NetworkDiagnosticsRoutines> receiver) {
-  network_diagnostics_.BindReceiver(std::move(receiver));
+  network_diagnostics_->BindReceiver(std::move(receiver));
 }
 
 NetworkHealthService* NetworkHealthService::GetInstance() {

chrome/browser/chromeos/net/network_health/network_health_service.h

@@ -25,7 +25,8 @@ class NetworkHealthService {
 
  private:
   NetworkHealth network_health_;
-  network_diagnostics::NetworkDiagnosticsImpl network_diagnostics_;
+  std::unique_ptr<network_diagnostics::NetworkDiagnosticsImpl>
+      network_diagnostics_;
 };
 
 }  // namespace network_health