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

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

Updates issue 1101

[bradfitz, 2012-03-17 19:48:33]

Hello golang-dev@googlegroups.com,

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

[iant, 2012-03-18 04:48:08]

LGTM

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

http://codereview.appspot.com/5849057/diff/1003/src/pkg/syscall/passfd_test.g...
src/pkg/syscall/passfd_test.go:44: println("running child process.")
Should remove the printlns before committing.

[bradfitz, 2012-03-18 05:20:01]

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

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

Updates issue 1101

R=golang-dev, iant
CC=golang-dev
http://codereview.appspot.com/5849057

[bsiegert, 2012-03-18 15:59:42]

I don't understand all of the code but there is a typo in a comment.

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

http://codereview.appspot.com/5849057/diff/4002/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,I<t,It,

[r, 2012-03-18 23:47:00]

I'd like to delete this test too, being the first test to go into syscall and
setting a precedent I am not certain about. Not the existence of tests, but how
to test non-portable code in a portable way.

[bradfitz, 2012-03-18 23:59:01]

I don't see what's wrong with +build to limit the tests to the operating
systems they're supposed to pass on.

The builds are passing.

I'm of the mindset that if we're shipping an API, it should be tested.
 syscall is no different, just because it's operating-system specific.
 os/user has tests too, and that's wildly different between different OSes.

I'd rather keep this.  It serves as a demonstration of how to pass fds in
Go, and it's already found bugs in FreeBSD and OpenBSD.  We don't have to
care about those bugs yet, but it found them.  If you don't like the word
"probably" before freebsd and openbsd, change them to "TODO".

Feel free to override me.


On Sun, Mar 18, 2012 at 4:47 PM, <r@golang.org> wrote:

> I'd like to delete this test too, being the first test to go into
> syscall and setting a precedent I am not certain about. Not the
> existence of tests, but how to test non-portable code in a portable way.
>
>
>
http://codereview.appspot.com/**5849057/<http://codereview.appspot.com/5849057/>
>

[r2, 2012-03-19 00:10:59]

On Mar 19, 2012, at 10:59 AM, Brad Fitzpatrick wrote:

> I don't see what's wrong with +build to limit the tests to the operating
systems they're supposed to pass on.

The problem is that in syscall in particular, a term like +linux doesn't really
say much.  Linux is not a single operating system. Outside syscall (that is,
inside other packages we export that use it) it is, but inside syscall there
really can be variations.

> The builds are passing.

That's not really on point. You wouldn't have checked the test in if it wasn't
correct in the environments we know.

> I'm of the mindset that if we're shipping an API, it should be tested. 
syscall is no different, just because it's operating-system specific.  os/user
has tests too, and that's wildly different between different OSes.

Tests are good, I agree, but I disagree with your second statement: syscall is
different. The os package promises, within reason, a common set of functions.
Syscall does nothing of the kind.

> I'd rather keep this.  It serves as a demonstration of how to pass fds in Go,
and it's already found bugs in FreeBSD and OpenBSD.  We don't have to care about
those bugs yet, but it found them.  If you don't like the word "probably" before
freebsd and openbsd, change them to "TODO".

I don't disagree with the wording. My point is precisely that the word
"probably" means, "we don't know", so here we are checking in a test that we
don't know works on all platforms. There is a real issue here, a deep one, and I
don't want to design how to address it now.
> 
> Feel free to override me.


These tests are setting a precedent right before the release of Go 1. That is
the reason I want to remove them. Not because they're bad or wrong or breaking
the build or because I don't like tests. But because checking in several hundred
lines of non-portable code right before cutting a major release is not good
strategy.

-rob

[r2, 2012-03-19 00:12:14]

And for the record: I expect the deletions to be temporary and for the tests to
come back in some form once we know how best to test the syscall package.

-rob