Skip to content

Commit

Permalink
Add MakeUploadEncryptedReportAction
Browse files Browse the repository at this point in the history 
Add `MakeUploadEncryptedReportAction` to ease the process of writing
tests on responses.

The return type of `ResponseBuilder::Build` is also corrected to be the
same as `ResponseCallback`.

Change-Id: I3a70fdce5733a546635e38d7965d2db195c82721
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3565486
Reviewed-by: Leonid Baraz <lbaraz@chromium.org>
Commit-Queue: Hong Xu <xuhong@google.com>
Cr-Commit-Position: refs/heads/main@{#988119}
  • Loading branch information
@xuhdev
xuhdev authored and Chromium LUCI CQ committed Apr 1, 2022
1 parent 6472203 commit e07a2a9
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 82 deletions.
7 changes: 1 addition & 6 deletions chrome/browser/ash/dbus/encrypted_reporting_service_provider_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,12 +182,7 @@ TEST_F(EncryptedReportingServiceProviderTest,
EXPECT_CALL(
cloud_policy_client_,
UploadEncryptedReport(::reporting::IsDataUploadRequestValid(), _, _))
.WillOnce(WithArgs<0, 2>(
Invoke([](base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback callback) {
std::move(callback).Run(
::reporting::ResponseBuilder(std::move(request)).Build());
})));
.WillOnce(::reporting::MakeUploadEncryptedReportAction());

reporting::UploadEncryptedRecordRequest request;
request.add_encrypted_record()->CheckTypeAndMergeFrom(record_);
Expand Down
67 changes: 19 additions & 48 deletions chrome/browser/policy/messaging_layer/upload/record_handler_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,8 @@ TEST_P(RecordHandlerImplTest, ForwardsRecordsToCloudPolicyClient) {
.force_confirm = force_confirm()};

EXPECT_CALL(*client_, UploadEncryptedReport(IsDataUploadRequestValid(), _, _))
.WillOnce(WithArgs<0, 2>(
Invoke([&force_confirm_by_server](
base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback callback) {
std::move(callback).Run(
ResponseBuilder(std::move(request))
.SetForceConfirm(force_confirm_by_server)
.Build());
})));
.WillOnce(MakeUploadEncryptedReportAction(std::move(
ResponseBuilder().SetForceConfirm(force_confirm_by_server))));

test::TestEvent<SignedEncryptionInfo> encryption_key_attached_event;
test::TestEvent<DmServerUploadService::CompletionResponse> responder_event;
Expand Down Expand Up @@ -164,11 +157,10 @@ TEST_P(RecordHandlerImplTest, MissingPriorityField) {
Invoke([&force_confirm_by_server](
base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback callback) {
base::Value::Dict response =
ResponseBuilder(std::move(request))
.SetForceConfirm(force_confirm_by_server)
.Build();
response.RemoveByDottedPath("lastSucceedUploadedRecord.priority");
auto response = ResponseBuilder(std::move(request))
.SetForceConfirm(force_confirm_by_server)
.Build();
response->RemoveByDottedPath("lastSucceedUploadedRecord.priority");
std::move(callback).Run(std::move(response));
})));

Expand Down Expand Up @@ -200,12 +192,11 @@ TEST_P(RecordHandlerImplTest, InvalidPriorityField) {
Invoke([&force_confirm_by_server](
base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback callback) {
base::Value::Dict response =
ResponseBuilder(std::move(request))
.SetForceConfirm(force_confirm_by_server)
.Build();
response.SetByDottedPath("lastSucceedUploadedRecord.priority",
"abc");
auto response = ResponseBuilder(std::move(request))
.SetForceConfirm(force_confirm_by_server)
.Build();
response->SetByDottedPath("lastSucceedUploadedRecord.priority",
"abc");
std::move(callback).Run(std::move(response));
})));

