Refactoring in client_socket_pool_base_unittest.cc.

Refactoring in client_socket_pool_base_unittest.cc.

This is a preparation to add tests for limiting total number of sockets.

TEST=Covered by net_unittests.

http://crbug.com/15093

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

[Paweł Hajdan Jr., 2009-07-02 23:32:54]

I plan to refactor it even further in the future, to have an array of socket
groups and priorities, and expected order.

My goal would be something like:

const ConnectionInfo[] connections = {
 { "a", kDefaultPriority, 2 },
 { "a", kDefaultPriority, 1 }.
};

CreateAndVerifyConnections(connections, arraysize(connections));

I will probably also need to allow having different socket limits for each
tests. These two changes should make the final test (after adding tests for
total limit) much more readable. I tried to do it in current design and the file
just became horrible (and I couldn't easily write all the tests).

[Paweł Hajdan Jr., 2009-07-06 16:34:56]

ping!

[willchan no longer on Chromium, 2009-07-06 17:11:03]

This is a good cleanup.  Be careful with your suggested follow-up refactor.  I
think you'll be fine as long as you only refactor a few specific tests into that
format, but there are a few pitfalls with data driven tests (hinted at
http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Value_Parame...
and discussed more fully in internal Google wikis).

http://codereview.chromium.org/149159/diff/1/2
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/149159/diff/1/2#newcode297
Line 297: void CreateConnections(scoped_ptr<TestSocketRequest>* reqs, size_t
count) {
It strikes me as odd that you have |count| as a variable, since you don't seem
to use it everywhere.  Notably, this code does not seem to handle the case when
count < kMaxSocketsPerGroup, or count != (kMaxSocketsPerGroup +
kNumPendingRequests).

[Paweł Hajdan Jr., 2009-07-06 20:17:38]

On 2009/07/06 17:11:03, willchan wrote:
> This is a good cleanup.  Be careful with your suggested follow-up refactor.  I
> think you'll be fine as long as you only refactor a few specific tests into
that
> format, but there are a few pitfalls with data driven tests (hinted at
>
http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Value_Parame...
> and discussed more fully in internal Google wikis).

Do you have pointers to the specific wiki resources about the pitfalls? I tried
searching moma, but got nothing related.

> http://codereview.chromium.org/149159/diff/1/2
> File net/socket/client_socket_pool_base_unittest.cc (right):
> 
> http://codereview.chromium.org/149159/diff/1/2#newcode297
> Line 297: void CreateConnections(scoped_ptr<TestSocketRequest>* reqs, size_t
> count) {
> It strikes me as odd that you have |count| as a variable, since you don't seem
> to use it everywhere.  Notably, this code does not seem to handle the case
when
> count < kMaxSocketsPerGroup, or count != (kMaxSocketsPerGroup +
> kNumPendingRequests).

That's right. I just moved the current logic. I plan to make it more flexible in
the next round of refactoring.

I noticed you didn't say "LGTM" but "It's a good cleanup", but I committed just
few seconds earlier. If you have any comments, I'd do them in the next (and I
think final) round of refactoring here.

[willchan no longer on Chromium, 2009-07-06 21:38:04]

On 2009/07/06 20:17:38, Paweł Hajdan Jr. wrote:
> On 2009/07/06 17:11:03, willchan wrote:
> > This is a good cleanup.  Be careful with your suggested follow-up refactor. 
I
> > think you'll be fine as long as you only refactor a few specific tests into
> that
> > format, but there are a few pitfalls with data driven tests (hinted at
> >
>
http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Value_Parame...
> > and discussed more fully in internal Google wikis).
> 
> Do you have pointers to the specific wiki resources about the pitfalls? I
tried
> searching moma, but got nothing related.
> 
> > http://codereview.chromium.org/149159/diff/1/2
> > File net/socket/client_socket_pool_base_unittest.cc (right):
> > 
> > http://codereview.chromium.org/149159/diff/1/2#newcode297
> > Line 297: void CreateConnections(scoped_ptr<TestSocketRequest>* reqs, size_t
> > count) {
> > It strikes me as odd that you have |count| as a variable, since you don't
seem
> > to use it everywhere.  Notably, this code does not seem to handle the case
> when
> > count < kMaxSocketsPerGroup, or count != (kMaxSocketsPerGroup +
> > kNumPendingRequests).
> 
> That's right. I just moved the current logic. I plan to make it more flexible
in
> the next round of refactoring.
> 
> I noticed you didn't say "LGTM" but "It's a good cleanup", but I committed
just
> few seconds earlier. If you have any comments, I'd do them in the next (and I
> think final) round of refactoring here.

That's fine.  I was mainly waiting for the answer you provided.