Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() 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 {
+class BluetoothGetServiceOrCharacteristicOrDescriptorTest
+    : public BluetoothTest {
  public:
-  // Creates |device_|, |services_|.
+  // Creates |device_|, |service_uuids_|.
   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");
+    service_uuids_.push_back("00000000-0000-1000-8000-00805f9b34fb");

[ortuno, 2017/04/03 03:36:37]

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.

[juncai, 2017/04/04 03:03:42]

Done.

     // 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_);
+    service_uuids_.push_back("00000001-0000-1000-8000-00805f9b34fb");
+    service_uuids_.push_back("00000001-0000-1000-8000-00805f9b34fb");
+    SimulateGattServicesDiscovered(device_, service_uuids_);
     base::RunLoop().RunUntilIdle();
     EXPECT_TRUE(device_->IsGattServicesDiscoveryComplete());
   }
 
  protected:
   BluetoothDevice* device_ = nullptr;
-  std::vector<std::string> services_;
+  std::vector<std::string> service_uuids_;

[ortuno, 2017/04/03 03:36:38]

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());

[juncai, 2017/04/04 03:03:42]

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().

[ortuno, 2017/04/04 03:32:11]

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.

[juncai, 2017/04/04 20:33:40]

Done.

 };
 
 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",
@@ skipping 1420 matching lines @@
 
   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)
-TEST_F(BluetoothGetServiceOrCharacteristicTest, GetPrimaryServices) {
+// TODO(https://crbug.com/507419): Run test on Windows once Bluetooth

[ortuno, 2017/04/03 03:36:38]

It's strange to have this since we already have many tests for services that run
on windows O.o

[juncai, 2017/04/04 03:03:42]

device_->IsConnected() returns false on Windows so the test is not able to run
on Windows.

[ortuno, 2017/04/04 03:32:11]

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...

[juncai, 2017/04/04 20:33:40]

Done.

+// connection is implemented.
+TEST_F(BluetoothGetServiceOrCharacteristicOrDescriptorTest,
+       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) {
+TEST_F(BluetoothGetServiceOrCharacteristicOrDescriptorTest,
+       GetPrimaryServicesByUUID) {
   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 =
-      "00000002-0000-1000-8000-00805f9b34fb";
+  std::vector<BluetoothRemoteGattService*> services0 =

[ortuno, 2017/04/03 03:36:38]

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());
  // ...
}

[juncai, 2017/04/04 03:03:42]

Done.

+      device_->GetPrimaryServicesByUUID(BluetoothUUID(service_uuids_[0]));
+  EXPECT_EQ(1u, services0.size());
+  EXPECT_EQ(BluetoothUUID(service_uuids_[0]), services0[0]->GetUUID());
+
+  std::vector<BluetoothRemoteGattService*> services1 =
+      device_->GetPrimaryServicesByUUID(BluetoothUUID(service_uuids_[1]));
+  EXPECT_EQ(2u, services1.size());
+  EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[0]->GetUUID());
+  EXPECT_EQ(BluetoothUUID(service_uuids_[1]), services1[1]->GetUUID());

[ortuno, 2017/04/03 03:36:38]

Add:

EXPECT_NEQ(services1[0]->GetIdentifier(), services1[1]->GetIdentifier());

[juncai, 2017/04/04 03:03:41]

Done.

+
   EXPECT_TRUE(device_
                   ->GetPrimaryServicesByUUID(
-                      BluetoothUUID(service_uuid_not_exist_in_setup))
+                      BluetoothUUID(BluetoothTestBase::kTestUUIDGenericAccess))
                   .empty());
 }
 
