Add positive and negative test cases for whether we...

Add positive and negative test cases for whether we
should resend a request when we read nothing or get
a read failure.

R=darin,eroman
BUG=2489
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2635

[wtc, 2008-09-25 23:40:09]

Darin, I'm not sure if turning the common code into a
member function of the "test fixture" class is the right
way to share code between two tests.

Darin, Eric, there are a lot of tests in this file.  Please
make sure the two negative tests at the end
(NonKeepAliveConnection*) are not redundant.  Also note that
the very last test case shows that we handle an EOF as a
HTTP/0.9 response of zero content length.  We could equally
well consider that a failure.  (IE7 does that.)

[wtc, 2008-09-26 00:10:26]

Another way to share code between the two
KeepAliveConnection* tests is to have one test
that first fails with ERR_CONNECTION_RESET
and then reads EOF.  I use two separate tests
because I seem to remember our testing philosophy
prefers tests that do just one thing.

What do you think?

[eroman, 2008-09-26 00:28:03]

LGTM.

> Another way to share code between the two
> KeepAliveConnection* tests is to have one test
> that first fails with ERR_CONNECTION_RESET
> and then reads EOF.  I use two separate tests
> because I seem to remember our testing philosophy
> prefers tests that do just one thing.

I like the separate test approach as you have it now, it is easy to follow. Not
a style expert though.

> Also note that
> the very last test case shows that we handle an EOF as a
> HTTP/0.9 response of zero content length.  We could equally
> well consider that a failure.  (IE7 does that.)

When I was testing this case on firefox for http/0.9 handling, they consider it
a 200 (and you get a blank page whose type is inferred as text/plain).

I personally prefer the IE style of giving an error.

If we want to change this policy, I am happy to take the bug (and can update the
tests later). IIRC safari also will give a network error, so there is at least
precedent.

http://codereview.chromium.org/4291/diff/1/2
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/4291/diff/1/2#newcode285
Line 285: EXPECT_EQ("xxxxxHTTP/1.1 404 Not Found\nServer: blah",
out.response_data);
There are a couple other places which use this pattern, would be nice to fix
those too. I annotated them with: "may want to fix this too." comment.

http://codereview.chromium.org/4291/diff/1/2#newcode370
Line 370: EXPECT_TRUE(response->headers->GetStatusLine() == "HTTP/1.1 200 OK");
may want to fix this too.

http://codereview.chromium.org/4291/diff/1/2#newcode375
Line 375: EXPECT_TRUE(response_data == kExpectedResponseData[i]);
may want to fix this too.

http://codereview.chromium.org/4291/diff/1/2#newcode420
Line 420: EXPECT_TRUE(response->headers->GetStatusLine() == "HTTP/1.0 200 OK");
may want to fix this too.

http://codereview.chromium.org/4291/diff/1/2#newcode425
Line 425: EXPECT_TRUE(response_data == "hello world");
may want to fix this too.

http://codereview.chromium.org/4291/diff/1/2#newcode486
Line 486: EXPECT_TRUE(response->headers->GetStatusLine() == "HTTP/1.1 200 OK");
may want to fix this too.

http://codereview.chromium.org/4291/diff/1/2#newcode491
Line 491: EXPECT_TRUE(response_data == kExpectedResponseData[i]);
may want to fix this too.

[wtc, 2008-09-26 18:14:23]

Eric, I fixed the six EXPECT_TRUE instances you pointed
out.  I also added a comment to note how various browsers
handle EOF on a non-keepalive connection.

Please review the new snapshot.  Thanks.

[eroman, 2008-09-26 18:17:12]

thanks! LGTM

[darin (slow to review), 2008-09-26 19:26:20]

nice, LGTM