Modify DeterministicSocketData to verify that the sequence number of reads and writes start at zero…

Modify DeterministicSocketData to verify that the sequence number of reads and writes start at zero and are continuous.


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

[Ryan Hamilton, 2012-07-18 17:33:08]

sleevi: can you take a look?  We talked about making this change over lunch last
week (or so).  It turned out to be pretty simple, though there were a couple of
tests that needed to be cleaned up a bit in order to pass.

[Ryan Sleevi, 2012-07-18 18:02:13]

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/socket/socket_t...
File net/socket/socket_test_util.cc (right):

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/socket/socket_t...
net/socket/socket_test_util.cc:267: EXPECT_EQ(write, write_count_);
Rather than EXPECT_TRUE(false) [which is really ADD_FAILURE()] and EXPECT_EQ, I
think it would be better to DCHECK and NOTREACHED()

These aren't failures within the test, these are failures with the code itself.
We already DCHECK on certain situations (eg: see PeekRead/PeakWrite()), and I
think the quicker we surface to the developer that they've made a programming
error, the better.

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/socket/socket_t...
File net/socket/socket_test_util.h (right):

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/socket/socket_t...
net/socket/socket_test_util.h:223: MockWrite* writes, size_t writes_count);
Is there a reason this isn't the default behaviour?

It seems troublesome to introduce an overloaded constructor that differs on only
the bool, and it's not clear from this CL what it buys. After all, the omitted
path is the same as verify_sequence_numbers == false

Alternatively, if this is only for DeterministicSocketDataProvider, I would move
this method there and have it handle checking inputs itself.

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/spdy/spdy_netwo...
File net/spdy/spdy_network_transaction_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/spdy/spdy_netwo...
net/spdy/spdy_network_transaction_spdy2_unittest.cc:5041: LOG(INFO) << "Here";
unnecessary?

[Ryan Hamilton, 2012-07-18 18:45:06]

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/socket/socket_t...
File net/socket/socket_test_util.cc (right):

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/socket/socket_t...
net/socket/socket_test_util.cc:267: EXPECT_EQ(write, write_count_);
On 2012/07/18 18:02:13, Ryan Sleevi wrote:
> Rather than EXPECT_TRUE(false) [which is really ADD_FAILURE()] and EXPECT_EQ,
I
> think it would be better to DCHECK and NOTREACHED()
> 
> These aren't failures within the test, these are failures with the code
itself.
> We already DCHECK on certain situations (eg: see PeekRead/PeakWrite()), and I
> think the quicker we surface to the developer that they've made a programming
> error, the better.

Done.  Fair enough.  Using EXPECT happened to be super handy when I was working
on this feature so I could see how many tests were afflicted by this
anti-pattern, but that's not really relevant anymore.

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/socket/socket_t...
File net/socket/socket_test_util.h (right):

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/socket/socket_t...
net/socket/socket_test_util.h:223: MockWrite* writes, size_t writes_count);
On 2012/07/18 18:02:13, Ryan Sleevi wrote:
> Is there a reason this isn't the default behaviour?

It's not the default behavior because StaticSocketDataProvider doesn't really
need sequence numbers.  It's really only the Deterministic version which does. 
(And also Ordered, but that is evil).

> It seems troublesome to introduce an overloaded constructor that differs on
only
> the bool, and it's not clear from this CL what it buys. After all, the omitted
> path is the same as verify_sequence_numbers == false

Fair enough, though I did not want to change all of the places where we
construct StaticSocketDataProviders in all the tests.
 
> Alternatively, if this is only for DeterministicSocketDataProvider, I would
move
> this method there and have it handle checking inputs itself.

Ah, ok.  So I actually started off trying to do that, but reads_ and writes_ are
private, not protected so they are not visible to Deterministic.  How bout I
simply make the Verify() method protected, and call it in the Deterministic
constructor.

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/spdy/spdy_netwo...
File net/spdy/spdy_network_transaction_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10795012/diff/2001/net/spdy/spdy_netwo...
net/spdy/spdy_network_transaction_spdy2_unittest.cc:5041: LOG(INFO) << "Here";
On 2012/07/18 18:02:13, Ryan Sleevi wrote:
> unnecessary?

Done.

[Ryan Sleevi, 2012-07-18 19:14:01]

You have the variables as arguments - can you not just pass them during
verification?

