Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2785383002:
Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. (Closed)
DescriptionUpdate unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc.
This is a follow-up CL to address comments at:
https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/bluetooth_device_unittest.cc
This CL does the following:
1. Updated test code for BluetoothDevice::GetPrimaryServicesByUUID()
2. Moved BluetoothDevice::GetCharacteristicsByUUID() to
BluetoothRemoteGattService::GetCharacteristicsByUUID()
3. Removed BluetoothDevice::GetDescriptorsByUUID() since there is
BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID()
BUG=552022
Review-Url: https://codereview.chromium.org/2785383002
Cr-Commit-Position: refs/heads/master@{#462169}
Committed: https://chromium.googlesource.com/chromium/src/+/b594d33b1b5a11701884592ba7c82babe0119516
Patch Set 1 : update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. #
Total comments: 34
Patch Set 2 : address comments #Patch Set 3 : address comments #
Total comments: 6
Patch Set 4 : address more comments #Patch Set 5 : address more comments #
Total comments: 16
Patch Set 6 : address comments #
Total comments: 4
Patch Set 7 : added TODO #
Total comments: 2
Messages
Total messages: 48 (34 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
juncai@chromium.org changed reviewers: + ortuno@chromium.org
Please take a look.
https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:69: service_uuids_.push_back("00000000-0000-1000-8000-00805f9b34fb"); Please use existing service uuids: https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.h?l=90 I would recommend Generic Access and Heart Rate. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:80: std::vector<std::string> service_uuids_; It might be better to name the UUIDs like you did in tests: BluetoothUUID unique_service_uuid; BluetoothUUID duplicate_service_uuid; BluetoothUUID unique_characteristic_uuid; BluetoothUUID duplicate_characteristic_uuid; BluetoothUUID unique_descriptor_uuid; BluetoothUUID duplicate_descriptor_uuid; BluetoothUUID absent_uuid; and initialize them in the constructor. This is to make the following lines more readable: EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[0]->GetUUID()); EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[1]->GetUUID()); vs. EXPECT_EQ(duplicate_uuid, services1[0]->GetUUID()); EXPECT_EQ(duplicate_uuid, services1[1]->GetUUID()); https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1521: // TODO(https://crbug.com/507419): Run test on Windows once Bluetooth It's strange to have this since we already have many tests for services that run on windows O.o https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1542: std::vector<BluetoothRemoteGattService*> services0 = To avoid services0, services1, etc. you could add brackets to scope the code: { std::vector<BluetoothRemoteGattService*> services = ... EXPECT_EQ(1u, services.size()); // ... } { std::vector<BluetoothRemoteGattService*> services = ... EXPECT_EQ(2u, services.size()); // ... } https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1551: EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[1]->GetUUID()); Add: EXPECT_NEQ(services1[0]->GetIdentifier(), services1[1]->GetIdentifier()); https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1567: std::vector<BluetoothRemoteGattService*> primary_services = Any reason why we need this code here? Could we move it to the constructor? That way we don't have to repeat it in the next test. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1573: std::string characteristic_uuid0 = "00000002-0000-1000-8000-00805f9b34fb"; Add standard UUIDs to the list of test UUIDs and use them here. I would recommend device_name[1] for generic access and heart_rate_measurement[2] for hear_rate. [1] https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu.... [2] https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu... https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1577: SimulateGattCharacteristic(primary_services[1], characteristic_uuid1, Add a characteristic with the same UUID to this service as well and test that two different characteristics are returned. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1582: std::vector<BluetoothRemoteGattCharacteristic*> characteristics0 = Also consider using brackets to avoid characteristics0, characteristics1, etc. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1603: std::string service_instance_id_not_exist_in_setup = Can we refactor this method to be in the service to avoid all of this testing. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1662: // Add several descriptors. No descriptors are added to |characteristic2|. We should just put all these setup code in the constructor. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1665: BluetoothUUID descriptor_uuid2("33333333-0000-1000-8000-00805f9b34fb"); Also add these to the list of test UUIDs. I would use client_characteristic_configuration[1] and characteristic_user_description[2] [1] https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu... [2] https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu... https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1678: std::vector<device::BluetoothRemoteGattDescriptor*> descriptors1 = nit: add new line https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1714: EXPECT_TRUE( Can we refactor this method to be in the characteristic and avoid this test?
Description was changed from ========== Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. This is a follow-up CL to address comments at: https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... This CL updated test code for BluetoothDevice::GetPrimaryServicesByUUID(), BluetoothDevice::GetCharacteristicsByUUID(), and added test code for BluetoothDevice::GetDescriptorsByUUID(). BUG=552022 ========== to ========== Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. This is a follow-up CL to address comments at: https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... This CL updated test code for BluetoothDevice::GetPrimaryServicesByUUID(), BluetoothDevice::GetCharacteristicsByUUID() BUG=552022 ==========
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:69: service_uuids_.push_back("00000000-0000-1000-8000-00805f9b34fb"); On 2017/04/03 03:36:37, ortuno wrote: > Please use existing service uuids: > https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.h?l=90 > > I would recommend Generic Access and Heart Rate. Done. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:80: std::vector<std::string> service_uuids_; On 2017/04/03 03:36:38, ortuno wrote: > It might be better to name the UUIDs like you did in tests: > > BluetoothUUID unique_service_uuid; > BluetoothUUID duplicate_service_uuid; > BluetoothUUID unique_characteristic_uuid; > BluetoothUUID duplicate_characteristic_uuid; > BluetoothUUID unique_descriptor_uuid; > BluetoothUUID duplicate_descriptor_uuid; > > BluetoothUUID absent_uuid; > > and initialize them in the constructor. > > This is to make the following lines more readable: > > EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[0]->GetUUID()); > EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[1]->GetUUID()); > > vs. > > EXPECT_EQ(duplicate_uuid, services1[0]->GetUUID()); > EXPECT_EQ(duplicate_uuid, services1[1]->GetUUID()); It seems redundant to do this since there are already comments there when adding those UUIDs to the |service_uuids_|, and even if we do that, we still need to add them to the |service_uuids_| to pass them to SimulateGattServicesDiscovered(). https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1521: // TODO(https://crbug.com/507419): Run test on Windows once Bluetooth On 2017/04/03 03:36:38, ortuno wrote: > It's strange to have this since we already have many tests for services that run > on windows O.o device_->IsConnected() returns false on Windows so the test is not able to run on Windows. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1542: std::vector<BluetoothRemoteGattService*> services0 = On 2017/04/03 03:36:38, ortuno wrote: > To avoid services0, services1, etc. you could add brackets to scope the code: > > { > std::vector<BluetoothRemoteGattService*> services = ... > EXPECT_EQ(1u, services.size()); > // ... > } > > { > std::vector<BluetoothRemoteGattService*> services = ... > EXPECT_EQ(2u, services.size()); > // ... > } Done. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1551: EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[1]->GetUUID()); On 2017/04/03 03:36:38, ortuno wrote: > Add: > > EXPECT_NEQ(services1[0]->GetIdentifier(), services1[1]->GetIdentifier()); Done. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1567: std::vector<BluetoothRemoteGattService*> primary_services = On 2017/04/03 03:36:38, ortuno wrote: > Any reason why we need this code here? Could we move it to the constructor? That > way we don't have to repeat it in the next test. GetPrimaryServices() is the function to be tested by this BluetoothGetServiceOrCharacteristicOrDescriptorTest class, so it is better to not store the return value of the to-be-tested function into the class that is used to test it. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1573: std::string characteristic_uuid0 = "00000002-0000-1000-8000-00805f9b34fb"; On 2017/04/03 03:36:38, ortuno wrote: > Add standard UUIDs to the list of test UUIDs and use them here. I would > recommend device_name[1] for generic access and heart_rate_measurement[2] for > hear_rate. > > [1] > https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu.... > [2] > https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu... Done. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1577: SimulateGattCharacteristic(primary_services[1], characteristic_uuid1, On 2017/04/03 03:36:38, ortuno wrote: > Add a characteristic with the same UUID to this service as well and test that > two different characteristics are returned. Done. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1582: std::vector<BluetoothRemoteGattCharacteristic*> characteristics0 = On 2017/04/03 03:36:38, ortuno wrote: > Also consider using brackets to avoid characteristics0, characteristics1, etc. Done. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1603: std::string service_instance_id_not_exist_in_setup = On 2017/04/03 03:36:38, ortuno wrote: > Can we refactor this method to be in the service to avoid all of this testing. https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... I noticed that BluetoothRemoteGattService class is a pure abstract class. So this method can not be moved there. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1662: // Add several descriptors. No descriptors are added to |characteristic2|. On 2017/04/03 03:36:38, ortuno wrote: > We should just put all these setup code in the constructor. The same function already exists at BluetoothRemoteGattCharacteristic: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... And there is test at: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... So this function is removed the BluetoothDevice, and also removed the test code. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1665: BluetoothUUID descriptor_uuid2("33333333-0000-1000-8000-00805f9b34fb"); On 2017/04/03 03:36:38, ortuno wrote: > Also add these to the list of test UUIDs. I would use > client_characteristic_configuration[1] and characteristic_user_description[2] > > [1] > https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu... > [2] > https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu... ditto. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1678: std::vector<device::BluetoothRemoteGattDescriptor*> descriptors1 = On 2017/04/03 03:36:38, ortuno wrote: > nit: add new line ditto. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1714: EXPECT_TRUE( On 2017/04/03 03:36:38, ortuno wrote: > Can we refactor this method to be in the characteristic and avoid this test? ditto.
https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:80: std::vector<std::string> service_uuids_; On 2017/04/04 at 03:03:42, juncai wrote: > On 2017/04/03 03:36:38, ortuno wrote: > > It might be better to name the UUIDs like you did in tests: > > > > BluetoothUUID unique_service_uuid; > > BluetoothUUID duplicate_service_uuid; > > BluetoothUUID unique_characteristic_uuid; > > BluetoothUUID duplicate_characteristic_uuid; > > BluetoothUUID unique_descriptor_uuid; > > BluetoothUUID duplicate_descriptor_uuid; > > > > BluetoothUUID absent_uuid; > > > > and initialize them in the constructor. > > > > This is to make the following lines more readable: > > > > EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[0]->GetUUID()); > > EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[1]->GetUUID()); > > > > vs. > > > > EXPECT_EQ(duplicate_uuid, services1[0]->GetUUID()); > > EXPECT_EQ(duplicate_uuid, services1[1]->GetUUID()); > > It seems redundant to do this since there are already comments there when adding those UUIDs to the |service_uuids_|, and even if we do that, we still need to add them to the |service_uuids_| to pass them to SimulateGattServicesDiscovered(). If you read the test its hard to understand why device_->GetPrimaryServicesByUUID(BluetoothUUID(service_uuids_[0])); returns one service whereas device_->GetPrimaryServicesByUUID(BluetoothUUID(service_uuids_[1])); returns two services. I was going to suggest commenting what was going on but I believe having the code be self documenting is better. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1521: // TODO(https://crbug.com/507419): Run test on Windows once Bluetooth On 2017/04/04 at 03:03:42, juncai wrote: > On 2017/04/03 03:36:38, ortuno wrote: > > It's strange to have this since we already have many tests for services that run > > on windows O.o > > device_->IsConnected() returns false on Windows so the test is not able to run on Windows. We already run many Service tests[1] on Windows so we should be able to still test this on Windows. You can add a #if defined(OS_WINDOWS) around the connection code and unblock the test. [1] https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1603: std::string service_instance_id_not_exist_in_setup = On 2017/04/04 at 03:03:42, juncai wrote: > On 2017/04/03 03:36:38, ortuno wrote: > > Can we refactor this method to be in the service to avoid all of this testing. > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... > I noticed that BluetoothRemoteGattService class is a pure abstract class. So this method can not be moved there. Can you open an issue explaining what needs to be done to move this method to BluetoothRemoteGattService and put that issue on top in the method's description and here? I think its easy enough to mark it as GoodFirstBug. https://codereview.chromium.org/2785383002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1575: std::string characteristic_uuid0 = "00002a00-0000-1000-8000-00805f9b34fb"; Move these to bluetooth_test.h https://codereview.chromium.org/2785383002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1605: } Compare the identifiers to make sure they are two different characteristics. https://codereview.chromium.org/2785383002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1629: std::string characteristic_uuid_not_exist_in_setup = Also move this to bluetooth_test.h
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...
Description was changed from ========== Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. This is a follow-up CL to address comments at: https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... This CL updated test code for BluetoothDevice::GetPrimaryServicesByUUID(), BluetoothDevice::GetCharacteristicsByUUID() BUG=552022 ========== to ========== Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. This is a follow-up CL to address comments at: https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... This CL does the following: 1. Updated test code for BluetoothDevice::GetPrimaryServicesByUUID() 2. Moved BluetoothDevice::GetCharacteristicsByUUID() to BluetoothRemoteGattService::GetCharacteristicsByUUID() 3. Removed BluetoothDevice::GetDescriptorsByUUID() since there is BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID() BUG=552022 ==========
Description was changed from ========== Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. This is a follow-up CL to address comments at: https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... This CL does the following: 1. Updated test code for BluetoothDevice::GetPrimaryServicesByUUID() 2. Moved BluetoothDevice::GetCharacteristicsByUUID() to BluetoothRemoteGattService::GetCharacteristicsByUUID() 3. Removed BluetoothDevice::GetDescriptorsByUUID() since there is BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID() BUG=552022 ========== to ========== Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. This is a follow-up CL to address comments at: https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... This CL does the following: 1. Updated test code for BluetoothDevice::GetPrimaryServicesByUUID() 2. Moved BluetoothDevice::GetCharacteristicsByUUID() to BluetoothRemoteGattService::GetCharacteristicsByUUID() 3. Removed BluetoothDevice::GetDescriptorsByUUID() since there is BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID() BUG=552022 ==========
https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:80: std::vector<std::string> service_uuids_; On 2017/04/04 03:32:11, ortuno wrote: > On 2017/04/04 at 03:03:42, juncai wrote: > > On 2017/04/03 03:36:38, ortuno wrote: > > > It might be better to name the UUIDs like you did in tests: > > > > > > BluetoothUUID unique_service_uuid; > > > BluetoothUUID duplicate_service_uuid; > > > BluetoothUUID unique_characteristic_uuid; > > > BluetoothUUID duplicate_characteristic_uuid; > > > BluetoothUUID unique_descriptor_uuid; > > > BluetoothUUID duplicate_descriptor_uuid; > > > > > > BluetoothUUID absent_uuid; > > > > > > and initialize them in the constructor. > > > > > > This is to make the following lines more readable: > > > > > > EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[0]->GetUUID()); > > > EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[1]->GetUUID()); > > > > > > vs. > > > > > > EXPECT_EQ(duplicate_uuid, services1[0]->GetUUID()); > > > EXPECT_EQ(duplicate_uuid, services1[1]->GetUUID()); > > > > It seems redundant to do this since there are already comments there when > adding those UUIDs to the |service_uuids_|, and even if we do that, we still > need to add them to the |service_uuids_| to pass them to > SimulateGattServicesDiscovered(). > > If you read the test its hard to understand why > > device_->GetPrimaryServicesByUUID(BluetoothUUID(service_uuids_[0])); > > returns one service whereas > > device_->GetPrimaryServicesByUUID(BluetoothUUID(service_uuids_[1])); > > returns two services. > > I was going to suggest commenting what was going on but I believe having the > code be self documenting is better. Done. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1521: // TODO(https://crbug.com/507419): Run test on Windows once Bluetooth On 2017/04/04 03:32:11, ortuno wrote: > On 2017/04/04 at 03:03:42, juncai wrote: > > On 2017/04/03 03:36:38, ortuno wrote: > > > It's strange to have this since we already have many tests for services that > run > > > on windows O.o > > > > device_->IsConnected() returns false on Windows so the test is not able to run > on Windows. > > We already run many Service tests[1] on Windows so we should be able to still > test this on Windows. You can add a #if defined(OS_WINDOWS) around the > connection code and unblock the test. > > [1] > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... Done. https://codereview.chromium.org/2785383002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_unittest.cc:1603: std::string service_instance_id_not_exist_in_setup = On 2017/04/04 03:32:11, ortuno wrote: > On 2017/04/04 at 03:03:42, juncai wrote: > > On 2017/04/03 03:36:38, ortuno wrote: > > > Can we refactor this method to be in the service to avoid all of this > testing. > > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... > > I noticed that BluetoothRemoteGattService class is a pure abstract class. So > this method can not be moved there. > > Can you open an issue explaining what needs to be done to move this method to > BluetoothRemoteGattService and put that issue on top in the method's description > and here? I think its easy enough to mark it as GoodFirstBug. Moved it to BluetoothRemoteGattService. Done. https://codereview.chromium.org/2785383002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1575: std::string characteristic_uuid0 = "00002a00-0000-1000-8000-00805f9b34fb"; On 2017/04/04 03:32:12, ortuno wrote: > Move these to bluetooth_test.h Not used any more. https://codereview.chromium.org/2785383002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1605: } On 2017/04/04 03:32:12, ortuno wrote: > Compare the identifiers to make sure they are two different characteristics. Done. https://codereview.chromium.org/2785383002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1629: std::string characteristic_uuid_not_exist_in_setup = On 2017/04/04 03:32:12, ortuno wrote: > Also move this to bluetooth_test.h Not used any more.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:69: #if defined(OS_WIN) // TODO(crbug.com/507419): Check connection once CreateGattConnection is implemented on windows. https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:76: service_uuids_.push_back(unique_service_uuid_.value()); nit: use canonical_value. value() can change depending on the format of the UUID. https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_remote_gatt_service.cc (right): https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service.cc:21: GetCharacteristics(); Nice! I thought we might have to refactor all impls to use the same map but this is much better :) https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:194: SimulateGattServicesDiscovered(device, services); SimulateGattServicesDiscovered( device, std::vector<std::string>({kTestUUIDGenericAccess, kTestUUIDHeartRate})); https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:198: std::string characteristic_uuid1 = "11111111-0000-1000-8000-00805f9b34fb"; Add standard UUIDs to the list of test UUIDs and use those here. // bluetooth_test.h // Services static const std::string kTestUUIDGenericAccess; // ... // Characteristics static const std::string kTestUUIDDeviceName; static const std::string kTestUUIDHeartRateMeasurement; // here SimulateGattCharacteristic(service1, kTestUUIDDeviceName, /* properties */ 0); SimulateGattCharacteristic(service2, kTestUUIDHeartRateMeasurement, /* properties */ 0); SimulateGattCharacteristic(service2, kTestUUIDHeartRateMeasurement, /* properties */ 0); https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:208: std::vector<BluetoothRemoteGattCharacteristic*> characteristics1 = Consider using brackets to avoid characteristics1, characteristics2, etc. https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:211: EXPECT_EQ(characteristic_uuid1, characteristics1[0]->GetUUID().value()); Use canonical_value here and elsewhere. value() depends on things like how the UUID was constructed and its format, canonical_value() is always the same.
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/2785383002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:69: #if defined(OS_WIN) On 2017/04/04 22:14:12, ortuno wrote: > // TODO(crbug.com/507419): Check connection once CreateGattConnection is > implemented on windows. Done. https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:76: service_uuids_.push_back(unique_service_uuid_.value()); On 2017/04/04 22:14:12, ortuno wrote: > nit: use canonical_value. value() can change depending on the format of the > UUID. Done. https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_remote_gatt_service.cc (right): https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service.cc:21: GetCharacteristics(); On 2017/04/04 22:14:12, ortuno wrote: > Nice! I thought we might have to refactor all impls to use the same map but this > is much better :) Thanks! :). https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:194: SimulateGattServicesDiscovered(device, services); On 2017/04/04 22:14:12, ortuno wrote: > SimulateGattServicesDiscovered( > device, std::vector<std::string>({kTestUUIDGenericAccess, > kTestUUIDHeartRate})); The added code is the same as the following in the same test file: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... I think this can make the test code consistent in the same file. And in the follow-ups, we can do what you suggested and change them all to use the ones you mentioned in the bluetooth_test.h and also add some new uuids. In this way, we don't have to change bluetooth_test.h and bluetooth_test.cc in this CL and make the goal of this CL clear. What do you think? https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:198: std::string characteristic_uuid1 = "11111111-0000-1000-8000-00805f9b34fb"; On 2017/04/04 22:14:12, ortuno wrote: > Add standard UUIDs to the list of test UUIDs and use those here. > > // bluetooth_test.h > > // Services > static const std::string kTestUUIDGenericAccess; > // ... > // Characteristics > static const std::string kTestUUIDDeviceName; > static const std::string kTestUUIDHeartRateMeasurement; > > // here > SimulateGattCharacteristic(service1, kTestUUIDDeviceName, /* properties */ 0); > SimulateGattCharacteristic(service2, kTestUUIDHeartRateMeasurement, /* > properties */ 0); > SimulateGattCharacteristic(service2, kTestUUIDHeartRateMeasurement, /* > properties */ 0); ditto. Same as: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... What about doing it in a follow-up CL? https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:208: std::vector<BluetoothRemoteGattCharacteristic*> characteristics1 = On 2017/04/04 22:14:12, ortuno wrote: > Consider using brackets to avoid characteristics1, characteristics2, etc. Done. https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:211: EXPECT_EQ(characteristic_uuid1, characteristics1[0]->GetUUID().value()); On 2017/04/04 22:14:12, ortuno wrote: > Use canonical_value here and elsewhere. value() depends on things like how the > UUID was constructed and its format, canonical_value() is always the same. Done.
https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:194: SimulateGattServicesDiscovered(device, services); On 2017/04/05 at 00:36:46, juncai wrote: > On 2017/04/04 22:14:12, ortuno wrote: > > SimulateGattServicesDiscovered( > > device, std::vector<std::string>({kTestUUIDGenericAccess, > > kTestUUIDHeartRate})); > > The added code is the same as the following in the same test file: > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... > > I think this can make the test code consistent in the same file. And in the follow-ups, we can do what you suggested and change them all to use the ones you mentioned in the bluetooth_test.h and also add some new uuids. In this way, we don't have to change bluetooth_test.h and bluetooth_test.cc in this CL and make the goal of this CL clear. What do you think? tldr; follow up sounds good! I would love to always fix the whole file but a lot of times we get patches from other teams and it seems unreasonable to ask them to fix all of our tests for us. For that reason I usually just ask new code to follow better patterns so at least our code gets better over time. If you can submit a follow up patch that fixes the problem for the whole file that would awesome! https://codereview.chromium.org/2785383002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:82: SimulateGattServicesDiscovered(device_, service_uuids_); service_uuids_ seems unused outside of the constructor so might as well just inline the array: SimulateGattServicesDiscovered(device_, std::vector<std::string>({ unique_service_uuid.canonical_value(), duplicate_service_uuid.canonical_value(), duplicate_service_uuid.canonical_value()})); Or if you prefer to keep service_uuids as a local variable: std::vector<std::string> service_uuids = { unique_service_uuid.canonical_value(), duplicate_service_uuid.canonical_value(), duplicate_service_uuid.canonical_value()}; https://codereview.chromium.org/2785383002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:84: #if defined(OS_WIN) Also add the TODO here.
Also lgtm!
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 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:194: SimulateGattServicesDiscovered(device, services); On 2017/04/05 00:53:10, ortuno wrote: > On 2017/04/05 at 00:36:46, juncai wrote: > > On 2017/04/04 22:14:12, ortuno wrote: > > > SimulateGattServicesDiscovered( > > > device, std::vector<std::string>({kTestUUIDGenericAccess, > > > kTestUUIDHeartRate})); > > > > The added code is the same as the following in the same test file: > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... > > > > I think this can make the test code consistent in the same file. And in the > follow-ups, we can do what you suggested and change them all to use the ones you > mentioned in the bluetooth_test.h and also add some new uuids. In this way, we > don't have to change bluetooth_test.h and bluetooth_test.cc in this CL and make > the goal of this CL clear. What do you think? > > tldr; follow up sounds good! > > I would love to always fix the whole file but a lot of times we get patches from > other teams and it seems unreasonable to ask them to fix all of our tests for > us. For that reason I usually just ask new code to follow better patterns so at > least our code gets better over time. If you can submit a follow up patch that > fixes the problem for the whole file that would awesome! I will submit a CL to do that. https://codereview.chromium.org/2785383002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:82: SimulateGattServicesDiscovered(device_, service_uuids_); On 2017/04/05 00:53:10, ortuno wrote: > service_uuids_ seems unused outside of the constructor so might as well just > inline the array: > > SimulateGattServicesDiscovered(device_, std::vector<std::string>({ > unique_service_uuid.canonical_value(), > duplicate_service_uuid.canonical_value(), > duplicate_service_uuid.canonical_value()})); > > Or if you prefer to keep service_uuids as a local variable: > > std::vector<std::string> service_uuids = { > unique_service_uuid.canonical_value(), > duplicate_service_uuid.canonical_value(), > duplicate_service_uuid.canonical_value()}; Done. https://codereview.chromium.org/2785383002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:84: #if defined(OS_WIN) On 2017/04/05 00:53:10, ortuno wrote: > Also add the TODO here. Done.
The CQ bit was checked by juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2785383002/#ps120001 (title: "added TODO")
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": 120001, "attempt_start_ts": 1491418895036080,
"parent_rev": "48b70aeb8ed663be110af6328bf49ac1110f0e3e", "commit_rev":
"b594d33b1b5a11701884592ba7c82babe0119516"}
Message was sent while issue was closed.
Description was changed from ========== Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. This is a follow-up CL to address comments at: https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... This CL does the following: 1. Updated test code for BluetoothDevice::GetPrimaryServicesByUUID() 2. Moved BluetoothDevice::GetCharacteristicsByUUID() to BluetoothRemoteGattService::GetCharacteristicsByUUID() 3. Removed BluetoothDevice::GetDescriptorsByUUID() since there is BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID() BUG=552022 ========== to ========== Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc. This is a follow-up CL to address comments at: https://codereview.chromium.org/2744243008/diff/80001/device/bluetooth/blueto... This CL does the following: 1. Updated test code for BluetoothDevice::GetPrimaryServicesByUUID() 2. Moved BluetoothDevice::GetCharacteristicsByUUID() to BluetoothRemoteGattService::GetCharacteristicsByUUID() 3. Removed BluetoothDevice::GetDescriptorsByUUID() since there is BluetoothRemoteGattCharacteristic::GetDescriptorsByUUID() BUG=552022 Review-Url: https://codereview.chromium.org/2785383002 Cr-Commit-Position: refs/heads/master@{#462169} Committed: https://chromium.googlesource.com/chromium/src/+/b594d33b1b5a11701884592ba7c8... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b594d33b1b5a11701884592ba7c8...
Message was sent while issue was closed.
https://codereview.chromium.org/2785383002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:81: service_uuids.push_back(duplicate_service_uuid_.canonical_value()); Out of curiosity why did you end up using push_back instead of an initializer list?
Message was sent while issue was closed.
https://codereview.chromium.org/2785383002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2785383002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:81: service_uuids.push_back(duplicate_service_uuid_.canonical_value()); On 2017/04/05 22:50:00, ortuno wrote: > Out of curiosity why did you end up using push_back instead of an initializer > list? From what I see, other places in this file use push_back: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device_unitte... So I thought just following the same pattern to make it consistent. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
