Update unit tests for BluetoothDevice::GetPrimaryServicesByUUID() etc.

device/bluetooth/bluetooth_remote_gatt_service_unittest.cc

Index: device/bluetooth/bluetooth_remote_gatt_service_unittest.cc
diff --git a/device/bluetooth/bluetooth_remote_gatt_service_unittest.cc b/device/bluetooth/bluetooth_remote_gatt_service_unittest.cc
index 4daf9fe0cc08f14a0c091580402fa289c8eafcd7..5c21f22a52de430b6cd0ffda5daaec249b1c1d00 100644
--- a/device/bluetooth/bluetooth_remote_gatt_service_unittest.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_service_unittest.cc
@@ -173,6 +173,60 @@ TEST_F(BluetoothRemoteGattServiceTest,
   EXPECT_EQ(service->GetCharacteristic(char_id1),
             service->GetCharacteristic(char_id1));
 }
+
+TEST_F(BluetoothRemoteGattServiceTest, GetCharacteristicsByUUID) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  InitWithFakeAdapter();
+  StartLowEnergyDiscoverySession();
+  BluetoothDevice* device = SimulateLowEnergyDevice(3);
+  device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                               GetConnectErrorCallback(Call::NOT_EXPECTED));
+  SimulateGattConnection(device);
+  base::RunLoop().RunUntilIdle();
+
+  // Simulate two primary GATT services.
+  std::vector<std::string> services;
+  services.push_back("00000000-0000-1000-8000-00805f9b34fb");
+  services.push_back("01010101-0101-1000-8000-00805f9b34fb");
+  SimulateGattServicesDiscovered(device, services);

[ortuno, 2017/04/04 22:14:12]

SimulateGattServicesDiscovered(
  device, std::vector<std::string>({kTestUUIDGenericAccess,
kTestUUIDHeartRate}));

[juncai, 2017/04/05 00:36:46]

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?

[ortuno, 2017/04/05 00:53:10]

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!

[juncai, 2017/04/05 19:01:23]

I will submit a CL to do that.

+  base::RunLoop().RunUntilIdle();
+  BluetoothRemoteGattService* service1 = device->GetGattServices()[0];
+  BluetoothRemoteGattService* service2 = device->GetGattServices()[1];
+  std::string characteristic_uuid1 = "11111111-0000-1000-8000-00805f9b34fb";

[ortuno, 2017/04/04 22:14:12]

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

[juncai, 2017/04/05 00:36:46]

ditto.

Same as:
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s...

What about doing it in a follow-up CL?

+  std::string characteristic_uuid2 = "22222222-0000-1000-8000-00805f9b34fb";
+  SimulateGattCharacteristic(service1, characteristic_uuid1,
+                             /* properties */ 0);
+  // 2 duplicate UUIDs creating 2 instances.
+  SimulateGattCharacteristic(service2, characteristic_uuid2,
+                             /* properties */ 0);
+  SimulateGattCharacteristic(service2, characteristic_uuid2,
+                             /* properties */ 0);
+
+  std::vector<BluetoothRemoteGattCharacteristic*> characteristics1 =

[ortuno, 2017/04/04 22:14:12]

Consider using brackets to avoid characteristics1, characteristics2, etc.

[juncai, 2017/04/05 00:36:46]

Done.

+      service1->GetCharacteristicsByUUID(BluetoothUUID(characteristic_uuid1));
+  EXPECT_EQ(1u, characteristics1.size());
+  EXPECT_EQ(characteristic_uuid1, characteristics1[0]->GetUUID().value());

[ortuno, 2017/04/04 22:14:12]

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.

[juncai, 2017/04/05 00:36:46]

Done.

+
+  std::vector<BluetoothRemoteGattCharacteristic*> characteristics2 =
+      service2->GetCharacteristicsByUUID(BluetoothUUID(characteristic_uuid2));
+  EXPECT_EQ(2u, characteristics2.size());
+  EXPECT_EQ(characteristic_uuid2, characteristics2[0]->GetUUID().value());
+  EXPECT_EQ(characteristic_uuid2, characteristics2[1]->GetUUID().value());
+  EXPECT_NE(characteristics2[0]->GetIdentifier(),
+            characteristics2[1]->GetIdentifier());
+
+  BluetoothUUID characteristic_uuid_not_exist_in_setup(
+      "33333333-0000-1000-8000-00805f9b34fb");
+  EXPECT_TRUE(
+      service1->GetCharacteristicsByUUID(characteristic_uuid_not_exist_in_setup)
+          .empty());
+  EXPECT_TRUE(
+      service2->GetCharacteristicsByUUID(characteristic_uuid_not_exist_in_setup)
+          .empty());
+}
 #endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
 #if defined(OS_MACOSX) || defined(OS_WIN)