Add a mock class for DiskMountManagerObserver. disk_mount_manager_observer_unittests will be rewrit…

chromeos/disks/disk_mount_manager_unittest.cc

Index: chromeos/disks/disk_mount_manager_unittest.cc
diff --git a/chromeos/disks/disk_mount_manager_unittest.cc b/chromeos/disks/disk_mount_manager_unittest.cc
index e104750629944de22db7f010562278d0808636f4..1f5e427ff80dd525b343aa12ea7b37aa2bcb8fa4 100644
--- a/chromeos/disks/disk_mount_manager_unittest.cc
+++ b/chromeos/disks/disk_mount_manager_unittest.cc
@@ -7,20 +7,19 @@
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/stringprintf.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_cros_disks_client.h"
 #include "chromeos/disks/disk_mount_manager.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::StringPrintf;
 using chromeos::disks::DiskMountManager;
 using chromeos::CrosDisksClient;
 using chromeos::DBusThreadManager;
 using chromeos::FakeCrosDisksClient;
-using testing::_;
-using testing::Field;
-using testing::InSequence;
-using testing::InvokeWithoutArgs;
+using chromeos::MountType;
+using chromeos::disks::MountCondition;
 
 namespace {
 
@@ -151,36 +150,325 @@ const TestMountPointInfo kTestMountPoints[] = {
   },
 };
 
