Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: test failes if leaving files without write permission in T.TempDir #40853

Open
atetubou opened this issue Aug 18, 2020 · 15 comments
Open

testing: test failes if leaving files without write permission in T.TempDir #40853

atetubou opened this issue Aug 18, 2020 · 15 comments
Milestone

Comments

@atetubou
Copy link
Contributor

@atetubou atetubou commented Aug 18, 2020

What version of Go are you using (go version)?

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/tikuta/Library/Caches/go-build"
GOENV="/Users/tikuta/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/tikuta/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/tikuta/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/tikuta/homebrew/Cellar/go/1.15/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/tikuta/homebrew/Cellar/go/1.15/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/jz/z42xl9mx2d576zq1q21hlggc00bk9x/T/go-build114835528=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I wrote a test leaving a file without write permission.

package test

import (
        "io/ioutil"
        "os"
        "path/filepath"
        "testing"
)

func TestNoDir(t *testing.T) {
        dir := t.TempDir()
        empty := filepath.Join(dir, "empty")

        if err := os.MkdirAll(filepath.Dir(empty), 0300); err != nil {
                t.Errorf("%v", err)
        }

        if err := ioutil.WriteFile(empty, nil, 0444); err != nil {
                t.Errorf("%v", err)
        }
}

func TestDir(t *testing.T) {
        dir := t.TempDir()
        empty := filepath.Join(dir, "a", "empty")

        if err := os.MkdirAll(filepath.Dir(empty), 0300); err != nil {
                t.Errorf("%v", err)
        }

        if err := ioutil.WriteFile(empty, nil, 0444); err != nil {
                t.Errorf("%v", err)
        }
}

What did you expect to see?

Both test passed.

What did you see instead?

Only TestNoDir passed.

$ go test -v go_test.go
=== RUN   TestNoDir
--- PASS: TestNoDir (0.00s)
=== RUN   TestDir
    testing.go:894: TempDir RemoveAll cleanup: openfdat /var/folders/jz/z42xl9mx2d576zq1q21hlggc00bk9x/T/TestDir492614072/001/a: permission denied
--- FAIL: TestDir (0.00s)
FAIL
FAIL    command-line-arguments  0.115s
FAIL
@atetubou
Copy link
Contributor Author

@atetubou atetubou commented Aug 18, 2020

Maybe this is bug of os.RemoveAll rather than T.TempDir?

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 18, 2020

I think this is a reasonable error. If the test does something that the test harness cannot undo there should be some indication to the user.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Aug 18, 2020

/cc @mpvl per owners.

Also /cc @bradfitz from #35998.

@dmitshur dmitshur added this to the Backlog milestone Aug 18, 2020
@atetubou
Copy link
Contributor Author

@atetubou atetubou commented Aug 19, 2020

I updated the test.

@davecheney
I see. But current behavior seems not consistent around the behavior if that is intentional.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 19, 2020

@atetubou would you be able to do the same test by hand in a shell, I think this is a Unix permissions quirk.

@atetubou
Copy link
Contributor Author

@atetubou atetubou commented Aug 20, 2020

@davecheney sorry, what do you mean by do the same test by hand?

@riannucci
Copy link

@riannucci riannucci commented Aug 20, 2020

I would expect the 'bash' equivalent to do rm -rf $testdir, not simply rm -r $testdir

@riannucci
Copy link

@riannucci riannucci commented Aug 20, 2020

Ah, I see; even rm -rf doesn't clean up in this case.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 21, 2020

I don’t think there is anything here to fix. If the test creates a directory with cannot be removed then the cleanup code should highlight this because this kind of test is not very hermetic.

@riannucci
Copy link

@riannucci riannucci commented Aug 21, 2020

Previous to T.TempDir the test was managing its own temporary directory, and had its own implementation of RemoveAll, since RemoveAll doesn't do stat changes to remove all that the process has permission to remove.

The code under test manipulates the permissions of the files it creates intentionally; I'm not sure what hermeticity has to do with it.

From my PoV, the TempDir provided by the test should only fail to remove if the test process has no permissions sufficient to remove it. Other than "os.RemoveAll doesn't remove everything that it could", is there a reason why the current behavior of t.TempDir is desirable?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 21, 2020

is there a reason why the current behavior of t.TempDir is desirable?

Simplicity. Why add complexity for a case that essentially never arises?

Let's just document the restriction: don't create unwritable directions in the directory returned by t.TempDir.

@riannucci
Copy link

@riannucci riannucci commented Aug 21, 2020

Simplicity. Why add complexity for a case that essentially never arises?

I can see that not addressing this case in testing will make the testing package simpler by making tests which run into this more complex. By definition, code which needs to create directories with different chmod values will not use this functionality (in case you're using "usage of t.TempDir" as the basis for "essentially never arises". Since t.TempDir is new for 1.15, I find it a bit early to make a claim about this essentially never happening.)

IMO interface simplicity and implementation simplicity are a tradeoff, and for testing code I would definitely err on the side of making the interface simpler in order to make the tests simpler, since burying the important test bits with infrastructure makes the tests less pithy.

Is there a general recommendation here that Go programs should not create folders without u+x? Or that such code should not be tested?

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 21, 2020

Maybe I can make the case that testing the operation of code in the presence of specific permissions on disk is not the majority of the test code written for go programs. I think the majority of the standard library supports this position, as does the corpus of open source code I’ve been exposed too.

For the cases where the test is specifically setting a condition where the code under test would fail — if the real code can’t remove that directory, what chance does a test helper have — there is t.Cleanup where the author of the test has a chance to hook the test tear down and assist.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 21, 2020

I can see that not addressing this case in testing will make the testing package simpler by making tests which run into this more complex.

Yes. To me that seems like the right tradeoff, given my belief that very very few tests need to create temporary unwritable directories, and that it is still possible to write such tests using ioutil.TempDir rather than t.TempDir. I understand that others may disagree.

@riannucci
Copy link

@riannucci riannucci commented Aug 21, 2020

if the real code can’t remove that directory

Note that the code under test has the purpose of preparing this directory, in production the 'real code' does not remove it. The directory is later removed by a different program (which does successfully remove w/ chmod). From my PoV the "effort" that os.RemoveAll makes is arbitrary; It knows how to do a depth-first recursion, but doesn't know how to do chmod. It also not really what I would call "simple" (https://golang.org/src/os/removeall_at.go). The current implementation already pays for the 'stat' call necessary to see the permission bits on intermediate nodes*, too, for example.

it is still possible to write such tests using ioutil.TempDir rather than t.TempDir

Yes, that's what our tests will need to continue to do.

I understand that others may disagree.

I do, but I respect your position here; there is no 'right' answer here, only tradeoffs and different weights for those tradeoffs :)

edit: s/modes/nodes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.