bluetooth: Make in progress errors consistent across android

device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc

Index: device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
diff --git a/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc b/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
index 89a9a732d4c93426c59c333067f2c8ba07f05027..99f827a00b19c5444f78e3ed255ace76b36afe0a 100644
--- a/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
+++ b/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc
@@ -27,28 +27,88 @@ namespace device {
 #if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 class BluetoothRemoteGattCharacteristicTest : public BluetoothTest {
  public:
+  BluetoothDevice* GetDeviceWithGattAttributes(int device_ordinal,
+                                               size_t num_services,
+                                               size_t num_characteristics,
+                                               int properties) {
+    BluetoothDevice* device = SimulateLowEnergyDevice(device_ordinal);
+
+    device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                                 GetConnectErrorCallback(Call::NOT_EXPECTED));
+    SimulateGattConnection(device);
+    base::RunLoop().RunUntilIdle();

[scheib, 2017/04/21 23:33:11]

Heads up: This pattern is changing:
https://codereview.chromium.org/2818533003/diff/240001/device/bluetooth/bluez...
In that change this unittest file isn't modified, but perhaps we should start
using new pattern.

[ortuno, 2017/04/28 03:47:12]

Those tests used a slightly different pattern in which they actually quit the
runloop in their callbacks. We just let the runloop go idle in our callbacks. It
could be useful when testing reentrancy but for the rest of the tests I like the
simplicity of just calling:

base::RunLoop().RunUntilIdle();

+
+    std::vector<std::string> service_uuids;
+    for (size_t s = 0; s < num_services; s++) {
+      service_uuids.push_back(kTestUUIDHeartRate);
+    }
+    SimulateGattServicesDiscovered(device, service_uuids);
+
+    base::RunLoop().RunUntilIdle();
+    DCHECK(num_services == device->GetGattServices().size());
+
+    for (BluetoothRemoteGattService* service : device->GetGattServices()) {
+      for (size_t c = 0; c < num_characteristics; c++) {
+        SimulateGattCharacteristic(service, kTestUUIDHeartRateMeasurement,
+                                   properties);
+      }
+      DCHECK(num_characteristics == service->GetCharacteristics().size());
+    }
+
+    ResetEventCounts();
+    return device;
+  }
+
   // Creates adapter_, device_, service_, characteristic1_, & characteristic2_.
   // |properties| will be used for each characteristic.
   void FakeCharacteristicBoilerplate(int properties = 0) {
     InitWithFakeAdapter();
     StartLowEnergyDiscoverySession();
-    device_ = SimulateLowEnergyDevice(3);
-    device_->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
-                                  GetConnectErrorCallback(Call::NOT_EXPECTED));
-    SimulateGattConnection(device_);
-    base::RunLoop().RunUntilIdle();
-
-    SimulateGattServicesDiscovered(
-        device_, std::vector<std::string>({kTestUUIDGenericAccess}));
-    base::RunLoop().RunUntilIdle();
-    ASSERT_EQ(1u, device_->GetGattServices().size());
+    device_ = GetDeviceWithGattAttributes(
+        3 /* device_ordinal */, 1 /* num_services */,
+        2 /* num_characteristics */, properties);
     service_ = device_->GetGattServices()[0];