-// Mocks DiskMountManager observer.
+// Event classes which represent each invocation of functions in |Observer|.
+// OnDeviceEvent()
+struct DeviceEvent {
+  DiskMountManager::DeviceEvent event_;
+  std::string device_path_;

[satorux1, 2016/08/31 07:29:03]

please drop the trailing _ from struct members

https://google.github.io/styleguide/cppguide.html#Variable_Names

[yamaguchi, 2016/08/31 10:10:21]

Done.

+
+  DeviceEvent() {}
+
+  DeviceEvent(DiskMountManager::DeviceEvent event,
+              const std::string& device_path)
+      : event_(event), device_path_(device_path) {}
+
+  DeviceEvent(const DeviceEvent& other)
+      : event_(other.event_), device_path_(other.device_path_) {}
+
+  bool operator==(const DeviceEvent& other) const {
+    return event_ == other.event_ && device_path_ == other.device_path_;
+  }

[satorux1, 2016/08/31 07:29:03]

is it possible to remove the constructors and the operator==()? a simpler struct
would be nicer if it works.

[yamaguchi, 2016/08/31 10:10:21]

- The blank default constructors are needed to avoid error in
ObserverEvent::Event, as it leaves the other 3 fields unused (not calling
explicit constructor).
- The copy constructor is not needed at the moment. Removed.
- The constructor with every field values are used because I preferred to write
tests in this style, but could be changed.
EXPECT_EQ(FormatEvent(...), observer_.GetFormatEvent(...))
- operator== is required for the same reason as above.

+
+  std::string DebugString() const {
+    return StringPrintf("OnDeviceEvent(%d, %s)", event_, device_path_.c_str());
+  }
+};
+
+// OnDiskEvent()
+struct DiskEvent {
+  DiskMountManager::DiskEvent event_;
+  const DiskMountManager::Disk* disk_;
+
+  DiskEvent() {}
+
+  DiskEvent(DiskMountManager::DiskEvent event,
+            const DiskMountManager::Disk* disk)
+      : event_(event), disk_(disk) {}
+
+  DiskEvent(const DiskEvent& other)
+      : event_(other.event_), disk_(other.disk_) {}
+
+  DiskEvent& operator=(const DiskEvent& other) {
+    event_ = other.event_;
+    disk_ = other.disk_;
+    return *this;
+  }
+
+  bool operator==(const DiskEvent& other) const {
+    return event_ == other.event_ && disk_ == other.disk_;
+  }
+
+  std::string DebugString() const {
+    return StringPrintf("OnDiskEvent(event=%d, device_path=%s, mount_path=%s",
+                        event_, disk_->device_path().c_str(),
+                        disk_->mount_path().c_str());
+  }
+};
+
+// OnFormatEvent()
+struct FormatEvent {
+  DiskMountManager::FormatEvent event_;
+  chromeos::FormatError error_code_;
+  std::string device_path_;
+
+  FormatEvent() {}
+  FormatEvent(DiskMountManager::FormatEvent event,
+              chromeos::FormatError error_code,
+              const std::string& device_path)
+      : event_(event), error_code_(error_code), device_path_(device_path) {}
+
+  bool operator==(const FormatEvent& other) const {
+    return event_ == other.event_ && error_code_ == other.error_code_ &&
+           device_path_ == other.device_path_;
+  }
+
+  std::string DebugString() const {
+    return StringPrintf("OnFormatEvent(%d, %d, %s)", event_, error_code_,
+                        device_path_.c_str());
+  }
+};
+
+// OnMountEvent()
+struct MountEvent {
+  DiskMountManager::MountEvent event_;
+  chromeos::MountError error_code_;
+  DiskMountManager::MountPointInfo mount_point_;
+
+  // Not passed to callback, but read by handlers. So it's captured upon
+  // callback.
+  std::shared_ptr<DiskMountManager::Disk> disk_;
+
+  MountEvent()
+      : mount_point_("",
+                     "",
+                     MountType::MOUNT_TYPE_INVALID,
+                     MountCondition::MOUNT_CONDITION_NONE) {}
+  ~MountEvent() {}
+
+  MountEvent(const MountEvent& other)
+      : event_(other.event_),
+        error_code_(other.error_code_),
+        mount_point_(other.mount_point_),
+        disk_(other.disk_) {}
+  MountEvent(DiskMountManager::MountEvent event,
+             chromeos::MountError error_code,
+             const DiskMountManager::MountPointInfo& mount_point,
+             std::shared_ptr<DiskMountManager::Disk> disk)
+      : event_(event),
+        error_code_(error_code),
+        mount_point_(mount_point),
+        disk_(disk) {}
+
+  bool operator==(const MountEvent& other) const;
+
+  std::string DebugString() const {
+    return StringPrintf("OnMountEvent(%d, %d, %s, %s, %d, %d)", event_,
+                        error_code_, mount_point_.source_path.c_str(),
+                        mount_point_.mount_path.c_str(),
+                        mount_point_.mount_type, mount_point_.mount_condition);
+  }
+};
+
+// Represents every event notified to |Observer|.
+class ObserverEvent {
+ private:
+  struct Event {
+    DeviceEvent device;
+    DiskEvent disk;
+    FormatEvent format;
+    MountEvent mount;
+    Event() {}
+    Event(const Event& other)
+        : device(other.device),
+          disk(other.disk),
+          format(other.format),
+          mount(other.mount) {}
+    ~Event() {}
+  } event_;
+  ObserverEvent() {}
+
+ public:
+  enum ObserverEventType {
+    DEVICE_EVENT,
+    DISK_EVENT,
+    FORMAT_EVENT,
+    MOUNT_EVENT
+  } type_;
+
+  static ObserverEvent FromDeviceEvent(DeviceEvent event) {
+    ObserverEvent result;
+    result.type_ = DEVICE_EVENT;
+    result.event_.device = event;
+    return result;
+  }
+
+  static ObserverEvent FromDiskEvent(DiskEvent event) {
+    ObserverEvent result;
+    result.type_ = DISK_EVENT;
+    result.event_.disk = event;
+    return result;
+  }
+
+  static ObserverEvent FromFormatEvent(FormatEvent event) {
+    ObserverEvent result;
+    result.type_ = FORMAT_EVENT;
+    result.event_.format = event;
+    return result;
+  }
+
+  static ObserverEvent FromMountEvent(MountEvent event) {
+    ObserverEvent result;
+    result.type_ = MOUNT_EVENT;
+    result.event_.mount = event;
+    return result;
+  }
+
+  ObserverEvent(const ObserverEvent& other) : type_(other.type_) {
+    switch (other.type_) {
+      case DEVICE_EVENT:
+        event_.device = other.event_.device;
+        break;
+      case DISK_EVENT:
+        event_.disk = other.event_.disk;
+        break;
+      case FORMAT_EVENT:
+        event_.format = other.event_.format;
+        break;
+      case MOUNT_EVENT:
+        event_.mount = other.event_.mount;
+        break;
+    }
+  }
+
+  const Event& GetEvent() const { return event_; }
+
+  ObserverEventType GetType() const { return type_; }
+
+  std::string DebugString() const {
+    switch (type_) {
+      case DEVICE_EVENT:
+        return event_.device.DebugString();
+      case DISK_EVENT:
+        return event_.disk.DebugString();
+      case FORMAT_EVENT:
+        return event_.format.DebugString();
+      case MOUNT_EVENT:
+        return event_.mount.DebugString();
+    }
+    return "(Unknown event type)";
+  }
+};
+
+// A mock |Observer| class which records all invocation of the methods invoked
+// from DiskMountManager and all the arguments passed to them.
 class MockDiskMountManagerObserver : public DiskMountManager::Observer {
  public:
-  virtual ~MockDiskMountManagerObserver() {}
-
-  MOCK_METHOD2(OnDiskEvent, void(DiskMountManager::DiskEvent event,
-                                 const DiskMountManager::Disk* disk));
-  MOCK_METHOD2(OnDeviceEvent, void(DiskMountManager::DeviceEvent event,
-                                   const std::string& device_path));
-  MOCK_METHOD3(OnMountEvent,
-      void(DiskMountManager::MountEvent event,
-           chromeos::MountError error_code,
-           const DiskMountManager::MountPointInfo& mount_point));
-  MOCK_METHOD3(OnFormatEvent,
-      void(DiskMountManager::FormatEvent event,
-           chromeos::FormatError error_code,
-           const std::string& device_path));
+  MockDiskMountManagerObserver(const DiskMountManager* manager)
+      : manager_(manager) {}
+  ~MockDiskMountManagerObserver() override {}
+
+  // Mock notify methods.
+  void OnDeviceEvent(DiskMountManager::DeviceEvent event,
+                     const std::string& device_path) override {
+    events_.push_back(
+        ObserverEvent::FromDeviceEvent(DeviceEvent(event, device_path)));
+  }
+
+  void OnDiskEvent(DiskMountManager::DiskEvent event,
+                   const DiskMountManager::Disk* disk) override {
+    // Take a snapshot (copy) of the Disk object at the time of invocation for
+    // later verification.
+    events_.push_back(ObserverEvent::FromDiskEvent(
+        DiskEvent(event, new DiskMountManager::Disk(*disk))));
+  }
+
+  void OnFormatEvent(DiskMountManager::FormatEvent event,
+                     chromeos::FormatError error_code,
+                     const std::string& device_path) override {
+    events_.push_back(ObserverEvent::FromFormatEvent(
+        FormatEvent(event, error_code, device_path)));
+  }
+
+  void OnMountEvent(
+      DiskMountManager::MountEvent event,
+      chromeos::MountError error_code,
+      const DiskMountManager::MountPointInfo& mount_point) override {
+    // Take a snapshot (copy) of a Disk object at the time of invocation.
+    // It can be verified later besides the arguments.
+    events_.push_back(ObserverEvent::FromMountEvent(MountEvent(
+        event, error_code, mount_point,
+        std::shared_ptr<DiskMountManager::Disk>(new DiskMountManager::Disk(
+            *manager_->disks().find(mount_point.source_path)->second)))));
+  }
+
+  // Get invocation history to be verified by testcases.
+  DeviceEvent GetDeviceEvent(size_t index) {
+    DCHECK_GE(events_.size(), index + 1);
+    DCHECK(ObserverEvent::DEVICE_EVENT == events_.at(index).GetType());
+    return events_[index].GetEvent().device;
+  }
+
+  DiskEvent GetDiskEvent(size_t index) {
+    DCHECK_GE(events_.size(), index + 1);
+    DCHECK(ObserverEvent::DISK_EVENT == events_.at(index).GetType());
+    return events_[index].GetEvent().disk;
+  }
+
+  FormatEvent GetFormatEvent(size_t index) {
+    DCHECK_GE(events_.size(), index + 1);
+    DCHECK(ObserverEvent::FORMAT_EVENT == events_.at(index).GetType());
+    return events_[index].GetEvent().format;
+  }
+
+  MountEvent GetMountEvent(size_t index) {
+    DCHECK_GE(events_.size(), index + 1);
+    DCHECK(ObserverEvent::MOUNT_EVENT == events_.at(index).GetType());
+    return events_[index].GetEvent().mount;
+  }
+
+  size_t GetEventCount() { return events_.size(); }
+
+  const ObserverEvent& GetEvent(size_t index) const {
+    return events_.at(index);
+  }
+
+  const std::vector<ObserverEvent>& GetEvents() const { return events_; }
+
+ private:
+  // Pointer to the manager object to which this |Observer| is registered.
+  const DiskMountManager* manager_;
+
+  // Records all invocations.
+  std::vector<ObserverEvent> events_;
 };
 
