Adding BluetoothTestBase::CheckNotifySessionValue() method

Adding BluetoothTestBase::CheckNotifySessionValue() method

The default implementation uses the descriptors. This method is overriden
by BluetoothTestMac, to check |gatt_notify_characteristic_attempts_| and
|last_notify_value| instance variables.

BUG=

Review-Url: https://codereview.chromium.org/2695573002
Cr-Commit-Position: refs/heads/master@{#452665}
Committed: https://chromium.googlesource.com/chromium/src/+/1c547d7f20b926af6a2c63d748973715f5c1b22f

[jlebel, 2017-02-13 00:50:17]

jlebel@chromium.org changed reviewers:
+ ortuno@chromium.org

[jlebel, 2017-02-13 00:50:18]

Hello Giovanni,

I'm trying to simplify the unit tests related to the notification.

Let me know what you think.

Thanks,

Jérôme,

[jlebel, 2017-02-13 10:29:47]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 10:30:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 11:41:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 11:41:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jlebel, 2017-02-13 19:44:55]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 19:46:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 21:15:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 21:15:03]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-02-20 07:28:02]

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:95: void
BluetoothTestBase::CheckNotifySessionValue(
Great idea! Two suggestiongs:

1. What do you think about separating the attempts from the value checking?
2. What do you think about an enum to make the first value clearer? e.g. enum
class NotifyValueState { NOTIFY, INDICATE, NOTIFY_AND_INDICATE}.

I think both of those things make the code more readable:

CheckNotifySessionValue(1 /* expected_ccc_descriptor_value */, 1 /* attempts
*/);

vs.

ExpectedChangeNotifyValueAttempts(1);
ExpectedNotifyValue(NotifyValueState:NOTIFY);

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:98: EXPECT_EQ(attempts,
gatt_write_descriptor_attempts_);
Could we include gatt_notify_characteristic_attempts_ here?

[ortuno, 2017-02-20 07:32:50]

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:95: void
BluetoothTestBase::CheckNotifySessionValue(
On 2017/02/20 at 07:28:02, ortuno wrote:
> Great idea! Two suggestiongs:
> 
> 1. What do you think about separating the attempts from the value checking?
> 2. What do you think about an enum to make the first value clearer? e.g. enum
class NotifyValueState { NOTIFY, INDICATE, NOTIFY_AND_INDICATE}.

Small correction: This should be {NOTIFY, INDICATE} no need for
NOTIFY_AND_INDICATE.

> 
> I think both of those things make the code more readable:
> 
> CheckNotifySessionValue(1 /* expected_ccc_descriptor_value */, 1 /* attempts
*/);
> 
> vs.
> 
> ExpectedChangeNotifyValueAttempts(1);
> ExpectedNotifyValue(NotifyValueState:NOTIFY);

[jlebel, 2017-02-21 21:35:40]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 21:36:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jlebel, 2017-02-21 21:46:30]

It looks better now.

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:95: void
BluetoothTestBase::CheckNotifySessionValue(
On 2017/02/20 07:28:02, ortuno wrote:
> Great idea! Two suggestiongs:
> 
> 1. What do you think about separating the attempts from the value checking?
> 2. What do you think about an enum to make the first value clearer? e.g. enum
> class NotifyValueState { NOTIFY, INDICATE, NOTIFY_AND_INDICATE}.
> 
> I think both of those things make the code more readable:
> 
> CheckNotifySessionValue(1 /* expected_ccc_descriptor_value */, 1 /* attempts
> */);
> 
> vs.
> 
> ExpectedChangeNotifyValueAttempts(1);
> ExpectedNotifyValue(NotifyValueState:NOTIFY);

Done.

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:98: EXPECT_EQ(attempts,
gatt_write_descriptor_attempts_);
On 2017/02/20 07:28:02, ortuno wrote:
> Could we include gatt_notify_characteristic_attempts_ here?

This is mac only. So either I use #define for mac only, or I override the method
for the mac. What do you prefer?

[commit-bot: I haz the power, 2017-02-21 21:51:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 21:51:28]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ortuno, 2017-02-22 03:26:22]

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:98: EXPECT_EQ(attempts,
gatt_write_descriptor_attempts_);
On 2017/02/21 at 21:46:30, jlebel wrote:
> On 2017/02/20 07:28:02, ortuno wrote:
> > Could we include gatt_notify_characteristic_attempts_ here?
> 
> This is mac only. So either I use #define for mac only, or I override the
method for the mac. What do you prefer?

Actually all platforms use gatt_notify_characteristic_attempts_. See comment in
latest patchset.

https://codereview.chromium.org/2695573002/diff/60001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (left):

https://codereview.chromium.org/2695573002/diff/60001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:377:
gatt_notify_characteristic_attempts_ = 0;
Both android[1] and windows[2] have "StartNotifications" functions that we need
to call in addition to writing to the descriptor. That's what this variable was
counting. We should include it in ExpectedChangeNotifyValueAttempts 


[1] Android function
https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html#...,
boolean)
[2] Windows function
https://msdn.microsoft.com/en-us/library/windows/hardware/hh450804(v=vs.85).aspx
[3] Android using the variable
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test_and...
[4] Windows using the variable:
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test_win...

