code review 6129052: syscall: add a test for passing an fd over a unix socket

syscall: add a test for passing an fd over a unix socket

Re-submitting previously reverted change 160ec5506cb7.

[bradfitz, 2012-04-30 04:36:27]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2012-04-30 04:43:40]

http://codereview.appspot.com/6129052/diff/3002/src/pkg/syscall/passfd_test.go
File src/pkg/syscall/passfd_test.go (right):

http://codereview.appspot.com/6129052/diff/3002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:47: return // not fatalf, so defers above run.
Fatalf should still run them, no? i double-checked the code for Fatalf.
ditto below.

http://codereview.appspot.com/6129052/diff/3002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:109: // Look for our fd. I<t should be fd 3, but
we work around an fd leak
s/<//

http://codereview.appspot.com/6129052/diff/3002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:122: fmt.Println("failed to find unix fd")
not a failure?
below too.

[bradfitz, 2012-04-30 04:48:11]

http://codereview.appspot.com/6129052/diff/3002/src/pkg/syscall/passfd_test.go
File src/pkg/syscall/passfd_test.go (right):

http://codereview.appspot.com/6129052/diff/3002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:47: return // not fatalf, so defers above run.
On 2012/04/30 04:43:40, r wrote:
> Fatalf should still run them, no? i double-checked the code for Fatalf.
> ditto below.

Done.

http://codereview.appspot.com/6129052/diff/3002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:109: // Look for our fd. I<t should be fd 3, but
we work around an fd leak
On 2012/04/30 04:43:40, r wrote:
> s/<//

Done.

http://codereview.appspot.com/6129052/diff/3002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:122: fmt.Println("failed to find unix fd")
On 2012/04/30 04:43:40, r wrote:
> not a failure?
> below too.

This isn't a test.  See the comment at the top of the "Test" function.  It's a
child process that the parent test reads the output of.

[bradfitz, 2012-04-30 04:48:25]

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

Please take another look.

[r, 2012-04-30 04:54:47]

http://codereview.appspot.com/6129052/diff/7002/src/pkg/syscall/passfd_test.go
File src/pkg/syscall/passfd_test.go (right):

http://codereview.appspot.com/6129052/diff/7002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:95: // Not a real test. This is the helper child
process for TestPassFD.
this bothers me. i see the trick but it's creepy that there's a test that looks
like a test in all respects, and even runs, but is responsible for its own
shutdown.

you can put that logic into TestPassFD itself, since you have the env variable.
there, say

if os.Getenv ... { passFDChild(); return }

comment the hell out of it,  and change the name here

http://codereview.appspot.com/6129052/diff/7002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:96: func TestPassFDChild(*testing.T) {
this bugs me.

how about putting this logic into TestP

[r, 2012-04-30 04:55:07]

http://codereview.appspot.com/6129052/diff/7002/src/pkg/syscall/passfd_test.go
File src/pkg/syscall/passfd_test.go (right):

http://codereview.appspot.com/6129052/diff/7002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:40: cmd := exec.Command(os.Args[0],
"-test.run=TestPassFDChild", "--", tempDir)
it's a regexp. anchor it to be safe.

[r2, 2012-04-30 04:57:11]

On Apr 30, 2012, at 2:54 PM, r@golang.org wrote:

> how about putting this logic into TestP

cool. this is a bug in rietveld. the edit block died and i had to open another
line to complete the comment

[bradfitz, 2012-04-30 05:03:44]

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

Please take another look.

[bradfitz, 2012-04-30 05:04:24]

http://codereview.appspot.com/6129052/diff/7002/src/pkg/syscall/passfd_test.go
File src/pkg/syscall/passfd_test.go (right):

http://codereview.appspot.com/6129052/diff/7002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:40: cmd := exec.Command(os.Args[0],
"-test.run=TestPassFDChild", "--", tempDir)
On 2012/04/30 04:55:07, r wrote:
> it's a regexp. anchor it to be safe.

Done.

http://codereview.appspot.com/6129052/diff/7002/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:95: // Not a real test. This is the helper child
process for TestPassFD.
On 2012/04/30 04:54:48, r wrote:
> this bothers me. i see the trick but it's creepy that there's a test that
looks
> like a test in all respects, and even runs, but is responsible for its own
> shutdown.
> 
> you can put that logic into TestPassFD itself, since you have the env
variable.
> there, say
> 
> if os.Getenv ... { passFDChild(); return }
> 
> comment the hell out of it,  and change the name here

Done.

[bradfitz, 2012-04-30 05:08:41]

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

Please take another look.

[r, 2012-04-30 05:10:44]

LGTM

http://codereview.appspot.com/6129052/diff/3004/src/pkg/syscall/passfd_test.go
File src/pkg/syscall/passfd_test.go (right):

http://codereview.appspot.com/6129052/diff/3004/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:22: func TestPassFD(t *testing.T) {
// The test calls itself using 'go test' to create the child process, using the
// environment variable to identify the child.

[bradfitz, 2012-04-30 05:31:25]

*** Submitted as http://code.google.com/p/go/source/detail?r=15260ef90c98 ***

syscall: add a test for passing an fd over a unix socket

Re-submitting previously reverted change 160ec5506cb7.

R=golang-dev, r, r
CC=golang-dev
http://codereview.appspot.com/6129052