-// Expect |is_read_only| value of a disk object keyed by |source_path|.
-void ExpectDiskReadOnly(const DiskMountManager* manager,
-                        const std::string& source_path,
-                        bool expected) {
-  EXPECT_EQ(expected,
-            manager->disks().find(source_path)->second->is_read_only());
+// Shift operators of ostream.
+// Needed to print values in case of EXPECT_* failure in gtest.

[satorux1, 2016/08/31 07:29:03]

If there are not many places, maybe the following is suffice:

EXPECT_EQ(foo, bar) << foo.DebugString() << " != " bar.DebugString()

[yamaguchi, 2016/08/31 10:10:21]

Currently it's used 11 times in the tests.
So I think having these would make some sense.

+std::ostream& operator<<(std::ostream& stream,
+                         const DeviceEvent& device_event) {
+  return stream << device_event.DebugString();
+}
+
+std::ostream& operator<<(std::ostream& stream, const DiskEvent& disk_event) {
+  return stream << disk_event.DebugString();
+}
+
+std::ostream& operator<<(std::ostream& stream,
+                         const FormatEvent& format_event) {
+  return stream << format_event.DebugString();
+}
+
+std::ostream& operator<<(std::ostream& stream, const MountEvent& mount_event) {
+  return stream << mount_event.DebugString();
+}
+
+std::ostream& operator<<(std::ostream& stream, const ObserverEvent& event) {
+  return stream << event.DebugString();
 }
 
 class DiskMountManagerTest : public testing::Test {
  public:
-  DiskMountManagerTest() {}
+  DiskMountManagerTest() : observer_(NULL) {}
   ~DiskMountManagerTest() override {}
 
   // Sets up test dbus tread manager and disks mount manager.
@@ -195,6 +483,7 @@ class DiskMountManagerTest : public testing::Test {
 
     InitDisksAndMountPoints();
 
+    observer_ = MockDiskMountManagerObserver(DiskMountManager::GetInstance());
     DiskMountManager::GetInstance()->AddObserver(&observer_);
   }
 
@@ -271,32 +560,28 @@ class DiskMountManagerTest : public testing::Test {
 // Tests that the observer gets notified on attempt to format non existent mount
 // point.
 TEST_F(DiskMountManagerTest, Format_NotMounted) {
-  EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                                       chromeos::FORMAT_ERROR_UNKNOWN,
-                                       "/mount/non_existent"))
-      .Times(1);
   DiskMountManager::GetInstance()->FormatMountedDevice("/mount/non_existent");
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                        chromeos::FORMAT_ERROR_UNKNOWN, "/mount/non_existent"),
+            observer_.GetFormatEvent(0));
 }
 
 // Tests that the observer gets notified on attempt to format read-only mount
 // point.
 TEST_F(DiskMountManagerTest, Format_ReadOnly) {
-  EXPECT_CALL(observer_,
-              OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                            chromeos::FORMAT_ERROR_DEVICE_NOT_ALLOWED,
-                            kReadOnlyMountpath))
-      .Times(1);
   DiskMountManager::GetInstance()->FormatMountedDevice(kReadOnlyMountpath);
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                        chromeos::FORMAT_ERROR_DEVICE_NOT_ALLOWED,
+                        kReadOnlyMountpath),
+            observer_.GetFormatEvent(0));
 }
 
 // Tests that it is not possible to format archive mount point.
 TEST_F(DiskMountManagerTest, Format_Archive) {
-  EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                                       chromeos::FORMAT_ERROR_UNKNOWN,
-                                       "/archive/source_path"))
-      .Times(1);
-
   DiskMountManager::GetInstance()->FormatMountedDevice("/archive/mount_path");
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                        chromeos::FORMAT_ERROR_UNKNOWN, "/archive/source_path"),
+            observer_.GetFormatEvent(0));
 }
 
 // Tests that format fails if the device cannot be unmounted.
