code review 9103045: os/exec: make exec_test.go:TestExtraFilesFDShuffle portable.

os/exec: make exec_test.go:TestExtraFilesFDShuffle portable.

[cnicolaou, 2013-05-02 17:26:51]

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

[minux1, 2013-05-05 08:29:37]

LGTM.

Although the fix alone might not be important enough to
be included Go 1.1, but this enables a test for a bug whose
fix does include in Go 1.1, so IMHO we should enable that
test before release and hence accept this CL into Go 1.1.

[bradfitz, 2013-06-18 15:55:37]

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

os/exec: make exec_test.go:TestExtraFilesFDShuffle portable.

R=golang-dev, minux.ma
CC=golang-dev
https://codereview.appspot.com/9103045

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[bradfitz, 2013-06-18 15:58:39]

This might break OS X 10.6's builders.  If it does I'll make the test skip
on that platform.



On Tue, Jun 18, 2013 at 8:55 AM, <bradfitz@golang.org> wrote:

> *** Submitted as
>
https://code.google.com/p/go/**source/detail?r=600c5389c9d3<https://code.goog...
>
> os/exec: make exec_test.go:**TestExtraFilesFDShuffle portable.
>
> R=golang-dev, minux.ma
> CC=golang-dev
>
https://codereview.appspot.**com/9103045<https://codereview.appspot.com/9103045>
>
> Committer: Brad Fitzpatrick <bradfitz@golang.org>
>
>
>
https://codereview.appspot.**com/9103045/<https://codereview.appspot.com/9103...
>

[albert.strasheim, 2013-06-19 12:40:14]

go version devel +4298a713c372 Tue Jun 18 17:15:26 2013 -0700 linux/amd64

$ go test os/exec -cpu=1,1
--- FAIL: TestExtraFilesFDShuffle (0.01 seconds)
exec_test.go:278: bad test value for stderr pipe
FAIL
FAIL	os/exec	0.545s

[dfc, 2013-06-19 12:43:55]

This is what I see on darwin/amd64, 10.7.5

^C^Xodessa(~/go/src) % go test os/exec -v
=== RUN TestEcho
--- PASS: TestEcho (0.02 seconds)
=== RUN TestCatStdin
--- PASS: TestCatStdin (0.02 seconds)
=== RUN TestCatGoodAndBadFile
--- PASS: TestCatGoodAndBadFile (0.02 seconds)
=== RUN TestNoExistBinary
--- PASS: TestNoExistBinary (0.00 seconds)
=== RUN TestExitStatus
--- PASS: TestExitStatus (0.02 seconds)
=== RUN TestPipes
--- PASS: TestPipes (0.02 seconds)
=== RUN TestPipeLookPathLeak
--- PASS: TestPipeLookPathLeak (0.03 seconds)
=== RUN TestExtraFilesFDShuffle
--- PASS: TestExtraFilesFDShuffle (0.02 seconds)
=== RUN TestExtraFiles
--- PASS: TestExtraFiles (0.06 seconds)
=== RUN TestExtraFilesRace
--- PASS: TestExtraFilesRace (0.26 seconds)
=== RUN TestHelperProcess
--- PASS: TestHelperProcess (0.00 seconds)
=== RUN TestLookPathNotFound
--- PASS: TestLookPathNotFound (0.00 seconds)
=== RUN TestLookPathUnixEmptyPath
--- PASS: TestLookPathUnixEmptyPath (0.00 seconds)
PASS
ok   os/exec 0.734s
odessa(~/go/src) % go test os/exec -v -cpu=1,1
=== RUN TestEcho
--- PASS: TestEcho (0.02 seconds)
=== RUN TestCatStdin
--- PASS: TestCatStdin (0.02 seconds)
=== RUN TestCatGoodAndBadFile
--- PASS: TestCatGoodAndBadFile (0.02 seconds)
=== RUN TestNoExistBinary
--- PASS: TestNoExistBinary (0.00 seconds)
=== RUN TestExitStatus
--- PASS: TestExitStatus (0.02 seconds)
=== RUN TestPipes
--- PASS: TestPipes (0.02 seconds)
=== RUN TestPipeLookPathLeak
--- PASS: TestPipeLookPathLeak (0.02 seconds)
=== RUN TestExtraFilesFDShuffle
--- PASS: TestExtraFilesFDShuffle (0.02 seconds)
=== RUN TestExtraFiles
--- PASS: TestExtraFiles (0.07 seconds)
=== RUN TestExtraFilesRace
--- PASS: TestExtraFilesRace (0.26 seconds)
=== RUN TestHelperProcess
--- PASS: TestHelperProcess (0.00 seconds)
=== RUN TestLookPathNotFound
--- PASS: TestLookPathNotFound (0.00 seconds)
=== RUN TestLookPathUnixEmptyPath
--- PASS: TestLookPathUnixEmptyPath (0.00 seconds)
=== RUN TestEcho
--- PASS: TestEcho (0.02 seconds)
=== RUN TestCatStdin
--- PASS: TestCatStdin (0.02 seconds)
=== RUN TestCatGoodAndBadFile
--- PASS: TestCatGoodAndBadFile (0.02 seconds)
=== RUN TestNoExistBinary
--- PASS: TestNoExistBinary (0.00 seconds)
=== RUN TestExitStatus
--- PASS: TestExitStatus (0.02 seconds)
=== RUN TestPipes
--- PASS: TestPipes (0.02 seconds)
=== RUN TestPipeLookPathLeak
--- PASS: TestPipeLookPathLeak (0.03 seconds)
=== RUN TestExtraFilesFDShuffle
runtime: kevent on fd 4 failed with 9

