arc: Add unittests for ArcMetricsService

ArcMetricsService used to be simple, but it's not anymore and needs
some tests. For example, since b/62703685 has been fully implemented,
ArcMetricsService::ReportBootProgress() may receive event timestamps
that are smaller than |arc_start_time|.

BUG=b:65128488
BUG=b:62703685
TEST=try

Change-Id: Idd2aed6bdc2cbdb970d8e9b0e02987377ae1cfd6
Reviewed-on: https://chromium-review.googlesource.com/785110Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519739}

chromeos/dbus/fake_session_manager_client.cc

@@ -335,10 +335,10 @@ void FakeSessionManagerClient::EmitArcBooted(
 void FakeSessionManagerClient::GetArcStartTime(
     DBusMethodCallback<base::TimeTicks> callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback),
-                                arc_available_ ? base::make_optional(
-                                                     base::TimeTicks::Now())
-                                               : base::nullopt));
+      FROM_HERE,
+      base::BindOnce(std::move(callback),
+                     arc_available_ ? base::make_optional(arc_start_time_)
+                                    : base::nullopt));
 }
 
 void FakeSessionManagerClient::RemoveArcData(

chromeos/dbus/fake_session_manager_client.h

@@ -12,6 +12,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
+#include "base/time/time.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/session_manager_client.h"
 
@@ -146,6 +147,9 @@ class FakeSessionManagerClient : public SessionManagerClient {
   }
 
   void set_arc_available(bool available) { arc_available_ = available; }
+  void set_arc_start_time(base::TimeTicks arc_start_time) {
+    arc_start_time_ = arc_start_time;
+  }
 
   void set_low_disk(bool low_disk) { low_disk_ = low_disk; }
 
@@ -174,6 +178,8 @@ class FakeSessionManagerClient : public SessionManagerClient {
   int notify_lock_screen_dismissed_call_count_;
 
   bool arc_available_;
+  base::TimeTicks arc_start_time_;
+
   bool low_disk_ = false;
   // Pseudo running container id. If not running, empty.
   std::string container_instance_id_;

components/arc/BUILD.gn

@@ -182,6 +182,8 @@ static_library("arc_test_support") {
     "test/fake_voice_interaction_framework_instance.h",
     "test/fake_wallpaper_instance.cc",
     "test/fake_wallpaper_instance.h",
+    "test/test_browser_context.cc",
+    "test/test_browser_context.h",
   ]
 
   public_deps = [
@@ -190,6 +192,10 @@ static_library("arc_test_support") {
 
   deps = [
     "//base",
+    "//components/keyed_service/content",
+    "//components/prefs:test_support",
+    "//components/user_prefs",
+    "//content/test:test_support",
     "//mojo/common:common_base",
     "//mojo/edk/embedder:headers",
   ]
@@ -211,6 +217,7 @@ source_set("unit_tests") {
     "intent_helper/intent_filter_unittest.cc",
     "intent_helper/link_handler_model_unittest.cc",
     "intent_helper/page_transition_util_unittest.cc",
+    "metrics/arc_metrics_service_unittest.cc",
     "power/arc_power_bridge_unittest.cc",
   ]
 
@@ -222,11 +229,13 @@ source_set("unit_tests") {
     "//chromeos",
     "//chromeos:power_manager_proto",
     "//chromeos:test_support_without_gmock",
+    "//components/keyed_service/content",
     "//components/prefs:test_support",
     "//components/signin/core/account_id",
     "//components/user_manager",
     "//components/user_manager:test_support",
     "//content/public/common",
+    "//content/test:test_support",
     "//device/bluetooth",
     "//mojo/public/cpp/system:system",
     "//services/device/public/cpp/test:test_support",

components/arc/DEPS

@@ -25,4 +25,7 @@ specific_include_rules = {
   "arc_util_unittest.cc": [
     "+ui/aura",
   ],
+  ".*_unittest.cc": [
+    "+content/public/test/test_browser_thread_bundle.h"
+  ]
 }

components/arc/metrics/arc_metrics_service.cc

@@ -67,6 +67,11 @@ class ArcMetricsServiceFactory
 
 }  // namespace
 
+// static
+BrowserContextKeyedServiceFactory* ArcMetricsService::GetFactory() {
+  return ArcMetricsServiceFactory::GetInstance();
+}
+
 // static
 ArcMetricsService* ArcMetricsService::GetForBrowserContext(
     content::BrowserContext* context) {

components/arc/metrics/arc_metrics_service.h

@@ -17,6 +17,8 @@
 #include "components/arc/connection_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
 
+class BrowserContextKeyedServiceFactory;
+
 namespace content {
 class BrowserContext;
 }  // namespace content
@@ -29,6 +31,9 @@ class ArcBridgeService;
 class ArcMetricsService : public KeyedService,
                           public mojom::MetricsHost {
  public:
+  // Returns the factory instance for this class.
+  static BrowserContextKeyedServiceFactory* GetFactory();
+
   // Returns singleton instance for the given BrowserContext,
   // or nullptr if the browser |context| is not allowed to use ARC.
   static ArcMetricsService* GetForBrowserContext(

components/arc/metrics/arc_metrics_service_unittest.cc

+// Copyright 2017 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 "components/arc/metrics/arc_metrics_service.h"
+
+#include <algorithm>
+#include <array>
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "base/metrics/histogram_samples.h"
+#include "base/run_loop.h"
+#include "base/test/histogram_tester.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_session_manager_client.h"
+#include "components/arc/arc_bridge_service.h"
+#include "components/arc/arc_service_manager.h"
+#include "components/arc/test/fake_arc_session.h"
+#include "components/arc/test/test_browser_context.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace arc {
+namespace {
+
+// The event names the container sends to Chrome.
+constexpr std::array<const char*, 11> kBootEvents{
+    "boot_progress_start",
+    "boot_progress_preload_start",
+    "boot_progress_preload_end",
+    "boot_progress_system_run",
+    "boot_progress_pms_start",
+    "boot_progress_pms_system_scan_start",
+    "boot_progress_pms_data_scan_start",
+    "boot_progress_pms_scan_end",
+    "boot_progress_pms_ready",
+    "boot_progress_ams_ready",
+    "boot_progress_enable_screen"};
+
+ArcMetricsService* GetArcMetricsService(content::BrowserContext* context) {
+  ArcMetricsService::GetFactory()->SetTestingFactoryAndUse(
+      context,
+      [](content::BrowserContext* context) -> std::unique_ptr<KeyedService> {
+        return std::make_unique<ArcMetricsService>(
+            context, ArcServiceManager::Get()->arc_bridge_service());
+      });
+  return ArcMetricsService::GetForBrowserContext(context);
+}
+
+class ArcMetricsServiceTest : public testing::Test {
+ public:
+  ArcMetricsServiceTest()
+      : arc_service_manager_(std::make_unique<ArcServiceManager>()),
+        context_(std::make_unique<TestBrowserContext>()),
+        metrics_service_(GetArcMetricsService(context_.get())) {
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
+        std::make_unique<chromeos::FakeSessionManagerClient>());
+    GetSessionManagerClient()->set_arc_available(true);
+  }
+
+  ~ArcMetricsServiceTest() override {
+    metrics_service_->Shutdown();
+    chromeos::DBusThreadManager::Shutdown();
+  }
+
+  ArcMetricsService* metrics_service() { return metrics_service_; }
+
+ protected:
+  void SetArcStartTimeInMs(uint64_t arc_start_time_in_ms) {
+    const base::TimeTicks arc_start_time =
+        base::TimeDelta::FromMilliseconds(arc_start_time_in_ms) +
+        base::TimeTicks();
+    GetSessionManagerClient()->set_arc_start_time(arc_start_time);
+  }
+
+  std::vector<mojom::BootProgressEventPtr> GetBootProgressEvents(
+      uint64_t start_in_ms,
+      uint64_t step_in_ms) {
+    std::vector<mojom::BootProgressEventPtr> events;
+    for (size_t i = 0; i < kBootEvents.size(); ++i) {
+      events.emplace_back(mojom::BootProgressEvent::New(
+          kBootEvents[i], start_in_ms + (step_in_ms * i)));
+    }
+    return events;
+  }
+
+ private:
+  chromeos::FakeSessionManagerClient* GetSessionManagerClient() {
+    return static_cast<chromeos::FakeSessionManagerClient*>(
+        chromeos::DBusThreadManager::Get()->GetSessionManagerClient());
+  }
+
+  content::TestBrowserThreadBundle thread_bundle_;
+  std::unique_ptr<ArcServiceManager> arc_service_manager_;
+  std::unique_ptr<TestBrowserContext> context_;
+
+  ArcMetricsService* const metrics_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcMetricsServiceTest);
+};
+
+// Tests that ReportBootProgress() actually records UMA stats.
+TEST_F(ArcMetricsServiceTest, ReportBootProgress_FirstBoot) {
+  // Start the full ARC container at t=10. Also set boot_progress_start to 10,
+  // boot_progress_preload_start to 11, and so on.
+  constexpr uint64_t kArcStartTimeMs = 10;
+  SetArcStartTimeInMs(kArcStartTimeMs);
+  std::vector<mojom::BootProgressEventPtr> events(
+      GetBootProgressEvents(kArcStartTimeMs, 1 /* step_in_ms */));
+
+  // Call ReportBootProgress() and then confirm that
+  // Arc.boot_progress_start.FirstBoot is recorded with 0 (ms),
+  // Arc.boot_progress_preload_start.FirstBoot is with 1 (ms), etc.
+  base::HistogramTester tester;
+  metrics_service()->ReportBootProgress(std::move(events),
+                                        mojom::BootType::FIRST_BOOT);
+  base::RunLoop().RunUntilIdle();
+  for (size_t i = 0; i < kBootEvents.size(); ++i) {
+    tester.ExpectUniqueSample(
+        std::string("Arc.") + kBootEvents[i] + ".FirstBoot", i,
+        1 /* count of the sample */);
+  }
+  // Confirm that Arc.AndroidBootTime.FirstBoot is also recorded, and has the
+  // same value as "Arc.boot_progress_enable_screen.FirstBoot".
+  std::unique_ptr<base::HistogramSamples> samples =
+      tester.GetHistogramSamplesSinceCreation(
+          "Arc." + std::string(kBootEvents.back()) + ".FirstBoot");
+  ASSERT_TRUE(samples.get());
+  tester.ExpectUniqueSample("Arc.AndroidBootTime.FirstBoot", samples->sum(), 1);
+}
+
+// Does the same but with negative values and FIRST_BOOT_AFTER_UPDATE.
+TEST_F(ArcMetricsServiceTest, ReportBootProgress_FirstBootAfterUpdate) {
+  // Start the full ARC container at t=10. Also set boot_progress_start to 5,
+  // boot_progress_preload_start to 7, and so on. This can actually happen
+  // because the mini container can finish up to boot_progress_preload_end
+  // before the full container is started.
+  constexpr uint64_t kArcStartTimeMs = 10;
+  SetArcStartTimeInMs(kArcStartTimeMs);
+  std::vector<mojom::BootProgressEventPtr> events(
+      GetBootProgressEvents(kArcStartTimeMs - 5, 2 /* step_in_ms */));
+
+  // Call ReportBootProgress() and then confirm that
+  // Arc.boot_progress_start.FirstBoot is recorded with 0 (ms),
+  // Arc.boot_progress_preload_start.FirstBoot is with 0 (ms), etc. Unlike our
+  // performance dashboard where negative performance numbers are treated as-is,
+  // UMA treats them as zeros.
+  base::HistogramTester tester;
+  // This time, use mojom::BootType::FIRST_BOOT_AFTER_UPDATE.
+  metrics_service()->ReportBootProgress(
+      std::move(events), mojom::BootType::FIRST_BOOT_AFTER_UPDATE);
+  base::RunLoop().RunUntilIdle();
+  for (size_t i = 0; i < kBootEvents.size(); ++i) {
+    const int expected = std::max<int>(0, i * 2 - 5);
+    tester.ExpectUniqueSample(
+        std::string("Arc.") + kBootEvents[i] + ".FirstBootAfterUpdate",
+        expected, 1);
+  }
+  std::unique_ptr<base::HistogramSamples> samples =
+      tester.GetHistogramSamplesSinceCreation(
+          "Arc." + std::string(kBootEvents.back()) + ".FirstBootAfterUpdate");
+  ASSERT_TRUE(samples.get());
+  tester.ExpectUniqueSample("Arc.AndroidBootTime.FirstBootAfterUpdate",
+                            samples->sum(), 1);
+}
+
+// Does the same but with REGULAR_BOOT.
+TEST_F(ArcMetricsServiceTest, ReportBootProgress_RegularBoot) {
+  constexpr uint64_t kArcStartTimeMs = 10;
+  SetArcStartTimeInMs(kArcStartTimeMs);
+  std::vector<mojom::BootProgressEventPtr> events(
+      GetBootProgressEvents(kArcStartTimeMs - 5, 2 /* step_in_ms */));
+
+  base::HistogramTester tester;
+  metrics_service()->ReportBootProgress(std::move(events),
+                                        mojom::BootType::REGULAR_BOOT);
+  base::RunLoop().RunUntilIdle();
+  for (size_t i = 0; i < kBootEvents.size(); ++i) {
+    const int expected = std::max<int>(0, i * 2 - 5);
+    tester.ExpectUniqueSample(
+        std::string("Arc.") + kBootEvents[i] + ".RegularBoot", expected, 1);
+  }
+  std::unique_ptr<base::HistogramSamples> samples =
+      tester.GetHistogramSamplesSinceCreation(
+          "Arc." + std::string(kBootEvents.back()) + ".RegularBoot");
+  ASSERT_TRUE(samples.get());
+  tester.ExpectUniqueSample("Arc.AndroidBootTime.RegularBoot", samples->sum(),
+                            1);
+}
+
+// Tests that no UMA is recorded when nothing is reported.
+TEST_F(ArcMetricsServiceTest, ReportBootProgress_EmptyResults) {
+  SetArcStartTimeInMs(100);
+  std::vector<mojom::BootProgressEventPtr> events;  // empty
+
+  base::HistogramTester tester;
+  metrics_service()->ReportBootProgress(std::move(events),
+                                        mojom::BootType::FIRST_BOOT);
+  base::RunLoop().RunUntilIdle();
+  for (size_t i = 0; i < kBootEvents.size(); ++i) {
+    tester.ExpectTotalCount(std::string("Arc.") + kBootEvents[i] + ".FirstBoot",
+                            0);
+  }
+  tester.ExpectTotalCount("Arc.AndroidBootTime.FirstBoot", 0);
+}
+
+// Tests that no UMA is recorded when BootType is invalid.
+TEST_F(ArcMetricsServiceTest, ReportBootProgress_InvalidBootType) {
+  SetArcStartTimeInMs(100);
+  std::vector<mojom::BootProgressEventPtr> events(
+      GetBootProgressEvents(123, 456));
+  base::HistogramTester tester;
+  metrics_service()->ReportBootProgress(std::move(events),
+                                        mojom::BootType::UNKNOWN);
+  base::RunLoop().RunUntilIdle();
+  for (const std::string& suffix :
+       {".FirstBoot", ".FirstBootAfterUpdate", ".RegularBoot"}) {
+    tester.ExpectTotalCount("Arc." + (kBootEvents.front() + suffix), 0);
+    tester.ExpectTotalCount("Arc." + (kBootEvents.back() + suffix), 0);
+    tester.ExpectTotalCount("Arc.AndroidBootTime" + suffix, 0);
+  }
+}
+
+}  // namespace
+}  // namespace arc

components/arc/test/DEPS

+specific_include_rules = {
+  "test_browser_context.h": [
+    "+content/public/test/test_browser_context.h",
+  ],
+  "test_browser_context.cc": [
+    "+components/user_prefs/user_prefs.h",
+  ]
+}
+

components/arc/test/test_browser_context.cc

+// Copyright 2017 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 "components/arc/test/test_browser_context.h"
+
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/user_prefs/user_prefs.h"
+
+namespace arc {
+
+TestBrowserContext::TestBrowserContext()
+    : browser_context_dependency_manager_(
+          BrowserContextDependencyManager::GetInstance()) {
+  user_prefs::UserPrefs::Set(this, &prefs_);
+}
+
+TestBrowserContext::~TestBrowserContext() {
+  browser_context_dependency_manager_->DestroyBrowserContextServices(this);
+}
+
+}  // namespace arc

components/arc/test/test_browser_context.h

+// Copyright 2017 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.
+
+#ifndef COMPONENTS_ARC_TEST_TEST_BROWSER_CONTEXT_H_
+#define COMPONENTS_ARC_TEST_TEST_BROWSER_CONTEXT_H_
+
+#include "base/macros.h"
+#include "components/prefs/testing_pref_service.h"
+#include "content/public/test/test_browser_context.h"
+
+class BrowserContextDependencyManager;
+
+namespace arc {
+
+// A browser context for testing that can be used for getting objects
+// through ArcBrowserContextKeyedServiceFactoryBase<>.
+class TestBrowserContext : public content::TestBrowserContext {
+ public:
+  TestBrowserContext();
+  ~TestBrowserContext() override;
+
+ private:
+  BrowserContextDependencyManager* const browser_context_dependency_manager_;
+  TestingPrefServiceSimple prefs_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestBrowserContext);
+};
+
+}  // namespace arc
+
+#endif  // COMPONENTS_ARC_TEST_TEST_BROWSER_CONTEXT_H_