code review 7363056: os/exec: Pass tests on Plan 9

os/exec: Pass tests on Plan 9

Adjust the exit status string for Plan 9.

Upon allocating >100 file descriptors, Plan 9
raises a warning. Moreover, the Go runtime for
32-bit version of Plan 9 keeps /dev/bintime
open for its implementation of runtime.nanotime().
This change accounts for these things in
TestExtraFiles.

[akumar, 2013-02-26 01:34:53]

Hello rsc@golang.org, rminnich@gmail.com, ality@pbrane.org, bradfitz@golang.org
(cc: golang-dev@googlegroups.com),

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

[bradfitz, 2013-02-26 01:45:37]

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go
File src/pkg/os/exec/exec_test.go (right):

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go...
src/pkg/os/exec/exec_test.go:89: e := "exit status 42"
not that this is further away, I might call this "want".

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go...
src/pkg/os/exec/exec_test.go:161: return
return isn't necessary. Skip ends the test.

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go...
src/pkg/os/exec/exec_test.go:238: // a warning. The second line contains the
a warning on stdout?  can we not use CombinedOutput and instead do:

var stderr, stdout bytes.Buffer
c.Stdout = &stdout
c.Stderr = &stderr

then:
if stdout.String() != text {
     t.Errorf("got stdout %q, stderr %q; want %q on stdout", stdout.String(),
stderr.String(), text)
}

If we have to do this, I'd prefer some of this cleanup be moved to a function.

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go...
src/pkg/os/exec/exec_test.go:379: skipfds := os.Stderr.Fd() + 2
this appears twice now.  move it to a function?

[akumar, 2013-02-26 06:33:32]

PTAL.

I made a change to make sure that the
output from "read3" in TestHelperProcess
goes to stdout instead of stderr.

Please see if this, along with the other
changes, are to your liking.

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go
File src/pkg/os/exec/exec_test.go (right):

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go...
src/pkg/os/exec/exec_test.go:89: e := "exit status 42"
On 2013/02/26 01:45:37, bradfitz wrote:
> not that this is further away, I might call this "want".

Done.

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go...
src/pkg/os/exec/exec_test.go:161: return
On 2013/02/26 01:45:37, bradfitz wrote:
> return isn't necessary. Skip ends the test.

Done.

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go...
src/pkg/os/exec/exec_test.go:238: // a warning. The second line contains the
On 2013/02/26 01:45:37, bradfitz wrote:
> a warning on stdout?

Actually, everything was being output to stderr.
I've changed that behaviour below, in TestHelperProcess.

> can we not use CombinedOutput and instead do:
> 
> var stderr, stdout bytes.Buffer
> c.Stdout = &stdout
> c.Stderr = &stderr
> 
> then:
> if stdout.String() != text {
>      t.Errorf("got stdout %q, stderr %q; want %q on stdout", stdout.String(),
> stderr.String(), text)
> }

Done.

https://codereview.appspot.com/7363056/diff/5001/src/pkg/os/exec/exec_test.go...
src/pkg/os/exec/exec_test.go:379: skipfds := os.Stderr.Fd() + 2
On 2013/02/26 01:45:37, bradfitz wrote:
> this appears twice now.  move it to a function?

Done.

[bradfitz, 2013-02-26 15:04:49]

LGTM after nits below.

https://codereview.appspot.com/7363056/diff/11001/src/pkg/os/exec/exec_test.go
File src/pkg/os/exec/exec_test.go (right):

https://codereview.appspot.com/7363056/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:156: func skipfds(n uintptr) uintptr {
drop the n parameter. call this basefds.

// basefds returns the number of expected file descriptors
// to be present in a process at start.

https://codereview.appspot.com/7363056/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:157: skip := os.Stderr.Fd() + n
n := os.Stderr.Fd() + 1

https://codereview.appspot.com/7363056/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:163: return skip + 1
n++

[akumar, 2013-02-26 16:39:51]

PTAL.

Also updated the fd leaks checks to use basefds() and basefds()+1 rather than
skipfds(1) and skipfds(2), respectively.

https://codereview.appspot.com/7363056/diff/11001/src/pkg/os/exec/exec_test.go
File src/pkg/os/exec/exec_test.go (right):

https://codereview.appspot.com/7363056/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:156: func skipfds(n uintptr) uintptr {
On 2013/02/26 15:04:49, bradfitz wrote:
> drop the n parameter. call this basefds.
> 
> // basefds returns the number of expected file descriptors
> // to be present in a process at start.
> 

Done.

https://codereview.appspot.com/7363056/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:157: skip := os.Stderr.Fd() + n
On 2013/02/26 15:04:49, bradfitz wrote:
> n := os.Stderr.Fd() + 1

Done.

https://codereview.appspot.com/7363056/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:163: return skip + 1
On 2013/02/26 15:04:49, bradfitz wrote:
> n++

Done.

[akumar, 2013-02-26 17:36:53]

Yes please.

On 26 February 2013 17:33, Anthony Martin <ality@pbrane.org> wrote:
> LGTM
>
> Do you need me to submit this?

[ality, 2013-02-26 17:41:04]

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

os/exec: Pass tests on Plan 9

Adjust the exit status string for Plan 9.

Upon allocating >100 file descriptors, Plan 9
raises a warning. Moreover, the Go runtime for
32-bit version of Plan 9 keeps /dev/bintime
open for its implementation of runtime.nanotime().
This change accounts for these things in
TestExtraFiles.

R=rsc, rminnich, ality, bradfitz
CC=golang-dev
https://codereview.appspot.com/7363056

Committer: Anthony Martin <ality@pbrane.org>