Support ChromeOsUserReport in ReportUploader for Chrome OS

Previously, ReportUploader is designed for reporting
ChromeDesktopReportRequest on Windows, Mac and Linux. To supporting
Chrome OS, now we change this class a little bit to reuse its logic.

Bug: 1010213
Change-Id: I724d203534ec1a831700842181aec4f1121b6d13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1908542Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Commit-Queue: Anqing Zhao <anqing@google.com>
Cr-Commit-Position: refs/heads/master@{#715039}

chrome/browser/enterprise_reporting/report_uploader.cc

@@ -52,7 +52,7 @@ void ReportUploader::Upload() {
                                       weak_ptr_factory_.GetWeakPtr());
 
 #if defined(OS_CHROMEOS)
-  // Pending for another change list for cloud_policy_client.
+  client_->UploadChromeOsUserReport(std::move(request), std::move(callback));
 #else
   client_->UploadChromeDesktopReport(std::move(request), std::move(callback));
 #endif

chrome/browser/enterprise_reporting/report_uploader_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/task_environment.h"
+#include "build/build_config.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -20,13 +21,22 @@ using ::testing::WithArgs;
 
 namespace enterprise_reporting {
 namespace {
-constexpr const char* kOsUserNames[] = {"name1", "name2"};
+constexpr const char* kBrowserVersionNames[] = {"name1", "name2"};
 constexpr char kResponseMetricsName[] = "Enterprise.CloudReportingResponse";
 
 }  // namespace
 
 class ReportUploaderTest : public ::testing::Test {
  public:
+  // Different CloudPolicyClient proxy function will be used in test cases based
+  // on the current operation system. They share same retry and error handling
+  // behaviors provided by ReportUploader.
+#if defined(OS_CHROMEOS)
+#define UploadReportProxy UploadChromeOsUserReportProxy
+#else
+#define UploadReportProxy UploadChromeDesktopReportProxy
+#endif
+
   ReportUploaderTest()
       : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
     CreateUploader(0);
@@ -36,11 +46,13 @@ class ReportUploaderTest : public ::testing::Test {
   void UploadReportAndSetExpectation(
       int number_of_request,
       ReportUploader::ReportStatus expected_status) {
-    DCHECK_LE(number_of_request, 2) << "Please update kOsUserNames above.";
+    DCHECK_LE(number_of_request, 2)
+        << "Please update kBrowserVersionNames above.";
     ReportUploader::Requests requests;
     for (int i = 0; i < number_of_request; i++) {
-      auto request = std::make_unique<em::ChromeDesktopReportRequest>();
-      request->set_os_user_name(kOsUserNames[i]);
+      auto request = std::make_unique<ReportUploader::Request>();
+      request->mutable_browser_report()->set_browser_version(
+          kBrowserVersionNames[i]);
       requests.push(std::move(request));
     }
     has_responded_ = false;
@@ -96,7 +108,7 @@ class ReportUploaderTestWithTransientError
       public ::testing::WithParamInterface<policy::DeviceManagementStatus> {};
 
 TEST_F(ReportUploaderTest, Success) {
-  EXPECT_CALL(client_, UploadChromeDesktopReportProxy(_, _))
+  EXPECT_CALL(client_, UploadReportProxy(_, _))
       .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(true)));
   UploadReportAndSetExpectation(/*number_of_request=*/1,
                                 ReportUploader::kSuccess);
@@ -109,7 +121,7 @@ TEST_F(ReportUploaderTest, Success) {
 
 TEST_F(ReportUploaderTest, PersistentError) {
   CreateUploader(/* retry_count = */ 1);
-  EXPECT_CALL(client_, UploadChromeDesktopReportProxy(_, _))
+  EXPECT_CALL(client_, UploadReportProxy(_, _))
       .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)));
   client_.SetStatus(policy::DM_STATUS_SERVICE_DEVICE_NOT_FOUND);
   UploadReportAndSetExpectation(/*number_of_request=*/2,
@@ -123,7 +135,7 @@ TEST_F(ReportUploaderTest, PersistentError) {
 
 TEST_F(ReportUploaderTest, RequestTooBigError) {
   CreateUploader(/* *retyr_count = */ 2);
-  EXPECT_CALL(client_, UploadChromeDesktopReportProxy(_, _))
+  EXPECT_CALL(client_, UploadReportProxy(_, _))
       .Times(2)
       .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)))
       .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)));
@@ -139,7 +151,7 @@ TEST_F(ReportUploaderTest, RequestTooBigError) {
 }
 
 TEST_F(ReportUploaderTest, RetryAndSuccess) {
-  EXPECT_CALL(client_, UploadChromeDesktopReportProxy(_, _))
+  EXPECT_CALL(client_, UploadReportProxy(_, _))
       .Times(2)
       .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)))
       .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(true)));