@@ -305,25 +590,6 @@ TEST_F(DiskMountManagerTest, Format_FailToUnmount) {
   // In this test unmount will fail, and there should be no attempt to
   // format the device.
 
-  // Set up expectations for observer mock.
-  // Observer should be notified that unmount attempt fails and format task
-  // failed to start.
-  {
-    InSequence s;
-
-    EXPECT_CALL(observer_,
-        OnMountEvent(DiskMountManager::UNMOUNTING,
-                     chromeos::MOUNT_ERROR_INTERNAL,
-                     Field(&DiskMountManager::MountPointInfo::mount_path,
-                           "/device/mount_path")))
-        .Times(1);
-
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                                         chromeos::FORMAT_ERROR_UNKNOWN,
-                                         "/device/source_path"))
-        .Times(1);
-  }
-
   fake_cros_disks_client_->MakeUnmountFail();
   // Start test.
   DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
@@ -331,6 +597,16 @@ TEST_F(DiskMountManagerTest, Format_FailToUnmount) {
   // Cros disks will respond asynchronoulsy, so let's drain the message loop.
   message_loop_.RunUntilIdle();
 
+  // Observer should be notified that unmount attempt fails and format task
+  // failed to start.
+  MountEvent mount_event = observer_.GetMountEvent(0);
+  EXPECT_EQ(DiskMountManager::UNMOUNTING, mount_event.event_);
+  EXPECT_EQ(chromeos::MOUNT_ERROR_INTERNAL, mount_event.error_code_);
+  EXPECT_EQ("/device/mount_path", mount_event.mount_point_.mount_path);
+
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                        chromeos::FORMAT_ERROR_UNKNOWN, "/device/source_path"),
+            observer_.GetFormatEvent(1));
   EXPECT_EQ(1, fake_cros_disks_client_->unmount_call_count());
   EXPECT_EQ("/device/mount_path",
             fake_cros_disks_client_->last_unmount_device_path());
