Refactoring of StorageModule-TestStorageModule

1) Make StorageModule to be instantiated only by factory method
2) Make StorageModule overridable by TestStorageModule
3) Allow TestStorageModule to be constructed directly

In the process I started to adjust tests, and ended up with rather
significant refactoring there too:
4) Use TestEvent instead of callback_ and result_ in the test class;
   it allows to make multiple calls in the same test fixture (not
   happening right now, but will likely be needed in the future).
5) Eliminate specialized test classes and use MOCK_METHOD + ON_CALL +
   optionally WillByDefault to achieve the same result. This makes sure
   that later changes in one test class are not skipped in another -
   we now have only one.

Bug: b:153364303
Change-Id: I4f7bcc956380c8b313ca4c2dc1f5ee6c716f586c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2277505Reviewed-by: Zach Trudo <zatrudo@google.com>
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#784494}

chrome/browser/policy/messaging_layer/encryption/encryption_module.h

@@ -15,7 +15,7 @@
 namespace reporting {
 
 // TODO(b/153659559) Temporary EncryptionModule until the real one is ready.
-class EncryptionModule : public base::RefCounted<EncryptionModule> {
+class EncryptionModule : public base::RefCountedThreadSafe<EncryptionModule> {
  public:
   EncryptionModule() = default;
 
@@ -30,7 +30,7 @@ class EncryptionModule : public base::RefCounted<EncryptionModule> {
   virtual ~EncryptionModule() = default;
 
  private:
-  friend base::RefCounted<EncryptionModule>;
+  friend base::RefCountedThreadSafe<EncryptionModule>;
 };
 
 }  // namespace reporting

chrome/browser/policy/messaging_layer/encryption/test_encryption_module.cc

@@ -8,18 +8,18 @@
 
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
 
+using ::testing::Invoke;
+
 namespace reporting {
 namespace test {
 
-StatusOr<std::string> TestEncryptionModule::EncryptRecord(
-    base::StringPiece record) const {
-  return std::string(record);
+TestEncryptionModule::TestEncryptionModule() {
+  ON_CALL(*this, EncryptRecord)
+      .WillByDefault(
+          Invoke([](base::StringPiece record) { return std::string(record); }));
 }
 
-StatusOr<std::string> AlwaysFailsEncryptionModule::EncryptRecord(
-    base::StringPiece record) const {
-  return Status(error::UNKNOWN, "Failing for tests");
-}
+TestEncryptionModule::~TestEncryptionModule() = default;
 
 }  // namespace test
 }  // namespace reporting

chrome/browser/policy/messaging_layer/encryption/test_encryption_module.h

@@ -9,6 +9,7 @@
 
 #include "chrome/browser/policy/messaging_layer/public/report_queue.h"
 #include "chrome/browser/policy/messaging_layer/util/statusor.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace reporting {
@@ -17,23 +18,15 @@ namespace test {
 // An |EncryptionModule| that does no encryption.
 class TestEncryptionModule : public EncryptionModule {
  public:
-  TestEncryptionModule() = default;
+  TestEncryptionModule();
 
-  StatusOr<std::string> EncryptRecord(base::StringPiece record) const override;
+  MOCK_METHOD(StatusOr<std::string>,
+              EncryptRecord,
+              (base::StringPiece record),
+              (const override));
 
  protected:
-  ~TestEncryptionModule() override = default;
-};
-
-// A |TestEncryptionModule| that always fails on |EncryptRecord| calls.
-class AlwaysFailsEncryptionModule final : public TestEncryptionModule {
- public:
-  AlwaysFailsEncryptionModule() = default;
-
-  StatusOr<std::string> EncryptRecord(base::StringPiece record) const override;
-
- protected:
-  ~AlwaysFailsEncryptionModule() override = default;
+  ~TestEncryptionModule() override;
 };
 
 }  // namespace test

chrome/browser/policy/messaging_layer/public/report_client.cc

@@ -21,8 +21,8 @@ namespace reporting {
 
 using base::MakeRefCounted;
 
-ReportingClient::ReportingClient()
-    : storage_(MakeRefCounted<StorageModule>()),
+ReportingClient::ReportingClient(scoped_refptr<StorageModule> storage)
+    : storage_(std::move(storage)),
       encryption_(MakeRefCounted<EncryptionModule>()) {}
 
 ReportingClient::~ReportingClient() = default;
@@ -43,11 +43,13 @@ StatusOr<ReportingClient*> ReportingClient::GetInstance() {
   return instance->ValueOrDie().get();
 }
 
-// TODO(chromium:1078512) As part of completing the StorageModule and
-// EncryptionModule, this create function will need to be updated to check for
-// successful creation of the StorageModule and EncryptionModule.
+// TODO(chromium:1078512) As part of completing the EncryptionModule,
+// this create function will need to be updated to check for
+// successful creation of the EncryptionModule too.
 StatusOr<std::unique_ptr<ReportingClient>> ReportingClient::Create() {
-  auto client = base::WrapUnique<ReportingClient>(new ReportingClient);
+  ASSIGN_OR_RETURN(scoped_refptr<StorageModule> storage,
+                   StorageModule::Create());
+  auto client = base::WrapUnique<ReportingClient>(new ReportingClient(storage));
   return client;
 }
 

chrome/browser/policy/messaging_layer/public/report_client.h

@@ -47,7 +47,7 @@ class ReportingClient {
       std::unique_ptr<ReportQueueConfiguration> config);
 
  private:
-  ReportingClient();
+  explicit ReportingClient(scoped_refptr<StorageModule> storage);
 
   static StatusOr<ReportingClient*> GetInstance();
 

chrome/browser/policy/messaging_layer/storage/storage.h

@@ -45,6 +45,7 @@ class Storage : public base::RefCountedThreadSafe<Storage> {
     virtual void ProcessBlob(Priority priority,
                              StatusOr<base::span<const uint8_t>> data,
                              base::OnceCallback<void(bool)> processed_cb) = 0;
+
     // Finalizes the upload (e.g. sends the message to the server and gets
     // response).
     virtual void Completed(Priority priority, Status final_status) = 0;

chrome/browser/policy/messaging_layer/storage/storage_module.cc

@@ -5,13 +5,19 @@
 #include <utility>
 
 #include "base/callback.h"
+#include "base/no_destructor.h"
 #include "chrome/browser/policy/messaging_layer/storage/storage_module.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
+#include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "components/policy/proto/record.pb.h"
 #include "components/policy/proto/record_constants.pb.h"
 
 namespace reporting {
 
+StorageModule::StorageModule() = default;
+
+StorageModule::~StorageModule() = default;
+
 void StorageModule::AddRecord(reporting::EncryptedRecord record,
                               reporting::Priority priority,
                               base::OnceCallback<void(Status)> callback) {
@@ -19,4 +25,12 @@ void StorageModule::AddRecord(reporting::EncryptedRecord record,
       Status(error::UNIMPLEMENTED, "AddRecord isn't implemented"));
 }
 
+// static
+StatusOr<scoped_refptr<StorageModule>> StorageModule::Create() {
+  scoped_refptr<StorageModule> instance =
+      // Cannot base::MakeRefCounted, since constructor is protected.
+      base::WrapRefCounted(new StorageModule());
+  return instance;
+}
+
 }  // namespace reporting

chrome/browser/policy/messaging_layer/storage/storage_module.h

@@ -10,15 +10,17 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/policy/messaging_layer/util/status.h"
+#include "chrome/browser/policy/messaging_layer/util/statusor.h"
 #include "components/policy/proto/record.pb.h"
 #include "components/policy/proto/record_constants.pb.h"
 
 namespace reporting {
 
 // TODO(b/153659559) Temporary StorageModule until the real one is ready.
-class StorageModule : public base::RefCounted<StorageModule> {
+class StorageModule : public base::RefCountedThreadSafe<StorageModule> {
  public:
-  StorageModule() = default;
+  // Factory method creates |StorageModule| object.
+  static StatusOr<scoped_refptr<StorageModule>> Create();
 
   StorageModule(const StorageModule& other) = delete;
   StorageModule& operator=(const StorageModule& other) = delete;
@@ -30,10 +32,14 @@ class StorageModule : public base::RefCounted<StorageModule> {
                          base::OnceCallback<void(Status)> callback);
 
  protected:
-  virtual ~StorageModule() = default;
+  // Constructor can only be called by |Create| factory method.
+  StorageModule();
+
+  // Refcounted object must have destructor declared protected or private.
+  virtual ~StorageModule();
 
  private:
-  friend base::RefCounted<StorageModule>;
+  friend base::RefCountedThreadSafe<StorageModule>;
 };
 
 }  // namespace reporting

chrome/browser/policy/messaging_layer/storage/test_storage_module.cc

@@ -10,29 +10,41 @@
 #include "chrome/browser/policy/messaging_layer/public/report_queue.h"
 #include "components/policy/proto/record.pb.h"
 #include "components/policy/proto/record_constants.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::testing::Invoke;
+
 namespace reporting {
 namespace test {
 
-using reporting::EncryptedRecord;
-using reporting::Priority;
+TestStorageModule::TestStorageModule() {
+  ON_CALL(*this, AddRecord)
+      .WillByDefault(Invoke(this, &TestStorageModule::AddRecordSuccessfully));
+}
 
-void TestStorageModule::AddRecord(EncryptedRecord record,
-                                  Priority priority,
-                                  base::OnceCallback<void(Status)> callback) {
-  ASSERT_TRUE(
-      wrapped_record_.ParseFromString(record.encrypted_wrapped_record()));
-  priority_ = priority;
-  std::move(callback).Run(Status::StatusOK());
+TestStorageModule::~TestStorageModule() = default;
+
+WrappedRecord TestStorageModule::wrapped_record() const {
+  EXPECT_TRUE(wrapped_record_.has_value());
+  return wrapped_record_.value();
 }
 
-void AlwaysFailsStorageModule::AddRecord(
+Priority TestStorageModule::priority() const {
+  EXPECT_TRUE(priority_.has_value());
+  return priority_.value();
+}
+
+void TestStorageModule::AddRecordSuccessfully(
     EncryptedRecord record,
     Priority priority,
     base::OnceCallback<void(Status)> callback) {
-  std::move(callback).Run(Status(error::UNKNOWN, "Failing for Tests"));
+  WrappedRecord wrapped_record;
+  ASSERT_TRUE(
+      wrapped_record.ParseFromString(record.encrypted_wrapped_record()));
+  wrapped_record_ = wrapped_record;
+  priority_ = priority;
+  std::move(callback).Run(Status::StatusOK());
 }
-
 }  // namespace test
 }  // namespace reporting

chrome/browser/policy/messaging_layer/storage/test_storage_module.h

@@ -8,46 +8,40 @@
 #include <utility>
 
 #include "base/callback.h"
+#include "base/optional.h"
 #include "chrome/browser/policy/messaging_layer/public/report_queue.h"
 #include "components/policy/proto/record.pb.h"
 #include "components/policy/proto/record_constants.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace reporting {
 namespace test {
 
-// A |StorageModule| that stores the wrapped record and priority and calls the
-// callback with an OK status.
 class TestStorageModule : public StorageModule {
  public:
-  TestStorageModule() = default;
+  TestStorageModule();
 
-  void AddRecord(reporting::EncryptedRecord record,
-                 reporting::Priority priority,
-                 base::OnceCallback<void(Status)> callback) override;
+  MOCK_METHOD(void,
+              AddRecord,
+              (EncryptedRecord record,
+               Priority priority,
+               base::OnceCallback<void(Status)> callback),
+              (override));
 
-  reporting::WrappedRecord wrapped_record() { return wrapped_record_; }
-
-  reporting::Priority priority() { return priority_; }
+  WrappedRecord wrapped_record() const;
+  Priority priority() const;
 
  protected:
-  ~TestStorageModule() override = default;
+  ~TestStorageModule() override;
 
  private:
-  reporting::WrappedRecord wrapped_record_;
-  reporting::Priority priority_;
-};
-
-// A |TestStorageModule| that always fails on |AddRecord| calls.
-class AlwaysFailsStorageModule final : public TestStorageModule {
- public:
-  AlwaysFailsStorageModule() = default;
-
-  void AddRecord(reporting::EncryptedRecord record,
-                 reporting::Priority priority,
-                 base::OnceCallback<void(Status)> callback) override;
+  void AddRecordSuccessfully(EncryptedRecord record,
+                             Priority priority,
+                             base::OnceCallback<void(Status)> callback);
 
- protected:
-  ~AlwaysFailsStorageModule() override = default;
+  base::Optional<WrappedRecord> wrapped_record_;
+  base::Optional<Priority> priority_;
 };
 
 }  // namespace test