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: surprising interaction of subtests with TempDir #46624

Closed
cespare opened this issue Jun 7, 2021 · 8 comments
Closed

testing: surprising interaction of subtests with TempDir #46624

cespare opened this issue Jun 7, 2021 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@cespare
Copy link
Contributor

cespare commented Jun 7, 2021

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

I tested with Go 1.16.4 and tip (e1fa260).

Does this issue reproduce with the latest release?

Yes.

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

linux/amd64

What did you do?

Here's a demo (playground):

package main

import (
	"fmt"
	"path/filepath"
	"testing"
)

func TestTempdir(t *testing.T) {
	testGlob(t)
	t.Run("x=[1,2,3]", func(t *testing.T) {
		testGlob(t)
	})
	t.Run("x=[]", func(t *testing.T) {
		testGlob(t)
	})
}

func testGlob(t *testing.T) {
	dir := t.TempDir()
	glob := filepath.Join(dir, "*.txt")
	fmt.Println("glob:", glob)
	if _, err := filepath.Glob(glob); err != nil {
		t.Fatal("glob error:", err)
	}
}

What did you expect to see?

It'd be nice if this just worked.

What did you see instead?

$ go test -v ./x
=== RUN   TestTempdir
glob: /tmp/TestTempdir1889981746/001/*.txt
=== RUN   TestTempdir/x=[1,2,3]
glob: /tmp/TestTempdir_x=[1,2,3]163097497/001/*.txt
=== RUN   TestTempdir/x=[]
glob: /tmp/TestTempdir_x=[]3868477592/001/*.txt
    x_test.go:24: glob error: syntax error in pattern
--- FAIL: TestTempdir (0.00s)
    --- PASS: TestTempdir/x=[1,2,3] (0.00s)
    --- FAIL: TestTempdir/x=[] (0.00s)
FAIL

When naming some subtest, you probably aren't considering that the name is going to become part of the path of any t.TempDir(). This can have some surprising interactions, such as this case where the name includes [], making any glob pattern inside the tempdir invalid.

I suspect there are other lurking surprises. For example, if you use a * inside the test name, that is passed on to os.MkdirTemp as-is where it has a special meaning (it indicates where the random number should be inserted). Not broken, but also not intended. Or your subtest name can include characters such as !"#$&'(),;<=>?[]^{|} and backtick which may require shell quoting.

Perhaps t.TempDir shouldn't name its directory after the name of the test, or else perhaps it should use a limited set of allowed symbols rather than trying to remove a few disallowed ones (like /).

(This is a continuation of the issues uncovered in #38465 -- cc @bradfitz @tklauser)

@tklauser
Copy link
Member

tklauser commented Jun 7, 2021

Perhaps t.TempDir shouldn't name its directory after the name of the test, or else perhaps it should use a limited set of allowed symbols rather than trying to remove a few disallowed ones (like /).

I think it might still useful for the directory to somewhat resemble the name of the test. But I agree that trying to remove disallowed symbols is error-prone and might still break on certain platforms. Using a limited set of symbols (e.g. alphanumeric characters plus a few safe ones) sounds good to me.

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 7, 2021
@cherrymui cherrymui added this to the Backlog milestone Jun 7, 2021
@cherrymui
Copy link
Member

cc @ianlancetaylor @bradfitz

@ianlancetaylor
Copy link
Contributor

Yes, let's just drop unusual characters, or replace them with - or something.

@gopherbot
Copy link

Change https://golang.org/cl/326010 mentions this issue: testing: drop unusual characters from TempDir directory name

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 16, 2021
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 16, 2021
@ianlancetaylor
Copy link
Contributor

@gopherbot Please open backport to 1.16.

@gopherbot
Copy link

Backport issue(s) opened: #50645 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@ianlancetaylor
Copy link
Contributor

(The fix is already in 1.17.)

@gopherbot
Copy link

Change https://golang.org/cl/378914 mentions this issue: [release-branch.go1.16] testing: drop unusual characters from TempDir directory name

gopherbot pushed a commit that referenced this issue Jan 26, 2022
… directory name

Only use safe characters of the test name for the os.MkdirTemp pattern.
This currently includes the alphanumeric characters and ASCII
punctuation characters known not to interact with globs.

For #46624
Fixes #50645

Change-Id: I402c34775b943fed9b97963c52f79245cc16dc1d
Reviewed-on: https://go-review.googlesource.com/c/go/+/326010
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 97cee43)
Reviewed-on: https://go-review.googlesource.com/c/go/+/378914
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
@golang golang locked and limited conversation to collaborators Jan 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants