bluetooth: Update the map of GATT services when services resolve

device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc

Index: device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc
diff --git a/device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc b/device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc
index c5fa376674e42d7fd0d27ab85f7f658b8748117c..16cd850495066e03a3cdb7e0f01eb23430f6a44d 100644
--- a/device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc
+++ b/device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc
@@ -129,6 +129,74 @@ class BluetoothGattBlueZTest : public testing::Test {
     ASSERT_TRUE(adapter_->IsPresent());
   }
 
+  void BatteryServiceShouldBeComplete(BluetoothDevice* device) {
+    ASSERT_TRUE(device);
+    ASSERT_GE(device->GetGattServices().size(), 1u);
+
+    BluetoothRemoteGattService* service = device->GetGattService(
+        fake_bluetooth_gatt_service_client_->GetBatteryServicePath().value());
+
+    ASSERT_TRUE(service);
+    EXPECT_TRUE(service->IsPrimary());
+    EXPECT_EQ(BluetoothUUID(
+                  bluez::FakeBluetoothGattServiceClient::kBatteryServiceUUID),
+              service->GetUUID());
+    EXPECT_EQ(service, device->GetGattService(service->GetIdentifier()));
+  }
+
+  void HeartRateServiceShouldBeComplete(BluetoothDevice* device) {
+    ASSERT_TRUE(device);
+    ASSERT_GE(device->GetGattServices().size(), 1u);
+
+    BluetoothRemoteGattService* service = device->GetGattService(
+        fake_bluetooth_gatt_service_client_->GetHeartRateServicePath().value());
+
+    ASSERT_TRUE(service);
+    EXPECT_TRUE(service->IsPrimary());
+    EXPECT_EQ(BluetoothUUID(
+                  bluez::FakeBluetoothGattServiceClient::kHeartRateServiceUUID),
+              service->GetUUID());
+    EXPECT_EQ(service, device->GetGattService(service->GetIdentifier()));
+    EXPECT_EQ(3U, service->GetCharacteristics().size());
+
+    BluetoothRemoteGattCharacteristic* characteristic =
+        service->GetCharacteristic(fake_bluetooth_gatt_characteristic_client_
+                                       ->GetBodySensorLocationPath()
+                                       .value());
+    ASSERT_TRUE(characteristic);
+    EXPECT_EQ(BluetoothUUID(bluez::FakeBluetoothGattCharacteristicClient::
+                                kBodySensorLocationUUID),
+              characteristic->GetUUID());
+    EXPECT_TRUE(characteristic->GetDescriptors().empty());
+
+    characteristic =
+        service->GetCharacteristic(fake_bluetooth_gatt_characteristic_client_
+                                       ->GetHeartRateControlPointPath()
+                                       .value());
+    ASSERT_TRUE(characteristic);
+    EXPECT_EQ(BluetoothUUID(bluez::FakeBluetoothGattCharacteristicClient::
+                                kHeartRateControlPointUUID),
+              characteristic->GetUUID());
+    EXPECT_TRUE(characteristic->GetDescriptors().empty());
+
+    characteristic =
+        service->GetCharacteristic(fake_bluetooth_gatt_characteristic_client_
+                                       ->GetHeartRateMeasurementPath()
+                                       .value());
+    ASSERT_TRUE(characteristic);
+    EXPECT_EQ(BluetoothUUID(bluez::FakeBluetoothGattCharacteristicClient::
+                                kHeartRateMeasurementUUID),
+              characteristic->GetUUID());
+
+    ASSERT_EQ(1u, characteristic->GetDescriptors().size());
+
+    BluetoothRemoteGattDescriptor* descriptor =
+        characteristic->GetDescriptors()[0];
+    EXPECT_EQ(
+        BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid(),
+        descriptor->GetUUID());
+  }
+
   void AdapterCallback(scoped_refptr<BluetoothAdapter> adapter) {
     adapter_ = adapter;
     if (base::MessageLoop::current() &&
@@ -366,62 +434,6 @@ TEST_F(BluetoothGattBlueZTest, GattServiceAddedAndRemoved) {
                       bluez::FakeBluetoothDeviceClient::kLowEnergyAddress));
 }
 
