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

device/bluetooth/bluetooth_device_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "device/bluetooth/bluetooth_device.h"
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "base/run_loop.h"
@@ skipping 28 matching lines @@
   return static_cast<int8_t>(tx_power);
 }
 
 }  // namespace
 
 #endif
 
 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.
   const char* const kValidFormats[] = {
     "1A:2B:3C:4D:5E:6F",
     "1a:2B:3c:4D:5e:6F",
     "1a:2b:3c:4d:5e:6f",
     "1A-2B-3C-4D-5E-6F",
     "1a-2B-3c-4D-5e-6F",
     "1a-2b-3c-4d-5e-6f",
@@ skipping 1417 matching lines @@
   EXPECT_EQ(BLUETOOTH_TRANSPORT_LE, device->GetType());
 
   BluetoothDevice* device2 = SimulateLowEnergyDevice(6);
   EXPECT_EQ(BLUETOOTH_TRANSPORT_DUAL, device2->GetType());
 
   BluetoothDevice* device3 = SimulateClassicDevice();
   EXPECT_EQ(BLUETOOTH_TRANSPORT_CLASSIC, device3->GetType());
 }
 #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