-    SimulateGattCharacteristic(service_, kTestUUIDDeviceName, properties);
-    SimulateGattCharacteristic(service_, kTestUUIDDeviceName, properties);
-    ASSERT_EQ(2u, service_->GetCharacteristics().size());
-    characteristic1_ = service_->GetCharacteristics()[0];
-    characteristic2_ = service_->GetCharacteristics()[1];
-    ResetEventCounts();
+    std::vector<BluetoothRemoteGattCharacteristic*> characteristics =
+        service_->GetCharacteristics();
+    characteristic1_ = characteristics[0];
+    characteristic2_ = characteristics[1];
+  }
+
+  std::pair<BluetoothRemoteGattCharacteristic*,
+            BluetoothRemoteGattCharacteristic*>
+  GetCharacteristicsInDifferentServices(int properties) {

[scheib, 2017/04/21 23:33:11]

Consider naming: I named a few fixture methods such as
FakeCharacteristicBoilerplate to help indicate that it was intended to
encapsulate the boilerplate setup code at the beginning of a test. The naming
here of Get____ sounds as if it's OK to run any time and more than once. Perhaps
it can? But, e.g. the Init code to date takes a "setting up from scratch"
perspective.

Also, is there any reason to not return the characteristics via test fixture
member variables? It would save the initial lines of each test and just use
descriptive members.

In fact, any reason all these scenarios can't be available in the single
FakeCharacteristicBoilerplate? Do we ever need to have fewer devices, fewer
services?

+    InitWithFakeAdapter();
+    StartLowEnergyDiscoverySession();
+    BluetoothDevice* device = GetDeviceWithGattAttributes(
+        3 /* device_ordinal */, 2 /* num_services */,
+        1 /* num_characteristics */, properties);
+    std::vector<BluetoothRemoteGattService*> services =
+        device->GetGattServices();
+    BluetoothRemoteGattCharacteristic* char_in_service1 =
+        services[0]->GetCharacteristics()[0];
+    BluetoothRemoteGattCharacteristic* char_in_service2 =
+        services[1]->GetCharacteristics()[0];
+    return std::make_pair(char_in_service1, char_in_service2);
+  }
+
+  std::pair<BluetoothRemoteGattCharacteristic*,
+            BluetoothRemoteGattCharacteristic*>
+  GetCharacteristicsInDifferentDevices(int properties) {
+    InitWithFakeAdapter();
+    StartLowEnergyDiscoverySession();
+    BluetoothDevice* device1 = GetDeviceWithGattAttributes(
+        3 /* device_ordinal */, 2 /* num_services */,

[scheib, 2017/04/21 23:33:11]

1 service only needed

+        1 /* num_characteristics */, properties);
+    BluetoothRemoteGattCharacteristic* char_in_device1 =
+        device1->GetGattServices()[0]->GetCharacteristics()[0];
+
+    BluetoothDevice* device2 = GetDeviceWithGattAttributes(
+        4 /* device_ordinal */, 1 /* num_services */,
+        1 /* num_characteristics */, properties);
+    BluetoothRemoteGattCharacteristic* char_in_device2 =
+        device2->GetGattServices()[0]->GetCharacteristics()[0];
+
+    return std::make_pair(char_in_device1, char_in_device2);
   }
 
   enum class StartNotifySetupError {
@@ -667,10 +727,12 @@ TEST_F(BluetoothRemoteGattCharacteristicTest, WriteRemoteCharacteristic_Twice) {
 }
 #endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
-#if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
-// Tests ReadRemoteCharacteristic on two characteristics.
+#if defined(OS_ANDROID)
+// Tests parallel ReadRemoteCharacteristic on two characteristic in the same
+// service.
+// TODO(crbug.com/657921): Enable on macOS and Windows.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
-       ReadRemoteCharacteristic_MultipleCharacteristics) {
+       ReadRemoteCharacteristic_MultipleCharacteristicsInService) {
   if (!PlatformSupportsLowEnergy()) {
     LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
     return;
@@ -682,80 +744,271 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
       GetReadValueCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
   characteristic2_->ReadRemoteCharacteristic(
+      GetReadValueCallback(Call::NOT_EXPECTED),
+      GetGattErrorCallback(Call::EXPECTED));
+
+  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(1, error_callback_count_);
+  EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS,
+            last_gatt_error_code_);
+
+  ResetEventCounts();
+
+  std::vector<uint8_t> test_vector1({111});
+  SimulateGattCharacteristicRead(characteristic1_, test_vector1);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(test_vector1, last_read_value_);
+  EXPECT_EQ(test_vector1, characteristic1_->GetValue());
+  EXPECT_EQ(1, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests parallel ReadRemoteCharacteristic on two characteristic in the same
+// service.
+// TODO(crbug.com/657921): Enable on macOS and Windows.
+TEST_F(BluetoothRemoteGattCharacteristicTest,
+       WriteRemoteCharacteristic_MultipleCharacteristicsInService) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate(
+      BluetoothRemoteGattCharacteristic::PROPERTY_WRITE));
+
+  std::vector<uint8_t> test_vector1({111});
+  characteristic1_->WriteRemoteCharacteristic(
+      test_vector1, GetCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  EXPECT_EQ(test_vector1, last_write_value_);
+
+  std::vector<uint8_t> test_vector2({222});
+  characteristic2_->WriteRemoteCharacteristic(
+      test_vector2, GetCallback(Call::NOT_EXPECTED),
+      GetGattErrorCallback(Call::EXPECTED));
+  EXPECT_EQ(test_vector1, last_write_value_);
+
+  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(1, error_callback_count_);
+  EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS,
+            last_gatt_error_code_);
+
+  ResetEventCounts();
+
+  SimulateGattCharacteristicWrite(characteristic1_);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests parallel ReadRemoteCharacteristic on two characteristics on different
+// services but on the same device.

[scheib, 2017/04/21 23:33:11]

I pause thinking about the number of test permutations we need. As we expand to
other gatt operations the possible interactions will grow significantly. 

{ initial: read, write} x 
{ followed by: read, write} x 
{ same characteristic, same-service characteristics, same-device
characteristics, same-adapter characteristics}

== 16 permutations already.

As Gatt operations increase and we include descriptors the result goes even
higher.

So, perhaps we should either do some spot checking of all the permutations. If
so, I think a comment block that explains all the permutations and highlighting
which one's we're doing would help. I think this approach is OK since we know
the mechanism and I don't think it's going to need exhaustive testing.

Or, build test fixture methods to help us run all permutations.

+// TODO(crbug.com/657921): Enable on macOS and Windows.
+TEST_F(BluetoothRemoteGattCharacteristicTest,
+       ReadRemoteCharacteristic_MultipleCharacteristicsInDevice) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+
+  BluetoothRemoteGattCharacteristic* characteristic_in_service1;
+  BluetoothRemoteGattCharacteristic* characteristic_in_service2;
+  std::tie(characteristic_in_service1, characteristic_in_service2) =
+      GetCharacteristicsInDifferentServices(
+          BluetoothRemoteGattCharacteristic::PROPERTY_READ);
+
+  characteristic_in_service1->ReadRemoteCharacteristic(
       GetReadValueCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
+  characteristic_in_service2->ReadRemoteCharacteristic(
+      GetReadValueCallback(Call::NOT_EXPECTED),
+      GetGattErrorCallback(Call::EXPECTED));
+
+  EXPECT_EQ(1, gatt_read_characteristic_attempts_);
   EXPECT_EQ(0, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
 
-  std::vector<uint8_t> test_vector1;
-  test_vector1.push_back(111);
-  SimulateGattCharacteristicRead(characteristic1_, test_vector1);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(1, error_callback_count_);
+  EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS,
+            last_gatt_error_code_);
+
+  ResetEventCounts();
+
+  std::vector<uint8_t> test_vector1({111});
+  SimulateGattCharacteristicRead(characteristic_in_service1, test_vector1);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(test_vector1, last_read_value_);
+  EXPECT_EQ(test_vector1, characteristic_in_service1->GetValue());
+  EXPECT_EQ(1, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID)
+// Tests parallel WriteRemoteCharacteristic on two characteristics on different
+// services but on the same device.
+// TODO(crbug.com/657921): Enable on macOS and Windows.
+TEST_F(BluetoothRemoteGattCharacteristicTest,
+       WriteRemoteCharacteristic_MultipleCharacteristicsInDevice) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  BluetoothRemoteGattCharacteristic* characteristic_in_service1;
+  BluetoothRemoteGattCharacteristic* characteristic_in_service2;
+  std::tie(characteristic_in_service1, characteristic_in_service2) =
+      GetCharacteristicsInDifferentServices(
+          BluetoothRemoteGattCharacteristic::PROPERTY_WRITE);
+
+  std::vector<uint8_t> test_vector1({111});
+  characteristic_in_service1->WriteRemoteCharacteristic(
+      test_vector1, GetCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  EXPECT_EQ(test_vector1, last_write_value_);
+
+  std::vector<uint8_t> test_vector2({222});
+  characteristic_in_service2->WriteRemoteCharacteristic(
+      test_vector2, GetCallback(Call::NOT_EXPECTED),
+      GetGattErrorCallback(Call::EXPECTED));
+  EXPECT_EQ(test_vector1, last_write_value_);
+
+  EXPECT_EQ(1, gatt_write_characteristic_attempts_);
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
 
-  std::vector<uint8_t> test_vector2;
-  test_vector2.push_back(222);
-  SimulateGattCharacteristicRead(characteristic2_, test_vector2);
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(test_vector2, last_read_value_);
 
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(1, error_callback_count_);
+  EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS,
+            last_gatt_error_code_);
+
+  ResetEventCounts();
+
+  SimulateGattCharacteristicWrite(characteristic_in_service1);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+}
+#endif  // defined(OS_ANDROID)
+
+#if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
+// Tests parallel ReadRemoteCharacteristic on two characteristics on different
+// devices.
+TEST_F(BluetoothRemoteGattCharacteristicTest,
+       ReadRemoteCharacteristic_MultipleCharacteristicsInAdapter) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+
+  BluetoothRemoteGattCharacteristic* characteristic_in_device1;
+  BluetoothRemoteGattCharacteristic* characteristic_in_device2;
+  std::tie(characteristic_in_device1, characteristic_in_device2) =
+      GetCharacteristicsInDifferentDevices(
+          BluetoothRemoteGattCharacteristic::PROPERTY_READ);
+
+  characteristic_in_device1->ReadRemoteCharacteristic(
+      GetReadValueCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+  characteristic_in_device2->ReadRemoteCharacteristic(
+      GetReadValueCallback(Call::EXPECTED),
+      GetGattErrorCallback(Call::NOT_EXPECTED));
+
+#if !defined(OS_WIN)
+  // TODO(crbug.com/713991): Remove #if once all platforms increate the counters
+  // at the same time.
   EXPECT_EQ(2, gatt_read_characteristic_attempts_);
+#endif
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+
+  std::vector<uint8_t> test_vector1({111});
+  SimulateGattCharacteristicRead(characteristic_in_device1, test_vector1);
+  std::vector<uint8_t> test_vector2({222});
+  SimulateGattCharacteristicRead(characteristic_in_device2, test_vector2);
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(test_vector1, characteristic_in_device1->GetValue());
+  EXPECT_EQ(test_vector2, characteristic_in_device2->GetValue());
+  EXPECT_EQ(test_vector2, last_read_value_);
   EXPECT_EQ(2, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
-  EXPECT_EQ(test_vector1, characteristic1_->GetValue());
-  EXPECT_EQ(test_vector2, characteristic2_->GetValue());

[scheib, 2017/04/21 23:33:11]

The meaning of the test is changing. Before, it was that two characteristics can
hold 2 different values.  Now, it is that two simultaneous reads error out
correctly. Unless there's another test that verifies the former (I don't think
there is) then I think we should keep it and duplicate for the error situation.
(ditto for write)

 }
 #endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 
 #if defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
-// Tests WriteRemoteCharacteristic on two characteristics.
+// Tests parallel WriteRemoteCharacteristic on two characteristics on different
+// devices.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
-       WriteRemoteCharacteristic_MultipleCharacteristics) {
+       WriteRemoteCharacteristic_MultipleCharacteristicsInAdapter) {
   if (!PlatformSupportsLowEnergy()) {
     LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
     return;
   }
-  ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate(
-      BluetoothRemoteGattCharacteristic::PROPERTY_WRITE));
+  BluetoothRemoteGattCharacteristic* characteristic_in_device1;
+  BluetoothRemoteGattCharacteristic* characteristic_in_device2;
+  std::tie(characteristic_in_device1, characteristic_in_device2) =
+      GetCharacteristicsInDifferentDevices(
+          BluetoothRemoteGattCharacteristic::PROPERTY_WRITE);
 
