code review 21360043: go.net/ipv4: fix flaky test cases

go.net/ipv4: fix flaky test cases

Fixes issue 5709.
Fixes issue 5811.

[mikio, 2013-11-17 06:26:10]

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

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

[mikio, 2013-11-20 03:15:09]

ping

[mikio, 2013-12-08 11:40:17]

ping

[gobot, 2013-12-20 16:26:05]

Replacing golang-dev with golang-codereviews.

[dfc, 2013-12-20 22:43:02]

Sorry it took so long to review this, 

re issue 5709: i am seeing this failure with my network disconnected

--- FAIL: TestUDPMulticastSockopt (0.00 seconds)
        multicastsockopt_test.go:121: ipv4.PacketConn.JoinGroup(224.0.0.249:0)
failed: setsockopt: no such device

[mikio, 2013-12-20 23:30:00]

On Sat, Dec 21, 2013 at 7:43 AM,  <dave@cheney.net> wrote:

> re issue 5709: i am seeing this failure with my network disconnected
>
> --- FAIL: TestUDPMulticastSockopt (0.00 seconds)
>         multicastsockopt_test.go:121:
> ipv4.PacketConn.JoinGroup(224.0.0.249:0) failed: setsockopt: no such
> device

which OS? what version? output of netstat -rn when you disconnected (and how)?
is that reproducible? (or it happens just after bootup?)

[dfc, 2013-12-20 23:31:24]

On Sat, Dec 21, 2013 at 10:30 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Sat, Dec 21, 2013 at 7:43 AM,  <dave@cheney.net> wrote:
>
>> re issue 5709: i am seeing this failure with my network disconnected
>>
>> --- FAIL: TestUDPMulticastSockopt (0.00 seconds)
>>         multicastsockopt_test.go:121:
>> ipv4.PacketConn.JoinGroup(224.0.0.249:0) failed: setsockopt: no such
>> device

> which OS? what version? output of netstat -rn when you disconnected (and
how)?
> is that reproducible? (or it happens just after bootup?)

linux/amd64, ubuntu 13.10

lucky(~) % netstat -rn
Kernel IP routing table
Destination     Gateway         Genmask         Flags   MSS Window  irtt Iface

I just turned off my network via the wifi widget in the top right hand
corner and ran the tests

[mikio, 2013-12-24 07:08:21]

ptal

[mikio, 2013-12-31 08:21:34]

ping

[dfc, 2013-12-31 10:23:32]

On 2013/12/31 08:21:34, mikio wrote:
> ping

Reviewing now. I'm sorry that it has taken me so long to review this. In the
future, please try to keep the diff as small as you can, 700+ lines changed is a
lot.

[dfc, 2013-12-31 10:48:05]

LGTM. Thank you for your patience.

