SPDY - unforked spdy_framer and spdy_protocol tests.

SPDY - unkorked spdy_framer and spdy_protocol tests.
Used TEST_P to parameterize SPDY/2 vs SPDY/3 versions
so that spdy_framer_test and spdy_protocol_test are similar
to the server code.

BUG=117755
R=rch
TEST=network unit tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126839

[Ryan Hamilton, 2012-03-13 19:59:12]

Yes, this looks good to me.  I'm not a huge fan of having these two tests
(_framer and _protocol) being parameterized while the rest of our tests are
forked.  Perhaps we'll parameterize all of them later :>

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_framer...
File net/spdy/spdy_framer_test.cc (right):

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_framer...
net/spdy/spdy_framer_test.cc:17: int SPDY_VERSION_FOR_TESTS = 2;
I think I would make these member variables instead of globals.  I would also
change the names, but I understand the desire to use the same names as the
server code as so I'm ok if you keep them the same.

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_protoc...
File net/spdy/spdy_protocol_test.cc (right):

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_protoc...
net/spdy/spdy_protocol_test.cc:48: int SPDY_VERSION_FOR_TESTS = 2;
Ditto on my advice from the other test about making this a member variable.

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_protoc...
net/spdy/spdy_protocol_test.cc:66: class SpdyProtocolDeathTest
I might be missing something, but if this class is identical to
SpdyProtocolTest, is there a reason we need to have 2 different classes?

(And if we really need it to be a distinct class perhaps we can just do:

class SpdyProtocolDeathTest : public SpdyProtocolTest {};

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_protoc...
net/spdy/spdy_protocol_test.cc:84: TEST(SpdyProtocolTest, ProtocolConstants) {
Can the various TEST()s be replaced with TEST_P()s?

[ramant (doing other things), 2012-03-13 21:43:02]

Made all the changes you had suggested.

Thanks very much Ryan,
raman

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_framer...
File net/spdy/spdy_framer_test.cc (right):

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_framer...
net/spdy/spdy_framer_test.cc:17: int SPDY_VERSION_FOR_TESTS = 2;
On 2012/03/13 19:59:12, Ryan Hamilton wrote:
> I think I would make these member variables instead of globals.  I would also
> change the names, but I understand the desire to use the same names as the
> server code as so I'm ok if you keep them the same.

Done.

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_protoc...
File net/spdy/spdy_protocol_test.cc (right):

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_protoc...
net/spdy/spdy_protocol_test.cc:48: int SPDY_VERSION_FOR_TESTS = 2;
On 2012/03/13 19:59:12, Ryan Hamilton wrote:
> Ditto on my advice from the other test about making this a member variable.

Done.

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_protoc...
net/spdy/spdy_protocol_test.cc:66: class SpdyProtocolDeathTest
On 2012/03/13 19:59:12, Ryan Hamilton wrote:
> I might be missing something, but if this class is identical to
> SpdyProtocolTest, is there a reason we need to have 2 different classes?
> 
> (And if we really need it to be a distinct class perhaps we can just do:
> 
> class SpdyProtocolDeathTest : public SpdyProtocolTest {};

Done.

https://chromiumcodereview.appspot.com/9689078/diff/1009/net/spdy/spdy_protoc...
net/spdy/spdy_protocol_test.cc:84: TEST(SpdyProtocolTest, ProtocolConstants) {
On 2012/03/13 19:59:12, Ryan Hamilton wrote:
> Can the various TEST()s be replaced with TEST_P()s?

Done.

[Ryan Hamilton, 2012-03-14 17:32:07]

https://chromiumcodereview.appspot.com/9689078/diff/1012/net/spdy/spdy_framer...
File net/spdy/spdy_framer_test.cc (right):

https://chromiumcodereview.appspot.com/9689078/diff/1012/net/spdy/spdy_framer...
net/spdy/spdy_framer_test.cc:19: unsigned char kVer = 2;
Please eliminate this too.  Using spdy_version_ instead of kVer seems
reasonable, to me.

https://chromiumcodereview.appspot.com/9689078/diff/1012/net/spdy/spdy_framer...
net/spdy/spdy_framer_test.cc:547: // Default SPDY version for unit tests.
// Version of SPDY protocol to be used

https://chromiumcodereview.appspot.com/9689078/diff/1012/net/spdy/spdy_framer...
net/spdy/spdy_framer_test.cc:612: if (spdy_version_ < 3) {
I notice that this same predicate is used repeatedly in the test (spdy_version_
< 3).  It might be worth adding a method IsSpdy2() or IsSpdy3() to
SpdyFramerTest and using that instead.  You might want/need to do that server
side first, so feel free to do it in another CL if you wish.

https://chromiumcodereview.appspot.com/9689078/diff/1012/net/spdy/spdy_protoc...
File net/spdy/spdy_protocol_test.cc (right):

https://chromiumcodereview.appspot.com/9689078/diff/1012/net/spdy/spdy_protoc...
net/spdy/spdy_protocol_test.cc:61: // Default SPDY version for unit tests.
// Version of the SPDY protocol to be used

https://chromiumcodereview.appspot.com/9689078/diff/1012/net/spdy/spdy_protoc...
net/spdy/spdy_protocol_test.cc:130: EXPECT_EQ((spdy_version_ < 3) ? 2 : 4,
syn_frame->header_block_len());
Same comment about have an IsSpdy2() method.

[rtenneti, 2012-03-15 02:24:12]

Hi Ryan,
  Made the changes you have suggested. Will submit these changes to server also.

thanks,
raman

[Ryan Hamilton, 2012-03-15 02:48:29]

lgtm

[ramant (doing other things), 2012-03-15 04:36:04]

thanks Ryan.

http://codereview.chromium.org/9689078/diff/1012/net/spdy/spdy_protocol_test.cc
File net/spdy/spdy_protocol_test.cc (right):

http://codereview.chromium.org/9689078/diff/1012/net/spdy/spdy_protocol_test....
net/spdy/spdy_protocol_test.cc:61: // Default SPDY version for unit tests.
On 2012/03/14 17:32:08, Ryan Hamilton wrote:
> // Version of the SPDY protocol to be used

Done.