I do not know if -cpu=1,1 is an expected mode for this test.

On Wed, Jun 19, 2013 at 10:40 PM,  <fullung@gmail.com> wrote:
> go version devel +4298a713c372 Tue Jun 18 17:15:26 2013 -0700
> linux/amd64
>
> $ go test os/exec -cpu=1,1
> --- FAIL: TestExtraFilesFDShuffle (0.01 seconds)
> exec_test.go:278: bad test value for stderr pipe
> FAIL
> FAIL    os/exec 0.545s
>
>
> https://codereview.appspot.com/9103045/
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[albert.strasheim, 2013-06-19 12:51:13]

Hello

On 2013/06/19 12:43:55, dfc wrote:
> I do not know if -cpu=1,1 is an expected mode for this test.

The test.cpu flag seems useful, although run.bash currently only uses it in
runtime and sync tests.

Since I started running my tests, I think we've had less than 5 tests that broke
when run with multiple test.cpu values.

The few times it happened, it got fixed. I think it's worth not letting the test
suite decay along this axis.

Regards

Albert

[dfc, 2013-06-19 12:59:26]

Hi Albert,

Please don't interpret what I am saying as a criticism. I am extremely grateful
for the work you do to ensure the std lib is of a high quality. 

My point, if indeed I have one, is run.bash doesn't test the packages like this,
so it is understandable that their authors may not consider this case. I
certainly don't. 

I think that if -CPU=x,x is an expected gate for tests to pass then the should
be enshrined in run.bash.

Cheers

Dave



On 19/06/2013, at 22:51, fullung@gmail.com wrote:

> Hello
> 
> On 2013/06/19 12:43:55, dfc wrote:
>> I do not know if -cpu=1,1 is an expected mode for this test.
> 
> The test.cpu flag seems useful, although run.bash currently only uses it
> in runtime and sync tests.
> 
> Since I started running my tests, I think we've had less than 5 tests
> that broke when run with multiple test.cpu values.
> 
> The few times it happened, it got fixed. I think it's worth not letting
> the test suite decay along this axis.
> 
> Regards
> 
> Albert
> 
> https://codereview.appspot.com/9103045/

[minux1, 2013-06-19 13:15:14]

On Wed, Jun 19, 2013 at 8:59 PM, Dave Cheney <dave@cheney.net> wrote:
> I am extremely grateful for the work you do to ensure the std lib is of a high
quality.
Me too. Thank you Albert for your stress testing the std lib.
> My point, if indeed I have one, is run.bash doesn't test the packages like
this, so it is understandable that their authors may not consider this case. I
certainly don't.
> I think that if -CPU=x,x is an expected gate for tests to pass then the should
be enshrined in run.bash.
however this will make the test take twice the time on the builder,
can we afford that?

