Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(231)

Issue 7308058: code review 7308058: net: add unixgram dial test (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by mikio
Modified:
12 years ago
Reviewers:
CC:
golang-dev, dave_cheney.net
Visibility:
Public.

Description

net: add unixgram dial test Also replaces testing.Errof with testing.Fatalf, make use of ICMP mock.

Patch Set 1 : diff -r aba17f1b93db https://code.google.com/p/go #

Patch Set 2 : diff -r 052cab218361 https://code.google.com/p/go #

Total comments: 6

Patch Set 3 : diff -r 67fd3609446a https://code.google.com/p/go #

Total comments: 7

Patch Set 4 : diff -r bcf28085191a https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+151 lines, -184 lines) Patch
M src/pkg/net/conn_test.go View 1 2 3 4 chunks +18 lines, -16 lines 0 comments Download
M src/pkg/net/packetconn_test.go View 1 2 3 5 chunks +57 lines, -27 lines 0 comments Download
M src/pkg/net/protoconn_test.go View 1 2 3 13 chunks +76 lines, -141 lines 0 comments Download

Messages

Total messages: 7
mikio
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
12 years, 1 month ago (2013-02-08 01:50:09 UTC) #1
dave_cheney.net
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): ...
12 years, 1 month ago (2013-02-08 03:28:22 UTC) #2
mikio
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#newcode99 > src/pkg/net/conn_test.go:99: t.Fatalf("net.Listener.Accept ...
12 years, 1 month ago (2013-02-08 12:28:55 UTC) #3
dave_cheney.net
> >> https://codereview.appspot.com/7308058/diff/7001/src/pkg/net/conn_test.go#newcode99 >> src/pkg/net/conn_test.go:99: t.Fatalf("net.Listener.Accept failed: %v", >> err) >> cannot use Fatalf here, ...
12 years, 1 month ago (2013-02-08 12:33:31 UTC) #4
mikio
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
12 years, 1 month ago (2013-02-08 13:44:52 UTC) #5
dave_cheney.net
LGTM, modulo the comments to move the defers closer to the place the resources are ...
12 years ago (2013-02-19 02:02:57 UTC) #6
mikio
12 years ago (2013-03-03 07:00:04 UTC) #7
*** 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b