Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2292473002:
Add a mock class for DiskMountManagerObserver. disk_mount_manager_observer_unittests will be rewrit… (Closed)
DescriptionAdd a mock class for DiskMountManagerObserver.
disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock.
BUG=641943
Committed: https://crrev.com/7bec88d891d568be4392e3e5d74d04b79ff4e667
Cr-Commit-Position: refs/heads/master@{#418182}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Implement DebugString() for ObserverEvent. #Patch Set 3 : Move the mock class to unittest source as it's dedicated to the test. #Patch Set 4 : Add TODO comment. #
Total comments: 12
Patch Set 5 : Implement tests using new mock. #Patch Set 6 : Use struct instead of classes for individual event types #Patch Set 7 : Reformat #Patch Set 8 : Bring struct fields to the top of declarations #
Total comments: 10
Patch Set 9 : Update reflecting review comments. #Patch Set 10 : Add function comments. #
Total comments: 10
Patch Set 11 : Use inherited classes for events. #
Total comments: 7
Patch Set 12 : Move some helper function to member of the mock class. Verify event count before get. #Patch Set 13 : Remove unused function. Add function comments. #
Total comments: 9
Patch Set 14 : style fix #
Total comments: 4
Patch Set 15 : Take snaposhot of objects in each event constructor. Stop using shared_ptr. #Patch Set 16 : Remove unused default constructors. #
Total comments: 12
Patch Set 17 : Expect exact number of invocations. #Messages
Total messages: 43 (14 generated)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_observer_unittests will be rewritten using this class instead of gmock. BUG= ========== to ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_observer_unittests will be rewritten using this class instead of gmock. BUG= ==========
Description was changed from ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_observer_unittests will be rewritten using this class instead of gmock. BUG= ========== to ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock. BUG= ==========
Description was changed from ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock. BUG= ========== to ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock. BUG=641943 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I will revise the change a bit more. https://codereview.chromium.org/2292473002/diff/1/chromeos/disks/mock_disk_mo... File chromeos/disks/mock_disk_mount_manager_observer.h (right): https://codereview.chromium.org/2292473002/diff/1/chromeos/disks/mock_disk_mo... chromeos/disks/mock_disk_mount_manager_observer.h:205: DeviceEvent GetDeviceEvent(size_t index); It'd be easier to have tests get and verify ObserverEvents objects rather than doing such for individual types for each event type. Hide these 4 functions and implement comparator and DebugString() in ObserverEvent, so that the tests can verify results like: ASSERT_EQ(ObserverEvent::FromDeviceEvent(...), observer_.GetEvent(1));
yamaguchi@chromium.org changed reviewers: + satorux@google.com
PTAL. This is the first part of the refactoring for getting off of gmock. https://codereview.chromium.org/2292473002/diff/1/chromeos/disks/mock_disk_mo... File chromeos/disks/mock_disk_mount_manager_observer.h (right): https://codereview.chromium.org/2292473002/diff/1/chromeos/disks/mock_disk_mo... chromeos/disks/mock_disk_mount_manager_observer.h:205: DeviceEvent GetDeviceEvent(size_t index); On 2016/08/30 02:20:10, yamaguchi wrote: > It'd be easier to have tests get and verify ObserverEvents objects rather than > doing such for individual types for each event type. > Hide these 4 functions and implement comparator and DebugString() in > ObserverEvent, so that the tests can verify results like: > ASSERT_EQ(ObserverEvent::FromDeviceEvent(...), observer_.GetEvent(1)); In the current unit test, MountEvent is not examined for all fields. The tests examine only 1 field using testing::Field (within gmock EXPECT_CALL), and it would be reasonable for the purpose of the test. Such test will be rewritten using this class by GetMountEvent() first and then EXPECT_EQ for each field in it. So I think we don't need to implement euqals (==) for all events now. Example: https://cs.chromium.org/chromium/src/chromeos/disks/disk_mount_manager_unitte...
satorux@chromium.org changed reviewers: + satorux@chromium.org
do you plan to use this class outside of disk_mount_manager_observer_unittests ? otherwise, i think you can just put this code in disk_mount_manager_unittest.cc
On 2016/08/30 04:58:31, satorux1 wrote: > do you plan to use this class outside of disk_mount_manager_observer_unittests > ? otherwise, i think you can just put this code in > disk_mount_manager_unittest.cc No, the mock class will be used only by disk_mount_manager_unittests. But I am afraid it may make the single file much longer.
I think it's OK and simpler to put the new code in
disk_mount_manager_unittest.cc.
Also I think you can just define functions in classes like:
class Foo {
void Function() {
...
}
};
instead of separating the function definition and declaration. That'd make the
code concise in unittest.cc.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/30 06:04:45, satorux1 wrote:
> I think it's OK and simpler to put the new code in
> disk_mount_manager_unittest.cc.
>
> Also I think you can just define functions in classes like:
>
> class Foo {
> void Function() {
> ...
> }
> };
>
> instead of separating the function definition and declaration. That'd make the
> code concise in unittest.cc.
Done. PTAL.
https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:8: #include <sstream> Streams are not recommended. let's use StringPrintf or something. https://google.github.io/styleguide/cppguide.html#Streams https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:160: // MockDiskMountManagerObserver. Can you update the tests in the same patch? That'd make the patch big, but I'd like to see how the new tests would look like in the same patch. https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:179: // Event classes which represents each invocation of functions in |Observer|. represents -> represent https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:181: class DeviceEvent { can this be just a struct? ditto with other similar classes. https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:471: assert(events_.size() >= index + 1); assert -> DCHECK
https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:471: assert(events_.size() >= index + 1); On 2016/08/30 07:27:29, satorux1 wrote: > assert -> DCHECK for this casek DCHECK_GE
ptal https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:8: #include <sstream> On 2016/08/30 07:27:29, satorux1 wrote: > Streams are not recommended. let's use StringPrintf or something. > > https://google.github.io/styleguide/cppguide.html#Streams Done. https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:160: // MockDiskMountManagerObserver. On 2016/08/30 07:27:29, satorux1 wrote: > Can you update the tests in the same patch? That'd make the patch big, but I'd > like to see how the new tests would look like in the same patch. Updated the tests. https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:179: // Event classes which represents each invocation of functions in |Observer|. On 2016/08/30 07:27:29, satorux1 wrote: > represents -> represent Done. https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:181: class DeviceEvent { On 2016/08/30 07:27:29, satorux1 wrote: > can this be just a struct? ditto with other similar classes. Done. https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:471: assert(events_.size() >= index + 1); On 2016/08/30 07:27:29, satorux1 wrote: > assert -> DCHECK Done. https://codereview.chromium.org/2292473002/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:471: assert(events_.size() >= index + 1); On 2016/08/30 07:28:06, satorux1 wrote: > On 2016/08/30 07:27:29, satorux1 wrote: > > assert -> DCHECK > > for this casek DCHECK_GE Done.
https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:157: std::string device_path_; please drop the trailing _ from struct members https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:170: } is it possible to remove the constructors and the operator==()? a simpler struct would be nicer if it works. https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:446: // Needed to print values in case of EXPECT_* failure in gtest. If there are not many places, maybe the following is suffice: EXPECT_EQ(foo, bar) << foo.DebugString() << " != " bar.DebugString() https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:713: void VerifyMountEvent(MountEvent mount_event, function comment is missing. please check other places too https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:756: EXPECT_EQ(3U, observer_.GetEventCount()); EXPECT_EQ -> ASSERT_EQ? GetFormatEvent(2) shouldn't be checked (and can be unsafe) if GetEventCount() < 3. Please check other plces too
https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:157: std::string device_path_; On 2016/08/31 07:29:03, satorux1 wrote: > please drop the trailing _ from struct members > > https://google.github.io/styleguide/cppguide.html#Variable_Names Done. https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:170: } On 2016/08/31 07:29:03, satorux1 wrote: > is it possible to remove the constructors and the operator==()? a simpler struct > would be nicer if it works. - 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. https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:446: // Needed to print values in case of EXPECT_* failure in gtest. On 2016/08/31 07:29:03, satorux1 wrote: > If there are not many places, maybe the following is suffice: > > EXPECT_EQ(foo, bar) << foo.DebugString() << " != " bar.DebugString() > Currently it's used 11 times in the tests. So I think having these would make some sense. https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:713: void VerifyMountEvent(MountEvent mount_event, On 2016/08/31 07:29:03, satorux1 wrote: > function comment is missing. please check other places too > > https://google.github.io/styleguide/cppguide.html#Function_Comments Done. https://codereview.chromium.org/2292473002/diff/140001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:756: EXPECT_EQ(3U, observer_.GetEventCount()); On 2016/08/31 07:29:03, satorux1 wrote: > EXPECT_EQ -> ASSERT_EQ? > > GetFormatEvent(2) shouldn't be checked (and can be unsafe) if GetEventCount() < > 3. > > Please check other plces too 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. https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:276: Event(DeviceEvent device) : device(device) {} Added overloaded constructors so as to eliminate the default constructor of Event and simplify the logic in the outer class.
https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:153: // Event classes which represent each invocation of functions in |Observer|. Instead of describing multiple classes at once here, please describe each class separately like: // Represents an invocation of DiskMountManager::ObserverOnDeviceEvent() https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:263: class ObserverEvent { Instead of this class, maybe we can use inheritance here? struct Event { } struct DeviceEvent : public Event { ... EventType type() { return DEVICE_EVENT }; } struct DiskEvent : public Event { ... EventType type() { return DISK_EVENT }; } This way, MockDiskMountManagerObserver can record events like: events_.push_back(MakeUnique<DeviceEvent>(...)); ... std::vector<std::unique_ptr<Event>> events_; GetFormatEvent() would look like: DCHECK_GT(events_.size(), index); DCHECK_EQ(DEVICE_EVENT, events_[index]->type()); return static_cast<const DeviceEvent&>(*events_[index]); https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:377: DCHECK_GE(events_.size(), index + 1); DCHECK_GT(events_.size(), index) may be easier to read. https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:534: DiskMountManager::GetInstance()->FormatMountedDevice("/mount/non_existent"); Add ASSERT_GT(1, observer_.GetEventCount()) here and other places?
https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:153: // Event classes which represent each invocation of functions in |Observer|. On 2016/09/01 06:18:44, satorux1 wrote: > Instead of describing multiple classes at once here, please describe each class > separately like: > > // Represents an invocation of DiskMountManager::ObserverOnDeviceEvent() Done. https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:263: class ObserverEvent { On 2016/09/01 06:18:44, satorux1 wrote: > Instead of this class, maybe we can use inheritance here? > > struct Event { > } > > struct DeviceEvent : public Event { > ... > EventType type() { return DEVICE_EVENT }; > } > > struct DiskEvent : public Event { > ... > EventType type() { return DISK_EVENT }; > } > > This way, MockDiskMountManagerObserver can record events like: > > events_.push_back(MakeUnique<DeviceEvent>(...)); > ... > std::vector<std::unique_ptr<Event>> events_; > > GetFormatEvent() would look like: > > DCHECK_GT(events_.size(), index); > DCHECK_EQ(DEVICE_EVENT, events_[index]->type()); > return static_cast<const DeviceEvent&>(*events_[index]); Done. https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:377: DCHECK_GE(events_.size(), index + 1); On 2016/09/01 06:18:44, satorux1 wrote: > DCHECK_GT(events_.size(), index) may be easier to read. Done. https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:728: size_t CountMountEvents( Should these 2 functions be moved into MockDiskMountManagerObserver class because the logic might be somewhat coupled with the internal data structure?
https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:534: DiskMountManager::GetInstance()->FormatMountedDevice("/mount/non_existent"); On 2016/09/01 06:18:44, satorux1 wrote: > Add ASSERT_GT(1, observer_.GetEventCount()) here and other places? did you miss this comment? https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:350: size_t GetEventCount() { return events_.size(); } function comment is missing. one line comment should suffice. https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:352: const std::vector<std::unique_ptr<ObserverEvent>>& GetEvents() const { ditto https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:728: size_t CountMountEvents( On 2016/09/02 13:25:42, yamaguchi wrote: > Should these 2 functions be moved into MockDiskMountManagerObserver class > because the logic might be somewhat coupled with the internal data structure? Sounds reasonable to me. Please move there.
https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/180001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:534: DiskMountManager::GetInstance()->FormatMountedDevice("/mount/non_existent"); On 2016/09/06 04:30:43, satorux1 wrote: > On 2016/09/01 06:18:44, satorux1 wrote: > > Add ASSERT_GT(1, observer_.GetEventCount()) here and other places? > > did you miss this comment? Sorry, I overlooked it. Done. https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:350: size_t GetEventCount() { return events_.size(); } On 2016/09/06 04:30:43, satorux1 wrote: > function comment is missing. one line comment should suffice. Done. https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:352: const std::vector<std::unique_ptr<ObserverEvent>>& GetEvents() const { On 2016/09/06 04:30:43, satorux1 wrote: > ditto This function was removed as this field has been encupsulated by the Count*Events functions. https://codereview.chromium.org/2292473002/diff/200001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:728: size_t CountMountEvents( On 2016/09/06 04:30:43, satorux1 wrote: > On 2016/09/02 13:25:42, yamaguchi wrote: > > Should these 2 functions be moved into MockDiskMountManagerObserver class > > because the logic might be somewhat coupled with the internal data structure? > > Sounds reasonable to me. Please move there. Done. https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:900: TEST_F(DiskMountManagerTest, MountPath_ReadOnlyDevice) { note: This test function has been merged from head.
https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:326: // Verify if the |index|th invocation is OnDeviceEvent() and return details. Nit: Get->Gets Verify->Verifies https://google.github.io/styleguide/cppguide.html#Function_Comments Please fix other places too https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:361: const std::string& mount_path) { Cannot this take const MountEvent& just like CountFormatEvents() below? If that's possible, maybe we can generalize the two functions into one function CountEvents() that takes const ObserverEvent& ? https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:366: } nit: {} is generally omitted for simple single-line statements like this. The style guide says: > In general, curly braces are not required for single-line statements, but they are allowed if you like them;
https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:326: // Verify if the |index|th invocation is OnDeviceEvent() and return details. On 2016/09/06 07:08:46, satorux1 wrote: > Nit: Get->Gets Verify->Verifies > > https://google.github.io/styleguide/cppguide.html#Function_Comments > > Please fix other places too Done. https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:361: const std::string& mount_path) { On 2016/09/06 07:08:46, satorux1 wrote: > Cannot this take const MountEvent& just like CountFormatEvents() below? If > that's possible, maybe we can generalize the two functions into one function > CountEvents() that takes const ObserverEvent& ? I think taking (unnamed namespace)::MountEvent& in this case will make the test a slightly misleading as it'd require to build the entire expected object. This function only compares some limited fields in the object which the test needs to, like the existing code does by testing::Field. I think we may feed predicates to generalized CountEvent() and use std::count_if, but I think this would be acceptable for now as there are only 2 patterns. https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:366: } On 2016/09/06 07:08:46, satorux1 wrote: > nit: {} is generally omitted for simple single-line statements like this. The > style guide says: > > > > In general, curly braces are not required for single-line statements, but they > are allowed if you like them; Done.
https://codereview.chromium.org/2292473002/diff/260001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/260001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:250: std::shared_ptr<DiskMountManager::Disk> disk; shared_ptr is rarely used in Chrome code. Can you replace this with something else? https://codereview.chromium.org/2292473002/diff/260001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:325: // Get invocation history to be verified by testcases. Get -> Gets
https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:361: const std::string& mount_path) { On 2016/09/06 09:29:03, yamaguchi wrote: > On 2016/09/06 07:08:46, satorux1 wrote: > > Cannot this take const MountEvent& just like CountFormatEvents() below? If > > that's possible, maybe we can generalize the two functions into one function > > CountEvents() that takes const ObserverEvent& ? > > I think taking (unnamed namespace)::MountEvent& in this case will make the test > a slightly misleading as it'd require to build the entire expected object. > This function only compares some limited fields in the object which the test > needs to Ah OK, but MountEvent has only 4 members and 3 of them are tested here. If the other one (disk) can be optional, taking the entire object may be ok? It's up to you. > , like the existing code does by testing::Field. > I think we may feed predicates to generalized CountEvent() and use > std::count_if, but I think this would be acceptable for now as there are only 2 > patterns.
https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/240001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:361: const std::string& mount_path) { On 2016/09/08 06:24:21, satorux1 wrote: > On 2016/09/06 09:29:03, yamaguchi wrote: > > On 2016/09/06 07:08:46, satorux1 wrote: > > > Cannot this take const MountEvent& just like CountFormatEvents() below? If > > > that's possible, maybe we can generalize the two functions into one function > > > CountEvents() that takes const ObserverEvent& ? > > > > I think taking (unnamed namespace)::MountEvent& in this case will make the > test > > a slightly misleading as it'd require to build the entire expected object. > > This function only compares some limited fields in the object which the test > > needs to > > Ah OK, but MountEvent has only 4 members and 3 of them are tested here. If the > other one (disk) can be optional, taking the entire object may be ok? It's up to > you. > > > > , like the existing code does by testing::Field. > > I think we may feed predicates to generalized CountEvent() and use > > std::count_if, but I think this would be acceptable for now as there are only > 2 > > patterns. > One concern was that Disk doesn't have a constructor with no arguments, but a one with all 20 field values. Either the test cases or another constructor MountEvent would need to contain a call to that constructor and I thought that it may make the test somewhat redundant, compared to the existing code. So I prefer this way for now. https://codereview.chromium.org/2292473002/diff/260001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/260001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:250: std::shared_ptr<DiskMountManager::Disk> disk; On 2016/09/08 06:03:55, satorux1 wrote: > shared_ptr is rarely used in Chrome code. Can you replace this with something > else? Done. Also replaced the similar one for DiskEvent. https://codereview.chromium.org/2292473002/diff/260001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:325: // Get invocation history to be verified by testcases. On 2016/09/08 06:03:55, satorux1 wrote: > Get -> Gets Done.
almost there! https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:218: // OnFormatEvent() Represents an invocation of |DiskMountManager::Observer::OnFormatEvent()|. https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:356: size_t matched = 0; nit: num_matched https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:357: for (auto& it : events_) { const auto& ? https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:372: size_t matched = 0; num_matched https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:373: for (auto& it : events_) { const auto& ? https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:506: ASSERT_GE(1U, observer_->GetEventCount()); If you can be sure it should be 1, change to ASSERT_EQ? this applies to other places too
https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:218: // OnFormatEvent() On 2016/09/13 05:00:56, satorux1 wrote: > Represents an invocation of |DiskMountManager::Observer::OnFormatEvent()|. Done. https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:356: size_t matched = 0; On 2016/09/13 05:00:56, satorux1 wrote: > nit: num_matched Done. https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:357: for (auto& it : events_) { On 2016/09/13 05:00:56, satorux1 wrote: > const auto& ? Done. https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:372: size_t matched = 0; On 2016/09/13 05:00:56, satorux1 wrote: > num_matched Done. https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:373: for (auto& it : events_) { On 2016/09/13 05:00:56, satorux1 wrote: > const auto& ? Done. https://codereview.chromium.org/2292473002/diff/300001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:506: ASSERT_GE(1U, observer_->GetEventCount()); On 2016/09/13 05:00:56, satorux1 wrote: > If you can be sure it should be 1, change to ASSERT_EQ? this applies to other > places too Done. I had once thought that it'd be more robust for future change, but now I think we should expect the exact number of events here because the remaining part of the tests verify specific events happening in a specific order, and having extra events will indicate some potential bug.
lgtm
The CQ bit was checked by yamaguchi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock. BUG=641943 ========== to ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock. BUG=641943 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock. BUG=641943 ========== to ========== Add a mock class for DiskMountManagerObserver. disk_mount_manager_unittest.cc will be rewritten using this class instead of gmock. BUG=641943 Committed: https://crrev.com/7bec88d891d568be4392e3e5d74d04b79ff4e667 Cr-Commit-Position: refs/heads/master@{#418182} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/7bec88d891d568be4392e3e5d74d04b79ff4e667 Cr-Commit-Position: refs/heads/master@{#418182}
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 418182 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/2338763002/ by yamaguchi@chromium.org. The reason for reverting is: memory leak detected in unittest.
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/2340433002/ by kjellander@chromium.org. The reason for reverting is: Breaks "Linux Chromium OS ASan LSan Tests (1)" like this: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... Snippet: ==24096==ERROR: LeakSanitizer: detected memory leaks Direct leak of 40 byte(s) in 1 object(s) allocated from: #0 0x55a84b in operator new(unsigned long) (/b/swarming/w/iroDkF6G/out/Release/chromeos_unittests+0x55a84b) #1 0x805c51 in (anonymous namespace)::DiskMountManagerTest::SetUp() chromeos/disks/disk_mount_manager_unittest.cc:428:9 #2 0x2c3a936 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2458:12 #3 0x2c3a936 in testing::Test::Run() testing/gtest/src/gtest.cc:2470 #4 0x2c3cafb in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:11 #5 0x2c3d8b6 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:28 #6 0x2c518b6 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:43 #7 0x2c50f17 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 #8 0x2c50f17 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255 #9 0x290b60b in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46 #10 0x290b60b in base::TestSuite::Run() base/test/test_suite.cc:246 #11 0x2910983 in Run base/callback.h:64:12 #12 0x2910983 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, int, int, bool, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/test/launcher/unit_test_launcher.cc:206 #13 0x2910665 in base::LaunchUnitTests(int, char**, base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/test/launcher/unit_test_launcher.cc:445:10 #14 0x2909844 in main base/test/run_all_unittests.cc:12:10 #15 0x7f3c903677ec in __libc_start_main /build/eglibc-oqps9y/eglibc-2.15/csu/libc-start.c:226 . |
