bluetooth: Update the map of GATT services when services resolve

device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc

 // Copyright 2016 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 <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
 
@@ skipping 111 matching lines @@
 
   void GetAdapter() {
     device::BluetoothAdapterFactory::GetAdapter(base::Bind(
         &BluetoothGattBlueZTest::AdapterCallback, base::Unretained(this)));
     base::MessageLoop::current()->Run();
     ASSERT_TRUE(adapter_.get() != NULL);
     ASSERT_TRUE(adapter_->IsInitialized());
     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() &&
         base::MessageLoop::current()->is_running()) {
       base::MessageLoop::current()->QuitWhenIdle();
     }
   }
 
   void SuccessCallback() { ++success_callback_count_; }
 
@@ skipping 217 matching lines @@
   EXPECT_EQ(2, observer.gatt_service_added_count());
   EXPECT_EQ(2, observer.gatt_service_removed_count());
   EXPECT_FALSE(observer.last_gatt_service_id().empty());
   EXPECT_EQ(BluetoothUUID(
                 bluez::FakeBluetoothGattServiceClient::kHeartRateServiceUUID),
             observer.last_gatt_service_uuid());
   EXPECT_EQ(NULL, adapter_->GetDevice(
                       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.
   fake_bluetooth_device_client_->CreateDevice(
       dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
       dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
   BluetoothDevice* device =
       adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress);
   bluez::FakeBluetoothDeviceClient::Properties* properties =
       fake_bluetooth_device_client_->GetProperties(
           dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
 
   ASSERT_TRUE(device);
 
   TestBluetoothAdapterObserver observer(adapter_);
 
   // Expose the fake Heart Rate Service.
   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();
+  properties->services_resolved.ReplaceValue(true);
+
   EXPECT_TRUE(device->IsGattServicesDiscoveryComplete());
   EXPECT_EQ(1u, device->GetGattServices().size());
   EXPECT_EQ(device, observer.last_device());
   EXPECT_EQ(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress,
             observer.last_device_address());
 
   // Disconnect from the device:
   device->Disconnect(base::Bind(&BluetoothGattBlueZTest::SuccessCallback,
                                 base::Unretained(this)),
                      base::Bind(&BluetoothGattBlueZTest::ErrorCallback,
@@ skipping 13 matching lines @@
       base::Bind(&BluetoothGattBlueZTest::ConnectErrorCallback,
                  base::Unretained(this)));
   properties->connected.ReplaceValue(true);
   EXPECT_TRUE(device->IsConnected());
 
   // Verify that service discovery can be done again:
   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();
+  properties->services_resolved.ReplaceValue(true);
+
   EXPECT_TRUE(device->IsGattServicesDiscoveryComplete());
   EXPECT_EQ(1u, device->GetGattServices().size());
 }
 
 TEST_F(BluetoothGattBlueZTest, GattCharacteristicAddedAndRemoved) {
   fake_bluetooth_device_client_->CreateDevice(
       dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
       dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
   BluetoothDevice* device =
       adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress);
@@ skipping 142 matching lines @@
   EXPECT_EQ(0, observer.gatt_descriptor_value_changed_count());
 
   descriptor = characteristic->GetDescriptors()[0];
   EXPECT_EQ(
       BluetoothRemoteGattDescriptor::ClientCharacteristicConfigurationUuid(),
       descriptor->GetUUID());
   EXPECT_EQ(descriptor->GetUUID(), observer.last_gatt_descriptor_uuid());
   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());
 
   // 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

[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) {
   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);
 
@@ skipping 633 matching lines @@
 
   EXPECT_EQ(1, success_callback_count_);
   EXPECT_EQ(0, error_callback_count_);
   EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
   EXPECT_TRUE(characteristic->IsNotifying());
   EXPECT_EQ(1U, update_sessions_.size());
   EXPECT_TRUE(update_sessions_[0]->IsActive());
 }
 
 }  // namespace bluez