There are a whole bunch of little nits which could be addressed in a followup CL
if you just want to submit this one before 2014 :)

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
File ipv4/multicastsockopt_test.go (right):

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
ipv4/multicastsockopt_test.go:20: {"ip4", ":icmp", "0.0.0.0", &net.IPAddr{IP:
net.IPv4(224, 0, 0, 250)}},  // see RFC 4727
please add the comment as discussed later in the review.

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
ipv4/multicastsockopt_test.go:28: ifi := loopbackInterface()
same

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
ipv4/multicastsockopt_test.go:55: ifi := loopbackInterface()
same

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
ipv4/multicastsockopt_test.go:84: ttl := 255
const ttl = 255

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go
File ipv4/unicast_test.go (right):

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:92: ifi := loopbackInterface()
if ifi := ... ; ifi == nil { // }

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:116: if err :=
p.SetWriteDeadline(time.Now().Add(time.Millisecond * 100)); err != nil {
nit: could you please write this as 100 * time.Millisecond.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:120: t.Fatalf("ipv4.PacketConn.WriteTo failed: %v", err)
ipv6 has a short write test, should it be added here as well ?

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:123: if err :=
p.SetReadDeadline(time.Now().Add(time.Millisecond * 100)); err != nil {
same

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:182: b := make([]byte, 128)
rb := make(...) // this is what the ipv6 tests are moving too.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:200: t.Fatalf("got type=%v, code=%v; expected type=%v,
code=%v", m.Type, m.Code, ipv4.ICMPTypeEchoReply, 0)
s/expected/want

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:214: ifi := loopbackInterface()
see above

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test.go
File ipv4/unicastsockopt_test.go (right):

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:20: ifi := loopbackInterface()
if ifi := ... ; ifi == nil { 

}

ifi isn't used in the scope of the test, just for the precondition.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:49: {"ip4", ":icmp", "127.0.0.1"},
please add a comment above this line to let the reader know that any tests past
this point will be skipped if not root. That is to say, if I reversed these
lines, the udp4 test wouldn't be run.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:57: ifi := loopbackInterface()
same

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:84: ifi := loopbackInterface()
same

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:115: t.Logf("skipping IP_TOS test on %q",
runtime.GOOS)
t.Skipf

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:117: if err := c.SetTOS(tos); err != nil {
does this need to be a default block ? can't it just be in the outer scope ?

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:127: ttl := 255
const ttl = 255

[mikio, 2013-12-31 13:26:07]

On Tue, Dec 31, 2013 at 7:48 PM,  <dave@cheney.net> wrote:

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
> ipv4/multicastsockopt_test.go:20: {"ip4", ":icmp", "0.0.0.0",
> &net.IPAddr{IP: net.IPv4(224, 0, 0, 250)}},  // see RFC 4727
> please add the comment as discussed later in the review.

??? what comment do you need? I have no plan to describe the detail of
each protocol stack implementation here.

[dfc, 2013-12-31 13:34:08]

Oh, maybe it was on the ipv6 review. 

Basically if you are not root then when the test hits that slice entry it will
skip the remainder of the test. Please add a comment to let people know that
they shouldn't blindly add new test cases to the struct but instead should add
them above that line to ensure they are run. 

> On 1 Jan 2014, at 0:26, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> 
> On Tue, Dec 31, 2013 at 7:48 PM,  <dave@cheney.net> wrote:
> 
>
https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
>> ipv4/multicastsockopt_test.go:20: {"ip4", ":icmp", "0.0.0.0",
>> &net.IPAddr{IP: net.IPv4(224, 0, 0, 250)}},  // see RFC 4727
>> please add the comment as discussed later in the review.
> 
> ??? what comment do you need? I have no plan to describe the detail of
> each protocol stack implementation here.

[mikio, 2013-12-31 13:45:34]

On Tue, Dec 31, 2013 at 10:34 PM, Dave Cheney <dave@cheney.net> wrote:

> Oh, maybe it was on the ipv6 review.

I don't understand.

> Basically if you are not root then when the test hits that slice entry it will
skip the remainder of the test. Please add a comment to let people know that
they shouldn't blindly add new test cases to the struct but instead should add
them above that line to ensure they are run.

Hm, sounds like t.Skip may be wrong here.

[dfc, 2013-12-31 13:48:39]

Maybe try, t.Errorf ; continue and see how that looks. 



> On 1 Jan 2014, at 0:45, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> 
>> On Tue, Dec 31, 2013 at 10:34 PM, Dave Cheney <dave@cheney.net> wrote:
>> 
>> Oh, maybe it was on the ipv6 review.
> 
> I don't understand.
> 
>> Basically if you are not root then when the test hits that slice entry it
will skip the remainder of the test. Please add a comment to let people know
that they shouldn't blindly add new test cases to the struct but instead should
add them above that line to ensure they are run.
> 
> Hm, sounds like t.Skip may be wrong here.

[mikio, 2014-04-27 10:27:08]

*** Submitted as
https://code.google.com/p/go/source/detail?r=c74c376caac2&repo=net ***

go.net/ipv4: fix flaky test cases

Fixes issue 5709.
Fixes issue 5811.

LGTM=dave
R=dave
CC=golang-codereviews
https://codereview.appspot.com/21360043

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
File ipv4/multicastsockopt_test.go (right):

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
ipv4/multicastsockopt_test.go:20: {"ip4", ":icmp", "0.0.0.0", &net.IPAddr{IP:
net.IPv4(224, 0, 0, 250)}},  // see RFC 4727
will address in the following cls, once cls for dragonfly landed

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
ipv4/multicastsockopt_test.go:28: ifi := loopbackInterface()
ditto

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
ipv4/multicastsockopt_test.go:55: ifi := loopbackInterface()
ditto

https://codereview.appspot.com/21360043/diff/140001/ipv4/multicastsockopt_tes...
ipv4/multicastsockopt_test.go:84: ttl := 255
On 2013/12/31 10:48:05, dfc wrote:
> const ttl = 255

Done.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go
File ipv4/unicast_test.go (right):

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:92: ifi := loopbackInterface()
thanks but no thanks, ifi will be used in later cls; for specifying output
interface

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:116: if err :=
p.SetWriteDeadline(time.Now().Add(time.Millisecond * 100)); err != nil {
On 2013/12/31 10:48:05, dfc wrote:
> nit: could you please write this as 100 * time.Millisecond.

Done.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:120: t.Fatalf("ipv4.PacketConn.WriteTo failed: %v", err)
in another cl

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:123: if err :=
p.SetReadDeadline(time.Now().Add(time.Millisecond * 100)); err != nil {
ditto

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:182: b := make([]byte, 128)
On 2013/12/31 10:48:05, dfc wrote:
> rb := make(...) // this is what the ipv6 tests are moving too.

Done.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:200: t.Fatalf("got type=%v, code=%v; expected type=%v,
code=%v", m.Type, m.Code, ipv4.ICMPTypeEchoReply, 0)
applying s/expected/want/ to all messages would be in another cl

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicast_test.go#newc...
ipv4/unicast_test.go:214: ifi := loopbackInterface()
ditto

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test.go
File ipv4/unicastsockopt_test.go (right):

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:20: ifi := loopbackInterface()
ditto

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:49: {"ip4", ":icmp", "127.0.0.1"},
ditto

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:57: ifi := loopbackInterface()
ditto

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:84: ifi := loopbackInterface()
ditto

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:115: t.Logf("skipping IP_TOS test on %q",
runtime.GOOS)
On 2013/12/31 10:48:05, dfc wrote:
> t.Skipf 

Done.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:117: if err := c.SetTOS(tos); err != nil {
On 2013/12/31 10:48:05, dfc wrote:
> does this need to be a default block ? can't it just be in the outer scope ?

Done.

https://codereview.appspot.com/21360043/diff/140001/ipv4/unicastsockopt_test....
ipv4/unicastsockopt_test.go:127: ttl := 255
On 2013/12/31 10:48:05, dfc wrote:
> const ttl = 255

Done.