Add synchronous callback support to important_file_writer.cc

base/files/important_file_writer_unittest.cc

 // Copyright (c) 2012 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 "base/files/important_file_writer.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ skipping 32 matching lines @@
   }
 
  private:
   const std::string data_;
 };
 
 class SuccessfulWriteObserver {
  public:
   SuccessfulWriteObserver() : successful_write_observed_(false) {}
 
-  // Register on_successful_write() to be called on the next successful write
+  // Register OnSuccessfulWrite() to be called on the next successful write
   // of |writer|.
   void ObserveNextSuccessfulWrite(ImportantFileWriter* writer);
 
-  // Returns true if a successful write was observed via on_successful_write()
+  // Returns true if a successful write was observed via OnSuccessfulWrite()
   // and resets the observation state to false regardless.
   bool GetAndResetObservationState();
 
  private:
-  void on_successful_write() {
+  void OnSuccessfulWrite() {
     EXPECT_FALSE(successful_write_observed_);
     successful_write_observed_ = true;
   }
 
   bool successful_write_observed_;
 
   DISALLOW_COPY_AND_ASSIGN(SuccessfulWriteObserver);
 };
 
 void SuccessfulWriteObserver::ObserveNextSuccessfulWrite(
     ImportantFileWriter* writer) {
-  writer->RegisterOnNextSuccessfulWriteCallback(base::Bind(
-      &SuccessfulWriteObserver::on_successful_write, base::Unretained(this)));
+  writer->RegisterOnNextSuccessfulWriteReply(base::Bind(
+      &SuccessfulWriteObserver::OnSuccessfulWrite, base::Unretained(this)));
 }
 
 bool SuccessfulWriteObserver::GetAndResetObservationState() {
   bool was_successful_write_observed = successful_write_observed_;
   successful_write_observed_ = false;
   return was_successful_write_observed;
 }
 
+enum WriteCallbackObservationState {
+  NOT_CALLED,
+  CALLED_WITH_ERROR,
+  CALLED_WITH_SUCCESS,
+};
+
+class SynchronousWriteCallbackObserver {
+ public:
+  SynchronousWriteCallbackObserver() : observation_state_(NOT_CALLED) {}
+
+  // Register OnWrite() to be called on the next write of |writer|.
+  void ObserveNextWriteCallback(ImportantFileWriter* writer);
+
+  // Returns true if a write was observed via OnWrite()
+  // and resets the observation state to false regardless.
+  WriteCallbackObservationState GetAndResetObservationState();
+
+ private:
+  void OnWrite(bool success) {
+    EXPECT_EQ(NOT_CALLED, observation_state_);
+    observation_state_ = success ? CALLED_WITH_SUCCESS : CALLED_WITH_ERROR;
+  }
+
+  WriteCallbackObservationState observation_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(SynchronousWriteCallbackObserver);
+};
+
+void SynchronousWriteCallbackObserver::ObserveNextWriteCallback(
+    ImportantFileWriter* writer) {
+  writer->RegisterOnNextWriteSynchronousCallback(base::Bind(
+      &SynchronousWriteCallbackObserver::OnWrite, base::Unretained(this)));
+}
+
+WriteCallbackObservationState
+SynchronousWriteCallbackObserver::GetAndResetObservationState() {
+  WriteCallbackObservationState state = observation_state_;
+  observation_state_ = NOT_CALLED;
+  return state;
+}
+
 }  // namespace
 
 class ImportantFileWriterTest : public testing::Test {
  public:
   ImportantFileWriterTest() { }
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     file_ = temp_dir_.GetPath().AppendASCII("test-file");
   }
 
  protected:
-  SuccessfulWriteObserver successful_write_observer_;
+  SuccessfulWriteObserver successful_write_reply_observer_;
+  SynchronousWriteCallbackObserver write_callback_observer_;
   FilePath file_;
   MessageLoop loop_;
 
  private:
   ScopedTempDir temp_dir_;
 };
 
 TEST_F(ImportantFileWriterTest, Basic) {
   ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
   EXPECT_FALSE(PathExists(writer.path()));
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   writer.WriteNow(WrapUnique(new std::string("foo")));
   RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("foo", GetFileContent(writer.path()));
 }
 
-TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) {
-  ImportantFileWriter writer(file_, ThreadTaskRunnerHandle::Get());
+TEST_F(ImportantFileWriterTest, BasicWithWriteObservers) {
+  base::Thread file_writer_thread("ImportantFileWriter test thread");
+  file_writer_thread.Start();
+  base::WaitableEvent wait_helper(
+      base::WaitableEvent::ResetPolicy::AUTOMATIC,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  ImportantFileWriter writer(file_, file_writer_thread.task_runner());
+
   EXPECT_FALSE(PathExists(writer.path()));
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  successful_write_reply_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
+  writer.WriteNow(WrapUnique(new std::string("foo")));
+
+  // Make sure tasks posted by WriteNow() have ran before continuing.
+  file_writer_thread.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&base::WaitableEvent::Signal, base::Unretained(&wait_helper)));
+  wait_helper.Wait();

[gab, 2016/09/13 20:47:37]

Note: I just grew tired of having to do and suggest this pattern in multiple
places and am introducing base::Thread::FlushForTesting() in
https://codereview.chromium.org/2337253003/.

You're welcome to rebase on top of it, say this CL is on branch "foo" with
"master" as upstream then you can:

git checkout -b flushfortesting master
git cl patch 2337253003
git rebase flushfortesting foo
git branch -u flushfortesting

And the next time you git cl upload, Rietveld (and CQ) will know that you have a
pending dependency (the diff will be as expected and CQ dry-run will be able to
try the patch by also applying the dependent :-)).

[proberge, 2016/09/14 14:33:33]

Done. Thanks!

+
+  // The |successful_write_reply_observer_| should not yet have been
+  // notified yet but the |write_callback_observer_| should have been.
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
+
+  // There should be a pending task to notify |successful_write_reply_observer_|
+  // in this message loop.
+  RunLoop().RunUntilIdle();
+
+  // Confirm that the observer is invoked.
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
+  ASSERT_TRUE(PathExists(writer.path()));
+  EXPECT_EQ("foo", GetFileContent(writer.path()));
+
+  // Confirm that re-installing the observers works for another write.
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  successful_write_reply_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
+  writer.WriteNow(WrapUnique(new std::string("bar")));
+
+  file_writer_thread.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&base::WaitableEvent::Signal, base::Unretained(&wait_helper)));
+  wait_helper.Wait();
+
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_SUCCESS,
+            write_callback_observer_.GetAndResetObservationState());
+
+  RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(successful_write_reply_observer_.GetAndResetObservationState());
+  ASSERT_TRUE(PathExists(writer.path()));
+  EXPECT_EQ("bar", GetFileContent(writer.path()));
+
+  // Confirm that writing again without re-installing the observers doesn't
+  // result in a notification.
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  writer.WriteNow(WrapUnique(new std::string("baz")));
+
+  file_writer_thread.task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&base::WaitableEvent::Signal, base::Unretained(&wait_helper)));
+  wait_helper.Wait();
+  RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  ASSERT_TRUE(PathExists(writer.path()));
+  EXPECT_EQ("baz", GetFileContent(writer.path()));
+}
+
+TEST_F(ImportantFileWriterTest, FailedWriteWithWriteObservers) {
+  // Use an invalid file path (relative paths are invalid) to get a
+  // FILE_ERROR_ACCESS_DENIED error when trying to write the file.
+  ImportantFileWriter writer(FilePath().AppendASCII("bad/../path"),
+                             ThreadTaskRunnerHandle::Get());
+  EXPECT_FALSE(PathExists(writer.path()));
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(NOT_CALLED, write_callback_observer_.GetAndResetObservationState());
+  successful_write_reply_observer_.ObserveNextSuccessfulWrite(&writer);
+  write_callback_observer_.ObserveNextWriteCallback(&writer);
   writer.WriteNow(WrapUnique(new std::string("foo")));
   RunLoop().RunUntilIdle();
 
-  // Confirm that the observer is invoked.
-  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
-  ASSERT_TRUE(PathExists(writer.path()));
-  EXPECT_EQ("foo", GetFileContent(writer.path()));
-
-  // Confirm that re-installing the observer works for another write.
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
-  writer.WriteNow(WrapUnique(new std::string("bar")));
-  RunLoop().RunUntilIdle();
-
-  EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
-  ASSERT_TRUE(PathExists(writer.path()));
-  EXPECT_EQ("bar", GetFileContent(writer.path()));
-
-  // Confirm that writing again without re-installing the observer doesn't
-  // result in a notification.
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  writer.WriteNow(WrapUnique(new std::string("baz")));
-  RunLoop().RunUntilIdle();
-
-  EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  ASSERT_TRUE(PathExists(writer.path()));
-  EXPECT_EQ("baz", GetFileContent(writer.path()));
+  // Confirm that the successful write observer was not invoked, and that the
+  // write observer was invoked with its boolean parameter set to false.
+  EXPECT_FALSE(successful_write_reply_observer_.GetAndResetObservationState());
+  EXPECT_EQ(CALLED_WITH_ERROR,
+            write_callback_observer_.GetAndResetObservationState());
+  EXPECT_FALSE(PathExists(writer.path()));
 }
 
 TEST_F(ImportantFileWriterTest, ScheduleWrite) {
   ImportantFileWriter writer(file_,
                              ThreadTaskRunnerHandle::Get(),
                              TimeDelta::FromMilliseconds(25));
   EXPECT_FALSE(writer.HasPendingWrite());
   DataSerializer serializer("foo");
   writer.ScheduleWrite(&serializer);
   EXPECT_TRUE(writer.HasPendingWrite());
@@ skipping 32 matching lines @@
   writer.ScheduleWrite(&baz);
   ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, MessageLoop::QuitWhenIdleClosure(),
       TimeDelta::FromMilliseconds(100));
   RunLoop().Run();
   ASSERT_TRUE(PathExists(writer.path()));
   EXPECT_EQ("baz", GetFileContent(writer.path()));
 }
 
 }  // namespace base