bluetooth: android: Enable characteristic change notification events.

device/bluetooth/bluetooth_gatt_characteristic_unittest.cc

Index: device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
diff --git a/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc b/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
index 2db7df7d58093c647ed46558d0ef9c9a50894b00..0dd81581adffa43eb1d465190e4edb1ef7f6191a 100644
--- a/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
+++ b/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/run_loop.h"
 #include "build/build_config.h"
 #include "device/bluetooth/bluetooth_gatt_service.h"
+#include "device/bluetooth/test/test_bluetooth_adapter_observer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_ANDROID)
@@ -24,7 +25,8 @@ namespace device {
 class BluetoothGattCharacteristicTest : public BluetoothTest {
  public:
   // Creates adapter_, device_, service_, characteristic1_, & characteristic2_.
-  void FakeCharacteristicBoilerplate() {
+  // |properties| will be used for each characteristic.
+  void FakeCharacteristicBoilerplate(int properties = 0) {
     InitWithFakeAdapter();
     StartLowEnergyDiscoverySession();
     device_ = DiscoverLowEnergyDevice(3);
@@ -37,14 +39,48 @@ class BluetoothGattCharacteristicTest : public BluetoothTest {
     SimulateGattServicesDiscovered(device_, services);
     ASSERT_EQ(1u, device_->GetGattServices().size());
     service_ = device_->GetGattServices()[0];
-    SimulateGattCharacteristic(service_, uuid, /* properties */ 0);
-    SimulateGattCharacteristic(service_, uuid, /* properties */ 0);
+    SimulateGattCharacteristic(service_, uuid, properties);
+    SimulateGattCharacteristic(service_, uuid, properties);
     ASSERT_EQ(2u, service_->GetCharacteristics().size());
     characteristic1_ = service_->GetCharacteristics()[0];
     characteristic2_ = service_->GetCharacteristics()[1];
     ResetEventCounts();
   }
 
+  // Constructs characteristics with |properties|, calls StartNotifySession,
+  // and verifies the appropriate |expected_config_descriptor_value| is written.
+  void StartNotifyBoilerplate(int properties,
+                              uint16_t expected_config_descriptor_value) {
+    ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate(properties));
+    SimulateGattDescriptor(
+        characteristic1_,
+        /* Client Characteristic Configuration descriptor's standard UUID: */
+        "00002902-0000-1000-8000-00805F9B34FB");
+    ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+    characteristic1_->StartNotifySession(
+        GetNotifyCallback(Call::EXPECTED),
+        GetGattErrorCallback(Call::NOT_EXPECTED));
+    EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
+    EXPECT_EQ(0, callback_count_);
+    SimulateGattNotifySessionStarted(characteristic1_);
+    EXPECT_EQ(1, callback_count_);
+    EXPECT_EQ(0, error_callback_count_);
+    ASSERT_EQ(1u, notify_sessions_.size());
+    ASSERT_TRUE(notify_sessions_[0]);
+    EXPECT_EQ(characteristic1_->GetIdentifier(),
+              notify_sessions_[0]->GetCharacteristicIdentifier());
+    EXPECT_TRUE(notify_sessions_[0]->IsActive());
+
+    // Verify the Client Characteristic Configuration descriptor was written to.
+    EXPECT_EQ(1, gatt_write_descriptor_attempts_);
+    std::vector<uint8_t> written_vector(
+        &expected_config_descriptor_value,

[Jeffrey Yasskin, 2016/01/14 00:13:42]

You mean to pull the bytes out of here, right? Instead, this truncates
expected_config_descriptor_value to uint8_t, and then reads the non-existent
uint16_t after it in memory, and casts that value to uint8_t. To avoid depending
on the endian-ness of Chrome's platform, you should use bitmasks and shifts to
pull the bytes out of expected_config_descriptor_value.

[scheib, 2016/01/14 01:12:26]

Thanks - double check this version.

+        &expected_config_descriptor_value +
+            sizeof(expected_config_descriptor_value));
+    EXPECT_EQ(written_vector, last_write_value_);
+  }
+
   BluetoothDevice* device_ = nullptr;
   BluetoothGattService* service_ = nullptr;
   BluetoothGattCharacteristic* characteristic1_ = nullptr;
@@ -638,30 +674,45 @@ TEST_F(BluetoothGattCharacteristicTest, WriteRemoteCharacteristic_ReadPending) {
 #endif  // defined(OS_ANDROID)
 
 #if defined(OS_ANDROID)
-// Tests StartNotifySession success.
-TEST_F(BluetoothGattCharacteristicTest, StartNotifySession) {
+// StartNotifySession fails if characteristic doesn't have Notify or Indicate
+// property.
+TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoNotifyOrIndicate) {
   ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate());
 
-  characteristic1_->StartNotifySession(
-      GetNotifyCallback(Call::EXPECTED),
-      GetGattErrorCallback(Call::NOT_EXPECTED));
-  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
-  EXPECT_EQ(0, callback_count_);
-  SimulateGattNotifySessionStarted(characteristic1_);
-  EXPECT_EQ(1, callback_count_);
+  characteristic1_->StartNotifySession(GetNotifyCallback(Call::NOT_EXPECTED),
+                                       GetGattErrorCallback(Call::EXPECTED));
+  EXPECT_EQ(0, gatt_notify_characteristic_attempts_);
+
+  // The expected error callback is asynchronous:
   EXPECT_EQ(0, error_callback_count_);
-  ASSERT_EQ(1u, notify_sessions_.size());
-  ASSERT_TRUE(notify_sessions_[0]);
-  EXPECT_EQ(characteristic1_->GetIdentifier(),
-            notify_sessions_[0]->GetCharacteristicIdentifier());
-  EXPECT_TRUE(notify_sessions_[0]->IsActive());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, error_callback_count_);
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// StartNotifySession fails if the characteristic is missing the Client
+// Characteristic Configuration descriptor.
+TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_NoConfigDescriptor) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+
+  characteristic1_->StartNotifySession(GetNotifyCallback(Call::NOT_EXPECTED),
+                                       GetGattErrorCallback(Call::EXPECTED));
+  EXPECT_EQ(0, gatt_notify_characteristic_attempts_);
+
+  // The expected error callback is asynchronous:
+  EXPECT_EQ(0, error_callback_count_);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, error_callback_count_);
 }
 #endif  // defined(OS_ANDROID)
 
 #if defined(OS_ANDROID)
 // Tests StartNotifySession synchronous failure.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_SynchronousError) {

