Refactor tests to use TestEvent

Bug: None
Change-Id: Ic04c54b9b5db82f9c17d5abd2b4e79b2b9e15dcc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2461748
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Auto-Submit: Leonid Baraz <lbaraz@chromium.org>
Reviewed-by: Zach Trudo <zatrudo@google.com>
Cr-Commit-Position: refs/heads/master@{#815422}

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

@@ -29,52 +29,35 @@ namespace {
 //
 //   TestEvent<ResType> e;
 //   ... Do some async work passing e.cb() as a completion callback of
-//       base::OnceCallback<void(ResType* res)> type which also may perform
-//       some other action specified by |done| callback provided by the caller.
+//       base::OnceCallback<void(ResType* res)> type which also may perform some
+//       other action specified by |done| callback provided by the caller.
 //   ... = e.result();  // Will wait for e.cb() to be called and return the
-//                      // collected result.
-//
-// Or, when the callback is not expected to be invoked:
-//
-//   TestEvent<ResType> e(/*expected_to_complete=*/false);
-//   ... Start work passing e.cb() as a completion callback,
-//       which will not happen.
+//       collected result.
 //
 template <typename ResType>
 class TestEvent {
  public:
-  explicit TestEvent(bool expected_to_complete = true)
-      : expected_to_complete_(expected_to_complete),
-        completed_(base::WaitableEvent::ResetPolicy::MANUAL,
-                   base::WaitableEvent::InitialState::NOT_SIGNALED) {}
-  ~TestEvent() {
-    if (expected_to_complete_) {
-      EXPECT_TRUE(completed_.IsSignaled()) << "Not responded";
-    } else {
-      EXPECT_FALSE(completed_.IsSignaled()) << "Responded";
-    }
-  }
+  TestEvent() : run_loop_(std::make_unique<base::RunLoop>()) {}
+  ~TestEvent() { EXPECT_FALSE(run_loop_->running()) << "Not responded"; }
   TestEvent(const TestEvent& other) = delete;
   TestEvent& operator=(const TestEvent& other) = delete;
   ResType result() {
-    completed_.Wait();
+    run_loop_->Run();
     return std::forward<ResType>(result_);
   }
 
   // Completion callback to hand over to the processing method.
   base::OnceCallback<void(ResType res)> cb() {
-    DCHECK(!completed_.IsSignaled());
     return base::BindOnce(
-        [](base::WaitableEvent* completed, ResType* result, ResType res) {
+        [](base::RunLoop* run_loop, ResType* result, ResType res) {
           *result = std::forward<ResType>(res);
-          completed->Signal();
+          run_loop->Quit();
         },
-        base::Unretained(&completed_), base::Unretained(&result_));
+        base::Unretained(run_loop_.get()), base::Unretained(&result_));
   }
 
  private:
-  bool expected_to_complete_;
-  base::WaitableEvent completed_;
+  std::unique_ptr<base::RunLoop> run_loop_;
   ResType result_;
 };
 

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

@@ -28,52 +28,35 @@ namespace {
 //
 //   TestEvent<ResType> e;
 //   ... Do some async work passing e.cb() as a completion callback of
-//       base::OnceCallback<void(ResType* res)> type which also may perform
-//       some other action specified by |done| callback provided by the caller.
+//       base::OnceCallback<void(ResType* res)> type which also may perform some
+//       other action specified by |done| callback provided by the caller.
 //   ... = e.result();  // Will wait for e.cb() to be called and return the
-//                      // collected result.
-//
-// Or, when the callback is not expected to be invoked:
-//
-//   TestEvent<ResType> e(/*expected_to_complete=*/false);
-//   ... Start work passing e.cb() as a completion callback,
-//       which will not happen.
+//       collected result.
 //
 template <typename ResType>
 class TestEvent {
  public:
-  explicit TestEvent(bool expected_to_complete = true)
-      : expected_to_complete_(expected_to_complete),
-        completed_(base::WaitableEvent::ResetPolicy::MANUAL,
-                   base::WaitableEvent::InitialState::NOT_SIGNALED) {}
-  ~TestEvent() {
-    if (expected_to_complete_) {
-      EXPECT_TRUE(completed_.IsSignaled()) << "Not responded";
-    } else {
-      EXPECT_FALSE(completed_.IsSignaled()) << "Responded";
-    }
-  }
+  TestEvent() : run_loop_(std::make_unique<base::RunLoop>()) {}
+  ~TestEvent() { EXPECT_FALSE(run_loop_->running()) << "Not responded"; }
   TestEvent(const TestEvent& other) = delete;
   TestEvent& operator=(const TestEvent& other) = delete;
   ResType result() {
-    completed_.Wait();
+    run_loop_->Run();
     return std::forward<ResType>(result_);
   }
 
   // Completion callback to hand over to the processing method.
   base::OnceCallback<void(ResType res)> cb() {
-    DCHECK(!completed_.IsSignaled());
     return base::BindOnce(
-        [](base::WaitableEvent* completed, ResType* result, ResType res) {
+        [](base::RunLoop* run_loop, ResType* result, ResType res) {
           *result = std::forward<ResType>(res);
-          completed->Signal();
+          run_loop->Quit();
         },
-        base::Unretained(&completed_), base::Unretained(&result_));
+        base::Unretained(run_loop_.get()), base::Unretained(&result_));
   }
 
  private:
-  bool expected_to_complete_;
-  base::WaitableEvent completed_;
+  std::unique_ptr<base::RunLoop> run_loop_;
   ResType result_;
 };
 

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

@@ -1012,7 +1012,7 @@ class StorageQueue::ConfirmContext : public TaskRunnerContext<Status> {
 
   void OnStart() override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(confirm_sequence_checker_);
-    Response(storage_queue_->RemoveUnusedFiles(seq_number_));
+    Response(storage_queue_->RemoveConfirmedData(seq_number_));
   }
 
   // Confirmed sequencing number.
@@ -1028,7 +1028,7 @@ void StorageQueue::Confirm(uint64_t seq_number,
   Start<ConfirmContext>(seq_number, std::move(completion_cb), this);
 }
 
-Status StorageQueue::RemoveUnusedFiles(uint64_t seq_number) {
+Status StorageQueue::RemoveConfirmedData(uint64_t seq_number) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(storage_queue_sequence_checker_);
   if (first_seq_number_ <= seq_number) {
     first_seq_number_ = seq_number + 1;

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

@@ -12,6 +12,7 @@
 
 #include "base/callback.h"
 #include "base/files/file.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_refptr.h"
@@ -146,7 +147,7 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
   // Confirms acceptance of the records up to |seq_number| (inclusively).
   // All records with sequencing numbers <= this one can be removed from
   // the StorageQueue, and can no longer be uploaded.
-  // Helper methods: RemoveUnusedFiles.
+  // Helper methods: RemoveConfirmedData.
   void Confirm(uint64_t seq_number,
                base::OnceCallback<void(Status)> completion_cb);
 
@@ -256,6 +257,13 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
   // as the last one for the next record.
   void UpdateRecordDigest(WrappedRecord* wrapped_record);
 
+  // Helper method for Init(): process single data file.
+  // Return seq_number from <prefix>.<seq_number> file name, or Status
+  // in case there is any error.
+  StatusOr<uint64_t> AddDataFile(
+      const base::FilePath& full_name,
+      const base::FileEnumerator::FileInfo& file_info);
+
   // Helper method for Init(): enumerates all data files in the directory.
   // Valid file names are <prefix>.<seq_number>, any other names are ignored.
   Status EnumerateDataFiles();
@@ -312,9 +320,10 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
       uint64_t seq_number,
       std::vector<scoped_refptr<SingleFile>>* files) const;
 
-  // Helper method for Confirm: Removes files that only have records with seq
-  // numbers below or equal to |seq_number|.
-  Status RemoveUnusedFiles(uint64_t seq_number);
+  // Helper method for Confirm: Moves |first_seq_number_| to (|seq_number|+1)
+  // and removes files that only have records with seq numbers below or equal to
+  // |seq_number| (below |first_seq_number_|).
+  Status RemoveConfirmedData(uint64_t seq_number);
 
   // Immutable options, stored at the time of creation.
   const Options options_;
@@ -332,8 +341,8 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
   // Next sequencing number to store (not assigned yet).
   uint64_t next_seq_number_ = 0;
 
-  // First unconfirmed sequencing number (no records with lower
-  // sequencing number are guaranteed to exist in store).
+  // First sequencing number store still has (no records with lower
+  // sequencing number exist in store).
   uint64_t first_seq_number_ = 0;
 
   // Ordered map of the files by ascending sequence number.

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

@@ -47,30 +47,27 @@ namespace {
 template <typename ResType>
 class TestEvent {
  public:
-  TestEvent()
-      : completed_(base::WaitableEvent::ResetPolicy::MANUAL,
-                   base::WaitableEvent::InitialState::NOT_SIGNALED) {}
-  ~TestEvent() { EXPECT_TRUE(completed_.IsSignaled()) << "Not responded"; }
+  TestEvent() : run_loop_(std::make_unique<base::RunLoop>()) {}
+  ~TestEvent() { EXPECT_FALSE(run_loop_->running()) << "Not responded"; }
   TestEvent(const TestEvent& other) = delete;
   TestEvent& operator=(const TestEvent& other) = delete;
   ResType result() {
-    completed_.Wait();
+    run_loop_->Run();
     return std::forward<ResType>(result_);
   }
 
   // Completion callback to hand over to the processing method.
   base::OnceCallback<void(ResType res)> cb() {
-    DCHECK(!completed_.IsSignaled());
     return base::BindOnce(
-        [](base::WaitableEvent* completed, ResType* result, ResType res) {
+        [](base::RunLoop* run_loop, ResType* result, ResType res) {
           *result = std::forward<ResType>(res);
-          completed->Signal();
+          run_loop->Quit();
         },
-        base::Unretained(&completed_), base::Unretained(&result_));
+        base::Unretained(run_loop_.get()), base::Unretained(&result_));
   }
 
  private:
-  base::WaitableEvent completed_;
+  std::unique_ptr<base::RunLoop> run_loop_;
   ResType result_;
 };
 

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

@@ -49,30 +49,27 @@ namespace {
 template <typename ResType>
 class TestEvent {
  public:
-  TestEvent()
-      : completed_(base::WaitableEvent::ResetPolicy::MANUAL,
-                   base::WaitableEvent::InitialState::NOT_SIGNALED) {}
-  ~TestEvent() { EXPECT_TRUE(completed_.IsSignaled()) << "Not responded"; }
+  TestEvent() : run_loop_(std::make_unique<base::RunLoop>()) {}
+  ~TestEvent() { EXPECT_FALSE(run_loop_->running()) << "Not responded"; }
   TestEvent(const TestEvent& other) = delete;
   TestEvent& operator=(const TestEvent& other) = delete;
   ResType result() {
-    completed_.Wait();
+    run_loop_->Run();
     return std::forward<ResType>(result_);
   }
 
   // Completion callback to hand over to the processing method.
   base::OnceCallback<void(ResType res)> cb() {
-    DCHECK(!completed_.IsSignaled());
     return base::BindOnce(
-        [](base::WaitableEvent* completed, ResType* result, ResType res) {
+        [](base::RunLoop* run_loop, ResType* result, ResType res) {
           *result = std::forward<ResType>(res);
-          completed->Signal();
+          run_loop->Quit();
         },
-        base::Unretained(&completed_), base::Unretained(&result_));
+        base::Unretained(run_loop_.get()), base::Unretained(&result_));
   }
 
  private:
-  base::WaitableEvent completed_;
+  std::unique_ptr<base::RunLoop> run_loop_;
   ResType result_;
 };
 

components/policy/proto/record.proto

@@ -90,10 +90,16 @@ message SequencingInformation {
 
 // |WrappedRecord| as it is stored on disc, and sent to the server.
 message EncryptedRecord {
-  // Encrypted Wrapped Record (required)
+  // Encrypted Wrapped Record
   // |WrappedRecord| encrypted with the |encryption_key| in |encryption_info|.
+  // When absent, indicates gap - respective record is irreparably corrupt or
+  // missing from Storage, and server side should log it as such and no longer
+  // expect client to deliver it.
   optional bytes encrypted_wrapped_record = 1;
 
+  // Must be present to facilitate decryption of encrypted record.
+  // If missing, the record is either not encrypted or missing.
+  // TODO(b/153651358): Disable an option to send record not encrypted.
   optional EncryptionInfo encryption_info = 2;
 
   optional SequencingInformation sequencing_information = 3;