code review 1806043: os: Use TempFile with default TempDir for temp test files

os: Use TempFile with default TempDir for temp test files

Use io/ioutil.TempFile with default os.TempDir for temporary test files.
For os_test.go temporary test files, use a local file system and OS
independent directory names. Avoid problems with NFS.

Fixes issue 848.

[peterGo, 2010-07-11 18:58:02]

http://codereview.appspot.com/1806043/diff/1/2
File src/pkg/os/os_test.go (right):

http://codereview.appspot.com/1806043/diff/1/2#newcode1
src/pkg/os/os_test.go:1: // Copyright 2009 The Go Authors. All rights reserved.

os.TestChown and os.TestChtimes (Issue 848) have encountered problems with NFS;
they should be run on a local file system. To avoid NFS problems, TestChown used
the /tmp directory, an OS dependent name, assuming it to be local. TestChown and
TestChtimes have been changed to use io/ioutil.TempFile with default os.TempDir,
assuming it to be local. Other os/os_test.go tests which create temporary test
files have been revised too. Formerly, they used subdirectories of $GOROOT,
which may or may not use NFS. Temporary test files are now created, closed, and
removed in a uniform manner.

[peterGo, 2010-07-11 18:59:29]

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

I'd like you to review this change.

[peterGo, 2010-07-11 22:03:28]

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

Please take another look.

[adg, 2010-07-12 02:04:34]

Some nits below.

http://codereview.appspot.com/1806043/diff/7001/8001
File src/pkg/os/os_test.go (right):

http://codereview.appspot.com/1806043/diff/7001/8001#newcode383
src/pkg/os/os_test.go:383: defer Remove(f.Name())
defer statements are executed LIFO, so switch the Close and Remove lines here
and elsewhere.

http://codereview.appspot.com/1806043/diff/7001/8001#newcode391
src/pkg/os/os_test.go:391: t.Fatalf("fchmod %s 0123: %s", f.Name(), err)
s/fchmod/chmod/

http://codereview.appspot.com/1806043/diff/7001/8001#newcode419
src/pkg/os/os_test.go:419: t.Fatalf("fstat %s: %s", f.Name(), err)
s/fstat/stat/

[peterGo, 2010-07-12 03:52:17]

http://codereview.appspot.com/1806043/diff/7001/8001
File src/pkg/os/os_test.go (right):

http://codereview.appspot.com/1806043/diff/7001/8001#newcode383
src/pkg/os/os_test.go:383: defer Remove(f.Name())
On 2010/07/12 02:04:34, adg wrote:
> defer statements are executed LIFO, so switch the Close and Remove lines here
> and elsewhere.

Done.

http://codereview.appspot.com/1806043/diff/7001/8001#newcode391
src/pkg/os/os_test.go:391: t.Fatalf("fchmod %s 0123: %s", f.Name(), err)
On 2010/07/12 02:04:34, adg wrote:
> s/fchmod/chmod/

Done.

http://codereview.appspot.com/1806043/diff/7001/8001#newcode419
src/pkg/os/os_test.go:419: t.Fatalf("fstat %s: %s", f.Name(), err)
On 2010/07/12 02:04:34, adg wrote:
> s/fstat/stat/

Done.

[peterGo, 2010-07-12 03:53:30]

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

Please take another look.

[adg, 2010-07-12 03:57:37]

LGTM

http://codereview.appspot.com/1806043/diff/14001/15001
File src/pkg/os/os_test.go (right):

http://codereview.appspot.com/1806043/diff/14001/15001#newcode486
src/pkg/os/os_test.go:486: defer f.Close()
remove this defer as we call f.Close explicitly a couple of lines down.

[peterGo, 2010-07-12 04:12:59]

http://codereview.appspot.com/1806043/diff/14001/15001
File src/pkg/os/os_test.go (right):

http://codereview.appspot.com/1806043/diff/14001/15001#newcode486
src/pkg/os/os_test.go:486: defer f.Close()
On 2010/07/12 03:57:37, adg wrote:
> remove this defer as we call f.Close explicitly a couple of lines down. 

I was aware of this. But what if we trip up before we get to the f.Close()? It
won't hurt to execute the Close again. In fact, because it's good practice to be
safe, os.Close specifically has a guard to allow this.

file.fd = -1 // so it can't be closed again
http://golang.org/src/pkg/os/file_unix.go?h=Close#L48

[adg, 2010-07-12 04:59:38]

On 12 July 2010 14:12,  <go.peter.90@gmail.com> wrote:
>
> http://codereview.appspot.com/1806043/diff/14001/15001
> File src/pkg/os/os_test.go (right):
>
> http://codereview.appspot.com/1806043/diff/14001/15001#newcode486
> src/pkg/os/os_test.go:486: defer f.Close()
> On 2010/07/12 03:57:37, adg wrote:
>>
>> remove this defer as we call f.Close explicitly a couple of lines
>
> down.
>
> I was aware of this. But what if we trip up before we get to the
> f.Close()? It won't hurt to execute the Close again. In fact, because
> it's good practice to be safe, os.Close specifically has a guard to
> allow this.
>
> file.fd = -1 // so it can't be closed again
> http://golang.org/src/pkg/os/file_unix.go?h=Close#L48

You're right, it's probably a moot point, but if f.Write() fails then
we've got bigger problems than just closing the fd... ;-)

Your call either way.

Andrew

[rsc1, 2010-07-12 21:38:01]

LGTM

leaving for adg

[adg, 2010-07-13 00:31:59]

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

os: Use TempFile with default TempDir for temp test files

Use io/ioutil.TempFile with default os.TempDir for temporary test files.
For os_test.go temporary test files, use a local file system and OS
independent directory names. Avoid problems with NFS.

Fixes issue 848.

R=adg
CC=golang-dev
http://codereview.appspot.com/1806043

Committer: Andrew Gerrand <adg@golang.org>