code review 7369052: net: make use of testing.B.Skip and reflect.DeepEqual i...

net: make use of testing.B.Skip and reflect.DeepEqual in test

This CL addresses the comments on CL 7368046 that I've overlooked.

Update issue 4866.

[mikio, 2013-02-25 11:07:34]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2013-02-25 11:14:36]

Thank you for addressing these points. This is very close.

https://codereview.appspot.com/7369052/diff/3001/src/pkg/net/interface_test.go
File src/pkg/net/interface_test.go (right):

https://codereview.appspot.com/7369052/diff/3001/src/pkg/net/interface_test.g...
src/pkg/net/interface_test.go:13: // tests, or nil.
of nil if ... (please finish this sentence). 

it might be useful to explain why it does not return nil, err.

https://codereview.appspot.com/7369052/diff/3001/src/pkg/net/interface_test.g...
src/pkg/net/interface_test.go:122: b.Skip("loopback interface not found")
nice, thank you

[mikio, 2013-02-25 11:27:23]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2013-02-25 11:34:52]

LGTM. A few final comments.

https://codereview.appspot.com/7369052/diff/7002/src/pkg/net/interface_test.go
File src/pkg/net/interface_test.go (right):

https://codereview.appspot.com/7369052/diff/7002/src/pkg/net/interface_test.g...
src/pkg/net/interface_test.go:14: // found.
It returns nil if no suitable interface is found.

https://codereview.appspot.com/7369052/diff/7002/src/pkg/net/interface_test.g...
src/pkg/net/interface_test.go:121: ifi := loopbackInterface()
if you passed *testing.B into loopbackInterface() then you can use the skip
directly, and then you don't need to document returning nil form
loopbackInterface()

[mikio, 2013-02-25 11:42:52]

thanks for your review.

I guess we can use existing mock ICMP stuff and this
loopbackInterface for fixing fragile server, unicast and 
multicast listenner tests.

https://codereview.appspot.com/7369052/diff/7002/src/pkg/net/interface_test.go
File src/pkg/net/interface_test.go (right):

https://codereview.appspot.com/7369052/diff/7002/src/pkg/net/interface_test.g...
src/pkg/net/interface_test.go:121: ifi := loopbackInterface()
On 2013/02/25 11:34:52, dfc wrote:
> if you passed *testing.B into loopbackInterface() then you can use the skip
> directly, and then you don't need to document returning nil form
> loopbackInterface()

in that case we can't use loopbackInterface for other tests.

[dave_cheney.net, 2013-02-25 11:55:12]

> in that case we can't use loopbackInterface for other tests.

What if you made the signature something like this ?

func loopbackInterface(skipper interface{ Skip(...interface{}) } } *Interface {
... }

[mikio, 2013-02-25 12:45:44]

On Mon, Feb 25, 2013 at 8:55 PM,  <dave@cheney.net> wrote:

> What if you made the signature something like this ?
>
> func loopbackInterface(skipper interface{ Skip(...interface{}) } }
> *Interface { ... }

i'm not keen on that idea because it will conceal line decorator
of testing package, make it hard to locate the failure line.

[mikio, 2013-02-25 14:05:50]

*** Submitted as https://code.google.com/p/go/source/detail?r=cac5b17d8699 ***

net: make use of testing.B.Skip and reflect.DeepEqual in test

This CL addresses the comments on CL 7368046 that I've overlooked.

Update issue 4866.

R=golang-dev, dave
CC=golang-dev
https://codereview.appspot.com/7369052

https://codereview.appspot.com/7369052/diff/7002/src/pkg/net/interface_test.go
File src/pkg/net/interface_test.go (right):

https://codereview.appspot.com/7369052/diff/7002/src/pkg/net/interface_test.g...
src/pkg/net/interface_test.go:14: // found.
On 2013/02/25 11:34:52, dfc wrote:
> It returns nil if no suitable interface is found.

Done.