Add unit tests for BluetoothDevice::GetPrimaryServices() etc.

device/bluetooth/bluetooth_device_unittest.cc

Index: device/bluetooth/bluetooth_device_unittest.cc
diff --git a/device/bluetooth/bluetooth_device_unittest.cc b/device/bluetooth/bluetooth_device_unittest.cc
index 9dcb4a1a7be0f094445f25151c8061088db20c55..ab478fc7386df4a49d9bf91a0645488354e48239 100644
--- a/device/bluetooth/bluetooth_device_unittest.cc
+++ b/device/bluetooth/bluetooth_device_unittest.cc
@@ -46,6 +46,39 @@ int8_t ToInt8(BluetoothTest::TestTxPower tx_power) {
 using UUIDSet = BluetoothDevice::UUIDSet;
 using ServiceDataMap = BluetoothDevice::ServiceDataMap;
 
+class BluetoothGetServiceOrCharacteristicTest : public BluetoothTest {
+ public:
+  // Creates |device_|, |services_|.
+  void FakeServiceBoilerplate() {
+    InitWithFakeAdapter();
+    StartLowEnergyDiscoverySession();
+    device_ = SimulateLowEnergyDevice(3);
+    EXPECT_FALSE(device_->IsConnected());
+
+    // Connect to the device.
+    ResetEventCounts();
+    device_->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                                  GetConnectErrorCallback(Call::NOT_EXPECTED));
+    TestBluetoothAdapterObserver observer(adapter_);
+    SimulateGattConnection(device_);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(device_->IsConnected());
+
+    // Discover services.
+    services_.push_back("00000000-0000-1000-8000-00805f9b34fb");
+    // 2 duplicate UUIDs creating 2 instances.
+    services_.push_back("00000001-0000-1000-8000-00805f9b34fb");
+    services_.push_back("00000001-0000-1000-8000-00805f9b34fb");
+    SimulateGattServicesDiscovered(device_, services_);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(device_->IsGattServicesDiscoveryComplete());
+  }
+
+ protected:
+  BluetoothDevice* device_ = nullptr;
+  std::vector<std::string> services_;
+};
+
 TEST(BluetoothDeviceTest, CanonicalizeAddressFormat_AcceptsAllValidFormats) {
   // There are three valid separators (':', '-', and none).
   // Case shouldn't matter.
@@ -1483,4 +1516,107 @@ TEST_F(BluetoothTest, GetDeviceTransportType) {
 }
 #endif  // defined(OS_CHROMEOS) || defined(OS_LINUX)
 
+#if defined(OS_ANDROID) || defined(OS_MACOSX)

[ortuno, 2017/03/28 03:07:49]

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.

[juncai, 2017/03/29 17:49:52]

reopen the issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=552022
and will do this in a follow-up CL.

+TEST_F(BluetoothGetServiceOrCharacteristicTest, GetPrimaryServices) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  ASSERT_NO_FATAL_FAILURE(FakeServiceBoilerplate());
+
+  EXPECT_EQ(3u, device_->GetPrimaryServices().size());
+}
+
+TEST_F(BluetoothGetServiceOrCharacteristicTest, GetPrimaryServicesByUUID) {

[ortuno, 2017/03/28 03:07:49]

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.

[juncai, 2017/03/29 17:49:53]

ditto.

+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  ASSERT_NO_FATAL_FAILURE(FakeServiceBoilerplate());
+
+  EXPECT_EQ(
+      1u,
+      device_->GetPrimaryServicesByUUID(BluetoothUUID(services_[0])).size());
+  EXPECT_EQ(
+      2u,
+      device_->GetPrimaryServicesByUUID(BluetoothUUID(services_[1])).size());
+  std::string service_uuid_not_exist_in_setup =

[ortuno, 2017/03/28 03:07:49]

nit: use existing test UUIDs:
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

[juncai, 2017/03/29 17:49:53]

ditto.

+      "00000002-0000-1000-8000-00805f9b34fb";
+  EXPECT_TRUE(device_
+                  ->GetPrimaryServicesByUUID(
+                      BluetoothUUID(service_uuid_not_exist_in_setup))
+                  .empty());
+}
+
+TEST_F(BluetoothGetServiceOrCharacteristicTest, GetCharacteristicsByUUID) {

[ortuno, 2017/03/28 03:07:49]

Similarly here we should make sure the right characteristics are being returned.

[juncai, 2017/03/29 17:49:53]

ditto.

+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  ASSERT_NO_FATAL_FAILURE(FakeServiceBoilerplate());
+
+  std::vector<BluetoothRemoteGattService*> primary_services =
+      device_->GetPrimaryServices();
+  std::string service_instance_id0 = primary_services[0]->GetIdentifier();
+  std::string service_instance_id1 = primary_services[1]->GetIdentifier();
+  std::string service_instance_id2 = primary_services[2]->GetIdentifier();
+
+  std::string characteristic_uuid0 = "00000002-0000-1000-8000-00805f9b34fb";
+  std::string characteristic_uuid1 = "00000003-0000-1000-8000-00805f9b34fb";
+  SimulateGattCharacteristic(primary_services[0], characteristic_uuid0,
+                             0 /* properties */);
+  SimulateGattCharacteristic(primary_services[1], characteristic_uuid1,
+                             0 /* properties */);
+  SimulateGattCharacteristic(primary_services[2], characteristic_uuid1,
+                             0 /* properties */);
+
+  EXPECT_EQ(1u,
+            device_
+                ->GetCharacteristicsByUUID(service_instance_id0,

[ortuno, 2017/03/28 03:07:49]

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?

[juncai, 2017/03/29 17:49:53]

For 1: it is mentioned in the TODO at:
https://codereview.chromium.org/2615323002/diff/40001/content/browser/bluetoo...
For 2: will investigate more.

[ortuno, 2017/03/29 22:45:42]

I see. Let's move them to the appropriate attribute then. That will simplify our
code and require less tests.

+                                           BluetoothUUID(characteristic_uuid0))
+                .size());
+  EXPECT_EQ(1u,
+            device_
+                ->GetCharacteristicsByUUID(service_instance_id1,
+                                           BluetoothUUID(characteristic_uuid1))
+                .size());
+  EXPECT_EQ(1u,
+            device_
+                ->GetCharacteristicsByUUID(service_instance_id2,
+                                           BluetoothUUID(characteristic_uuid1))
+                .size());
+
+  std::string service_instance_id_not_exist_in_setup =
+      "non-existent platform specific service instance id";
+  EXPECT_TRUE(
+      device_
+          ->GetCharacteristicsByUUID(service_instance_id_not_exist_in_setup,
+                                     BluetoothUUID(characteristic_uuid0))
+          .empty());
+  EXPECT_TRUE(
+      device_
+          ->GetCharacteristicsByUUID(service_instance_id_not_exist_in_setup,
+                                     BluetoothUUID(characteristic_uuid1))
+          .empty());
+
+  std::string characteristic_uuid_not_exist_in_setup =
+      "00000005-0000-1000-8000-00805f9b34fb";
+  EXPECT_TRUE(device_
+                  ->GetCharacteristicsByUUID(
+                      service_instance_id0,
+                      BluetoothUUID(characteristic_uuid_not_exist_in_setup))
+                  .empty());
+  EXPECT_TRUE(device_
+                  ->GetCharacteristicsByUUID(
+                      service_instance_id1,
+                      BluetoothUUID(characteristic_uuid_not_exist_in_setup))
+                  .empty());
+  EXPECT_TRUE(device_
+                  ->GetCharacteristicsByUUID(
+                      service_instance_id2,
+                      BluetoothUUID(characteristic_uuid_not_exist_in_setup))
+                  .empty());
+}
+#endif  // defined(OS_ANDROID) || defined(OS_MACOSX)

[ortuno, 2017/03/28 03:07:49]

Do we already have tests for GetDescriptorsByUUID? If not why not add them with
this change?

[juncai, 2017/03/29 17:49:53]

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.

+
 }  // namespace device