-  std::vector<uint8_t> test_vector1;
-  test_vector1.push_back(111);
-  characteristic1_->WriteRemoteCharacteristic(
+  std::vector<uint8_t> test_vector1({111});
+  characteristic_in_device1->WriteRemoteCharacteristic(
       test_vector1, GetCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-#if defined(OS_ANDROID) || defined(OS_MACOSX)
+#if !defined(OS_WIN)
+  // TODO(crbug.com/713991): Remove #if once all platforms increate the counters
+  // at the same time.
   EXPECT_EQ(test_vector1, last_write_value_);
 #endif
 
-  std::vector<uint8_t> test_vector2;
-  test_vector2.push_back(222);
-  characteristic2_->WriteRemoteCharacteristic(
+  std::vector<uint8_t> test_vector2({222});
+  characteristic_in_device2->WriteRemoteCharacteristic(
       test_vector2, GetCallback(Call::EXPECTED),
       GetGattErrorCallback(Call::NOT_EXPECTED));
-#if defined(OS_ANDROID) || defined(OS_MACOSX)
+#if !defined(OS_WIN)
+  // TODO(crbug.com/713991): Remove #if once all platforms increate the counters
+  // at the same time.
   EXPECT_EQ(test_vector2, last_write_value_);
-#endif
 
+  EXPECT_EQ(2, gatt_write_characteristic_attempts_);
+#endif
   EXPECT_EQ(0, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
 
-  SimulateGattCharacteristicWrite(characteristic1_);
   base::RunLoop().RunUntilIdle();
-#if !(defined(OS_ANDROID) || defined(OS_MACOSX))
-  EXPECT_EQ(test_vector1, last_write_value_);
-#endif
 
-  SimulateGattCharacteristicWrite(characteristic2_);
-  base::RunLoop().RunUntilIdle();
-#if !(defined(OS_ANDROID) || defined(OS_MACOSX))
-  EXPECT_EQ(test_vector2, last_write_value_);
-#endif
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
 
-  EXPECT_EQ(2, gatt_write_characteristic_attempts_);
+  SimulateGattCharacteristicWrite(characteristic_in_device1);
+  SimulateGattCharacteristicWrite(characteristic_in_device2);
+  base::RunLoop().RunUntilIdle();
   EXPECT_EQ(2, callback_count_);
   EXPECT_EQ(0, error_callback_count_);
-
-  // TODO(591740): Remove if define for OS_ANDROID in this test.
 }
 #endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)