Expand Down Expand Up @@ -251,9 +242,8 @@ TEST_P(RecordHandlerImplTest, ReportsUploadFailure) {
auto test_records = BuildTestRecordsVector(kNumTestRecords, kGenerationId);

EXPECT_CALL(*client_, UploadEncryptedReport(IsDataUploadRequestValid(), _, _))
.WillOnce(WithArgs<2>(Invoke(
[](base::OnceCallback<void(absl::optional<base::Value::Dict>)>
callback) { std::move(callback).Run(absl::nullopt); })));
.WillOnce(MakeUploadEncryptedReportAction(
std::move(ResponseBuilder().SetNull(true))));

test::TestEvent<DmServerUploadService::CompletionResponse> response_event;

Expand Down Expand Up @@ -290,24 +280,12 @@ TEST_P(RecordHandlerImplTest, UploadsGapRecordOnServerFailure) {
::testing::InSequence seq;
EXPECT_CALL(*client_,
UploadEncryptedReport(IsDataUploadRequestValid(), _, _))
.WillOnce(WithArgs<0, 2>(
Invoke([](base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback callback) {
std::move(callback).Run(ResponseBuilder(std::move(request))
.SetSuccess(false)
.Build());
})));
.WillOnce(MakeUploadEncryptedReportAction(
std::move(ResponseBuilder().SetSuccess(false))));
EXPECT_CALL(*client_,
UploadEncryptedReport(IsGapUploadRequestValid(), _, _))
.WillOnce(WithArgs<0, 2>(
Invoke([&force_confirm_by_server](
base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback callback) {
std::move(callback).Run(
ResponseBuilder(std::move(request))
.SetForceConfirm(force_confirm_by_server)
.Build());
})));
.WillOnce(MakeUploadEncryptedReportAction(std::move(
ResponseBuilder().SetForceConfirm(force_confirm_by_server))));
}

test::TestEvent<DmServerUploadService::CompletionResponse> response_event;
Expand Down Expand Up @@ -376,15 +354,8 @@ TEST_P(RecordHandlerImplTest, AssignsRequestIdForRecordUploads) {
.force_confirm = force_confirm()};

EXPECT_CALL(*client_, UploadEncryptedReport(IsDataUploadRequestValid(), _, _))
.WillOnce(WithArgs<0, 2>(
Invoke([&force_confirm_by_server](
base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback callback) {
std::move(callback).Run(
ResponseBuilder(std::move(request))
.SetForceConfirm(force_confirm_by_server)
.Build());
})));
.WillOnce(MakeUploadEncryptedReportAction(std::move(
ResponseBuilder().SetForceConfirm(force_confirm_by_server))));

test::TestEvent<DmServerUploadService::CompletionResponse> responder_event;
RecordHandlerImpl handler(client_.get());
Expand Down
12 changes: 2 additions & 10 deletions chrome/browser/policy/messaging_layer/upload/upload_client_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ TEST_P(UploadClientTest, CreateUploadClientAndUploadRecords) {
client->SetDMToken(
policy::DMToken::CreateValidTokenForTesting("FAKE_DM_TOKEN").value());

const bool force_confirm_flag = force_confirm();
static constexpr char matched_record_template[] =
R"JSON(
{
Expand Down Expand Up @@ -188,15 +187,8 @@ TEST_P(UploadClientTest, CreateUploadClientAndUploadRecords) {
DoesRequestContainRecord(base::StringPrintf(
matched_record_template, 9))),
_, _))
.WillOnce(WithArgs<0, 2>(
Invoke([&force_confirm_flag](
base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback response_cb) {
std::move(response_cb)
.Run(ResponseBuilder(std::move(request))
.SetForceConfirm(force_confirm_flag)
.Build());
})));
.WillOnce(MakeUploadEncryptedReportAction(
std::move(ResponseBuilder().SetForceConfirm(force_confirm()))));

test::TestMultiEvent<SequenceInformation, bool> upload_success;
UploadClient::ReportSuccessfulUploadCallback upload_success_cb =
Expand Down
7 changes: 1 addition & 6 deletions chrome/browser/policy/messaging_layer/upload/upload_provider_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,7 @@ TEST_F(EncryptedReportingUploadProviderTest, SuccessfullyUploadsRecord) {
});
EXPECT_CALL(cloud_policy_client_,
UploadEncryptedReport(IsDataUploadRequestValid(), _, _))
.WillOnce(WithArgs<0, 2>(
Invoke([](base::Value::Dict request,
policy::CloudPolicyClient::ResponseCallback response_cb) {
std::move(response_cb)
.Run(ResponseBuilder(std::move(request)).Build());
})));
.WillOnce(::reporting::MakeUploadEncryptedReportAction());

auto records = std::make_unique<std::vector<EncryptedRecord>>();
records->push_back(record_);
Expand Down
44 changes: 39 additions & 5 deletions chrome/browser/policy/messaging_layer/util/test_response_payload.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,39 @@ ResponseBuilder::ResponseBuilder(const base::Value::Dict& request)
ResponseBuilder::ResponseBuilder(base::Value::Dict&& request)
: request_(std::move(request)) {}

ResponseBuilder::ResponseBuilder(const ResponseBuilder& other)
: request_(other.request_.Clone()), params_(other.params_) {}

ResponseBuilder& ResponseBuilder::SetForceConfirm(bool force_confirm) {
force_confirm_ = force_confirm;
params_.force_confirm = force_confirm;
return *this;
}

ResponseBuilder& ResponseBuilder::SetNull(bool null) {
params_.null = null;
return *this;
}

ResponseBuilder& ResponseBuilder::SetRequest(const base::Value::Dict& request) {
request_ = request.Clone();
return *this;
}

ResponseBuilder& ResponseBuilder::SetRequest(base::Value::Dict&& request) {
request_ = std::move(request);
return *this;
}

ResponseBuilder& ResponseBuilder::SetSuccess(bool success) {
success_ = success;
params_.success = success;
return *this;
}

