Cleanup style nits in SSLClientSocketTest

Cleanup style nits in SSLClientSocketTest

R=wtc
BUG=none
TEST=SSLClientSocketTest*

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

[Ryan Sleevi, 2010-10-26 23:37:58]

@wtc: Would you mind taking a look?

Several of the style nits you pointed out in reviewing
http://codereview.chromium.org/3812007/show were common patterns in the existing
test.

This CL cleans them up consistently, rather than the few pointed out in the new
code I added.

Note, I believe there was also a BUG here, but it happened not to be hit because
of timing/implementation details.

http://codereview.chromium.org/4130005/diff/5001/6001
File net/socket/ssl_client_socket_unittest.cc (left):

http://codereview.chromium.org/4130005/diff/5001/6001#oldcode132
net/socket/ssl_client_socket_unittest.cc:132: 
This looks to be a bug in the unit test. It will accept OK or
ERR_CERT_DATE_INVALID when connecting to the expired host, rather than only
accepting ERR_IO_PENDING or ERR_CERT_DATE_INVALID.

Looks like it wasn't reached because Connect() returned ERR_IO_PENDING, so it
was always hitting the inner expectation.

[wtc, 2010-10-28 01:18:13]

Thanks for the CL.  It looks good except for the problem
marked with "IMPORTANT" below.

http://codereview.chromium.org/4130005/diff/5001/6001
File net/socket/ssl_client_socket_unittest.cc (left):

http://codereview.chromium.org/4130005/diff/5001/6001#oldcode132
net/socket/ssl_client_socket_unittest.cc:132: 
On 2010/10/26 23:37:58, rsleevi wrote:
> This looks to be a bug in the unit test. It will accept OK or
> ERR_CERT_DATE_INVALID when connecting to the expired host, rather than only
> accepting ERR_IO_PENDING or ERR_CERT_DATE_INVALID.

You're right!

http://codereview.chromium.org/4130005/diff/5001/6001
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/4130005/diff/5001/6001#newcode124
net/socket/ssl_client_socket_unittest.cc:124: EXPECT_FALSE(sock->IsConnected());
Hmm... we should remove this because the socket could be
connected now.  This could be a source of test flakiness.

http://codereview.chromium.org/4130005/diff/8001/9001#newcode133
net/socket/ssl_client_socket_unittest.cc:133: }
IMPORTANT: the socket is disconnected for SSLClientSocketMac
at this point.  I think it's because of the "break-on-auth",
so SSLClientSocketMac verifies the server cert in the middle
of the handshake rather than at the end.

Also, we may change SSLClientSocketNSS to disconnect the
socket in the future.  The user may take indefinitely long
to allow the certificate error, so we don't want to tie up
the connection.  (This is what IE does.)

[Ryan Sleevi, 2010-11-07 23:40:51]

PTAL

http://codereview.chromium.org/4130005/diff/5001/6001
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/4130005/diff/5001/6001#newcode124
net/socket/ssl_client_socket_unittest.cc:124: EXPECT_FALSE(sock->IsConnected());
On 2010/10/28 01:18:13, wtc wrote:
> Hmm... we should remove this because the socket could be
> connected now.  This could be a source of test flakiness.

Good catch! I've removed it now for all of the tests that had it - now it will
simply wait for ERR_IO_PENDING results and then continue with the tests as
normal.

http://codereview.chromium.org/4130005/diff/8001/9001#newcode133
net/socket/ssl_client_socket_unittest.cc:133: }
On 2010/10/28 01:18:14, wtc wrote:
> IMPORTANT: the socket is disconnected for SSLClientSocketMac
> at this point.  I think it's because of the "break-on-auth",
> so SSLClientSocketMac verifies the server cert in the middle
> of the handshake rather than at the end.
> 
> Also, we may change SSLClientSocketNSS to disconnect the
> socket in the future.  The user may take indefinitely long
> to allow the certificate error, so we don't want to tie up
> the connection.  (This is what IE does.)

This wasn't the case when I tested on Mac, both with SSLClientSocketNSS and by
foring SSLClientSocketMac. I believe this is due to the rewrite of the state
machine, which has landed, which now closely mirrors that of the Win/NSS (it no
longer does break on auth).

However, to be on the safe side, I changed the expectation back to what it was,
with a comment explaining.

[wtc, 2010-11-10 01:05:50]

LGTM.

I had trouble doing good code review today, so please check
this CL carefully yourself before checking it in.  Thanks!

http://codereview.chromium.org/4130005/diff/15001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_unittest.cc (left):

http://codereview.chromium.org/4130005/diff/15001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:170: EXPECT_FALSE(sock->IsConnected());
On second thought, I think these two EXPECT statements (lines
170-171) are correct because of the way our message-driven IO
works -- after the sock->Connect() call on line 164 returns,
we have not given the current message loop a chance to process
more messages/run any tasks, so the state of 'sock' cannot
possibly change, and no new events will have been written to
the log.

But this point is not obvious, and is not important, so
I don't think we should add these two EXPECT statements back.

http://codereview.chromium.org/4130005/diff/15001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/4130005/diff/15001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:121: // Rather than testing whether or
not the underlying socket, test that the
Nit: "whether or not the underlying socket" is incomplete.
I guess it's missing "is connected" at the end?

http://codereview.chromium.org/4130005/diff/15001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_unittest.cc:338: if (rv != net::OK)
BUG: this should say
  if (rv == net::ERR_IO_PENDING)