-TEST_F(BluetoothGattBlueZTest, DiscoveryCompleteForCachedGattService) {
-  // This tests if the notifications on service discovered complete are invoked
-  // with the cached services and added to the GATT service map of |device|.
-  TestBluetoothAdapterObserver observer(adapter_);
-
-  // Create the device and pre-expose the fake Heart Rate service. This will
-  // synchronously expose characteristics.
-  fake_bluetooth_device_client_->CreateDevice(
-      dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
-      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kCachedLowEnergyPath));
-  BluetoothDevice* device =
-      adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress);
-  ASSERT_TRUE(device);
-
-  device::BluetoothRemoteGattService* service = device->GetGattServices()[0];
-  EXPECT_EQ(1u, device->GetGattServices().size());
-  EXPECT_EQ(1, observer.gatt_discovery_complete_count());
-  EXPECT_EQ(device, service->GetDevice());
-  EXPECT_EQ(service, device->GetGattService(service->GetIdentifier()));
-  EXPECT_EQ(3U, service->GetCharacteristics().size());
-}
-
-TEST_F(BluetoothGattBlueZTest, DiscoveryCompleteForNewGattService) {
-  TestBluetoothAdapterObserver observer(adapter_);
-
-  // This tests the discovery complete notification on a newly-added GATT
-  // service.
-  fake_bluetooth_device_client_->CreateDevice(
-      dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
-      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
-  BluetoothDevice* device =
-      adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress);
-  ASSERT_TRUE(device);
-
-  EXPECT_EQ(0u, device->GetGattServices().size());
-  EXPECT_EQ(0, observer.gatt_discovery_complete_count());
-
-  // Expose the fake Heart Rate service. This will asynchronously expose
-  // characteristics.
-  fake_bluetooth_gatt_service_client_->ExposeHeartRateService(
-      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
-
-  // Run the message loop so that the characteristics/descriptors appear.
-  base::MessageLoop::current()->Run();
-
-  // The discovery completed event of a newly-added GATT service should not be
-  // fired until ServicesResolved property becomes true. And the new service
-  // will be added immediately to the GATT service map of |device|.
-  BluetoothRemoteGattService* service = device->GetGattServices()[0];
-  EXPECT_EQ(1u, device->GetGattServices().size());
-  EXPECT_EQ(1, observer.gatt_discovery_complete_count());
-  EXPECT_EQ(device, service->GetDevice());
-  EXPECT_EQ(service, device->GetGattService(service->GetIdentifier()));
-  EXPECT_EQ(3U, service->GetCharacteristics().size());
-}
-
 TEST_F(BluetoothGattBlueZTest, ServicesDiscovered) {
   // Create a fake LE device. We store the device pointer here because this is a
   // test. It's unsafe to do this in production as the device might get deleted.
@@ -443,6 +455,8 @@ TEST_F(BluetoothGattBlueZTest, ServicesDiscovered) {
       dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
   // Run the message loop so that the characteristics/descriptors appear.
   base::MessageLoop::current()->Run();
+  properties->services_resolved.ReplaceValue(true);
+
   EXPECT_TRUE(device->IsGattServicesDiscoveryComplete());
   EXPECT_EQ(1u, device->GetGattServices().size());
   EXPECT_EQ(device, observer.last_device());
@@ -476,6 +490,8 @@ TEST_F(BluetoothGattBlueZTest, ServicesDiscovered) {
       dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
   // Run the message loop so that the characteristics/descriptors appear.
   base::MessageLoop::current()->Run();
+  properties->services_resolved.ReplaceValue(true);
+
   EXPECT_TRUE(device->IsGattServicesDiscoveryComplete());
   EXPECT_EQ(1u, device->GetGattServices().size());
 }
@@ -638,9 +654,10 @@ TEST_F(BluetoothGattBlueZTest, GattDescriptorAddedAndRemoved) {
   EXPECT_EQ(descriptor->GetIdentifier(), observer.last_gatt_descriptor_id());
 }
 
-TEST_F(BluetoothGattBlueZTest, AdapterAddedAfterGattService) {
+TEST_F(BluetoothGattBlueZTest, CachedServices) {

[scheib, 2016/07/01 00:40:53]

I like this test coverage, but after this change it will be hard to see that the
4 tests are related. Especially because ServicesDiscovered is seperated from
these, but also because names are not similar. Let's fix by some combination of:
+ Move these tests to just after ServicesDiscovered
+ Comment at top of tests the highly related tests and how they differ?
+ Name these more consistently? Discover{New|Cached}Services?

[ortuno, 2016/07/01 15:51:48]

I moved them after services discovered and changed the names. I think that makes
it obvious enough that these tests are related.

   // This unit test tests that all remote GATT objects are created for D-Bus
-  // objects that were already exposed.
+  // objects that were already exposed and all relevant events have been
+  // dispatched.
   adapter_ = NULL;
   ASSERT_FALSE(device::BluetoothAdapterFactory::HasSharedInstanceForTesting());
 
@@ -648,6 +665,11 @@ TEST_F(BluetoothGattBlueZTest, AdapterAddedAfterGattService) {
   fake_bluetooth_device_client_->CreateDevice(
       dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
       dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  bluez::FakeBluetoothDeviceClient::Properties* properties =
+      fake_bluetooth_device_client_->GetProperties(
+          dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  properties->services_resolved.ReplaceValue(false);
+
   fake_bluetooth_gatt_service_client_->ExposeHeartRateService(
       dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
   while (!fake_bluetooth_gatt_characteristic_client_->IsHeartRateVisible())
@@ -657,55 +679,127 @@ TEST_F(BluetoothGattBlueZTest, AdapterAddedAfterGattService) {
 
   // Create the adapter. This should create all the GATT objects.
   GetAdapter();
+  TestBluetoothAdapterObserver observer(adapter_);
+
+  // The device should exist but contain no cached services while the services

[vudentz, 2016/07/01 11:25:15]

Should this be the other way around, we are suppose to test a device with cached
attributes.

[ortuno, 2016/07/01 15:51:48]

We have no guarantee that the cached services are valid so we can't create
objects for them before all services are resolved. This test makes sure that
while services are being resolved we don't create objects for cached services,
but once services are resolved we create objects for them.

+  // haven't been resolved.
   BluetoothDevice* device =
       adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress);
-  ASSERT_TRUE(device);
-  EXPECT_EQ(1U, device->GetGattServices().size());
 
-  BluetoothRemoteGattService* service = device->GetGattServices()[0];
-  ASSERT_TRUE(service);
-  EXPECT_TRUE(service->IsPrimary());
-  EXPECT_EQ(BluetoothUUID(
-                bluez::FakeBluetoothGattServiceClient::kHeartRateServiceUUID),
-            service->GetUUID());
-  EXPECT_EQ(service, device->GetGattServices()[0]);
-  EXPECT_EQ(service, device->GetGattService(service->GetIdentifier()));
-  EXPECT_EQ(3U, service->GetCharacteristics().size());
+  EXPECT_TRUE(device->GetGattServices().empty());
 
-  BluetoothRemoteGattCharacteristic* characteristic =
-      service->GetCharacteristic(fake_bluetooth_gatt_characteristic_client_
-                                     ->GetBodySensorLocationPath()
-                                     .value());
-  ASSERT_TRUE(characteristic);
-  EXPECT_EQ(BluetoothUUID(bluez::FakeBluetoothGattCharacteristicClient::
-                              kBodySensorLocationUUID),
-            characteristic->GetUUID());
-  EXPECT_TRUE(characteristic->GetDescriptors().empty());
+  EXPECT_EQ(0, observer.gatt_services_discovered_count());
+  EXPECT_EQ(0, observer.gatt_service_added_count());
+  EXPECT_EQ(0, observer.gatt_discovery_complete_count());
+  EXPECT_EQ(0, observer.gatt_characteristic_added_count());
+  EXPECT_EQ(0, observer.gatt_descriptor_added_count());
 
-  characteristic = service->GetCharacteristic(
-      fake_bluetooth_gatt_characteristic_client_->GetHeartRateControlPointPath()
-          .value());
-  ASSERT_TRUE(characteristic);
-  EXPECT_EQ(BluetoothUUID(bluez::FakeBluetoothGattCharacteristicClient::
-                              kHeartRateControlPointUUID),
-            characteristic->GetUUID());
-  EXPECT_TRUE(characteristic->GetDescriptors().empty());
+  properties->services_resolved.ReplaceValue(true);
 
-  characteristic = service->GetCharacteristic(
-      fake_bluetooth_gatt_characteristic_client_->GetHeartRateMeasurementPath()
-          .value());
-  ASSERT_TRUE(characteristic);
-  EXPECT_EQ(BluetoothUUID(bluez::FakeBluetoothGattCharacteristicClient::
-                              kHeartRateMeasurementUUID),
-            characteristic->GetUUID());
-  EXPECT_EQ(1U, characteristic->GetDescriptors().size());
+  EXPECT_EQ(1, observer.gatt_services_discovered_count());
+  EXPECT_EQ(1, observer.gatt_service_added_count());
+  EXPECT_EQ(1, observer.gatt_discovery_complete_count());
+  EXPECT_EQ(3, observer.gatt_characteristic_added_count());
+  EXPECT_EQ(1, observer.gatt_descriptor_added_count());
 
-  BluetoothRemoteGattDescriptor* descriptor =
-      characteristic->GetDescriptors()[0];
-  ASSERT_TRUE(descriptor);
-  EXPECT_EQ(
-      BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid(),
-      descriptor->GetUUID());
+  HeartRateServiceShouldBeComplete(device);
+}
+
+TEST_F(BluetoothGattBlueZTest, NewServices) {
+  // This unit test tests that all remote GATT Objects are created for D-Bus
+  // objects that are newly exposed.
+  TestBluetoothAdapterObserver observer(adapter_);
+
+  // Create the fake D-Bus objects.
+  fake_bluetooth_device_client_->CreateDevice(
+      dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
+      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  bluez::FakeBluetoothDeviceClient::Properties* properties =
+      fake_bluetooth_device_client_->GetProperties(
+          dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+
+  properties->services_resolved.ReplaceValue(false);
+  fake_bluetooth_gatt_service_client_->ExposeHeartRateService(
+      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  while (!fake_bluetooth_gatt_characteristic_client_->IsHeartRateVisible())
+    base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(fake_bluetooth_gatt_service_client_->IsHeartRateVisible());
+  ASSERT_TRUE(fake_bluetooth_gatt_characteristic_client_->IsHeartRateVisible());
+
+  // The device should exist and contain new services even though the services
+  // haven't been resolved.
+  BluetoothDevice* device =
+      adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress);
+  EXPECT_EQ(1u, device->GetGattServices().size());
+  HeartRateServiceShouldBeComplete(device);
+
+  EXPECT_EQ(0, observer.gatt_services_discovered_count());
+  EXPECT_EQ(0, observer.gatt_discovery_complete_count());
+  // The rest of the events are tested in the corresponding
+  // {GATTObject}AddedAndRemoved tests.
+
+  properties->services_resolved.ReplaceValue(true);
+
+  EXPECT_EQ(1, observer.gatt_services_discovered_count());
+  EXPECT_EQ(1, observer.gatt_discovery_complete_count());
+}
+
+TEST_F(BluetoothGattBlueZTest, CachedAndNewServices) {
+  // This unit test tests that all remote GATT objects are created for D-Bus
+  // objects that were already exposed and for new GATT Objects.
+  adapter_ = NULL;
+  ASSERT_FALSE(device::BluetoothAdapterFactory::HasSharedInstanceForTesting());
+
+  // Create the fake D-Bus objects.
+  fake_bluetooth_device_client_->CreateDevice(
+      dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
+      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  bluez::FakeBluetoothDeviceClient::Properties* properties =
+      fake_bluetooth_device_client_->GetProperties(
+          dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  properties->services_resolved.ReplaceValue(false);
+
+  fake_bluetooth_gatt_service_client_->ExposeHeartRateService(
+      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  while (!fake_bluetooth_gatt_characteristic_client_->IsHeartRateVisible())
+    base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(fake_bluetooth_gatt_service_client_->IsHeartRateVisible());
+  ASSERT_TRUE(fake_bluetooth_gatt_characteristic_client_->IsHeartRateVisible());
+
+  // Create the adapter. This should create all the GATT objects.
+  GetAdapter();
+  TestBluetoothAdapterObserver observer(adapter_);
+
+  // The device should exist but contain no cached services while the services
+  // haven't been resolved.
+  BluetoothDevice* device =
+      adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress);
+
+  EXPECT_EQ(0, observer.gatt_services_discovered_count());
+  EXPECT_EQ(0, observer.gatt_discovery_complete_count());
+  EXPECT_TRUE(device->GetGattServices().empty());
+
+  fake_bluetooth_gatt_service_client_->ExposeBatteryService(
+      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  ASSERT_TRUE(fake_bluetooth_gatt_service_client_->IsBatteryServiceVisible());
+
+  EXPECT_EQ(0, observer.gatt_services_discovered_count());
+  EXPECT_EQ(0, observer.gatt_discovery_complete_count());
+  EXPECT_EQ(1u, device->GetGattServices().size());
+
+  // A newly added service should be complete even if services haven't been
+  // resolved.
+  BatteryServiceShouldBeComplete(device);
+
+  properties->services_resolved.ReplaceValue(true);
+
+  EXPECT_EQ(1, observer.gatt_services_discovered_count());
+  EXPECT_EQ(2, observer.gatt_discovery_complete_count());
+  EXPECT_EQ(2u, device->GetGattServices().size());
+
+  // Now that services have been resolved both services should be present.
+  HeartRateServiceShouldBeComplete(device);
+  BatteryServiceShouldBeComplete(device);
 }
 
 TEST_F(BluetoothGattBlueZTest, GattCharacteristicValue) {