[Jeffrey Yasskin, 2016/01/14 00:13:43]

Is "SynchronousError" the most descriptive name for this kind of failure? I
believe it's failing in the call to setCharacteristicNotification(), so it
should be "FailToSetCharacteristicNotification" or similar.

[scheib, 2016/01/14 01:12:26]

Done. Naming was intentionally abstracting platform details - but I've used your
example and commented directly what this translates to for Android. There isn't
an equivalent for Bluez or CoreBluetooth, (or I think Windows, but I'm still
unsure which API surface we'd use there) so this is likely to remain Android
only indefinitely. 

-  ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate());
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
 
   SimulateGattCharacteristicSetNotifyWillFailSynchronouslyOnce(

[Jeffrey Yasskin, 2016/01/14 00:13:42]

If you remove this line, does the test fail? I suspect you need to add the
descriptor in order to get to the attempt to call
setCharacteristicNotification().

[scheib, 2016/01/14 01:12:26]

It needed the descriptor. Code coverage here is brittle ;(, I'd reorganized for
better error messages. I don't think worth adding more testing instrumentation
for from every block - unsure of a better way.

[Jeffrey Yasskin, 2016/01/14 18:43:31]

One way to do it would be to have StartNotifyBoilerplate() take an integer
parameter saying which step to skip (or which failure to force), and then assert
that passing any of those integers causes a failure. Using
StartNotifyBoilerplace means that you also have a built-in test that you're
skipping a step in an otherwise-complete sequence. That said, we don't usually
test this way, so I don't insist on it.

[scheib, 2016/01/14 21:02:14]

Nice idea - I'll refactor to use it in a follow up.

       characteristic1_);
@@ -669,17 +720,108 @@ TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_SynchronousError) {
                                        GetGattErrorCallback(Call::EXPECTED));
   EXPECT_EQ(0, error_callback_count_);
   base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, error_callback_count_);
   EXPECT_EQ(0, gatt_notify_characteristic_attempts_);