@@ -349,25 +625,6 @@ TEST_F(DiskMountManagerTest, Format_FormatFailsToStart) {
   // In this test, unmount will succeed, but call to Format method will
   // fail.
 
-  // Set up expectations for observer mock.
-  // Observer should be notified that the device was unmounted and format task
-  // failed to start.
-  {
-    InSequence s;
-
-    EXPECT_CALL(observer_,
-        OnMountEvent(DiskMountManager::UNMOUNTING,
-                     chromeos::MOUNT_ERROR_NONE,
-                     Field(&DiskMountManager::MountPointInfo::mount_path,
-                           "/device/mount_path")))
-        .Times(1);
-
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                                         chromeos::FORMAT_ERROR_UNKNOWN,
-                                         "/device/source_path"))
-        .Times(1);
-  }
-
   fake_cros_disks_client_->MakeFormatFail();
   // Start the test.
   DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
@@ -375,6 +632,17 @@ TEST_F(DiskMountManagerTest, Format_FormatFailsToStart) {
   // Cros disks will respond asynchronoulsy, so let's drain the message loop.
   message_loop_.RunUntilIdle();
 
+  // Observer should be notified that the device was unmounted and format task
+  // failed to start.
+  MountEvent mount_event = observer_.GetMountEvent(0);
+  EXPECT_EQ(DiskMountManager::UNMOUNTING, mount_event.event_);
+  EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, mount_event.error_code_);
+  EXPECT_EQ("/device/mount_path", mount_event.mount_point_.mount_path);
+
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                        chromeos::FORMAT_ERROR_UNKNOWN, "/device/source_path"),
+            observer_.GetFormatEvent(1));
+
   EXPECT_EQ(1, fake_cros_disks_client_->unmount_call_count());
   EXPECT_EQ("/device/mount_path",
             fake_cros_disks_client_->last_unmount_device_path());
@@ -396,7 +664,16 @@ TEST_F(DiskMountManagerTest, Format_ConcurrentFormatCalls) {
   // CrosDisksClient will report that the format process for the first request
   // is successfully started.
 
-  // Set up expectations for observer mock.
+  fake_cros_disks_client_->set_unmount_listener(
+      base::Bind(&FakeCrosDisksClient::MakeUnmountFail,
+                 base::Unretained(fake_cros_disks_client_)));
+  // Start the test.
+  DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
+  DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
+
+  // Cros disks will respond asynchronoulsy, so let's drain the message loop.
+  message_loop_.RunUntilIdle();
+
   // The observer should get a FORMAT_STARTED event for one format request and a
   // FORMAT_COMPLETED with an error code for the other format request. The
   // formatting will be started only for the first request.
@@ -406,36 +683,17 @@ TEST_F(DiskMountManagerTest, Format_ConcurrentFormatCalls) {
   //
   // Note that in this test the format completion signal will not be simulated,
   // so the observer should not get FORMAT_COMPLETED signal.
-  {
-    InSequence s;
-
-    EXPECT_CALL(observer_,
-        OnMountEvent(DiskMountManager::UNMOUNTING,
-                     chromeos::MOUNT_ERROR_NONE,
-                     Field(&DiskMountManager::MountPointInfo::mount_path,
-                           "/device/mount_path")))
-        .Times(1);
-
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                                         chromeos::FORMAT_ERROR_UNKNOWN,
-                                         "/device/source_path"))
-        .Times(1);
-
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED,
-                                         chromeos::FORMAT_ERROR_NONE,
-                                         "/device/source_path"))
-        .Times(1);
-  }
-
-  fake_cros_disks_client_->set_unmount_listener(
-      base::Bind(&FakeCrosDisksClient::MakeUnmountFail,
-                 base::Unretained(fake_cros_disks_client_)));
-  // Start the test.
-  DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
-  DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
 