@@ -163,7 +175,7 @@ TEST_F(ReportUploaderTest, RetryAndSuccess) {
 }
 
 TEST_F(ReportUploaderTest, RetryAndFailedWithPersistentError) {
-  EXPECT_CALL(client_, UploadChromeDesktopReportProxy(_, _))
+  EXPECT_CALL(client_, UploadReportProxy(_, _))
       .Times(2)
       .WillRepeatedly(WithArgs<1>(policy::ScheduleStatusCallback(false)));
   CreateUploader(/* retry_count = */ 1);
@@ -189,7 +201,7 @@ TEST_F(ReportUploaderTest, RetryAndFailedWithPersistentError) {
 }
 
 TEST_F(ReportUploaderTest, RetryAndFailedWithTransientError) {
-  EXPECT_CALL(client_, UploadChromeDesktopReportProxy(_, _))
+  EXPECT_CALL(client_, UploadReportProxy(_, _))
       .Times(2)
       .WillRepeatedly(WithArgs<1>(policy::ScheduleStatusCallback(false)));
   CreateUploader(/* retry_count = */ 1);
@@ -216,21 +228,23 @@ TEST_F(ReportUploaderTest, MultipleReports) {
   {
     InSequence s;
     // First report
-    EXPECT_CALL(client_,
-                UploadChromeDesktopReportProxy(
-                    Property(&em::ChromeDesktopReportRequest::os_user_name,
-                             Eq(kOsUserNames[0])),
-                    _))
+    EXPECT_CALL(
+        client_,
+        UploadReportProxy(Property(&ReportUploader::Request::browser_report,
+                                   Property(&em::BrowserReport::browser_version,
+                                            Eq(kBrowserVersionNames[0]))),
+                          _))
         .Times(3)
         .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)))
         .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)))
         .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(true)));
     // Second report
-    EXPECT_CALL(client_,
-                UploadChromeDesktopReportProxy(
-                    Property(&em::ChromeDesktopReportRequest::os_user_name,
-                             Eq(kOsUserNames[1])),
-                    _))
+    EXPECT_CALL(
+        client_,
+        UploadReportProxy(Property(&ReportUploader::Request::browser_report,
+                                   Property(&em::BrowserReport::browser_version,
+                                            Eq(kBrowserVersionNames[1]))),
+                          _))
         .Times(2)
         .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)))
         .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)));
@@ -265,7 +279,7 @@ TEST_F(ReportUploaderTest, MultipleReports) {
 
 // Verified three DM server error that is transient.
 TEST_P(ReportUploaderTestWithTransientError, WithoutRetry) {
-  EXPECT_CALL(client_, UploadChromeDesktopReportProxy(_, _))
+  EXPECT_CALL(client_, UploadReportProxy(_, _))
       .WillOnce(WithArgs<1>(policy::ScheduleStatusCallback(false)));
   client_.SetStatus(GetParam());
   UploadReportAndSetExpectation(/*number_of_request=*/2,

chrome/test/BUILD.gn

@@ -3708,6 +3708,7 @@ test("unit_tests") {
       "../browser/enterprise_reporting/profile_report_generator_unittest.cc",
       "../browser/enterprise_reporting/report_generator_unittest.cc",
       "../browser/enterprise_reporting/report_request_queue_generator_unittest.cc",
+      "../browser/enterprise_reporting/report_uploader_unittest.cc",
       "../browser/enterprise_reporting/request_timer_unittest.cc",
       "../browser/first_run/first_run_unittest.cc",
       "../browser/font_family_cache_unittest.cc",
@@ -3999,10 +4000,8 @@ test("unit_tests") {
     # TODO(anqing): finally these two set of unit test will be suitable to
     # other platforms, rather than Chrome OS only.
     if (!is_chromeos) {
-      sources += [
-        "../browser/enterprise_reporting/report_scheduler_unittest.cc",
-        "../browser/enterprise_reporting/report_uploader_unittest.cc",
-      ]
+      sources +=
+          [ "../browser/enterprise_reporting/report_scheduler_unittest.cc" ]
     }
   }
 

components/policy/core/common/cloud/mock_cloud_policy_client.h

@@ -75,6 +75,16 @@ class MockCloudPolicyClient : public CloudPolicyClient {
                void(enterprise_management::ChromeDesktopReportRequest*,
                     const StatusCallback&));
 
+  void UploadChromeOsUserReport(
+      std::unique_ptr<enterprise_management::ChromeOsUserReportRequest> request,
+      const StatusCallback& callback) override {
+    UploadChromeOsUserReportProxy(request.get(), callback);
+  }
+  // Use Proxy function because unique_ptr can't be used in mock function.
+  MOCK_METHOD2(UploadChromeOsUserReportProxy,
+               void(enterprise_management::ChromeOsUserReportRequest*,
+                    const StatusCallback&));
+
   MOCK_METHOD2(UploadRealtimeReport, void(base::Value, const StatusCallback&));
 
   // Sets the DMToken.