Separate Authenticator and Session unittests.

Separate Authenticator and Session unittests.

Previously JingleSession unit tests were using real authenticators. Here
I changed them to always use FakeAuthenticator and added new tests for channel 
authentication in v1_authenticator_unittest.cc . Also, to make new tests pass,
fixed session-terminate message handling in JingleSession to return correct 
error code.

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

[Sergey Ulanov, 2011-12-03 00:31:19]

wez: ping

[Sergey Ulanov, 2011-12-09 23:24:24]

ping

[Wez, 2011-12-09 23:42:32]

Sorry that this took so long to get to. :(

It mostly looks good, but I think any non-trivial speed tests need moving out to
a benchmarking test-app for Chromoting (not in this CL, of course), and I've a
few suggestions for simplifying & generalising the FakeAuthenticator.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
File remoting/protocol/connection_tester.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.cc:116: if (!done_) {
It looks like this will cause any errors reported after we're "done" to be
ignored, which seems wrong; surely we should also test for valid end-of-stream
behaviour?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.cc:168: EXPECT_GT(packets_received_, 0);
You're testing for >0 on the basis that ==0 means nothing got through => broken,
but we have to expect some to get dropped?  That seems like a recipe for obscure
flake to me.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
File remoting/protocol/connection_tester.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:26: // This class is used by unit tests to
verify that connection between
nit: ... a connection ...

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:27: // two sockets works propertly.
typo: properly

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:27: // two sockets works propertly.
What do we mean by "works"?  We see a successful connection, we can exchange
data, or something else?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:28: class StreamConnectionTester {
Is there no general-purpose StreamSocket tester under net/?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:28: class StreamConnectionTester {
This might be better named StreamSocketTester?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:105: int broken_packets_;
nit: bad_packets_received_ would be less ambiguous; a "broken" packet could mean
one that got broken up into fragments in some settings!

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:26: done_callback.Run(net::OK, socket);
The ChannelAuthenticator allows asynchronous completion, and I think our current
implementation completes asynchronously, so this implementation should really
PostTask() completion, or better still test both the synchronous and
asynchronous completion cases.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:54: // Don't send the last message if
this is a host hat wants to reject
typo: hat -> that

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:14: class FakeChannelAuthenticator :
public ChannelAuthenticator {
nit: This is an implementation detail of FakeAuthenticator, so it needn't be in
this header.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:29: class FakeAuthenticator : public
Authenticator {
nit: This is an implementation detail of FakeAuthenticatorFactory, so needn't be
in this header.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:65: int round_trips);
If you replaced |round_trips| with |message_sequence|, an array consisting of
CLIENT / HOST entries, you could test alternate authentication sequences and the
number of entries in the sequence description would match the |messages_| count,
simplifying some of the implementation logic.

Actually, you could extend that to a general sequence of [CLIENT|HOST]_MSG,
[CLIENT|HOST]_REJECT, *_ACCEPT, *_ACCEPT_REJECT_CHANNEL and simplify a more of
the tester logic, I think.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.cc:82: std::vector<char>(buf->data(), buf->data()
+ buf_len)));
Can we always assume a MessageLoop where FakeSocket it used?  Or does the
(renamed to something else) Connect() need a comment to indicate that
requirement?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.cc:120: return net::OK;
Is this needed for this CL, or a while-you-were-here kind of cleanup?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:30: class FakeSocket : public net::StreamSocket
{
nit: It strikes me that FakeSocket should probably become a class of paired
instances, e.g with

CreateFakeSocketPair(&a, &b)

so you to "prime" a FakeSocket w/ input data you'd just write to the other
end...

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:38: void Connect(FakeSocket* peer_socket);
Isn't Connect() overloading net::StreamSocket::Connect()?  That seems bad/wrong?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:177: int not_connected_peers =
expect_fail ? 1 : 2;
Not clear on what this means; surely neither peer is connected if we fail? ;)

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:435: TEST_F(JingleSessionTest,
FLAKY_TestSpeed) {
I don't think this test belongs in a unit-test; it has no failure-mode except
(flaky) connection-setup, instead just printing timings, and by definition a
speed test needs to run for long enough to get reliable timings so it's too slow
for a unit test.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/v1_authent...
File remoting/protocol/v1_authenticator_unittest.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/v1_authent...
remoting/protocol/v1_authenticator_unittest.cc:34: const int kMessages = 100;
I understand the need to test that a successful connect gives a usable channel,
but why these specific numbers?  Why not fewer messages, or even just one
message in each direction, for example?

[Sergey Ulanov, 2011-12-12 22:52:00]

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
File remoting/protocol/connection_tester.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.cc:116: if (!done_) {
On 2011/12/09 23:42:33, Wez wrote:
> It looks like this will cause any errors reported after we're "done" to be
> ignored, which seems wrong; surely we should also test for valid end-of-stream
> behaviour?

There is no end-of-stream for P2P sockets: stream ether exists and should be
connected or it is destroyed. If one end closes connection then the other end
should be informed about it over the signalling channel and close it too. There
is Disconnect() method, but I don't see anything we can verify about it: Read()
and Write() are not supposed to be called after Disconnect() -
SSLClientSocketNSS DCHECK's if they are. In any case, the calling test can
verify end-of-stream behavior outside of this class if it needs to. The job of
this class is just to verify connectivity, but not correctness of a Socket
implementation in general.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.cc:168: EXPECT_GT(packets_received_, 0);
On 2011/12/09 23:42:33, Wez wrote:
> You're testing for >0 on the basis that ==0 means nothing got through =>
broken,
> but we have to expect some to get dropped?  That seems like a recipe for
obscure
> flake to me.

We send 100 packets with 10ms delay between packets to the same machine. It very
unlikely that all of them are dropped. We had this test for very long time and I
never saw it cause any flake.

You can apply the same argument to TCP connection: it is possible that a TCP
connection test never succeeds because the first SYN packets gets dropped every
time, but we need some connection tests nonetheless.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
File remoting/protocol/connection_tester.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:26: // This class is used by unit tests to
verify that connection between
On 2011/12/09 23:42:33, Wez wrote:
> nit: ... a connection ...

Done.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:27: // two sockets works propertly.
On 2011/12/09 23:42:33, Wez wrote:
> typo: properly

Done.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:27: // two sockets works propertly.
On 2011/12/09 23:42:33, Wez wrote:
> What do we mean by "works"?  We see a successful connection, we can exchange
> data, or something else?

Done.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:28: class StreamConnectionTester {
On 2011/12/09 23:42:33, Wez wrote:
> Is there no general-purpose StreamSocket tester under net/?
No, at least I didn't find one.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:28: class StreamConnectionTester {
On 2011/12/09 23:42:33, Wez wrote:
> This might be better named StreamSocketTester?

StreamSocketTester would imply that it tests socket implementation, but this
class tests connection between two sockets, i.e. it doesn't test all aspects of
StreamSocket (such as GetPeerAddress()), but verifies that to sockets are
connected.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.h:105: int broken_packets_;
On 2011/12/09 23:42:33, Wez wrote:
> nit: bad_packets_received_ would be less ambiguous; a "broken" packet could
mean
> one that got broken up into fragments in some settings!

Done.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:26: done_callback.Run(net::OK, socket);
On 2011/12/09 23:42:33, Wez wrote:
> The ChannelAuthenticator allows asynchronous completion, and I think our
current
> implementation completes asynchronously, so this implementation should really
> PostTask() completion, or better still test both the synchronous and
> asynchronous completion cases.

Done.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:54: // Don't send the last message if
this is a host hat wants to reject
On 2011/12/09 23:42:33, Wez wrote:
> typo: hat -> that

Done.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:14: class FakeChannelAuthenticator :
public ChannelAuthenticator {
On 2011/12/09 23:42:33, Wez wrote:
> nit: This is an implementation detail of FakeAuthenticator, so it needn't be
in
> this header.

It may be useful in some tests, and I don't think there is any harm in exposing
it here.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:29: class FakeAuthenticator : public
Authenticator {
On 2011/12/09 23:42:33, Wez wrote:
> nit: This is an implementation detail of FakeAuthenticatorFactory, so needn't
be
> in this header.

It is not an implementation detail. Tests needs to create authenticators without
factory for Connect() method.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:65: int round_trips);
On 2011/12/09 23:42:33, Wez wrote:
> If you replaced |round_trips| with |message_sequence|, an array consisting of
> CLIENT / HOST entries, you could test alternate authentication sequences and
the
> number of entries in the sequence description would match the |messages_|
count,
> simplifying some of the implementation logic.
> 
> Actually, you could extend that to a general sequence of [CLIENT|HOST]_MSG,
> [CLIENT|HOST]_REJECT, *_ACCEPT, *_ACCEPT_REJECT_CHANNEL and simplify a more of
> the tester logic, I think.

This would simplify implementation of fake authenticators but makes test code
more complicated. In the same time the case when more than one message is sent
in the same direction is not allowed, so I don't see any valid cases that are
not testable with the current approach.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.cc:82: std::vector<char>(buf->data(), buf->data()
+ buf_len)));
On 2011/12/09 23:42:33, Wez wrote:
> Can we always assume a MessageLoop where FakeSocket it used?  Or does the
> (renamed to something else) Connect() need a comment to indicate that
> requirement?

Both sockets must be on the same thread. added comments about it in
fake_session.h

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.cc:120: return net::OK;
On 2011/12/09 23:42:33, Wez wrote:
> Is this needed for this CL, or a while-you-were-here kind of cleanup?
Yes we need it for this CL. SSL layer needs this method to work, and with this
CL we now run SSL layer over FakeSocket in V1AuthenticatorTest

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:30: class FakeSocket : public net::StreamSocket
{
On 2011/12/09 23:42:33, Wez wrote:
> nit: It strikes me that FakeSocket should probably become a class of paired
> instances, e.g with
> 
> CreateFakeSocketPair(&a, &b)
> 
> so you to "prime" a FakeSocket w/ input data you'd just write to the other
> end...

That would make test code more complicated because calling Read() or Write()
methods in Socket interface isn't as simple as accessing written_data() and
AppendInputData() in this class. Also this class provides access to low-level
details such as read_pending() and input_pos() which wouldn't be available with
regular socket.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:38: void Connect(FakeSocket* peer_socket);
On 2011/12/09 23:42:33, Wez wrote:
> Isn't Connect() overloading net::StreamSocket::Connect()?  That seems
bad/wrong?
Renamed to SetPeer(), though I disagree that overloading is a problem in this
case because the two Connect() methods are easy to distinguish.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:177: int not_connected_peers =
expect_fail ? 1 : 2;
On 2011/12/09 23:42:33, Wez wrote:
> Not clear on what this means; surely neither peer is connected if we fail? ;)
It is a counter for QuitThreadOnCounter(). I agree that the name is not correct
anymore. In any case, we don't need it anymore, I removed it.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:435: TEST_F(JingleSessionTest,
FLAKY_TestSpeed) {
On 2011/12/09 23:42:33, Wez wrote:
> I don't think this test belongs in a unit-test; it has no failure-mode except
> (flaky) connection-setup, instead just printing timings, and by definition a
> speed test needs to run for long enough to get reliable timings so it's too
slow
> for a unit test.

Removed it from here.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/v1_authent...
File remoting/protocol/v1_authenticator_unittest.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/v1_authent...
remoting/protocol/v1_authenticator_unittest.cc:34: const int kMessages = 100;
On 2011/12/09 23:42:33, Wez wrote:
> I understand the need to test that a successful connect gives a usable
channel,
> but why these specific numbers?  Why not fewer messages, or even just one
> message in each direction, for example?

Done.

[Wez, 2011-12-13 00:12:38]

LGTM, but please see comments below.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
File remoting/protocol/connection_tester.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.cc:116: if (!done_) {
On 2011/12/12 22:52:00, sergeyu wrote:
> On 2011/12/09 23:42:33, Wez wrote:
> > It looks like this will cause any errors reported after we're "done" to be
> > ignored, which seems wrong; surely we should also test for valid
end-of-stream
> > behaviour?
> 
> There is no end-of-stream for P2P sockets: stream ether exists and should be
> connected or it is destroyed. If one end closes connection then the other end
> should be informed about it over the signalling channel and close it too.
There
> is Disconnect() method, but I don't see anything we can verify about it:
Read()
> and Write() are not supposed to be called after Disconnect() -
> SSLClientSocketNSS DCHECK's if they are. In any case, the calling test can
> verify end-of-stream behavior outside of this class if it needs to. The job of
> this class is just to verify connectivity, but not correctness of a Socket
> implementation in general.

You're right re EOF, but my point is that this code ignores errors occurring
after |done_| is set, which seems wrong; surely once we're "done" we teardown
and should not see errors?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.cc:168: EXPECT_GT(packets_received_, 0);
On 2011/12/12 22:52:00, sergeyu wrote:
> On 2011/12/09 23:42:33, Wez wrote:
> > You're testing for >0 on the basis that ==0 means nothing got through =>
> broken,
> > but we have to expect some to get dropped?  That seems like a recipe for
> obscure
> > flake to me.
> 
> We send 100 packets with 10ms delay between packets to the same machine. It
very
> unlikely that all of them are dropped. We had this test for very long time and
I
> never saw it cause any flake.

Fair enough.  Sending 100 packets with 10ms delay also means at least 1s to
complete the test, which isn't ideal.  Given that the test passes so long as at
least one message gets through, perhaps it should also complete as soon as the
first message gets through?

> You can apply the same argument to TCP connection: it is possible that a TCP
> connection test never succeeds because the first SYN packets gets dropped
every
> time, but we need some connection tests nonetheless.

True.  Assuming local loopback is reasonably reliable is fair, but we need to be
wary of weird behaviours under high load (i.e. try-bots).

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:65: int round_trips);
On 2011/12/12 22:52:00, sergeyu wrote:
> On 2011/12/09 23:42:33, Wez wrote:
> > If you replaced |round_trips| with |message_sequence|, an array consisting
of
> > CLIENT / HOST entries, you could test alternate authentication sequences and
> the
> > number of entries in the sequence description would match the |messages_|
> count,
> > simplifying some of the implementation logic.
> > 
> > Actually, you could extend that to a general sequence of [CLIENT|HOST]_MSG,
> > [CLIENT|HOST]_REJECT, *_ACCEPT, *_ACCEPT_REJECT_CHANNEL and simplify a more
of
> > the tester logic, I think.
> 
> This would simplify implementation of fake authenticators but makes test code
> more complicated. In the same time the case when more than one message is sent
> in the same direction is not allowed, so I don't see any valid cases that are
> not testable with the current approach.

Not sure what you mean "is not allowed"; the session implementations cope with
Authenticators that send multiple messages without receiving anything
in-between, right now?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:65: int round_trips);
On 2011/12/12 22:52:00, sergeyu wrote:
> On 2011/12/09 23:42:33, Wez wrote:
> > If you replaced |round_trips| with |message_sequence|, an array consisting
of
> > CLIENT / HOST entries, you could test alternate authentication sequences and
> the
> > number of entries in the sequence description would match the |messages_|
> count,
> > simplifying some of the implementation logic.
> > 
> > Actually, you could extend that to a general sequence of [CLIENT|HOST]_MSG,
> > [CLIENT|HOST]_REJECT, *_ACCEPT, *_ACCEPT_REJECT_CHANNEL and simplify a more
of
> > the tester logic, I think.
> 
> This would simplify implementation of fake authenticators but makes test code
> more complicated. In the same time the case when more than one message is sent
> in the same direction is not allowed, so I don't see any valid cases that are
> not testable with the current approach.

The extra complication to the individual tests is minimal, IMHO; just a case of
defining some auth-message sequences and passing them in?

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:38: void Connect(FakeSocket* peer_socket);
On 2011/12/12 22:52:00, sergeyu wrote:
> On 2011/12/09 23:42:33, Wez wrote:
> > Isn't Connect() overloading net::StreamSocket::Connect()?  That seems
> bad/wrong?
> Renamed to SetPeer(), though I disagree that overloading is a problem in this
> case because the two Connect() methods are easy to distinguish.

True, but the semantics of this Connect() method are non-obvious; does
connecting given an existing FakeSocket create an equivalent connection, or make
the two peers?  SetPeer() seems clearer to me.

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/fake_sess...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/fake_sess...
remoting/protocol/fake_session.h:35: //   b->SetPeer(a);
nit: You could actually have SetPeer() set the peer relationship in both
directions pretty easily... ;)

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/fake_sess...
remoting/protocol/fake_session.h:52: // net::Socket implementation.
nit: I prefer "interface" to "implementation"; the comment indicates that these
methods are from a particular interface.

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.cc:349:
CloseInternal(net::ERR_CONNECTION_ABORTED, OK);
This looks like a fix to JingleSession, presumably necessary for the tests to
pass, in which case it should be mentioned in the CL description.

[Sergey Ulanov, 2011-12-13 02:31:55]

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
File remoting/protocol/connection_tester.cc (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.cc:116: if (!done_) {
On 2011/12/13 00:12:38, Wez wrote:
> On 2011/12/12 22:52:00, sergeyu wrote:
> > On 2011/12/09 23:42:33, Wez wrote:
> > > It looks like this will cause any errors reported after we're "done" to be
> > > ignored, which seems wrong; surely we should also test for valid
> end-of-stream
> > > behaviour?
> > 
> > There is no end-of-stream for P2P sockets: stream ether exists and should be
> > connected or it is destroyed. If one end closes connection then the other
end
> > should be informed about it over the signalling channel and close it too.
> There
> > is Disconnect() method, but I don't see anything we can verify about it:
> Read()
> > and Write() are not supposed to be called after Disconnect() -
> > SSLClientSocketNSS DCHECK's if they are. In any case, the calling test can
> > verify end-of-stream behavior outside of this class if it needs to. The job
of
> > this class is just to verify connectivity, but not correctness of a Socket
> > implementation in general.
> 
> You're right re EOF, but my point is that this code ignores errors occurring
> after |done_| is set, which seems wrong; surely once we're "done" we teardown
> and should not see errors?

I see. I removed this if because we don't need it.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/connection...
remoting/protocol/connection_tester.cc:168: EXPECT_GT(packets_received_, 0);
On 2011/12/13 00:12:38, Wez wrote:
> On 2011/12/12 22:52:00, sergeyu wrote:
> > On 2011/12/09 23:42:33, Wez wrote:
> > > You're testing for >0 on the basis that ==0 means nothing got through =>
> > broken,
> > > but we have to expect some to get dropped?  That seems like a recipe for
> > obscure
> > > flake to me.
> > 
> > We send 100 packets with 10ms delay between packets to the same machine. It
> very
> > unlikely that all of them are dropped. We had this test for very long time
and
> I
> > never saw it cause any flake.
> 
> Fair enough.  Sending 100 packets with 10ms delay also means at least 1s to
> complete the test, which isn't ideal.  Given that the test passes so long as
at
> least one message gets through, perhaps it should also complete as soon as the
> first message gets through?

I think it's useful to send more than one message to verify that messages aren't
broken.

>
> > You can apply the same argument to TCP connection: it is possible that a TCP
> > connection test never succeeds because the first SYN packets gets dropped
> every
> > time, but we need some connection tests nonetheless.
> 
> True.  Assuming local loopback is reasonably reliable is fair, but we need to
be
> wary of weird behaviours under high load (i.e. try-bots).

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.h (right):

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:65: int round_trips);
On 2011/12/13 00:12:38, Wez wrote:
> On 2011/12/12 22:52:00, sergeyu wrote:
> > On 2011/12/09 23:42:33, Wez wrote:
> > > If you replaced |round_trips| with |message_sequence|, an array consisting
> of
> > > CLIENT / HOST entries, you could test alternate authentication sequences
and
> > the
> > > number of entries in the sequence description would match the |messages_|
> > count,
> > > simplifying some of the implementation logic.
> > > 
> > > Actually, you could extend that to a general sequence of
[CLIENT|HOST]_MSG,
> > > [CLIENT|HOST]_REJECT, *_ACCEPT, *_ACCEPT_REJECT_CHANNEL and simplify a
more
> of
> > > the tester logic, I think.
> > 
> > This would simplify implementation of fake authenticators but makes test
code
> > more complicated. In the same time the case when more than one message is
sent
> > in the same direction is not allowed, so I don't see any valid cases that
are
> > not testable with the current approach.
> 
> Not sure what you mean "is not allowed"; 

Authenticator state may not be set to MESSAGE_READY after
Authenticator::GetNextMessage() returns. This should be documented in
authenticator.h

> the session implementations cope with
> Authenticators that send multiple messages without receiving anything
> in-between, right now?

Yeah, I should probably change that code to DCHECK when Authenticator tries to
generates sequence of multiple messages.

http://codereview.chromium.org/8743023/diff/2001/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:65: int round_trips);
On 2011/12/13 00:12:38, Wez wrote:
> On 2011/12/12 22:52:00, sergeyu wrote:
> > On 2011/12/09 23:42:33, Wez wrote:
> > > If you replaced |round_trips| with |message_sequence|, an array consisting
> of
> > > CLIENT / HOST entries, you could test alternate authentication sequences
and
> > the
> > > number of entries in the sequence description would match the |messages_|
> > count,
> > > simplifying some of the implementation logic.
> > > 
> > > Actually, you could extend that to a general sequence of
[CLIENT|HOST]_MSG,
> > > [CLIENT|HOST]_REJECT, *_ACCEPT, *_ACCEPT_REJECT_CHANNEL and simplify a
more
> of
> > > the tester logic, I think.
> > 
> > This would simplify implementation of fake authenticators but makes test
code
> > more complicated. In the same time the case when more than one message is
sent
> > in the same direction is not allowed, so I don't see any valid cases that
are
> > not testable with the current approach.
> 
> The extra complication to the individual tests is minimal, IMHO; just a case
of
> defining some auth-message sequences and passing them in?

I've tried to do it in Patch Set 5. It saves 30 lines off FakeAuthenticator, yet
makes jingle_session_unittest.cc 38 lines longer, net loss of 8 lines :(. The
state tables would also have to be duplicated in pepper_session_unittest.cc. I
don't think any benefits this could give us are worth the time I wasted on it.

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/fake_sess...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/fake_sess...
remoting/protocol/fake_session.h:35: //   b->SetPeer(a);
On 2011/12/13 00:12:38, Wez wrote:
> nit: You could actually have SetPeer() set the peer relationship in both
> directions pretty easily... ;)

Done.

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/fake_sess...
remoting/protocol/fake_session.h:52: // net::Socket implementation.
On 2011/12/13 00:12:38, Wez wrote:
> nit: I prefer "interface" to "implementation"; the comment indicates that
these
> methods are from a particular interface.

Done.

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8743023/diff/16001/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.cc:349:
CloseInternal(net::ERR_CONNECTION_ABORTED, OK);
On 2011/12/13 00:12:38, Wez wrote:
> This looks like a fix to JingleSession, presumably necessary for the tests to
> pass, in which case it should be mentioned in the CL description.

Done.

[commit-bot: I haz the power, 2011-12-13 02:39:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8743023/20003

[commit-bot: I haz the power, 2011-12-13 03:54:41]

Change committed as 114178