unittests for Chromoting native messaging host.

unittests for Chromoting native messaging host.


BUG=232200
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203084

[Sergey Ulanov, 2013-05-18 01:59:31]

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/mock_d...
File remoting/host/setup/mock_daemon_controller.h (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/mock_d...
remoting/host/setup/mock_daemon_controller.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/mock_d...
remoting/host/setup/mock_daemon_controller.h:13: class MockDaemonController :
public DaemonController {
This class is used in only one place, so maybe move it to
native_messaging_host_unittest.cc? Also why not use GMock for it?

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_host.h (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host.h:43: void SetDaemonControllerForTest(
can daemon_controller be passed to the constructor? Then you wouldn't need this
method.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:18: #include
"remoting/host/setup/native_messaging_host.h"
nit: this should be first include for this file.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:122:
scoped_ptr<base::DictionaryValue> ReadMessageFromFile(
Can you use FileStreams to read from files?

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:314:
VerifyStopDaemonResponse(response.get());
This doesn't really verify that daemon controller was invoked with correct
parameters.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:328:
TEST_F(NativeMessagingHostTest, Id) {
Please add short description for each test, e.g.
 // Verify that response Id matches request Id.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_reader_unittest.cc (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_reader_unittest.cc:13: #include
"remoting/host/setup/native_messaging_reader.h"
nit: this should be first include in the file.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_reader_unittest.cc:36: base::FilePath
temp_file_path() const { return temp_file_path_; }
nit: make the corresponding fields protected, then you wouldn't need these
methods.

https://codereview.chromium.org/14979008/diff/2001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/remoting.gyp#newc...
remoting/remoting.gyp:2641: 'host/setup/native_messaging_host.cc',
now these files are compiled twice. Move them to remoting_host_setup_base and
add dependency from remoting_host_setup_base to the unittests.

[Lambros, 2013-05-22 21:42:18]

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/mock_d...
File remoting/host/setup/mock_daemon_controller.h (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/mock_d...
remoting/host/setup/mock_daemon_controller.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> nit: 2013

Done.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/mock_d...
remoting/host/setup/mock_daemon_controller.h:13: class MockDaemonController :
public DaemonController {
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> This class is used in only one place, so maybe move it to
> native_messaging_host_unittest.cc? Also why not use GMock for it?
Moved mock class into unittest.cc. I'm not sure if GMock works so well, since we
have so many Callbacks and scoped_ptr<DictionaryValue> parameters. I'm still
open to persuasion, though :)

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_host.h (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host.h:43: void SetDaemonControllerForTest(
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> can daemon_controller be passed to the constructor? Then you wouldn't need
this
> method.

Done.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:18: #include
"remoting/host/setup/native_messaging_host.h"
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> nit: this should be first include for this file.

Done.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:122:
scoped_ptr<base::DictionaryValue> ReadMessageFromFile(
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> Can you use FileStreams to read from files?
Probably, but not sure if there's any advantage, or any less code to type?

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:314:
VerifyStopDaemonResponse(response.get());
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> This doesn't really verify that daemon controller was invoked with correct
> parameters.
I've improved the mock object to try to address this.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:328:
TEST_F(NativeMessagingHostTest, Id) {
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> Please add short description for each test, e.g.
>  // Verify that response Id matches request Id.

Done.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_reader_unittest.cc (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_reader_unittest.cc:13: #include
"remoting/host/setup/native_messaging_reader.h"
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> nit: this should be first include in the file.

Done.

https://codereview.chromium.org/14979008/diff/2001/remoting/host/setup/native...
remoting/host/setup/native_messaging_reader_unittest.cc:36: base::FilePath
temp_file_path() const { return temp_file_path_; }
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> nit: make the corresponding fields protected, then you wouldn't need these
> methods.

Done.

https://codereview.chromium.org/14979008/diff/2001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/14979008/diff/2001/remoting/remoting.gyp#newc...
remoting/remoting.gyp:2641: 'host/setup/native_messaging_host.cc',
On 2013/05/18 01:59:31, Sergey Ulanov wrote:
> now these files are compiled twice. Move them to remoting_host_setup_base and
> add dependency from remoting_host_setup_base to the unittests.

Done.

[Sergey Ulanov, 2013-05-22 23:20:47]

lgtm

[commit-bot: I haz the power, 2013-05-22 23:27:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/14979008/2...

[commit-bot: I haz the power, 2013-05-23 07:05:38]

Retried try job too often on win_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Lambros, 2013-05-25 01:06:32]

Rewritten tests to use pipes instead of file IO, since it's likely to be less
flaky (and hopefully fixes the win trybot errors). This is quite a big change,
PTAL.

Also updated for the interface changes in http://crrev.com/202029 .

[Lambros, 2013-05-29 17:44:38]

On 2013/05/25 01:06:32, Lambros wrote:
> Rewritten tests to use pipes instead of file IO, since it's likely to be less
> flaky (and hopefully fixes the win trybot errors). This is quite a big change,
> PTAL.
> 
> Also updated for the interface changes in http://crrev.com/202029 .

ping?

[Sergey Ulanov, 2013-05-29 19:54:43]

lgtm

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/pipe.cc
File remoting/host/setup/pipe.cc (right):

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/pipe....
remoting/host/setup/pipe.cc:18: return CreatePipe(read_handle, write_handle,
NULL, 0) != FALSE;
FYI: Anonymous pipes on windows may have different semantics from named pipes
that are used for native messaging (e.g. they don't support overlapped IO and
don't support ReadFileEx() and WriteFileEx()). It probably doesn't matter so
it's fine to keep it as is.

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/pipe.h
File remoting/host/setup/pipe.h (right):

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/pipe....
remoting/host/setup/pipe.h:5: #ifndef REMOTING_HOST_SETUP_PIPE_H_
maybe better to call it test_util.h? This would make it clear that this file is
used for test only

[Sergey Ulanov, 2013-05-29 19:59:31]

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/nativ...
File remoting/host/setup/native_messaging_writer_unittest.cc (right):

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_writer_unittest.cc:75: EXPECT_EQ(0, read);
replace this with read <= 0 because the result returned here depends on OS.

[Lambros, 2013-05-29 23:50:22]

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/nativ...
File remoting/host/setup/native_messaging_writer_unittest.cc (right):

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_writer_unittest.cc:75: EXPECT_EQ(0, read);
On 2013/05/29 19:59:31, Sergey Ulanov wrote:
> replace this with read <= 0 because the result returned here depends on OS.

Done.

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/pipe.cc
File remoting/host/setup/pipe.cc (right):

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/pipe....
remoting/host/setup/pipe.cc:18: return CreatePipe(read_handle, write_handle,
NULL, 0) != FALSE;
On 2013/05/29 19:54:43, Sergey Ulanov wrote:
> FYI: Anonymous pipes on windows may have different semantics from named pipes
> that are used for native messaging (e.g. they don't support overlapped IO and
> don't support ReadFileEx() and WriteFileEx()). It probably doesn't matter so
> it's fine to keep it as is.
ok thanks for the info, I'll keep as-is.

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/pipe.h
File remoting/host/setup/pipe.h (right):

https://codereview.chromium.org/14979008/diff/56001/remoting/host/setup/pipe....
remoting/host/setup/pipe.h:5: #ifndef REMOTING_HOST_SETUP_PIPE_H_
On 2013/05/29 19:54:43, Sergey Ulanov wrote:
> maybe better to call it test_util.h? This would make it clear that this file
is
> used for test only

Done.

[commit-bot: I haz the power, 2013-05-29 23:51:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/14979008/6...

[commit-bot: I haz the power, 2013-05-30 05:20:31]

Change committed as 203084