Bluetooth: macOS: Adding counter for service discovery callbacks.

device/bluetooth/bluetooth_device_unittest.cc

Index: device/bluetooth/bluetooth_device_unittest.cc
diff --git a/device/bluetooth/bluetooth_device_unittest.cc b/device/bluetooth/bluetooth_device_unittest.cc
index 19826117c94d04328119fa93535cd1fdb4cc2c1b..d603530457459a50b002be76ade9190f207097d9 100644
--- a/device/bluetooth/bluetooth_device_unittest.cc
+++ b/device/bluetooth/bluetooth_device_unittest.cc
@@ -395,6 +395,99 @@ TEST_F(BluetoothTest, GetUUIDs_Connection) {
 }
 #endif  // defined(OS_ANDROID) || defined(OS_MACOSX)
 
+#if defined(OS_MACOSX)
+// Tests that receiving 2 notifications in a row from macOS that services has
+// changed is handled correctly. Each notification should generate a notfication
+// that the gatt device has changed, and each notification should ask to macOS
+// to scan for services. Only after the second service scan is received, the
+// device changed notification should be sent and the characteristic discovery
+// procedure should be started.
+// Android: This test doesn't apply to Android because there is no services
+// changed event that could arrive during a discovery procedure.
+TEST_F(BluetoothTest, TwoPendingServiceDiscoveryRequests) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+
+  InitWithFakeAdapter();
+  TestBluetoothAdapterObserver observer(adapter_);
+
+  BluetoothDevice* device = SimulateLowEnergyDevice(1);
+  device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                               GetConnectErrorCallback(Call::NOT_EXPECTED));
+  SimulateGattConnection(device);
+  EXPECT_EQ(1, observer.device_changed_count());
+  EXPECT_FALSE(device->IsGattServicesDiscoveryComplete());
+
+  observer.Reset();
+  SimulateGattServicesChanged(device);
+  EXPECT_EQ(1, observer.device_changed_count());
+  EXPECT_FALSE(device->IsGattServicesDiscoveryComplete());
+
+  // Fist system call to
+  // -[id<CBPeripheralDelegate> peripheral:didDiscoverServices:]
+  observer.Reset();
+  std::vector<std::string> services1;
+  services1.push_back(kTestUUIDHeartRate);
+  SimulateGattServicesDiscovered(device, services1);

[ortuno, 2017/02/28 23:28:11]

optional: You could do:

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

Same below.

[jlebel, 2017/02/28 23:52:16]

Done.

+  EXPECT_EQ(0, observer.device_changed_count());
+  EXPECT_FALSE(device->IsGattServicesDiscoveryComplete());
+  EXPECT_EQ(gatt_characteristic_discovery_attempts_, 0);
+
+  // Second system call to
+  // -[id<CBPeripheralDelegate> peripheral:didDiscoverServices:]
+  std::vector<std::string> services2;
+  services2.push_back(kTestUUIDImmediateAlert);
+  SimulateGattServicesDiscovered(device, services2);
+  EXPECT_EQ(1, observer.device_changed_count());
+  EXPECT_TRUE(device->IsGattServicesDiscoveryComplete());
+  EXPECT_EQ(gatt_characteristic_discovery_attempts_, 2);

[ortuno, 2017/02/28 02:06:50]

Shouldn't there be only one characteristic discovery attempt?

[jlebel, 2017/02/28 23:16:18]

We discover characteristics for kTestUUIDHeartRate and then for
kTestUUIDImmediateAlert. So -[MockCBPeripehral
discoverCharacteristics:forService:] is called twice.

[ortuno, 2017/02/28 23:28:11]

Ah that makes sense. Thanks.

+
+  EXPECT_EQ(2u, device->GetGattServices().size());
+}
+
+// Simulate an unexpected call to -[id<CBPeripheralDelegate>
+// peripheral:didDiscoverServices:]. This should not happen, but if it does
+// (buggy device?), a discovery cycle should be done.
+TEST_F(BluetoothTest, ExtraDidDiscoverServicesCall) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+
+  InitWithFakeAdapter();
+  TestBluetoothAdapterObserver observer(adapter_);
+
+  BluetoothDevice* device = SimulateLowEnergyDevice(1);
+  device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                               GetConnectErrorCallback(Call::NOT_EXPECTED));
+  SimulateGattConnection(device);
+  EXPECT_EQ(1, observer.device_changed_count());
+  EXPECT_FALSE(device->IsGattServicesDiscoveryComplete());
+
+  // Legitimate system call to
+  // -[id<CBPeripheralDelegate> peripheral:didDiscoverServices:]
+  observer.Reset();
+  SimulateGattServicesDiscovered(device, {});
+  EXPECT_EQ(1, observer.device_changed_count());
+  EXPECT_TRUE(device->IsGattServicesDiscoveryComplete());
+  EXPECT_EQ(gatt_characteristic_discovery_attempts_, 0);

[ortuno, 2017/02/28 02:06:50]

I'm surprised the legitimate system call doesn't result in a characteristic
discovery attempt.

[jlebel, 2017/02/28 23:16:18]

We have no service, therefore, -[MockCBPeripehral
discoverCharacteristics:forService:] can't be called.

[ortuno, 2017/02/28 23:28:11]

Ah right. optional: Add a service to make it clearer that a characteristic
discovery attempt will be initiated for legitimate notifications but not for
illegitimate notifications.

[jlebel, 2017/02/28 23:52:16]

If I do that, this DCHECK fails:
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s...

SimulateGattServicesDiscovered()  has been implemented to simulate all system
call related to "DidDiscover" (services, characteristics and descriptors). So if
our device already knows about a service, than this extra
SimulateGattServicesDiscovered() will call -[<id>CBPeripheralDelegate
peripheral:didDiscoverServices:] and then also -[id<CBPeripheralDelegate>
peripheral:didDiscoverCharacteristicsForService:error:].

So I guess that's one unittest that I can create for crrev.com/2638653002

[jlebel, 2017/03/04 16:59:02]

Done.

+  EXPECT_EQ(0u, device->GetGattServices().size());
+
+  // Unexpected system call to
+  // -[id<CBPeripheralDelegate> peripheral:didDiscoverServices:]
+  std::vector<std::string> services3;
+  services3.push_back(kTestUUIDLinkLoss);
+  SimulateGattServicesDiscovered(device, services3);
+  EXPECT_EQ(2, observer.device_changed_count());
+  EXPECT_TRUE(device->IsGattServicesDiscoveryComplete());
+  EXPECT_EQ(gatt_characteristic_discovery_attempts_, 1);
+
+  EXPECT_EQ(1u, device->GetGattServices().size());
+}
+#endif  // defined(OS_MACOSX)
+
 #if defined(OS_ANDROID) || defined(OS_MACOSX)
 // Tests Advertisement Data is updated correctly when we start discovery
 // during a connection.