[jlebel, 2017-02-22 09:24:30]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 09:25:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jlebel, 2017-02-22 10:23:52]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 10:24:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 10:49:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 10:49:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jlebel, 2017-02-22 11:06:06]

Patchset #5 (id:80001) has been deleted

[jlebel, 2017-02-22 12:02:09]

I tried to update the unit tests, but I'm not sure why
StartNotifySession_WriteDescriptorSynchronousError fails on Android.

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (right):

https://codereview.chromium.org/2695573002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:98: EXPECT_EQ(attempts,
gatt_write_descriptor_attempts_);
On 2017/02/22 03:26:21, ortuno wrote:
> On 2017/02/21 at 21:46:30, jlebel wrote:
> > On 2017/02/20 07:28:02, ortuno wrote:
> > > Could we include gatt_notify_characteristic_attempts_ here?
> > 
> > This is mac only. So either I use #define for mac only, or I override the
> method for the mac. What do you prefer?
> 
> Actually all platforms use gatt_notify_characteristic_attempts_. See comment
in
> latest patchset.

Done.

https://codereview.chromium.org/2695573002/diff/60001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.cc (left):

https://codereview.chromium.org/2695573002/diff/60001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:377:
gatt_notify_characteristic_attempts_ = 0;
On 2017/02/22 03:26:21, ortuno wrote:
> Both android[1] and windows[2] have "StartNotifications" functions that we
need
> to call in addition to writing to the descriptor. That's what this variable
was
> counting. We should include it in ExpectedChangeNotifyValueAttempts 
> 
> 
> [1] Android function
>
https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html#...,
> boolean)
> [2] Windows function
>
https://msdn.microsoft.com/en-us/library/windows/hardware/hh450804(v=vs.85).aspx
> [3] Android using the variable
>
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test_and...
> [4] Windows using the variable:
>
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test_win...

Sorry about that. I was a bit fast.

[ortuno, 2017-02-23 05:56:00]

lgtm with some documentation changes. Thanks for doing this :)

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1091:
ExpectedNotifyValue(NotifyValueState::NOTIFY);
This is failing because we were unable to write to the descriptor so the value
of the descriptor is currently 0. I believe that if you change this to

ExpectedNotifyValue(NotifyValueState::NONE)

it would work.

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test.h:410: // Tests if the notify value
attemps.
Tests that functions to change the notify value have been called |attempts|
times.

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test.h:413: // Tests if the notify session has
been changed. The default implementation
Tests that the notify value is |expected_value_state|. The default
implementation checks
that the correct value has been written to the CCC Descriptor.

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:138: last_notify_value = false;
nit: member variables should end with "_" so last_notify_value_

