Add TestInterface methods for adding shill stub devices and services

Add TestInterface methods for adding shill stub devices and services

This will help write functional tests for network handlers written on top of the Shill client code.

BUG=154072
TEST=NetworkSmsHandlerTest.SmsHandlerDbusStub passes


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165633

[satorux1, 2012-11-01 01:47:57]

LGTM, but please wait for hashimoto@'s comments.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_de...
File chromeos/dbus/mock_shill_device_client.h (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_de...
chromeos/dbus/mock_shill_device_client.h:74: MOCK_METHOD0(GetTestInterface,
TestInterface*(void));
(void) -> () ?

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_ma...
File chromeos/dbus/mock_shill_manager_client.h (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_ma...
chromeos/dbus/mock_shill_manager_client.h:45: MOCK_METHOD0(GetTestInterface,
TestInterface*(void));
(void) -> () ?

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_se...
File chromeos/dbus/mock_shill_service_client.h (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_se...
chromeos/dbus/mock_shill_service_client.h:55: MOCK_METHOD0(GetTestInterface,
TestInterface*(void));
(void) -> () ?

[hashimoto, 2012-11-01 04:54:34]

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device...
File chromeos/dbus/shill_device_client.cc (left):

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device...
chromeos/dbus/shill_device_client.cc:379: const DictionaryValueCallback&
callback) const {
nit: Please fix this indent while you are here.

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device...
File chromeos/dbus/shill_device_client.cc (right):

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device...
chromeos/dbus/shill_device_client.cc:191: virtual TestInterface*
GetTestInterface() {
nit: OVERRIDE

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device...
chromeos/dbus/shill_device_client.cc:352: virtual
ShillDeviceClient::TestInterface* GetTestInterface() {
nit: OVERRIDE

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device...
chromeos/dbus/shill_device_client.cc:374: virtual void RemoveDevice(const
std::string& device_path) {
nit: OVERRIDE

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device...
chromeos/dbus/shill_device_client.cc:414: base::DictionaryValue* properties;
nit: = NULL;

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.cc:164: virtual TestInterface*
GetTestInterface() {
nit: OVERRIDE

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.cc:236: base::ListValue* enabled_list;
nit: = NULL;

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.cc:257: base::ListValue* enabled_list;
nit: = NULL;

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.cc:290: virtual
ShillManagerClient::TestInterface* GetTestInterface() {
nit: OVERRIDE

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.cc:369: base::Value* value;
nit: = NULL;

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_servic...
File chromeos/dbus/shill_service_client.cc (right):

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.cc:156: virtual
ShillServiceClient::TestInterface* GetTestInterface() {
nit: OVERRIDE

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.cc:226: std::string ssid =
service_path.value();
Since this method is used not only for wifi networks, you might want to rename
this variable to service_path_str or just use service_path.value() directly.
Same goes for other methods.

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.cc:285: virtual
ShillServiceClient::TestInterface* GetTestInterface() {
nit: OVERRIDE

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.cc:372: base::Value* value;
nit: = NULL;

https://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_servic...
chromeos/dbus/shill_service_client.cc:384: base::DictionaryValue* properties;
nit: = NULL;

https://codereview.chromium.org/11365022/diff/3001/chromeos/network/network_s...
File chromeos/network/network_sms_handler_unittest.cc (right):

https://codereview.chromium.org/11365022/diff/3001/chromeos/network/network_s...
chromeos/network/network_sms_handler_unittest.cc:61:
DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface();
ASSERT_TRUE(manager_test)?

https://codereview.chromium.org/11365022/diff/3001/chromeos/network/network_s...
chromeos/network/network_sms_handler_unittest.cc:64:
DBusThreadManager::Get()->GetShillDeviceClient()->GetTestInterface();
ASSERT_TRUE(device_test)?

[stevenjb, 2012-11-01 18:45:59]

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_de...
File chromeos/dbus/mock_shill_device_client.h (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_de...
chromeos/dbus/mock_shill_device_client.h:74: MOCK_METHOD0(GetTestInterface,
TestInterface*(void));
On 2012/11/01 01:47:57, satorux1 wrote:
> (void) -> () ?

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_ma...
File chromeos/dbus/mock_shill_manager_client.h (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_ma...
chromeos/dbus/mock_shill_manager_client.h:45: MOCK_METHOD0(GetTestInterface,
TestInterface*(void));
On 2012/11/01 01:47:57, satorux1 wrote:
> (void) -> () ?

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_se...
File chromeos/dbus/mock_shill_service_client.h (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/mock_shill_se...
chromeos/dbus/mock_shill_service_client.h:55: MOCK_METHOD0(GetTestInterface,
TestInterface*(void));
On 2012/11/01 01:47:57, satorux1 wrote:
> (void) -> () ?

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device_...
File chromeos/dbus/shill_device_client.cc (left):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device_...
chromeos/dbus/shill_device_client.cc:379: const DictionaryValueCallback&
callback) const {
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: Please fix this indent while you are here.

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device_...
File chromeos/dbus/shill_device_client.cc (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device_...
chromeos/dbus/shill_device_client.cc:191: virtual TestInterface*
GetTestInterface() {
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device_...
chromeos/dbus/shill_device_client.cc:352: virtual
ShillDeviceClient::TestInterface* GetTestInterface() {
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device_...
chromeos/dbus/shill_device_client.cc:374: virtual void RemoveDevice(const
std::string& device_path) {
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_device_...
chromeos/dbus/shill_device_client.cc:414: base::DictionaryValue* properties;
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: = NULL;
Good catch. Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manager...
File chromeos/dbus/shill_manager_client.cc (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manager...
chromeos/dbus/shill_manager_client.cc:164: virtual TestInterface*
GetTestInterface() {
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manager...
chromeos/dbus/shill_manager_client.cc:236: base::ListValue* enabled_list;
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: = NULL;

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manager...
chromeos/dbus/shill_manager_client.cc:257: base::ListValue* enabled_list;
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: = NULL;

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manager...
chromeos/dbus/shill_manager_client.cc:290: virtual
ShillManagerClient::TestInterface* GetTestInterface() {
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_manager...
chromeos/dbus/shill_manager_client.cc:369: base::Value* value;
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: = NULL;

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_service...
File chromeos/dbus/shill_service_client.cc (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_service...
chromeos/dbus/shill_service_client.cc:156: virtual
ShillServiceClient::TestInterface* GetTestInterface() {
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_service...
chromeos/dbus/shill_service_client.cc:226: std::string ssid =
service_path.value();
On 2012/11/01 04:54:34, hashimoto wrote:
> Since this method is used not only for wifi networks, you might want to rename
> this variable to service_path_str or just use service_path.value() directly.
> Same goes for other methods.

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_service...
chromeos/dbus/shill_service_client.cc:285: virtual
ShillServiceClient::TestInterface* GetTestInterface() {
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_service...
chromeos/dbus/shill_service_client.cc:372: base::Value* value;
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: = NULL;

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/dbus/shill_service...
chromeos/dbus/shill_service_client.cc:384: base::DictionaryValue* properties;
On 2012/11/01 04:54:34, hashimoto wrote:
> nit: = NULL;

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/network/network_sm...
File chromeos/network/network_sms_handler_unittest.cc (right):

http://codereview.chromium.org/11365022/diff/3001/chromeos/network/network_sm...
chromeos/network/network_sms_handler_unittest.cc:61:
DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface();
On 2012/11/01 04:54:34, hashimoto wrote:
> ASSERT_TRUE(manager_test)?

Done.

http://codereview.chromium.org/11365022/diff/3001/chromeos/network/network_sm...
chromeos/network/network_sms_handler_unittest.cc:64:
DBusThreadManager::Get()->GetShillDeviceClient()->GetTestInterface();
On 2012/11/01 04:54:34, hashimoto wrote:
> ASSERT_TRUE(device_test)?

Done.

[hashimoto, 2012-11-02 02:06:46]

lgtm if ShillManagerClientStubImpl::EnableTechnology and DisableTechnology do
not have any severe problems. (see comments in shill_manager_client.cc)

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_devic...
File chromeos/dbus/shill_device_client.cc (right):

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_devic...
chromeos/dbus/shill_device_client.cc:252: return new base::DictionaryValue;
Could you make this method return the same value as GetProperties() in this
change or in a separate CL?

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:235: const ErrorCallback& error_callback)
OVERRIDE {
What if |type| is not present in kAvailableTechnologiesProperty?

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:256: const ErrorCallback& error_callback)
OVERRIDE {
ditto.

[stevenjb, 2012-11-02 02:43:25]

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_devic...
File chromeos/dbus/shill_device_client.cc (right):

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_devic...
chromeos/dbus/shill_device_client.cc:252: return new base::DictionaryValue;
On 2012/11/02 02:06:47, hashimoto wrote:
> Could you make this method return the same value as GetProperties() in this
> change or in a separate CL?

Done.

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_manag...
File chromeos/dbus/shill_manager_client.cc (right):

https://codereview.chromium.org/11365022/diff/13001/chromeos/dbus/shill_manag...
chromeos/dbus/shill_manager_client.cc:235: const ErrorCallback& error_callback)
OVERRIDE {
On 2012/11/02 02:06:47, hashimoto wrote:
> What if |type| is not present in kAvailableTechnologiesProperty?
It's a stub implementation for testing - it's a long way from being guaranteed
consistent :)
In the stub, the enabled list and available list are unrelated. I think that is
OK for testing.

[commit-bot: I haz the power, 2012-11-02 02:43:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11365022/13002

[hashimoto, 2012-11-02 03:12:45]

lgtm, thanks.

[commit-bot: I haz the power, 2012-11-02 06:27:39]

Change committed as 165633