Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2275173002:
Add unit test for BluetoothChooserController (Closed)
|
Created:
4 years, 4 months ago by juncai Modified:
4 years, 3 months ago Reviewers:
Jeffrey Yasskin CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd unit test for BluetoothChooserController
With more functionalities added to BluetoothChooserController,
it is necessary to add unit test to it.
BUG=639163
Committed: https://crrev.com/5613eb86a97282c8dbb3390433dbdf2253afb56a
Cr-Commit-Position: refs/heads/master@{#416408}
Patch Set 1 : added unit test for BluetoothChooserController #
Total comments: 28
Patch Set 2 : address comments #Patch Set 3 : added comment #
Total comments: 8
Patch Set 4 : address more comments #
Total comments: 2
Patch Set 5 : address comments #
Messages
Total messages: 36 (25 generated)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Description was changed from ========== Add unit test code for BluetoothChooserController With more functionalities have been added to BluetoothChooserController, it is necessary to add unit test to it. BUG=639163 ========== to ========== Add unit test for BluetoothChooserController With more functionalities have been added to BluetoothChooserController, it is necessary to add unit test to it. BUG=639163 ==========
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 unit test for BluetoothChooserController With more functionalities have been added to BluetoothChooserController, it is necessary to add unit test to it. BUG=639163 ========== to ========== Add unit test for BluetoothChooserController With more functionalities added to BluetoothChooserController, it is necessary to add unit test to it. BUG=639163 ==========
juncai@chromium.org changed reviewers: + jyasskin@chromium.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:237: size_t index = device_it - devices_.begin(); Why did this move back? https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:40: ~BluetoothChooserControllerTest() override = default; You don't need to mention a defaulted destructor. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:46: bluetooth_chooser_controller_.reset( Make this a value member, not a unique_ptr, and initialize it in the constructor. Like: BluetoothChooserControllerTest() : bluetooth_chooser_controller_( nullptr, base::Bind(&BluetoothChooserControllerTest::OnBluetoothChooserEvent, base::Unretained(this))) { } https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:48: mock_bluetooth_chooser_view_.reset(new MockBluetoothChooserView()); Also make mock_bluetooth_chooser_view_ a value member. Because you're using its default constructor, you don't even need to mention it in the constructor. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:49: bluetooth_chooser_controller_->set_view(mock_bluetooth_chooser_view_.get()); Since this can't fail, it's also fine to move it to the constructor, and then delete the now-empty SetUp(). https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:61: content::BluetoothChooser::Event event_; I'd call these last_event_ and last_device_id_. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:72: true /* is_gatt_connected */, true /* is_paired */, -1); Also comment the meaning of the -1: /* rssi */ https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:78: EXPECT_CALL(*mock_bluetooth_chooser_view_, OnOptionAdded(1)).Times(1); Since you have three separate sets of expectations, call testing::Mock::VerifyAndClearExpectations(&mock_bluetooth_chooser_view_) between each one, to make sure things happen at the right time. https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:135: TEST_F(BluetoothChooserControllerTest, AddAndRemoveDeviceWithSameName) { Try to name tests with an overall description of what they're asserting. For example, this could be "MultipleDevicesWithSameNameShowIds". https://big.corp.google.com/~jmcmaster/testing/2014/07/episode-339-writing-de... https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:213: bluetooth_chooser_controller_->AddOrUpdateDevice( It looks like you have several tests that need to be initialized with these three devices, and then have several sub-parts that don't affect each other. That's a good time to add a subclass of your main fixture which initializes the devices, and then split the test into multiple pieces with more descriptive names. If I've misread the tests' internal dependencies, disregard this. :) https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:303: bluetooth_chooser_controller_->RefreshOptions(); Does this not call any view methods?
The CQ bit was checked by juncai@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...
The CQ bit was checked by juncai@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...
https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:237: size_t index = device_it - devices_.begin(); On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > Why did this move back? The previous one works in release code, but debug code crashes: include/c++/4.6/debug/safe_iterator.682: error: attempt to compute the difference between a singular iterator to a past-the-end iterator. include/c++/4.6/debug/safe_iterator.682: error: attempt to compute the difference between a singular iterator to a dereferenceable (start-of-sequence) iterator. And after I changed the code to cache the |index|, it works fine. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:40: ~BluetoothChooserControllerTest() override = default; On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > You don't need to mention a defaulted destructor. Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:46: bluetooth_chooser_controller_.reset( On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > Make this a value member, not a unique_ptr, and initialize it in the > constructor. Like: > > BluetoothChooserControllerTest() > : bluetooth_chooser_controller_( > nullptr, > base::Bind(&BluetoothChooserControllerTest::OnBluetoothChooserEvent, > base::Unretained(this))) { > } Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:48: mock_bluetooth_chooser_view_.reset(new MockBluetoothChooserView()); On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > Also make mock_bluetooth_chooser_view_ a value member. Because you're using its > default constructor, you don't even need to mention it in the constructor. Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:49: bluetooth_chooser_controller_->set_view(mock_bluetooth_chooser_view_.get()); On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > Since this can't fail, it's also fine to move it to the constructor, and then > delete the now-empty SetUp(). Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:61: content::BluetoothChooser::Event event_; On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > I'd call these last_event_ and last_device_id_. Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:72: true /* is_gatt_connected */, true /* is_paired */, -1); On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > Also comment the meaning of the -1: /* rssi */ Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:78: EXPECT_CALL(*mock_bluetooth_chooser_view_, OnOptionAdded(1)).Times(1); On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > Since you have three separate sets of expectations, call > testing::Mock::VerifyAndClearExpectations(&mock_bluetooth_chooser_view_) between > each one, to make sure things happen at the right time. > https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:135: TEST_F(BluetoothChooserControllerTest, AddAndRemoveDeviceWithSameName) { On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > Try to name tests with an overall description of what they're asserting. For > example, this could be "MultipleDevicesWithSameNameShowIds". > https://big.corp.google.com/~jmcmaster/testing/2014/07/episode-339-writing-de... Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:213: bluetooth_chooser_controller_->AddOrUpdateDevice( On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > It looks like you have several tests that need to be initialized with these > three devices, and then have several sub-parts that don't affect each other. > That's a good time to add a subclass of your main fixture which initializes the > devices, and then split the test into multiple pieces with more descriptive > names. > > If I've misread the tests' internal dependencies, disregard this. :) Done. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:303: bluetooth_chooser_controller_->RefreshOptions(); On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > Does this not call any view methods? It does call view method, but through: https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... And it calls: https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch...
https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:237: size_t index = device_it - devices_.begin(); On 2016/08/27 00:21:30, juncai wrote: > On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > > Why did this move back? > > The previous one works in release code, but debug code crashes: > include/c++/4.6/debug/safe_iterator.682: > error: attempt to compute the difference between a singular iterator to a > past-the-end iterator. > > include/c++/4.6/debug/safe_iterator.682: > error: attempt to compute the difference between a singular iterator to a > dereferenceable (start-of-sequence) iterator. > > And after I changed the code to cache the |index|, it works fine. Oops, you're totally right. The .erase() invalidates device_it, so you can't use it after that point. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:303: bluetooth_chooser_controller_->RefreshOptions(); On 2016/08/27 00:21:30, juncai wrote: > On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > > Does this not call any view methods? > > It does call view method, but through: > https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... > And it calls: > https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... Please test that. https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:20: MockBluetoothChooserView() {} I believe you can also omit this line since it's in a .cc file. https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:252: EXPECT_CALL( Can you split this into 3 tests, with the divisions just before each EXPECT_CALL? Or do the subsequent assertions depend on state set up in the previous block? If there is essential state, please comment it. https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:317: SelectOneDeviceAndEventHandlerCalled) { I'd name these more like a sentence: "SelectingOneDeviceShouldCallEventHandler". This test also happens to select 3 devices, which the class doesn't need to handle.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:237: size_t index = device_it - devices_.begin(); On 2016/08/27 00:35:45, Jeffrey Yasskin wrote: > On 2016/08/27 00:21:30, juncai wrote: > > On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > > > Why did this move back? > > > > The previous one works in release code, but debug code crashes: > > include/c++/4.6/debug/safe_iterator.682: > > error: attempt to compute the difference between a singular iterator to a > > past-the-end iterator. > > > > include/c++/4.6/debug/safe_iterator.682: > > error: attempt to compute the difference between a singular iterator to a > > dereferenceable (start-of-sequence) iterator. > > > > And after I changed the code to cache the |index|, it works fine. > > Oops, you're totally right. The .erase() invalidates device_it, so you can't use > it after that point. Acknowledged. https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:303: bluetooth_chooser_controller_->RefreshOptions(); On 2016/08/27 00:35:45, Jeffrey Yasskin wrote: > On 2016/08/27 00:21:30, juncai wrote: > > On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > > > Does this not call any view methods? > > > > It does call view method, but through: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... > > And it calls: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... > > Please test that. The above calling sequence is for non-testing code. Since here uses a mock Bluetooth EventHandler, need to modify OnBluetoothChooserEvent::OnBluetoothChooserEvent() to test that. Done. https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:20: MockBluetoothChooserView() {} On 2016/08/27 00:35:46, Jeffrey Yasskin wrote: > I believe you can also omit this line since it's in a .cc file. Tried removing this, but got compile error: "error: constructor for 'BluetoothChooserControllerTest' must explicitly initialize the member 'mock_bluetooth_chooser_view_' which does not have a default constructor". I thought if removing the constructor, the compiler will provide a default one, but it seems it doesn't, strange... https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:252: EXPECT_CALL( On 2016/08/27 00:35:45, Jeffrey Yasskin wrote: > Can you split this into 3 tests, with the divisions just before each > EXPECT_CALL? Or do the subsequent assertions depend on state set up in the > previous block? If there is essential state, please comment it. Done. https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:317: SelectOneDeviceAndEventHandlerCalled) { On 2016/08/27 00:35:46, Jeffrey Yasskin wrote: > I'd name these more like a sentence: "SelectingOneDeviceShouldCallEventHandler". > This test also happens to select 3 devices, which the class doesn't need to > handle. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
ping, :).
LGTM, thanks! https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:303: bluetooth_chooser_controller_->RefreshOptions(); On 2016/08/27 02:15:56, juncai wrote: > On 2016/08/27 00:35:45, Jeffrey Yasskin wrote: > > On 2016/08/27 00:21:30, juncai wrote: > > > On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > > > > Does this not call any view methods? > > > > > > It does call view method, but through: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... > > > And it calls: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... > > > > Please test that. > > The above calling sequence is for non-testing code. Since here uses a mock > Bluetooth EventHandler, need to modify > OnBluetoothChooserEvent::OnBluetoothChooserEvent() to test that. > > Done. Argh, sorry, I mis-read the implementation of RefreshOptions. If it only calls a view method because the event handler tends to call another controller method, then the EXPECT_EQ(content::BluetoothChooser::Event::RESCAN, event_); is sufficient, and you can remove the extra assertion and "if (event == content::BluetoothChooser::Event::RESCAN) {" you added to OnBluetoothChooserEvent because of this request. Sorry for the unnecessary request. https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:20: MockBluetoothChooserView() {} On 2016/08/27 02:15:56, juncai wrote: > On 2016/08/27 00:35:46, Jeffrey Yasskin wrote: > > I believe you can also omit this line since it's in a .cc file. > > Tried removing this, but got compile error: > "error: constructor for 'BluetoothChooserControllerTest' must explicitly > initialize the member 'mock_bluetooth_chooser_view_' which does not have a > default constructor". > > I thought if removing the constructor, the compiler will provide a default one, > but it seems it doesn't, strange... Ah, it's because DISALLOW_COPY_AND_ASSIGN provides a deleted copy constructor, which suppresses the default constructor. So the default constructor stays. https://codereview.chromium.org/2275173002/diff/60001/chrome/browser/ui/bluet... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/60001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:302: EXPECT_CALL(mock_bluetooth_chooser_view_, Can you split this test into 3 with clear names also?
The CQ bit was checked by juncai@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...
https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/1/chrome/browser/ui/bluetooth... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:303: bluetooth_chooser_controller_->RefreshOptions(); On 2016/09/02 20:40:58, Jeffrey Yasskin wrote: > On 2016/08/27 02:15:56, juncai wrote: > > On 2016/08/27 00:35:45, Jeffrey Yasskin wrote: > > > On 2016/08/27 00:21:30, juncai wrote: > > > > On 2016/08/26 21:46:12, Jeffrey Yasskin wrote: > > > > > Does this not call any view methods? > > > > > > > > It does call view method, but through: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... > > > > And it calls: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch... > > > > > > Please test that. > > > > The above calling sequence is for non-testing code. Since here uses a mock > > Bluetooth EventHandler, need to modify > > OnBluetoothChooserEvent::OnBluetoothChooserEvent() to test that. > > > > Done. > > Argh, sorry, I mis-read the implementation of RefreshOptions. If it only calls a > view method because the event handler tends to call another controller method, > then the EXPECT_EQ(content::BluetoothChooser::Event::RESCAN, event_); is > sufficient, and you can remove the extra assertion and "if (event == > content::BluetoothChooser::Event::RESCAN) {" you added to > OnBluetoothChooserEvent because of this request. > > Sorry for the unnecessary request. Done. https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/40001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:20: MockBluetoothChooserView() {} On 2016/09/02 20:40:58, Jeffrey Yasskin wrote: > On 2016/08/27 02:15:56, juncai wrote: > > On 2016/08/27 00:35:46, Jeffrey Yasskin wrote: > > > I believe you can also omit this line since it's in a .cc file. > > > > Tried removing this, but got compile error: > > "error: constructor for 'BluetoothChooserControllerTest' must explicitly > > initialize the member 'mock_bluetooth_chooser_view_' which does not have a > > default constructor". > > > > I thought if removing the constructor, the compiler will provide a default > one, > > but it seems it doesn't, strange... > > Ah, it's because DISALLOW_COPY_AND_ASSIGN provides a deleted copy constructor, > which suppresses the default constructor. So the default constructor stays. Acknowledged. https://codereview.chromium.org/2275173002/diff/60001/chrome/browser/ui/bluet... File chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc (right): https://codereview.chromium.org/2275173002/diff/60001/chrome/browser/ui/bluet... chrome/browser/ui/bluetooth/bluetooth_chooser_controller_unittest.cc:302: EXPECT_CALL(mock_bluetooth_chooser_view_, On 2016/09/02 20:40:58, Jeffrey Yasskin wrote: > Can you split this test into 3 with clear names also? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/2275173002/#ps80001 (title: "address comments")
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 unit test for BluetoothChooserController With more functionalities added to BluetoothChooserController, it is necessary to add unit test to it. BUG=639163 ========== to ========== Add unit test for BluetoothChooserController With more functionalities added to BluetoothChooserController, it is necessary to add unit test to it. BUG=639163 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add unit test for BluetoothChooserController With more functionalities added to BluetoothChooserController, it is necessary to add unit test to it. BUG=639163 ========== to ========== Add unit test for BluetoothChooserController With more functionalities added to BluetoothChooserController, it is necessary to add unit test to it. BUG=639163 Committed: https://crrev.com/5613eb86a97282c8dbb3390433dbdf2253afb56a Cr-Commit-Position: refs/heads/master@{#416408} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5613eb86a97282c8dbb3390433dbdf2253afb56a Cr-Commit-Position: refs/heads/master@{#416408} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