testing -test.cpu=1,1 essentially test that a test doesn't modify its
global context.
i don't think it's important for normal tests, however, if a test
involves a mechanism
in runtime that will:
1. create new OS thread
2. create new goroutine
3. other implicit changes to the runtime
we should test that case explicitly that it doesn't "ruin" its own
context/environment.

a concrete example is the goroutine leakage test in net/http (although
it is not enabled
in -short tests)

[albert.strasheim, 2013-06-19 13:16:40]

Hello

On 2013/06/19 12:59:26, dfc wrote:
> Please don't interpret what I am saying as a criticism. I am extremely
grateful
> for the work you do to ensure the std lib is of a high quality. 

Sure, no problem.

> My point, if indeed I have one, is run.bash doesn't test the packages like
this,
> so it is understandable that their authors may not consider this case. I
> certainly don't. 
> I think that if -CPU=x,x is an expected gate for tests to pass then the should
> be enshrined in run.bash.

I think the reason it hasn't been enshrined there is that it is going to make
run.bash take too long on the slow builders.

One option could be to add something along the lines of
trytobreakitbeforesubmittingacl.bash which does:

for i in $*; do
go test -short $i
go test $i
go test -short -cpu=1,2,4,8,16,256 $i
go test -cpu=1,2,4,8,16,256 $i
for j in `seq 1 10`; do
GOMAXPROCS=$[1+$[RANDOM%256]] go test -short $i
GOMAXPROCS=$[1+$[RANDOM%256]] go test $i
done
done

if you could convince people to run that for CLs that change packages and/or add
tests.

Regards

Albert

[minux1, 2013-06-19 13:23:01]

On Wed, Jun 19, 2013 at 9:16 PM,  <fullung@gmail.com> wrote:
> One option could be to add something along the lines of
> trytobreakitbeforesubmittingacl.bash which does:
>
> for i in $*; do
> go test -short $i
> go test $i
> go test -short -cpu=1,2,4,8,16,256 $i
> go test -cpu=1,2,4,8,16,256 $i
> for j in `seq 1 10`; do
> GOMAXPROCS=$[1+$[RANDOM%256]] go test -short $i
> GOMAXPROCS=$[1+$[RANDOM%256]] go test $i
> done
> done
>
> if you could convince people to run that for CLs that change packages
> and/or add tests.
I have a concern that if a test isn't run by the builder, people will
tend to break
a test.
i think catching bugs like this is precisely the reason why we set up
the builders.

[albert.strasheim, 2013-06-19 13:28:18]

Hello

On Wed, Jun 19, 2013 at 1:22 PM, minux <minux.ma@gmail.com> wrote:
>> if you could convince people to run that for CLs that change packages
>> and/or add tests.
> I have a concern that if a test isn't run by the builder, people will
> tend to break
> a test.
> i think catching bugs like this is precisely the reason why we set up
> the builders.

Agreed.

Can we add a parameter to make.bash/run.bash that invokes extra tests
like these and have only fast builders use it?

Regards

Albert

[minux1, 2013-06-19 13:37:51]

On Wed, Jun 19, 2013 at 9:27 PM, Albert Strasheim <fullung@gmail.com> wrote:
> On Wed, Jun 19, 2013 at 1:22 PM, minux <minux.ma@gmail.com> wrote:
>>> if you could convince people to run that for CLs that change packages
>>> and/or add tests.
>> I have a concern that if a test isn't run by the builder, people will
>> tend to break
>> a test.
>> i think catching bugs like this is precisely the reason why we set up
>> the builders.
>
> Agreed.
>
> Can we add a parameter to make.bash/run.bash that invokes extra tests
> like these and have only fast builders use it?
this is possible.

what i have in mind is that we add some standard test to std lib that runs last
(like pkg/net/http/z_last_test.go), and checks for cases like:
1. remaining open fds (i.e. leaked fd)
2. remaining goroutines
3. broken net pollers (to catch the "runtime: kevent on fd 4 failed
with 9" error)

and then we make sure all builder test this.