-  // Cros disks will respond asynchronoulsy, so let's drain the message loop.
-  message_loop_.RunUntilIdle();
+  MountEvent mount_event = observer_.GetMountEvent(0);
+  EXPECT_EQ(DiskMountManager::UNMOUNTING, mount_event.event_);
+  EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, mount_event.error_code_);
+  EXPECT_EQ("/device/mount_path", mount_event.mount_point_.mount_path);
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                        chromeos::FORMAT_ERROR_UNKNOWN, "/device/source_path"),
+            observer_.GetFormatEvent(1));
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_STARTED,
+                        chromeos::FORMAT_ERROR_NONE, "/device/source_path"),
+            observer_.GetFormatEvent(2));
 
   EXPECT_EQ(2, fake_cros_disks_client_->unmount_call_count());
   EXPECT_EQ("/device/mount_path",
@@ -452,37 +710,19 @@ TEST_F(DiskMountManagerTest, Format_ConcurrentFormatCalls) {
   EXPECT_FALSE(HasMountPoint("/device/mount_path"));
 }
 
+void VerifyMountEvent(MountEvent mount_event,

[satorux1, 2016/08/31 07:29:03]

function comment is missing. please check other places too

https://google.github.io/styleguide/cppguide.html#Function_Comments

[yamaguchi, 2016/08/31 10:10:21]

Done.

+                      DiskMountManager::MountEvent mount_event_type,
+                      chromeos::MountError error_code,
+                      const std::string& source_path) {
+  EXPECT_EQ(mount_event_type, mount_event.event_);
+  EXPECT_EQ(error_code, mount_event.error_code_);
+  EXPECT_EQ(source_path, mount_event.mount_point_.mount_path);
+}
+
 // Tests the case when the format process actually starts and fails.
 TEST_F(DiskMountManagerTest, Format_FormatFails) {
   // Both unmount and format device cals are successful in this test.
 
-  // Set up expectations for observer mock.
-  // The observer should get notified that the device was unmounted and that
-  // formatting has started.
-  // After the formatting starts, the test will simulate failing
-  // FORMAT_COMPLETED signal, so the observer should also be notified the
-  // formatting has failed (FORMAT_COMPLETED event).
-  {
-    InSequence s;
-
-    EXPECT_CALL(observer_,
-        OnMountEvent(DiskMountManager::UNMOUNTING,
-                     chromeos::MOUNT_ERROR_NONE,
-                     Field(&DiskMountManager::MountPointInfo::mount_path,
-                           "/device/mount_path")))
-        .Times(1);
-
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED,
-                                         chromeos::FORMAT_ERROR_NONE,
-                                         "/device/source_path"))
-        .Times(1);
-
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                                         chromeos::FORMAT_ERROR_UNKNOWN,
-                                         "/device/source_path"))
-        .Times(1);
-  }
-
   // Start the test.
   DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
 
@@ -507,6 +747,21 @@ TEST_F(DiskMountManagerTest, Format_FormatFails) {
   // soon).
   fake_cros_disks_client_->SendFormatCompletedEvent(
       chromeos::FORMAT_ERROR_UNKNOWN, "/device/source_path");
+
+  // The observer should get notified that the device was unmounted and that
+  // formatting has started.
+  // After the formatting starts, the test will simulate failing
+  // FORMAT_COMPLETED signal, so the observer should also be notified the
+  // formatting has failed (FORMAT_COMPLETED event).
+  EXPECT_EQ(3U, observer_.GetEventCount());

[satorux1, 2016/08/31 07:29:03]

EXPECT_EQ -> ASSERT_EQ?

GetFormatEvent(2) shouldn't be checked (and can be unsafe) if GetEventCount() <
3.

Please check other plces too

[yamaguchi, 2016/08/31 10:10:21]

Done.
The other one in Format_ConsecutiveFormatCalls will be safe as the all following
checks are Count*, which don't depend on the number of events.

+  VerifyMountEvent(observer_.GetMountEvent(0), DiskMountManager::UNMOUNTING,
+                   chromeos::MOUNT_ERROR_NONE, "/device/mount_path");
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_STARTED,
+                        chromeos::FORMAT_ERROR_NONE, "/device/source_path"),
+            observer_.GetFormatEvent(1));
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                        chromeos::FORMAT_ERROR_UNKNOWN, "/device/source_path"),
+            observer_.GetFormatEvent(2));
 }
 
 // Tests the case when formatting completes successfully.
