Adding BLE connection finder unit tests.

Adding BLE connection finder unit tests.

Adds unit tests for the proximity_auth::BluetoothLowEnergyConnectionFinder
class.

BUG=485123
Committed: https://crrev.com/290ac0ba173c9799290a3ab617c79a5974c78def
Cr-Commit-Position: refs/heads/master@{#330918}

[sacomoto, 2015-05-18 16:40:56]

sacomoto@chromium.org changed reviewers:
+ msarda@chromium.org, tengs@chromium.org

[sacomoto, 2015-05-18 16:42:49]

PTAL.

[Tim Song, 2015-05-20 07:47:01]

Nice test!

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
File components/proximity_auth/ble/BUILD.gn (right):

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/BUILD.gn:30:
"bluetooth_low_energy_connection_finder_unittest.cc",
Please also add this test to the components_tests.gyp file.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (left):

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection.cc:124: 
Is this accidental?

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h
(right):

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:60: //
Creates a connection with |remote_device|. Exposed for testing.
nit: this comment isn't up to date anymore.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
File
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:68:
MOCK_METHOD0(CreateConnectionProxy, Connection*());
Why not just have CreateConnectionProxy() return scoped_ptr<Connection>? Does
testing::Return() not play well with scoped_ptrs?

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:116:
MOCK_METHOD1(OnConnectionFoundProxy, void(Connection* connection));
nit: add a new line here

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:277:
EXPECT_CALL(*adapter_, StartDiscoverySession(_, _))
We should refactor this block to a method and start the connection_finder in all
the tests above before adding devices. Calling DeviceAdded() without calling
Start() first isn't really representative of a normal flow.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:293:
connection_finder.DeviceAdded(adapter_.get(), device_.get());
Same here. It may be worth while to refactor this block to a method, since I
noticed other tests also set expectations for a BluetoothDevice and call
connection_finder.DeviceAdded().

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:301:
EXPECT_CALL(*this, OnConnectionFoundProxy(_));
Do you really need this proxy function? Can't you just check that
|last_found_connection_| is set?

[sacomoto, 2015-05-20 22:34:22]

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
File components/proximity_auth/ble/BUILD.gn (right):

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/BUILD.gn:30:
"bluetooth_low_energy_connection_finder_unittest.cc",
On 2015/05/20 07:47:00, Tim Song wrote:
> Please also add this test to the components_tests.gyp file.

Done.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
File components/proximity_auth/ble/bluetooth_low_energy_connection.cc (left):

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection.cc:124: 
On 2015/05/20 07:47:00, Tim Song wrote:
> Is this accidental?

Yes. Reverted.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
File components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h
(right):

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h:60: //
Creates a connection with |remote_device|. Exposed for testing.
On 2015/05/20 07:47:00, Tim Song wrote:
> nit: this comment isn't up to date anymore.

Done.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
File
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc
(right):

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:68:
MOCK_METHOD0(CreateConnectionProxy, Connection*());
On 2015/05/20 07:47:01, Tim Song wrote:
> Why not just have CreateConnectionProxy() return scoped_ptr<Connection>? Does
> testing::Return() not play well with scoped_ptrs?

Mock methods don't support return type of scoped_ptr<>. This,

MOCK_METHOD0(CreateConnection, scoped_ptr<Connection>());

won't compile. See for instance, here:

https://code.google.com/p/chromium/codesearch#chromium/src/dbus/mock_object_p...

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:116:
MOCK_METHOD1(OnConnectionFoundProxy, void(Connection* connection));
On 2015/05/20 07:47:01, Tim Song wrote:
> nit: add a new line here

Done.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:277:
EXPECT_CALL(*adapter_, StartDiscoverySession(_, _))
On 2015/05/20 07:47:01, Tim Song wrote:
> We should refactor this block to a method and start the connection_finder in
all
> the tests above before adding devices. Calling DeviceAdded() without calling
> Start() first isn't really representative of a normal flow. 

Done.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:293:
connection_finder.DeviceAdded(adapter_.get(), device_.get());
On 2015/05/20 07:47:01, Tim Song wrote:
> Same here. It may be worth while to refactor this block to a method, since I
> noticed other tests also set expectations for a BluetoothDevice and call
> connection_finder.DeviceAdded().

Done.

https://codereview.chromium.org/1129983004/diff/1/components/proximity_auth/b...
components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc:301:
EXPECT_CALL(*this, OnConnectionFoundProxy(_));
On 2015/05/20 07:47:01, Tim Song wrote:
> Do you really need this proxy function? Can't you just check that
> |last_found_connection_| is set?

Yes, you are right. I changed and removed the OnConnectionFoundProxy() mock.

[Tim Song, 2015-05-21 00:02:58]

LGTM

[sacomoto, 2015-05-21 08:13:11]

The CQ bit was checked by sacomoto@chromium.org

[commit-bot: I haz the power, 2015-05-21 08:13:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129983004/20001

[commit-bot: I haz the power, 2015-05-21 09:11:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-21 09:12:22]

Patchset 2 (id:??) landed as
https://crrev.com/290ac0ba173c9799290a3ab617c79a5974c78def
Cr-Commit-Position: refs/heads/master@{#330918}