[jlebel, 2017-02-23 18:40:32]

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1091:
ExpectedNotifyValue(NotifyValueState::NOTIFY);
On 2017/02/23 05:56:00, ortuno wrote:
> This is failing because we were unable to write to the descriptor so the value
> of the descriptor is currently 0. I believe that if you change this to
> 
> ExpectedNotifyValue(NotifyValueState::NONE)
> 
> it would work.

Ah yes, that was obvious...

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test.h:410: // Tests if the notify value
attemps.
On 2017/02/23 05:56:00, ortuno wrote:
> Tests that functions to change the notify value have been called |attempts|
> times.

Done.

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test.h:413: // Tests if the notify session has
been changed. The default implementation
On 2017/02/23 05:56:00, ortuno wrote:
> Tests that the notify value is |expected_value_state|. The default
> implementation checks
> that the correct value has been written to the CCC Descriptor.

Done.

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:138: last_notify_value = false;
On 2017/02/23 05:56:00, ortuno wrote:
> nit: member variables should end with "_" so last_notify_value_

Yes!

[jlebel, 2017-02-23 18:41:06]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-02-23 18:41:06]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org
Link to the patchset: https://codereview.chromium.org/2695573002/#ps120001
(title: ".")

[commit-bot: I haz the power, 2017-02-23 18:41:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 19:17:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 19:17:47]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jlebel, 2017-02-23 20:38:54]

Patchset #6 (id:120001) has been deleted

[jlebel, 2017-02-23 20:39:57]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:40:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jlebel, 2017-02-23 21:53:46]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[jlebel, 2017-02-23 21:54:06]

Patchset #8 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-02-23 21:54:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jlebel, 2017-02-23 21:58:53]

Hello Giovanni,

Can you review that last change in this patch (related to Android)?

Thanks,

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2695573002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1091:
ExpectedNotifyValue(NotifyValueState::NOTIFY);
On 2017/02/23 18:40:32, jlebel wrote:
> On 2017/02/23 05:56:00, ortuno wrote:
> > This is failing because we were unable to write to the descriptor so the
value
> > of the descriptor is currently 0. I believe that if you change this to
> > 
> > ExpectedNotifyValue(NotifyValueState::NONE)
> > 
> > it would work.
> 
> Ah yes, that was obvious...

Well it was not that obvious:
EXPECT_EQ(1, gatt_notify_characteristic_attempts_);
EXPECT_EQ(0, gatt_write_descriptor_attempts_);
ASSERT_EQ(0u, last_write_value_.size());

Should I leave it that way?

[ortuno, 2017-02-23 22:16:38]

lgtm

[commit-bot: I haz the power, 2017-02-23 22:49:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 22:49:38]

Dry run: This issue passed the CQ dry run.

[jlebel, 2017-02-23 23:00:59]

The CQ bit was checked by jlebel@chromium.org

[commit-bot: I haz the power, 2017-02-23 23:01:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 23:10:20]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1487890859749050,
"parent_rev": "1afab6869b24fbaaae6831564ae9ca68bf3e9883", "commit_rev":
"1c547d7f20b926af6a2c63d748973715f5c1b22f"}

[commit-bot: I haz the power, 2017-02-23 23:10:55]

Description was changed from

==========
Adding BluetoothTestBase::CheckNotifySessionValue() method

The default implementation uses the descriptors. This method is overriden
by BluetoothTestMac, to check |gatt_notify_characteristic_attempts_| and
|last_notify_value| instance variables.

BUG=
==========

to

==========
Adding BluetoothTestBase::CheckNotifySessionValue() method

The default implementation uses the descriptors. This method is overriden
by BluetoothTestMac, to check |gatt_notify_characteristic_attempts_| and
|last_notify_value| instance variables.

BUG=

Review-Url: https://codereview.chromium.org/2695573002
Cr-Commit-Position: refs/heads/master@{#452665}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c547d7f20b926af6a2c63d74897...
==========

[commit-bot: I haz the power, 2017-02-23 23:10:57]

Committed patchset #8 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/1c547d7f20b926af6a2c63d74897...