base::Value::Dict ResponseBuilder::Build() const {
absl::optional<base::Value::Dict> ResponseBuilder::Build() const {
if (params_.null) {
return absl::nullopt;
}

base::Value::Dict response;

// Attach sequenceInformation.
Expand All @@ -44,7 +66,7 @@ base::Value::Dict ResponseBuilder::Build() const {
const auto* const seq_info =
seq_info_it->GetDict().FindDict("sequenceInformation");
EXPECT_NE(seq_info, nullptr);
if (success_) {
if (params_.success) {
response.Set("lastSucceedUploadedRecord", seq_info->Clone());
} else {
response.SetByDottedPath("firstFailedUploadedRecord.failedUploadedRecord",
Expand All @@ -62,7 +84,7 @@ base::Value::Dict ResponseBuilder::Build() const {
}

// If forceConfirm confirm is expected, set it.
if (force_confirm_) {
if (params_.force_confirm) {
response.Set("forceConfirm", true);
}

Expand All @@ -84,4 +106,16 @@ base::Value::Dict ResponseBuilder::Build() const {
return response;
}

MakeUploadEncryptedReportAction::MakeUploadEncryptedReportAction(
ResponseBuilder&& response_builder)
: response_builder_(std::move(response_builder)) {}

void MakeUploadEncryptedReportAction::operator()(
base::Value::Dict request,
absl::optional<base::Value::Dict> context,
policy::CloudPolicyClient::ResponseCallback callback) {
response_builder_.SetRequest(std::move(request));
std::move(callback).Run(response_builder_.Build());
}

} // namespace reporting
62 changes: 55 additions & 7 deletions chrome/browser/policy/messaging_layer/util/test_response_payload.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@
#ifndef CHROME_BROWSER_POLICY_MESSAGING_LAYER_UTIL_TEST_RESPONSE_PAYLOAD_H_
#define CHROME_BROWSER_POLICY_MESSAGING_LAYER_UTIL_TEST_RESPONSE_PAYLOAD_H_

#include <cstddef>

#include "base/values.h"

#include "testing/gtest/include/gtest/gtest.h"
#include "components/policy/core/common/cloud/cloud_policy_client.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace reporting {

Expand All @@ -26,23 +24,73 @@ namespace reporting {
// .Build();
class ResponseBuilder {
public:
ResponseBuilder() = default;
// We need a copy constructor here because base::Value::Dict has deleted its
// copy constructor and gtest will implicitly copy a ResponseBuilder object
// when it uses |MakeUploadEncryptedReportAction|, which we cannot work
// around.
ResponseBuilder(const ResponseBuilder& other);
ResponseBuilder(ResponseBuilder&& other) = default;
explicit ResponseBuilder(const base::Value::Dict& request);
explicit ResponseBuilder(base::Value::Dict&& request);
ResponseBuilder& SetForceConfirm(bool force_confirm);
ResponseBuilder& SetNull(bool null);
ResponseBuilder& SetRequest(const base::Value::Dict& request);
ResponseBuilder& SetRequest(base::Value::Dict&& request);
ResponseBuilder& SetSuccess(bool success);

// Build the response to be fed to a
// |policy::CloudPolicyClient::ResponseCallback| object.
//
// Because we are focusing on testing here, the build here won't change
// members of the |ResponseBuilder| instance so that it can be called repeated
// for convenience. Additionally, this allows us to add more const qualifiers
// throughout the tests so that we reduce chances of incorrect test code
// (which itself isn't tested!).
base::Value::Dict Build() const;
absl::optional<base::Value::Dict> Build() const;

private:
// The request that was sent to the server.
base::Value::Dict request_;
bool success_ = true;
bool force_confirm_ = false;
// Parameters that can be tuned.
struct {
// Whether response should be a success or not
bool success = true;
// Whether forceConfirm is set to true or not
bool force_confirm = false;
// Whether response should be null
bool null = false;
} params_;
};

// Helper functor to be used with EXPECT_CALL and |UploadEncryptedReport|. It
// takes a |ResponseBuilder| object, and builds the response dict based on it
// and the input request from |UploadEncryptedReport|. Example:
//
//
// EXPECT_CALL(*client_,
// UploadEncryptedReport(IsDataUploadRequestValid(), _, _))
// .WillOnce(MakeUploadEncryptedReportAction(
// std::move(ResponseBuilder()
// .SetForceConfirm(force_confirm_by_server))));
//
// This class should be able to handle the majority of cases for defining mocked
// |UploadEncryptedReport| actions. In other circumstances, consider feeding a
// lambda function that uses |ResponseBuilder| to .WillOnce()/.WillRepeatedly().
//
// We don't define |MakeUploadEncryptedReportAction| a lambda function here
// because the mutable nature and the size of the argument list makes it far
// less readable.
class MakeUploadEncryptedReportAction {
public:
explicit MakeUploadEncryptedReportAction(
ResponseBuilder&& response_builder = ResponseBuilder());
void operator()(base::Value::Dict request,
absl::optional<base::Value::Dict> context,
policy::CloudPolicyClient::ResponseCallback callback);

private:
ResponseBuilder response_builder_;
};

} // namespace reporting
Expand Down

0 comments on commit e07a2a9

Please sign in to comment.