@@ -514,30 +769,6 @@ TEST_F(DiskMountManagerTest, Format_FormatSuccess) {
   // Set up cros disks client mocks.
   // Both unmount and format device cals are successful in this test.
 
-  // Set up expectations for observer mock.
-  // The observer should receive UNMOUNTING, FORMAT_STARTED and FORMAT_COMPLETED
-  // events (all of them without an error set).
-  {
-    InSequence s;
-
-    EXPECT_CALL(observer_,
-        OnMountEvent(DiskMountManager::UNMOUNTING,
-                     chromeos::MOUNT_ERROR_NONE,
-                     Field(&DiskMountManager::MountPointInfo::mount_path,
-                           "/device/mount_path")))
-        .Times(1);
-
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED,
-                                         chromeos::FORMAT_ERROR_NONE,
-                                         "/device/source_path"))
-        .Times(1);
-
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                                         chromeos::FORMAT_ERROR_NONE,
-                                         "/device/source_path"))
-        .Times(1);
-  }
-
   // Start the test.
   DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
 
@@ -560,6 +791,50 @@ TEST_F(DiskMountManagerTest, Format_FormatSuccess) {
   // Simulate cros_disks reporting success.
   fake_cros_disks_client_->SendFormatCompletedEvent(
       chromeos::FORMAT_ERROR_NONE, "/device/source_path");
+
+  // The observer should receive UNMOUNTING, FORMAT_STARTED and FORMAT_COMPLETED
+  // events (all of them without an error set).
+  VerifyMountEvent(observer_.GetMountEvent(0), DiskMountManager::UNMOUNTING,
+                   chromeos::MOUNT_ERROR_NONE, "/device/mount_path");
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_STARTED,
+                        chromeos::FORMAT_ERROR_NONE, "/device/source_path"),
+            observer_.GetFormatEvent(1));
+  EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                        chromeos::FORMAT_ERROR_NONE, "/device/source_path"),
+            observer_.GetFormatEvent(2));
+}
+
+size_t CountMountEvents(const std::vector<ObserverEvent>& events,
+                        DiskMountManager::MountEvent mount_event_type,
+                        chromeos::MountError error_code,
+                        const std::string& source_path) {
+  size_t matched = 0;
+  for (auto it : events) {
+    if (it.GetType() != ObserverEvent::MOUNT_EVENT) {
+      continue;
+    }
+    const MountEvent& mount_event = it.GetEvent().mount;
+    if (mount_event.event_ == mount_event_type &&
+        mount_event.error_code_ == error_code &&
+        mount_event.mount_point_.mount_path == source_path) {
+      matched++;
+    }
+  }
+  return matched;
+}
+
+size_t CountFormatEvents(const std::vector<ObserverEvent>& events,
+                         const FormatEvent& format_event) {
+  size_t matched = 0;
+  for (auto it : events) {
+    if (it.GetType() != ObserverEvent::FORMAT_EVENT) {
+      continue;
+    }
+    if (it.GetEvent().format == format_event) {
+      matched++;
+    }
+  }
+  return matched;
 }
 
 // Tests that it's possible to format the device twice in a row (this may not be
@@ -568,36 +843,6 @@ TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) {
   // All unmount and format device cals are successful in this test.
   // Each of the should be made twice (once for each formatting task).
 
-  // Set up expectations for observer mock.
-  // The observer should receive UNMOUNTING, FORMAT_STARTED and FORMAT_COMPLETED
-  // events (all of them without an error set) twice (once for each formatting
-  // task).
-  // Also, there should be a MOUNTING event when the device remounting is
-  // simulated.
-  EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED,
-                                       chromeos::FORMAT_ERROR_NONE,
-                                       "/device/source_path"))
-      .Times(2);
-
-  EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED,
-                                       chromeos::FORMAT_ERROR_NONE,
-                                       "/device/source_path"))
-      .Times(2);
-
-  EXPECT_CALL(observer_,
-      OnMountEvent(DiskMountManager::UNMOUNTING,
-                   chromeos::MOUNT_ERROR_NONE,
-                   Field(&DiskMountManager::MountPointInfo::mount_path,
-                         "/device/mount_path")))
-      .Times(2);
-
-  EXPECT_CALL(observer_,
-      OnMountEvent(DiskMountManager::MOUNTING,
-                   chromeos::MOUNT_ERROR_NONE,
-                   Field(&DiskMountManager::MountPointInfo::mount_path,
-                         "/device/mount_path")))
-      .Times(1);
-
   // Start the test.
   DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path");
 