-TEST_F(BluetoothGetServiceOrCharacteristicTest, GetCharacteristicsByUUID) {
+TEST_F(BluetoothGetServiceOrCharacteristicOrDescriptorTest,
+       GetCharacteristicsByUUID) {
   if (!PlatformSupportsLowEnergy()) {
     LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
     return;
   }
   ASSERT_NO_FATAL_FAILURE(FakeServiceBoilerplate());
 
   std::vector<BluetoothRemoteGattService*> primary_services =

[ortuno, 2017/04/03 03:36:38]

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.

[juncai, 2017/04/04 03:03:41]

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.

       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";

[ortuno, 2017/04/03 03:36:38]

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...

[juncai, 2017/04/04 03:03:41]

Done.

   std::string characteristic_uuid1 = "00000003-0000-1000-8000-00805f9b34fb";
   SimulateGattCharacteristic(primary_services[0], characteristic_uuid0,
                              0 /* properties */);
   SimulateGattCharacteristic(primary_services[1], characteristic_uuid1,

[ortuno, 2017/04/03 03:36:38]

Add a characteristic with the same UUID to this service as well and test that
two different characteristics are returned.

[juncai, 2017/04/04 03:03:42]

Done.

                              0 /* properties */);
   SimulateGattCharacteristic(primary_services[2], characteristic_uuid1,
                              0 /* properties */);
 
-  EXPECT_EQ(1u,
-            device_
-                ->GetCharacteristicsByUUID(service_instance_id0,
-                                           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::vector<BluetoothRemoteGattCharacteristic*> characteristics0 =

[ortuno, 2017/04/03 03:36:38]

Also consider using brackets to avoid characteristics0, characteristics1, etc.

[juncai, 2017/04/04 03:03:41]

Done.

+      device_->GetCharacteristicsByUUID(service_instance_id0,
+                                        BluetoothUUID(characteristic_uuid0));
+  EXPECT_EQ(1u, characteristics0.size());
+  EXPECT_EQ(BluetoothUUID(characteristic_uuid0),
+            characteristics0[0]->GetUUID());
+
+  std::vector<BluetoothRemoteGattCharacteristic*> characteristics1 =
+      device_->GetCharacteristicsByUUID(service_instance_id1,
+                                        BluetoothUUID(characteristic_uuid1));
+  EXPECT_EQ(1u, characteristics1.size());
+  EXPECT_EQ(BluetoothUUID(characteristic_uuid1),
+            characteristics1[0]->GetUUID());
+
+  std::vector<BluetoothRemoteGattCharacteristic*> characteristics2 =
+      device_->GetCharacteristicsByUUID(service_instance_id2,
+                                        BluetoothUUID(characteristic_uuid1));
+  EXPECT_EQ(1u, characteristics2.size());
+  EXPECT_EQ(BluetoothUUID(characteristic_uuid1),
+            characteristics2[0]->GetUUID());
 
   std::string service_instance_id_not_exist_in_setup =

[ortuno, 2017/04/03 03:36:38]

Can we refactor this method to be in the service to avoid all of this testing.

[juncai, 2017/04/04 03:03:42]

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.

[ortuno, 2017/04/04 03:32:11]

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.

[juncai, 2017/04/04 20:33:40]

Moved it to BluetoothRemoteGattService.

Done.

       "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))
@@ skipping 10 matching lines @@
                   ->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());
 }
+
+TEST_F(BluetoothGetServiceOrCharacteristicOrDescriptorTest,
+       GetDescriptorsByUUID) {
+  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 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 */);
+
+  BluetoothRemoteGattCharacteristic* characteristic0 =
+      primary_services[0]->GetCharacteristics()[0];
+  BluetoothRemoteGattCharacteristic* characteristic1 =
+      primary_services[1]->GetCharacteristics()[0];
+  BluetoothRemoteGattCharacteristic* characteristic2 =
+      primary_services[2]->GetCharacteristics()[0];
+
+  // Add several descriptors. No descriptors are added to |characteristic2|.

[ortuno, 2017/04/03 03:36:38]

We should just put all these setup code in the constructor.

[juncai, 2017/04/04 03:03:42]

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.

+  BluetoothUUID descriptor_uuid0("11111111-0000-1000-8000-00805f9b34fb");
+  BluetoothUUID descriptor_uuid1("22222222-0000-1000-8000-00805f9b34fb");
+  BluetoothUUID descriptor_uuid2("33333333-0000-1000-8000-00805f9b34fb");

[ortuno, 2017/04/03 03:36:38]

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...

[juncai, 2017/04/04 03:03:42]

ditto.

+  SimulateGattDescriptor(characteristic0, descriptor_uuid0.canonical_value());
+  SimulateGattDescriptor(characteristic0, descriptor_uuid1.canonical_value());
+  SimulateGattDescriptor(characteristic1, descriptor_uuid2.canonical_value());
+  SimulateGattDescriptor(characteristic1, descriptor_uuid2.canonical_value());
+
+  BluetoothUUID descriptor_uuid_not_exist_in_setup(
+      "12345678-0000-1000-8000-00805f9b34fb");
+
+  std::vector<device::BluetoothRemoteGattDescriptor*> descriptors0 =
+      device_->GetDescriptorsByUUID(characteristic0, descriptor_uuid0);
+  EXPECT_EQ(1u, descriptors0.size());
+  EXPECT_EQ(descriptor_uuid0, descriptors0[0]->GetUUID());
+  std::vector<device::BluetoothRemoteGattDescriptor*> descriptors1 =

[ortuno, 2017/04/03 03:36:38]

nit: add new line

[juncai, 2017/04/04 03:03:42]

ditto.

+      device_->GetDescriptorsByUUID(characteristic0, descriptor_uuid1);
+  EXPECT_EQ(1u, descriptors1.size());
+  EXPECT_EQ(descriptor_uuid1, descriptors1[0]->GetUUID());
+  EXPECT_TRUE(
+      device_->GetDescriptorsByUUID(characteristic0, descriptor_uuid2).empty());
+  EXPECT_TRUE(device_
+                  ->GetDescriptorsByUUID(characteristic0,
+                                         descriptor_uuid_not_exist_in_setup)
+                  .empty());
+
+  std::vector<device::BluetoothRemoteGattDescriptor*> descriptors2 =
+      device_->GetDescriptorsByUUID(characteristic1, descriptor_uuid2);
+  EXPECT_EQ(2u, descriptors2.size());
+  EXPECT_EQ(descriptor_uuid2, descriptors2[0]->GetUUID());
+  EXPECT_EQ(descriptor_uuid2, descriptors2[1]->GetUUID());
+  EXPECT_TRUE(
+      device_->GetDescriptorsByUUID(characteristic1, descriptor_uuid0).empty());
+  EXPECT_TRUE(
+      device_->GetDescriptorsByUUID(characteristic1, descriptor_uuid1).empty());
+  EXPECT_TRUE(device_
+                  ->GetDescriptorsByUUID(characteristic1,
+                                         descriptor_uuid_not_exist_in_setup)
+                  .empty());
+
+  EXPECT_TRUE(
+      device_->GetDescriptorsByUUID(characteristic2, descriptor_uuid0).empty());
+  EXPECT_TRUE(
+      device_->GetDescriptorsByUUID(characteristic2, descriptor_uuid1).empty());
+  EXPECT_TRUE(
+      device_->GetDescriptorsByUUID(characteristic2, descriptor_uuid2).empty());
+  EXPECT_TRUE(device_
+                  ->GetDescriptorsByUUID(characteristic2,
+                                         descriptor_uuid_not_exist_in_setup)
+                  .empty());
+
+  EXPECT_TRUE(

[ortuno, 2017/04/03 03:36:38]

Can we refactor this method to be in the characteristic and avoid this test?

[juncai, 2017/04/04 03:03:42]

ditto.

+      device_
+          ->GetDescriptorsByUUID(nullptr /* characteristic */, descriptor_uuid0)
+          .empty());
+}
+
 #endif  // defined(OS_ANDROID) || defined(OS_MACOSX)
 
 }  // namespace device