Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744243008:
Add unit tests for BluetoothDevice::GetPrimaryServices() etc. (Closed)
DescriptionAdd unit tests for BluetoothDevice::GetPrimaryServices() etc.
This CL adds unit tests for the following three functions:
BluetoothDevice::GetPrimaryServices()
BluetoothDevice::GetPrimaryServicesByUUID()
BluetoothDevice::GetCharacteristicsByUUID()
BUG=552022
Review-Url: https://codereview.chromium.org/2744243008
Cr-Commit-Position: refs/heads/master@{#458146}
Committed: https://chromium.googlesource.com/chromium/src/+/7f94243892116f4266e8f532db6977cd5f540c2d
Patch Set 1 : Add unit tests for BluetoothDevice::GetPrimaryServices() etc. #
Total comments: 14
Patch Set 2 : address scheib@'s comments #Patch Set 3 : clean up code #
Total comments: 2
Patch Set 4 : address more comments #Patch Set 5 : move test fixture to the top of the file #
Total comments: 13
Messages
Total messages: 34 (22 generated)
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...
juncai@chromium.org changed reviewers: + scheib@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/2744243008/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1492: InitWithFakeAdapter(); There's a lot of boilerplate for these tests. Consider if it's worth moving out to a helper method. I think the same fake setup could be used for all these service and characteristic tests. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1509: services.push_back("00000001-0000-1000-8000-00805f9b34fb"); multiple services, same ID. So, probably use 1 unique ID, and then 2 instances of a 2nd ID. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1516: // Disconnect from the device Everything after GetPrimaryServices is not needed. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1555: device->GetPrimaryServicesByUUID(BluetoothUUID("non-existent")).empty()); Let's use a valid UUID that doesn't exist in the fake that was setup. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1598: SimulateGattCharacteristic(primary_services[0], services[0], Let's use a unique ID for the characteristic instead of reusing the service UUID. Also, similar to services have 1 UUID used once, and another UUID have multiple instances. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1618: device->GetCharacteristicsByUUID("non-existent", characteristic_uuid_0) Let's use a valid UUID that doesn't exist in the fake that was setup.
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/2744243008/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1492: InitWithFakeAdapter(); On 2017/03/17 17:38:40, scheib wrote: > There's a lot of boilerplate for these tests. Consider if it's worth moving out > to a helper method. I think the same fake setup could be used for all these > service and characteristic tests. I think the above 5 lines are also repeated in other places in this test. Maybe that's on purpose? Even though these three added tests can use the same fake setup, but if put all of these setup code into a function, the following test code still needs to access the |device|, |services|, it seems it will affect the readability of the code. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1509: services.push_back("00000001-0000-1000-8000-00805f9b34fb"); On 2017/03/17 17:38:40, scheib wrote: > multiple services, same ID. So, probably use 1 unique ID, and then 2 instances > of a 2nd ID. Done. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1516: // Disconnect from the device On 2017/03/17 17:38:40, scheib wrote: > Everything after GetPrimaryServices is not needed. Done. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1555: device->GetPrimaryServicesByUUID(BluetoothUUID("non-existent")).empty()); On 2017/03/17 17:38:40, scheib wrote: > Let's use a valid UUID that doesn't exist in the fake that was setup. Done. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1598: SimulateGattCharacteristic(primary_services[0], services[0], On 2017/03/17 17:38:41, scheib wrote: > Let's use a unique ID for the characteristic instead of reusing the service > UUID. Also, similar to services have 1 UUID used once, and another UUID have > multiple instances. Done. https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1618: device->GetCharacteristicsByUUID("non-existent", characteristic_uuid_0) On 2017/03/17 17:38:40, scheib wrote: > Let's use a valid UUID that doesn't exist in the fake that was setup. Done.
https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1492: InitWithFakeAdapter(); On 2017/03/17 21:25:47, juncai wrote: > On 2017/03/17 17:38:40, scheib wrote: > > There's a lot of boilerplate for these tests. Consider if it's worth moving > out > > to a helper method. I think the same fake setup could be used for all these > > service and characteristic tests. > > I think the above 5 lines are also repeated in other places in this test. Maybe > that's on purpose? > > Even though these three added tests can use the same fake setup, but if put all > of these setup code into a function, the following test code still needs to > access the |device|, |services|, it seems it will affect the readability of the > code. Look at e.g. the test fixture setup done for the characteristics unittest. By doing the boilerplate in the test fixture the service and characteristic objects can be saved as member variables. Not all tests in this file have to use the same test fixture, so it's OK for these new tests to use a text fixture just for them. The previous tests didn't overlap as much in what they did vs the number of tests. By the time you add these tests the copy-paste is getting excessive -- especially because all these new tests can use the exact same setup. https://codereview.chromium.org/2744243008/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1623: "01:00:00:90:1E:BE/00000004-0000-1000-8000-00805f9b34fb,0"; This is a platform specific service identifier (from GetIdentifier), it's not a characteristic UUID.
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/2744243008/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1492: InitWithFakeAdapter(); On 2017/03/17 22:10:47, scheib wrote: > On 2017/03/17 21:25:47, juncai wrote: > > On 2017/03/17 17:38:40, scheib wrote: > > > There's a lot of boilerplate for these tests. Consider if it's worth moving > > out > > > to a helper method. I think the same fake setup could be used for all these > > > service and characteristic tests. > > > > I think the above 5 lines are also repeated in other places in this test. > Maybe > > that's on purpose? > > > > Even though these three added tests can use the same fake setup, but if put > all > > of these setup code into a function, the following test code still needs to > > access the |device|, |services|, it seems it will affect the readability of > the > > code. > > Look at e.g. the test fixture setup done for the characteristics unittest. By > doing the boilerplate in the test fixture the service and characteristic objects > can be saved as member variables. Not all tests in this file have to use the > same test fixture, so it's OK for these new tests to use a text fixture just for > them. The previous tests didn't overlap as much in what they did vs the number > of tests. By the time you add these tests the copy-paste is getting excessive -- > especially because all these new tests can use the exact same setup. Done. https://codereview.chromium.org/2744243008/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1623: "01:00:00:90:1E:BE/00000004-0000-1000-8000-00805f9b34fb,0"; On 2017/03/17 22:10:47, scheib wrote: > This is a platform specific service identifier (from GetIdentifier), it's not a > characteristic UUID. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, one last request, please keep fixtures at top of file, and I don't think there's a need to guard it with #if
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...
On 2017/03/18 00:48:14, scheib wrote: > lgtm, one last request, please keep fixtures at top of file, and I don't think > there's a need to guard it with #if 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 scheib@chromium.org Link to the patchset: https://codereview.chromium.org/2744243008/#ps80001 (title: "move test fixture to the top of the file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490036793938710,
"parent_rev": "8b17da783915297bb8a3a46a6484b8787c7db28d", "commit_rev":
"7f94243892116f4266e8f532db6977cd5f540c2d"}
Message was sent while issue was closed.
Description was changed from ========== Add unit tests for BluetoothDevice::GetPrimaryServices() etc. This CL adds unit tests for the following three functions: BluetoothDevice::GetPrimaryServices() BluetoothDevice::GetPrimaryServicesByUUID() BluetoothDevice::GetCharacteristicsByUUID() BUG=552022 ========== to ========== Add unit tests for BluetoothDevice::GetPrimaryServices() etc. This CL adds unit tests for the following three functions: BluetoothDevice::GetPrimaryServices() BluetoothDevice::GetPrimaryServicesByUUID() BluetoothDevice::GetCharacteristicsByUUID() BUG=552022 Review-Url: https://codereview.chromium.org/2744243008 Cr-Commit-Position: refs/heads/master@{#458146} Committed: https://chromium.googlesource.com/chromium/src/+/7f94243892116f4266e8f532db69... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7f94243892116f4266e8f532db69...
Message was sent while issue was closed.
ortuno@chromium.org changed reviewers: + ortuno@chromium.org
Message was sent while issue was closed.
Optional comments for follow up. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1519: #if defined(OS_ANDROID) || defined(OS_MACOSX) We usually add a note about why other platforms are not included here e.g.: TODO(crbug.com/...): Run test on windows once [insert reason why this doesn't work on windows] is fixed. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1530: TEST_F(BluetoothGetServiceOrCharacteristicTest, GetPrimaryServicesByUUID) { This test could be improved by either checking the UUID of the returned services or retrieving the services by other means and then comparing the returned values of GetPrimaryServicesByUUID. As is, our functions could be returning wrong services/characteristics/descriptors and we wouldn't know. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1543: std::string service_uuid_not_exist_in_setup = nit: use existing test UUIDs: https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?... https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1551: TEST_F(BluetoothGetServiceOrCharacteristicTest, GetCharacteristicsByUUID) { Similarly here we should make sure the right characteristics are being returned. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1575: ->GetCharacteristicsByUUID(service_instance_id0, Unrelated questions about these methods: 1. Why is GetCharacteristicsByUUID in BluetoothDevice rather than in BluetoothRemoteGattService? I think that would simplify the code a bit. 2. Why does GetDescriptorByUUID take a characteristic but GetCharacteristicsByUUID takes a characteritistic instance id? https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1620: #endif // defined(OS_ANDROID) || defined(OS_MACOSX) Do we already have tests for GetDescriptorsByUUID? If not why not add them with this change?
Message was sent while issue was closed.
https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1519: #if defined(OS_ANDROID) || defined(OS_MACOSX) On 2017/03/28 03:07:49, ortuno wrote: > We usually add a note about why other platforms are not included here e.g.: > > TODO(crbug.com/...): Run test on windows once [insert reason why this doesn't > work on windows] is fixed. reopen the issue: https://bugs.chromium.org/p/chromium/issues/detail?id=552022 and will do this in a follow-up CL. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1530: TEST_F(BluetoothGetServiceOrCharacteristicTest, GetPrimaryServicesByUUID) { On 2017/03/28 03:07:49, ortuno wrote: > This test could be improved by either checking the UUID of the returned services > or retrieving the services by other means and then comparing the returned values > of GetPrimaryServicesByUUID. As is, our functions could be returning wrong > services/characteristics/descriptors and we wouldn't know. ditto. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1543: std::string service_uuid_not_exist_in_setup = On 2017/03/28 03:07:49, ortuno wrote: > nit: use existing test UUIDs: > https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?... ditto. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1551: TEST_F(BluetoothGetServiceOrCharacteristicTest, GetCharacteristicsByUUID) { On 2017/03/28 03:07:49, ortuno wrote: > Similarly here we should make sure the right characteristics are being returned. ditto. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1575: ->GetCharacteristicsByUUID(service_instance_id0, On 2017/03/28 03:07:49, ortuno wrote: > Unrelated questions about these methods: > > 1. Why is GetCharacteristicsByUUID in BluetoothDevice rather than in > BluetoothRemoteGattService? I think that would simplify the code a bit. > 2. Why does GetDescriptorByUUID take a characteristic but > GetCharacteristicsByUUID takes a characteritistic instance id? For 1: it is mentioned in the TODO at: https://codereview.chromium.org/2615323002/diff/40001/content/browser/bluetoo... For 2: will investigate more. https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1620: #endif // defined(OS_ANDROID) || defined(OS_MACOSX) On 2017/03/28 03:07:49, ortuno wrote: > Do we already have tests for GetDescriptorsByUUID? If not why not add them with > this change? I don't think we have tests for GetDescriptorsByUUID. reopen the issue: https://bugs.chromium.org/p/chromium/issues/detail?id=552022 and will do this in a follow-up CL.
Message was sent while issue was closed.
Thanks! https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1575: ->GetCharacteristicsByUUID(service_instance_id0, On 2017/03/29 at 17:49:53, juncai wrote: > On 2017/03/28 03:07:49, ortuno wrote: > > Unrelated questions about these methods: > > > > 1. Why is GetCharacteristicsByUUID in BluetoothDevice rather than in > > BluetoothRemoteGattService? I think that would simplify the code a bit. > > 2. Why does GetDescriptorByUUID take a characteristic but > > GetCharacteristicsByUUID takes a characteritistic instance id? > > For 1: it is mentioned in the TODO at: > https://codereview.chromium.org/2615323002/diff/40001/content/browser/bluetoo... > For 2: will investigate more. I see. Let's move them to the appropriate attribute then. That will simplify our code and require less tests. |
