bluetooth: Fix crash when trying to read or write when operation pending

bluetooth: Fix crash when trying to read or write when operation pending

A return statement was missing so the callback would get called twice.

To avoid future bugs in which callbacks are called unexpectedly, we
add a new parameter to the getters of the mock callbacks. If
Call::EXPECTED is passed then the callback is expected to be called,
otherwise we the callback was not expected to be called and the test
will fail.

We add a TearDown implementation to BluetoothTestBase to check
no unexpected calls have been made to callbacks.

BUG=557571
Committed: https://crrev.com/8b6bdd4b1492c7a3607f73df02be0da625790afd
Cr-Commit-Position: refs/heads/master@{#361040}

[ortuno, 2015-11-20 21:01:28]

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

[ortuno, 2015-11-20 21:01:29]

scheib: PTAL

[scheib, 2015-11-20 23:08:20]

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

https://codereview.chromium.org/1465863003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:51: void BluetoothTestBase::TearDown() {
Let's also verify that the success callbacks made match the number expected.

https://codereview.chromium.org/1465863003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:72: return;
I think we want the counters always accurate, even if an unexpected call
happened. So, no return here. [ditto below]

https://codereview.chromium.org/1465863003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1465863003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:56: // called. Then
RunLoop().RunUntilIdle().
// Calls adapter_->StartDiscoverySessionWithFilter with Low Energy transport,
// and this fixture's callbacks expecting success. 
// Then RunLoop().RunUntilIdle().

[ortuno, 2015-11-21 01:12:49]

scheib: PTAL. Also removed EXPECT_EQ from places where it very obvious they were
not needed.

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

https://codereview.chromium.org/1465863003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:51: void BluetoothTestBase::TearDown() {
On 2015/11/20 at 23:08:20, scheib wrote:
> Let's also verify that the success callbacks made match the number expected.

Done.

https://codereview.chromium.org/1465863003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:72: return;
On 2015/11/20 at 23:08:20, scheib wrote:
> I think we want the counters always accurate, even if an unexpected call
happened. So, no return here. [ditto below]

Done.

https://codereview.chromium.org/1465863003/diff/20001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test.h (right):

https://codereview.chromium.org/1465863003/diff/20001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:56: // called. Then
RunLoop().RunUntilIdle().
On 2015/11/20 at 23:08:20, scheib wrote:
> // Calls adapter_->StartDiscoverySessionWithFilter with Low Energy transport,
> // and this fixture's callbacks expecting success. 
> // Then RunLoop().RunUntilIdle().

Done.

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

https://codereview.chromium.org/1465863003/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:221: int actual_error_callback_calls_ =
0;
Not sure about the name here. Maybe expected_expected_success_callback_calls_
and actual_expected_success_callback_calls_?

[scheib, 2015-11-21 05:40:22]

LGTM

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

https://codereview.chromium.org/1465863003/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:87: if (expected == Call::NOT_EXPECTED)
Optional, but logic for these may be simpler with the positive logic form of:

if (expected == Call::EXPECTED)

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

https://codereview.chromium.org/1465863003/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.h:221: int actual_error_callback_calls_ =
0;
On 2015/11/21 01:12:49, ortuno wrote:
> Not sure about the name here. Maybe expected_expected_success_callback_calls_
> and actual_expected_success_callback_calls_?

These names seem fine to me.
expected_expected is not expected as a name to me. ;)

[ortuno, 2015-11-22 04:45:02]

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

https://codereview.chromium.org/1465863003/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test.cc:87: if (expected == Call::NOT_EXPECTED)
On 2015/11/21 at 05:40:22, scheib wrote:
> Optional, but logic for these may be simpler with the positive logic form of:
> 
> if (expected == Call::EXPECTED)

Done.

[ortuno, 2015-11-22 04:45:07]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-11-22 04:45:07]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1465863003/#ps60001
(title: "Address scheib's comments")

[commit-bot: I haz the power, 2015-11-22 04:45:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1465863003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1465863003/60001

[commit-bot: I haz the power, 2015-11-22 06:12:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-22 06:14:00]

Patchset 4 (id:??) landed as
https://crrev.com/8b6bdd4b1492c7a3607f73df02be0da625790afd
Cr-Commit-Position: refs/heads/master@{#361040}