https://chromiumcodereview.appspot.com/10795012/diff/8001/net/socket/socket_t...
File net/socket/socket_test_util.cc (right):

https://chromiumcodereview.appspot.com/10795012/diff/8001/net/socket/socket_t...
net/socket/socket_test_util.cc:477: VerifyCorrectSequenceNumbers();
You have access to |reads|, |read_count|, |writes|, |write_count| here (they're
all arguments).

Just pass them to VerifyCorrectSequencedNumbers, rather than relying on
member-var access.

[Ryan Hamilton, 2012-07-18 21:58:20]

https://chromiumcodereview.appspot.com/10795012/diff/8001/net/socket/socket_t...
File net/socket/socket_test_util.cc (right):

https://chromiumcodereview.appspot.com/10795012/diff/8001/net/socket/socket_t...
net/socket/socket_test_util.cc:477: VerifyCorrectSequenceNumbers();
On 2012/07/18 19:14:01, Ryan Sleevi wrote:
> You have access to |reads|, |read_count|, |writes|, |write_count| here
(they're
> all arguments).
> 
> Just pass them to VerifyCorrectSequencedNumbers, rather than relying on
> member-var access.

*facepalm*  Duh!  Obviously :>

Thanks.  Done.

[Ryan Hamilton, 2012-07-20 16:31:18]

On 2012/07/18 21:58:20, Ryan Hamilton wrote:
>
https://chromiumcodereview.appspot.com/10795012/diff/8001/net/socket/socket_t...
> File net/socket/socket_test_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10795012/diff/8001/net/socket/socket_t...
> net/socket/socket_test_util.cc:477: VerifyCorrectSequenceNumbers();
> On 2012/07/18 19:14:01, Ryan Sleevi wrote:
> > You have access to |reads|, |read_count|, |writes|, |write_count| here
> (they're
> > all arguments).
> > 
> > Just pass them to VerifyCorrectSequencedNumbers, rather than relying on
> > member-var access.
> 
> *facepalm*  Duh!  Obviously :>
> 
> Thanks.  Done.

*ping*

[Ryan Sleevi, 2012-07-20 16:43:23]

On 2012/07/20 16:31:18, Ryan Hamilton wrote:
> On 2012/07/18 21:58:20, Ryan Hamilton wrote:
> >
>
https://chromiumcodereview.appspot.com/10795012/diff/8001/net/socket/socket_t...
> > File net/socket/socket_test_util.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10795012/diff/8001/net/socket/socket_t...
> > net/socket/socket_test_util.cc:477: VerifyCorrectSequenceNumbers();
> > On 2012/07/18 19:14:01, Ryan Sleevi wrote:
> > > You have access to |reads|, |read_count|, |writes|, |write_count| here
> > (they're
> > > all arguments).
> > > 
> > > Just pass them to VerifyCorrectSequencedNumbers, rather than relying on
> > > member-var access.
> > 
> > *facepalm*  Duh!  Obviously :>
> > 
> > Thanks.  Done.
> 
> *ping*

I am on the fence as to whether there should be a newline between reads and
read_count and writes and write_count, but it seems the style guide permits
omitting them.

Change LGTM

[commit-bot: I haz the power, 2012-07-20 17:51:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10795012/12011

[commit-bot: I haz the power, 2012-07-20 17:51:39]

Failed to apply patch for net/spdy/spdy_network_transaction_spdy2_unittest.cc:
While running patch -p1 --forward --force;
patching file net/spdy/spdy_network_transaction_spdy2_unittest.cc
Hunk #1 succeeded at 4879 (offset -96 lines).
Hunk #2 FAILED at 5024.
Hunk #3 succeeded at 4952 (offset -97 lines).
Hunk #4 succeeded at 5034 (offset -97 lines).
Hunk #5 FAILED at 5180.
Hunk #6 succeeded at 5115 (offset -98 lines).
2 out of 6 hunks FAILED -- saving rejects to file
net/spdy/spdy_network_transaction_spdy2_unittest.cc.rej

[commit-bot: I haz the power, 2012-07-20 18:01:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10795012/5003

[commit-bot: I haz the power, 2012-07-20 18:48:06]

Try job failure for 10795012-5003 (retry) on linux_rel for step "net_unittests".
It's a second try, previously, step "net_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-20 23:22:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10795012/2004

[commit-bot: I haz the power, 2012-07-21 00:35:41]

Change committed as 147749