-  EXPECT_EQ(0, callback_count_);
+  ASSERT_EQ(0u, notify_sessions_.size());
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests StartNotifySession descriptor write synchronous failure.
+TEST_F(BluetoothGattCharacteristicTest,
+       StartNotifySession_WriteDescriptorSynchronousError) {
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      /* Client Characteristic Configuration descriptor's standard UUID: */
+      "00002902-0000-1000-8000-00805F9B34FB");
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
+
+  // Fail to write to config descriptor synchronously.
+  SimulateGattDescriptorWriteWillFailSynchronouslyOnce(
+      characteristic1_->GetDescriptors()[0]);
+
+  characteristic1_->StartNotifySession(GetNotifyCallback(Call::NOT_EXPECTED),
+                                       GetGattErrorCallback(Call::EXPECTED));
+  EXPECT_EQ(0, error_callback_count_);
+  base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, error_callback_count_);
+  EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
   ASSERT_EQ(0u, notify_sessions_.size());
 }
 #endif  // defined(OS_ANDROID)
 
 #if defined(OS_ANDROID)
+// Tests StartNotifySession success.
+TEST_F(BluetoothGattCharacteristicTest, StartNotifySession) {
+  ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
+      /* properties: NOTIFY */ 0x10,
+      /* expected_config_descriptor_value: NOTIFY */ 1));
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests StartNotifySession success.
+TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_OnIndicate) {
+  ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
+      /* properties: INDICATE */ 0x20,

[Jeffrey Yasskin, 2016/01/14 00:13:42]

Please also check when both NOTIFY and INDICATE properties are set. It doesn't
matter which is written, but exactly 1 should be. (Testing that exactly NOTIFY
is written is fine.)

[scheib, 2016/01/14 01:12:26]

Done.

+      /* expected_config_descriptor_value: INDICATE */ 2));
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests Characteristic Value changes during a Notify Session.
+TEST_F(BluetoothGattCharacteristicTest, GattCharacteristicValueChanged) {
+  ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
+      /* properties: NOTIFY */ 0x10,
+      /* expected_config_descriptor_value: NOTIFY */ 1));
+
+  TestBluetoothAdapterObserver observer(adapter_);
+
+  std::vector<uint8_t> test_vector1;
+  test_vector1.push_back(111);
+  std::vector<uint8_t> test_vector2;
+  test_vector2.push_back(222);
+
+  SimulateGattCharacteristicChanged(characteristic1_, test_vector1);
+  EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
+  EXPECT_EQ(test_vector1, characteristic1_->GetValue());
+
+  SimulateGattCharacteristicChanged(characteristic1_, test_vector2);
+  EXPECT_EQ(2, observer.gatt_characteristic_value_changed_count());
+  EXPECT_EQ(test_vector2, characteristic1_->GetValue());
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests Characteristic Value changing after a Notify Session and objects being
+// destroyed.
+TEST_F(BluetoothGattCharacteristicTest,
+       GattCharacteristicValueChanged_AfterDeleted) {
+  ASSERT_NO_FATAL_FAILURE(StartNotifyBoilerplate(
+      /* properties: NOTIFY */ 0x10,
+      /* expected_config_descriptor_value: NOTIFY */ 1));
+
+  RememberCharacteristicForSubsequentAction(characteristic1_);
+  DeleteDevice(device_);
+
+  std::vector<uint8_t> empty_vector;
+  SimulateGattCharacteristicChanged(/* use remembered characteristic */ nullptr,

[Jeffrey Yasskin, 2016/01/14 00:13:42]

Please comment that the test here is that nothing crashes.

[scheib, 2016/01/14 01:12:26]

Done.

+                                    empty_vector);
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
 // Tests multiple StartNotifySession success.
 TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Multiple) {
-  ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate());
+  ASSERT_NO_FATAL_FAILURE(
+      FakeCharacteristicBoilerplate(/* properties: NOTIFY */ 0x10));
+  SimulateGattDescriptor(
+      characteristic1_,
+      /* Client Characteristic Configuration descriptor's standard UUID: */
+      "00002902-0000-1000-8000-00805F9B34FB");
+  ASSERT_EQ(1u, characteristic1_->GetDescriptors().size());
 
   characteristic1_->StartNotifySession(
       GetNotifyCallback(Call::EXPECTED),