@@ -649,6 +894,32 @@ TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) {
   // Simulate cros_disks reporting success.
   fake_cros_disks_client_->SendFormatCompletedEvent(
       chromeos::FORMAT_ERROR_NONE, "/device/source_path");
+
+  // The observer should receive UNMOUNTING, FORMAT_STARTED and FORMAT_COMPLETED
+  // events (all of them without an error set) twice (once for each formatting
+  // task).
+  // Also, there should be a MOUNTING event when the device remounting is
+  // simulated.
+  EXPECT_EQ(7U, observer_.GetEventCount());
+
+  EXPECT_EQ(
+      2U, CountFormatEvents(
+              observer_.GetEvents(),
+              FormatEvent(DiskMountManager::FORMAT_COMPLETED,
+                          chromeos::FORMAT_ERROR_NONE, "/device/source_path")));
+
+  EXPECT_EQ(2U, CountFormatEvents(observer_.GetEvents(),
+                                  FormatEvent(DiskMountManager::FORMAT_STARTED,
+                                              chromeos::FORMAT_ERROR_NONE,
+                                              "/device/source_path")));
+
+  EXPECT_EQ(
+      2U, CountMountEvents(observer_.GetEvents(), DiskMountManager::UNMOUNTING,
+                           chromeos::MOUNT_ERROR_NONE, "/device/mount_path"));
+
+  EXPECT_EQ(1U,
+            CountMountEvents(observer_.GetEvents(), DiskMountManager::MOUNTING,
+                             chromeos::MOUNT_ERROR_NONE, "/device/mount_path"));
 }
 
 TEST_F(DiskMountManagerTest, MountPath_RecordAccessMode) {
@@ -661,27 +932,6 @@ TEST_F(DiskMountManagerTest, MountPath_RecordAccessMode) {
   const std::string kMountPath1 = "/media/foo";
   const std::string kMountPath2 = "/media/bar";
 
-  // Event handlers of observers should be called.
-  EXPECT_CALL(
-      observer_,
-      OnMountEvent(
-          DiskMountManager::MOUNTING, chromeos::MOUNT_ERROR_NONE,
-          Field(&DiskMountManager::MountPointInfo::mount_path, kMountPath1)));
-  // For the 2nd source, the disk (block device) is not read-only but the
-  // test will mount it in read-only mode.
-  // Observers query |disks_| from |DiskMountManager| in its event handler for
-  // a mount completion event. Therefore |disks_| must be updated with correct
-  // |read_only| value before notifying to observers.
-  EXPECT_CALL(
-      observer_,
-      OnMountEvent(
-          DiskMountManager::MOUNTING, chromeos::MOUNT_ERROR_NONE,
-          Field(&DiskMountManager::MountPointInfo::mount_path, kMountPath2)))
-      .WillOnce(InvokeWithoutArgs(
-          // Verify if the disk appears read-only at the time of notification
-          // to observers.
-          [&]() { ExpectDiskReadOnly(manager, kSourcePath2, true); }));
-
   manager->MountPath(kSourcePath1, kSourceFormat, std::string(),
                      chromeos::MOUNT_TYPE_DEVICE,
                      chromeos::MOUNT_ACCESS_MODE_READ_WRITE);
@@ -696,6 +946,23 @@ TEST_F(DiskMountManagerTest, MountPath_RecordAccessMode) {
       chromeos::MOUNT_ERROR_NONE, kSourcePath2, chromeos::MOUNT_TYPE_DEVICE,
       kMountPath2);
 
+  // Event handlers of observers should be called.
+  VerifyMountEvent(observer_.GetMountEvent(0), DiskMountManager::MOUNTING,
+                   chromeos::MOUNT_ERROR_NONE, kMountPath1);
+  // For the 2nd source, the disk (block device) is not read-only but the
+  // test will mount it in read-only mode.
+  // Observers query |disks_| from |DiskMountManager| in its event handler for
+  // a mount completion event. Therefore |disks_| must be updated with correct
+  // |read_only| value before notifying to observers.
+  MountEvent secondMountEvent = observer_.GetMountEvent(1);
+  EXPECT_EQ(DiskMountManager::MOUNTING, secondMountEvent.event_);
+  EXPECT_EQ(chromeos::MOUNT_ERROR_NONE, secondMountEvent.error_code_);
+  EXPECT_EQ(kMountPath2, secondMountEvent.mount_point_.mount_path);
+  // Verify if the disk appears read-only at the time of notification to
+  // observers.
+  EXPECT_TRUE(secondMountEvent.disk_->is_read_only());
+
+  // Verify the final state of manager->disks.
   const DiskMountManager::DiskMap& disks = manager->disks();
   ASSERT_GT(disks.count(kSourcePath1), 0U);
   EXPECT_FALSE(disks.find(kSourcePath1)->second->is_read_only());