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

all: ensure that tests do not write to the current directory #28387

Open
bcmills opened this issue Oct 25, 2018 · 47 comments
Open

all: ensure that tests do not write to the current directory #28387

bcmills opened this issue Oct 25, 2018 · 47 comments
Assignees
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Oct 25, 2018

In #27957, @hyangah noticed that the tests for compress/bzip2 fail when GOROOT is not writable, and those tests are run whenever we run go test all in module mode (which is intended to be a useful default).

As noted in #28386, tests should not assume that they can write to the current directory. We should ensure that none of the tests in the standard library make that mistake.

@tkivisik

This comment has been minimized.

Copy link
Contributor

@tkivisik tkivisik commented Oct 27, 2018

Does that mean the following pattern would be fine (create and use a temporary folder)
e.g.

wd, _ := os.Getwd()
dir, err := ioutil.TempDir("", "testing")
if err != nil {
	log.Fatal(err)
}
defer os.RemoveAll(dir) // clean up
// change and restore finally
if err := os.Chdir(dir); err != nil {
	return
}
defer os.Chdir(wd)
// Create necessary files for a test
// Run tests
@dpinela

This comment has been minimized.

Copy link
Contributor

@dpinela dpinela commented Oct 28, 2018

@bcmills It isn't the tests for compress/bzip2 that fail in #27957, it's the ones for compress/flate, and in that particular case it only happens when they would fail anyway - they write the actual output to testdata/*.got when actual != expected. Considering these files need to outlive the test, I'm not sure where else we should put them. On the other hand, this case might be fine - if your GOROOT is read-only, then you're probably using a stable release of Go, which presumably means the tests are passing; if they're not, you're using a dev tree, which is almost certainly writable.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 28, 2018

Change https://golang.org/cl/145283 mentions this issue: os: ensure tests pass even if GOROOT is read-only

gopherbot pushed a commit that referenced this issue Oct 28, 2018
We achieve this by always running all tests that create files in a
fresh temporary directory, rather than just on darwin/{arm,arm64}.
As a bonus, this lets us simplify the cleanup code for these tests
and assume their working directory starts out empty.

Updates #28387

Change-Id: I952007ae390a2451c9a368da26c7f9f5af64b2ba
Reviewed-on: https://go-review.googlesource.com/c/145283
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Oct 29, 2018

@tkivisik, yes, but in most cases you shouldn't even need to bother with Getwd and Chdir.

I've found this pattern pretty helpful:

	dir, err := ioutil.TempDir("", path.Base(t.Name()))
	if err != nil {
		t.Fatal(err)
	}
	defer os.RemoveAll(dir)

I wonder whether we should just make that a method on *testing.T: the os.RemoveAll doesn't strictly need to be deferred right there at the call site (we can run it at any point after the test function returns), and it might be useful to have a standard flag (along the lines of -work) to leave the temporary directory in place.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Oct 29, 2018

@dpinela

Considering these files need to outlive the test, I'm not sure where else we should put them.

Do they belong in side-files at all, given that the -update flag is not set? It seems like most regressions will be fairly small diffs, in which case we could log the first few lines of the diff — and that would also make the failure logs more useful if the reader doesn't have filesystem access (such as in the case of a TryBot failure).

If someone wants to dig in and fix a massive regression, they can always use -update and look at the diff using git diff instead of a separate side file.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Oct 29, 2018

if your GOROOT is read-only, then you're probably using a stable release of Go, which presumably means the tests are passing

That's not a valid assumption in general. Platform- or architecture-dependent bugs and non-hermetic tests can all lead to failures even with a stable release.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 30, 2018

Change https://golang.org/cl/146119 mentions this issue: go/internal/gcimporter: ensure tests pass even if GOROOT is read-only

gopherbot pushed a commit that referenced this issue Nov 6, 2018
This mainly entails writing compiler output files to a temporary
directory, as well as the corrupted files in TestVersionHandling.

Updates #28387

Change-Id: I6b3619a91fff27011c7d73daa4febd14a6c5c348
Reviewed-on: https://go-review.googlesource.com/c/146119
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Nov 29, 2018
@darkfeline

This comment has been minimized.

Copy link

@darkfeline darkfeline commented Feb 4, 2019

The tests for os appear to be affected by this (the bug currently only mentions compress/bzip2 so I thought I should point it out). (#29676)

--- FAIL: TestStatError (0.00s)
    os_test.go:200: symlink no-such-file symlink: permission denied
--- FAIL: TestHardLink (0.00s)
    os_test.go:694: open "hardlinktestto" failed: open hardlinktestto: permission denied
--- FAIL: TestSymlink (0.00s)
    os_test.go:780: Create("symlinktestto") failed: open symlinktestto: permission denied
--- FAIL: TestLongSymlink (0.00s)
    os_test.go:847: symlink "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", "longsymlinktestfrom" failed: symlink 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef longsymlinktestfrom: permission denied
--- FAIL: TestRename (0.00s)
    os_test.go:868: open "renamefrom" failed: open renamefrom: permission denied
--- FAIL: TestRenameOverwriteDest (0.00s)
    os_test.go:896: write file "renameto" failed: open renameto: permission denied
--- FAIL: TestAppend (0.00s)
    os_test.go:1684: Open: open append.txt: permission denied
--- FAIL: TestSameFile (0.00s)
    os_test.go:1766: Create(a): open a: permission denied
FAIL
FAIL	os	18.381s
@dpinela

This comment has been minimized.

Copy link
Contributor

@dpinela dpinela commented Feb 9, 2019

@darkfeline Is that on tip? I think these failures are already fixed there. Just ran them again to check.

@darkfeline

This comment has been minimized.

Copy link

@darkfeline darkfeline commented Feb 18, 2019

@dpinela yes it is fixed on master, I had only checked the latest release.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 19, 2019

Change https://golang.org/cl/163037 mentions this issue: cmd/vet: do not write test vet binary to GOROOT

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 19, 2019

Change https://golang.org/cl/162987 mentions this issue: unix: do not invoke Mkfifo with a relative path in a read-only directory

gopherbot pushed a commit to golang/sys that referenced this issue Feb 19, 2019
Updates golang/go#28387

Change-Id: Ibcdc3f9cb3dc43b86b7e7d3539ed592219e54aba
Reviewed-on: https://go-review.googlesource.com/c/162987
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Feb 19, 2019
Updates #28387

Change-Id: Ie5a5f1f798eb5900f9c7bdef165abcca02dd0dde
Reviewed-on: https://go-review.googlesource.com/c/163037
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
nebulabox added a commit to nebulabox/go that referenced this issue Feb 20, 2019
Updates golang#28387

Change-Id: Ie5a5f1f798eb5900f9c7bdef165abcca02dd0dde
Reviewed-on: https://go-review.googlesource.com/c/163037
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
nebulabox added a commit to nebulabox/go that referenced this issue Feb 20, 2019
Updates golang#28387

Change-Id: Ie5a5f1f798eb5900f9c7bdef165abcca02dd0dde
Reviewed-on: https://go-review.googlesource.com/c/163037
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 18, 2019

Change https://golang.org/cl/207701 mentions this issue: cmd/go: fail tests immediately if they attempt to create a tempfile within GOROOT

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 18, 2019

Change https://golang.org/cl/207700 mentions this issue: cmd/go: convert TestMove* to script tests

gopherbot pushed a commit that referenced this issue Nov 18, 2019
Updates #28387
Updates #30316

Change-Id: I08eb0e144387735f7a7811a82e547a581991b335
Reviewed-on: https://go-review.googlesource.com/c/go/+/207697
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Nov 18, 2019
Updates #28387
Updates #30316

Change-Id: If2e66176e2c92a469cbab20e60f4439b0d8668bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/207700
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Nov 18, 2019
Updates #28387
Updates #30316

Change-Id: I06e50c8d148cb4d7e08cdc2ba90de5e91d35781d
Reviewed-on: https://go-review.googlesource.com/c/go/+/207699
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Nov 19, 2019
It was attempting to write a test binary to the working directory.

Updates #28387
Updates #30316

Change-Id: I82eca3a8a3e019dc6dacbe1f02a0583577694b93
Reviewed-on: https://go-review.googlesource.com/c/go/+/207614
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Nov 19, 2019
…ithin GOROOT

This will help to detect regressions of #28387 when running
'go test cmd/go' in a writable GOROOT.

Updates #28387
Updates #30316

Change-Id: I551e044111535404688b1a76e63163dfcb41bb5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/207701
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 19, 2019

Change https://golang.org/cl/207958 mentions this issue: cmd/go: convert TestTestCacheInputs to a script test

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 19, 2019

Change https://golang.org/cl/207959 mentions this issue: cmd/go: ignore irrelevant 'go test' failure in TestGoTestRaceInstallCgo

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 19, 2019

Change https://golang.org/cl/207960 mentions this issue: cmd/go: convert TestPackageMainTestImportsArchiveNotBinary to a script test

gopherbot pushed a commit that referenced this issue Nov 19, 2019
Updates #28387
Updates #30316

Change-Id: I48c6dd8619ea9602e9617ce11dfa05f1c70a485d
Reviewed-on: https://go-review.googlesource.com/c/go/+/207958
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Nov 19, 2019
This test runs 'go test -race -i runtime/race' and checks that it did
not overwrite cmd/cgo.

If GOROOT/pkg is read-only and GOROOT/pkg/$GOOS_$GOARCH_race is not
already populated, as are the conditions if the Go toolchain was
installed from source as root using 'make.bash', then 'go test -race
-i' itself will fail because it cannot install packages to GOROOT/pkg.

However, such a failure is not relevant to the test: even if 'go test
-race -i' fails, we can still verify that it did not incidentally
overwrite cmd/cgo.

Updates #28387
Updates #30316

Change-Id: Iff2f75a0aeb4c926290ac3062c83695604522078
Reviewed-on: https://go-review.googlesource.com/c/go/+/207959
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Nov 19, 2019
…t test

Updates #28387
Updates #30316

Change-Id: I31e04c89f2cc226f9b5110f14c8b80a18e937efb
Reviewed-on: https://go-review.googlesource.com/c/go/+/207960
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 19, 2019

Change https://golang.org/cl/207965 mentions this issue: cmd/go: consolidate TestInstalls into gopath_install script test

gopherbot pushed a commit that referenced this issue Nov 19, 2019
TestInstalls was already mostly redundant with
TestInstallInto{GOPATH,GOBIN}, except for one additional check for the
install location of cmd/fix.

We can't assume that GOROOT is writable in general, so we also can't
assume that the test will be able to reinstall cmd/fix at run time.
Moreover, other processes running in parallel may expect to invoke
cmd/fix themselves, so this test temporarily removing it could induce
systemwide flakes.

We could carefully construct a parallel GOROOT and install cmd/fix
into it, but we can get *almost* as much coverage — at a much lower
cost — by checking the output of 'go list' instead of actually
rebuilding and reinstalling the binary.

Updates #28387
Updates #30316

Change-Id: Id49f44a68b0c52dfabb84c665f63c4e7db58dd49
Reviewed-on: https://go-review.googlesource.com/c/go/+/207965
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 20, 2019

Change https://golang.org/cl/208117 mentions this issue: misc/cgo/testcarchive: avoid writing to GOROOT in tests

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 20, 2019

Change https://golang.org/cl/208119 mentions this issue: misc/cgo/testcshared: avoid writing to GOROOT in tests

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 20, 2019

Change https://golang.org/cl/208123 mentions this issue: message/pipeline: avoid writing to the testdata directory

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 20, 2019

Change https://golang.org/cl/208124 mentions this issue: misc/cgo/fortran: avoid writing to $PWD

gopherbot pushed a commit that referenced this issue Nov 20, 2019
The bash script that drives this test needs to know whether the
fortran compiler works, but it doesn't actually care about the
generated binary. Write that binary to /dev/null.

Updates #28387
Updates #30316

Change-Id: I4f86da1aeb939fc205f467511fc69235a6a9af26
Reviewed-on: https://go-review.googlesource.com/c/go/+/208124
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue Nov 22, 2019
Also add a -testwork flag to facilitate debugging the test itself.

Three of the tests of this package invoked 'go install -i
-buildmode=c-archive' in order to generate an archive as well as
multiple C header files.

Unfortunately, the behavior of the '-i' flag is inappropriately broad
for this use-case: it not only generates the library and header files
(as desired), but also attempts to install a number of (unnecessary)
archive files for transitive dependencies to
GOROOT/pkg/$GOOS_$GOARCH_shared, which may not be writable — for
example, if GOROOT is owned by the root user but the test is being run
by a non-root user.

Instead, for now we generate the header files for transitive dependencies
separately by running 'go tool cgo -exportheader'.

In the future, we should consider how to improve the ergonomics for
generating transitive header files without coupling that to
unnecessary library installation.

Updates #28387
Updates #30316
Updates #35715

Change-Id: I3d483f84e22058561efe740aa4885fc3f26137b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/208117
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Nov 22, 2019
The tests in this package invoked 'go install -i -buildmode=c-shared'
in order to generate an archive as well as multiple C header files.

Unfortunately, the behavior of the '-i' flag is inappropriately broad
for this use-case: it not only generates the library and header files
(as desired), but also attempts to install a number of (unnecessary)
archive files for transitive dependencies to
GOROOT/pkg/$GOOS_$GOARCH_testcshared_shared, which may not be writable
— for example, if GOROOT is owned by the root user but the test is
being run by a non-root user.

Instead, for now we generate the header files for transitive dependencies
separately by running 'go tool cgo -exportheader'.

In the future, we should consider how to improve the ergonomics for
generating transitive header files without coupling that to
unnecessary library installation.

Updates #28387
Updates #30316
Updates #35715

Change-Id: I622426a860828020d98f7040636f374e5c766d28
Reviewed-on: https://go-review.googlesource.com/c/go/+/208119
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit to golang/text that referenced this issue Nov 22, 2019
If this test is run as a dependency of some other module, the testdata
directory will be read-only. Moreover, if temporary files are written
to the working directory they are likely to interfere with concurrent
version-control operations, such as 'git commit -a'.

Updates golang/go#28387

Change-Id: I15cd9408c63f9b6aed50facbfefa26299392052f
Reviewed-on: https://go-review.googlesource.com/c/text/+/208123
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208482 mentions this issue: misc/cgo/testshared: do not write to GOROOT

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208481 mentions this issue: misc: remove use of relative directories in overlayDir functions

gopherbot pushed a commit that referenced this issue Nov 25, 2019
It turns out that the relative-path support never worked in the first
place.

It had been masked by the fact that we ~never invoke overlayDir with
an absolute path, which caused filepath.Rel to always return an error,
and overlayDir to always fall back to absolute paths.

Since the absolute paths seem to be working fine (and are simpler),
let's stick with those. As far as I can recall, the relative paths
were only a space optimization anyway.

Updates #28387
Updates #30316

Change-Id: Ie8cd28f3c41ca6497ace2799f4193d7f5dde7a37
Reviewed-on: https://go-review.googlesource.com/c/go/+/208481
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Nov 25, 2019
Instead of installing shared libraries to GOROOT/pkg, clone the
necessary files into a new GOROOT and run there.

Given that we now have a build cache, ideally we should not need to
install into GOROOT/pkg at all, but we can't fix that during the 1.14
code freeze.

Updates #28387
Updates #28553
Updates #30316

Change-Id: I83084a8ca29a5dffcd586c7fccc3f172cac57cc6
Reviewed-on: https://go-review.googlesource.com/c/go/+/208482
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Nov 25, 2019

This is now fixed for the standard library. Leaving open for the x/ modules.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.14, Unreleased Dec 5, 2019
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
6 participants
You can’t perform that action at this time.