code review 7308058: net: add unixgram dial test

net: add unixgram dial test

Also replaces testing.Errof with testing.Fatalf, make use of ICMP mock.

[mikio, 2013-02-08 01:50:09]

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-08 03:28:22]

Thank you, this is a nice cleanup. A few minor comments.

https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go
File src/pkg/net/conn_test.go (right):

https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#new...
src/pkg/net/conn_test.go:32: {"unix", testUnixAddr()},
nice

https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#new...
src/pkg/net/conn_test.go:91: switch ln.(type) {
        switch ln := ln.(type) {
        case *net.TCPListener:
                ln.SetDeadline(time.Now().Add(100 * time.Millisecond))
        case *net.UnixListener:
                ln.SetDeadline(time.Now().Add(100 * time.Millisecond))
        }

https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#new...
src/pkg/net/conn_test.go:99: t.Fatalf("net.Listener.Accept failed: %v", err)
cannot use Fatalf here, it is running in a goroutine

https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#new...
src/pkg/net/conn_test.go:100: }
maybe a comment here to explain why we call the next two functions and ignore
the results.

https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#new...
src/pkg/net/conn_test.go:111: t.Fatalf("net.Conn.Read failed: %v", err)
same

https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#new...
src/pkg/net/conn_test.go:114: t.Fatalf("net.Conn.Write failed: %v", err)
same

[mikio, 2013-02-08 12:28:55]

On Fri, Feb 8, 2013 at 12:28 PM,  <dave@cheney.net> wrote:

>
https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#new...
> src/pkg/net/conn_test.go:99: t.Fatalf("net.Listener.Accept failed: %v",
> err)
> cannot use Fatalf here, it is running in a goroutine

I thought so but after testing package re-read, now, wonder why?
Just tested attached, it looks tRunner handles TestABC well.

--
package net

import (
        "testing"
        "time"
)

func TestABC(t *testing.T) {
        go wrecker(t)
        time.Sleep(100 * time.Millisecond)
        t.Fatal("gone")
}

func wrecker(t *testing.T) {
        t.Fatal("clash")
}

[dave_cheney.net, 2013-02-08 12:33:31]

>
>>
https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#new...
>> src/pkg/net/conn_test.go:99: t.Fatalf("net.Listener.Accept failed: %v",
>> err)
>> cannot use Fatalf here, it is running in a goroutine
>
> I thought so but after testing package re-read, now, wonder why?
> Just tested attached, it looks tRunner handles TestABC well.

It used to be undocumented, but recently a note has been added to tell
people not to do Fatalf in a goroutine. Mostly it isn't a problem
because the Fatalf path is unlikely.

>
> --
> package net
>
> import (
>         "testing"
>         "time"
> )
>
> func TestABC(t *testing.T) {
>         go wrecker(t)
>         time.Sleep(100 * time.Millisecond)
>         t.Fatal("gone")
> }
>
> func wrecker(t *testing.T) {
>         t.Fatal("clash")
> }

[mikio, 2013-02-08 13:44:52]

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

Please take another look.

[dave_cheney.net, 2013-02-19 02:02:57]

LGTM, modulo the comments to move the defers closer to the place the resources
are allocated.

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/conn_test.go
File src/pkg/net/conn_test.go (right):

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/conn_test.go#new...
src/pkg/net/conn_test.go:107: defer c.Close()
please move line 107 above 102, so if c.LocalAddr() or c.RemoteAddr() panic, we
don't leak an fd.

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/packetconn_test.go
File src/pkg/net/packetconn_test.go (right):

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/packetconn_test....
src/pkg/net/packetconn_test.go:139: defer closer(c1, netstr[0], tt.addr1,
tt.addr2)
see previous comment

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/packetconn_test....
src/pkg/net/packetconn_test.go:150: defer c2.Close()
same

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/protoconn_test.go
File src/pkg/net/protoconn_test.go (right):

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/protoconn_test.g...
src/pkg/net/protoconn_test.go:258: defer c1.Close()
same

[mikio, 2013-03-03 07:00:04]

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

net: add unixgram dial test

Also replaces testing.Errof with testing.Fatalf, make use of ICMP mock.

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

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/conn_test.go
File src/pkg/net/conn_test.go (right):

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/conn_test.go#new...
src/pkg/net/conn_test.go:107: defer c.Close()
On 2013/02/19 02:02:57, dfc wrote:
> please move line 107 above 102, so if c.LocalAddr() or c.RemoteAddr() panic,
we
> don't leak an fd.

Done.

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/packetconn_test.go
File src/pkg/net/packetconn_test.go (right):

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/packetconn_test....
src/pkg/net/packetconn_test.go:139: defer closer(c1, netstr[0], tt.addr1,
tt.addr2)
On 2013/02/19 02:02:57, dfc wrote:
> see previous comment

Done.

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/protoconn_test.go
File src/pkg/net/protoconn_test.go (right):

https://codereview.appspot.com/7308058/diff/3004/src/pkg/net/protoconn_test.g...
src/pkg/net/protoconn_test.go:258: defer c1.Close()
On 2013/02/19 02:02:57, dfc wrote:
> same

Done.