False positive: os.TempDir() can't always be replaced by t.TempDir()

[alexandear]

Describe the bug

There are instances where replacing os.TempDir with t.TempDir() results in test failures.

To Reproduce

Steps to reproduce the behavior:

1. Clone the repo https://github.com/gohugoio/hugo .
2. Open the repo and checkout the commit using git checkout 7ee36b371.
3. Run ttempdir ./helpers/...

Expected behavior

The ttempdir should not report any issues.

Actual behavior

The following issue is reported:

/Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:490:2: os.TempDir() should be replaced by `t.TempDir()` in TestGetTempDir

Desktop:

• OS: macOS Sonoma 14.5

Additional context

os.TempDir on the line 490 cannot be replaced by t.TempDir. Attempting to do so results in the following error:

Details

=== RUN   TestGetTempDir
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/"
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/hugoTestFolder/  Foo bar  /", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/hugoTestFolder/  Foo bar  /"
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/hugoTestFolder/Foo.Bar/foo_Bar-Foo/", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/hugoTestFolder/Foo.Bar/foo_Bar-Foo/"
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/hugoTestFolder/fOObarfoo%bAR/", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/hugoTestFolder/fOObarfoo%bAR/"
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/hugoTestFolder/fOObarfoobAR/", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/hugoTestFolder/fOObarfoobAR/"
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/hugoTestFolder/FOo/BaR.html/", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/hugoTestFolder/FOo/BaR.html/"
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/hugoTestFolder/трям/трям/", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/hugoTestFolder/трям/трям/"
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/hugoTestFolder/은행/", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/hugoTestFolder/은행/"
    /Users/Oleksandr_Redko/src/github.com/gohugoio/hugo/helpers/path_test.go:513: Expected "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/TestGetTempDir592286977/001/hugoTestFolder/Банковский кассир/", got "/var/folders/pk/5dzf3qsj6l18s2b3zfw194840000gn/T/hugoTestFolder/Банковский кассир/"
--- FAIL: TestGetTempDir (0.00s)
FAIL

Please note that the descriptions of os.TempDir and t.TempDir are slightly different.

https://pkg.go.dev/os#TempDir

TempDir returns the default directory to use for temporary files.

On Unix systems, it returns $TMPDIR if non-empty, else /tmp. On Windows, it uses GetTempPath, returning the first non-empty value from %TMP%, %TEMP%, %USERPROFILE%, or the Windows directory. On Plan 9, it returns /tmp.

The directory is neither guaranteed to exist nor have accessible permissions.

https://pkg.go.dev/testing#T.TempDir

TempDir returns a temporary directory for the test to use. The directory is automatically removed > when the test and all its subtests complete. Each subsequent call to t.TempDir returns a unique > directory; if the directory creation fails, TempDir terminates the test by calling Fatal.

[peczenyj]

Hello

Thanks for the feedback,

The main reason why I am searching for os.TempDir() is: most of the time, people create files using os.TempDir() instead t.TempDir(), specialty on creating temporary files.

dir := os.TempDir()
file, err := os.CreateTemp(dir, "test-xxx-*")
// handle error
// defer / t.Cleanup to remove file

versus

dir := t.TempDir()
file, err := os.CreateTemp(dir, "test-xxx-*")
// handle error
// no need for defer / t.Cleanup to remove file, tempdir will be removed after test

since I can't easily check if os.CreateTemp was called with the os.TempDir or t.TempDir, I simplify the linter by searching all calls to os.TempDir() and it is very aggressive.

I can imagine two possible solutions:

1. disable the check of os.TempDir by default, this can be enabled via configuration if needed
2. add some option to skip some checks ( for instance I can look for a comment //nolint or I can explicit skip some file / package / function -- this should be easier to do inside golangci-lint, I think the go analysis tools does not support such annotations )

I think the option 1 seems a quick win on this case. What do you think @alexandear ?

Now, about this test in specific: there is a workaround:

on https://github.com/gohugoio/hugo/blob/master/helpers/path_test.go#L511C13-L511C64 we can see that we verify if the helper.GetTempDir returns the expected path

this helper uses afero.GetTempDir

https://github.com/spf13/afero/blob/v1.11.0/util.go#L104

since it calls os.TempDir and os.TempDir check for a env var TMPDIR based on https://pkg.go.dev/os#TempDir

$ git diff
diff --git a/helpers/path_test.go b/helpers/path_test.go
index 6f3699589..2b8eea934 100644
--- a/helpers/path_test.go
+++ b/helpers/path_test.go
@@ -487,7 +487,9 @@ func TestWriteToDisk(t *testing.T) {
 }
 
 func TestGetTempDir(t *testing.T) {
-       dir := os.TempDir()
+       dir := t.TempDir()
+       t.Setenv("TMPDIR", dir)
+
        if helpers.FilePathSeparator != dir[len(dir)-1:] {
                dir = dir + helpers.FilePathSeparator
        }

$ go test ./helpers/...
ok  	github.com/gohugoio/hugo/helpers	1.531s

It is, indeed, possible on this example change os.TempDir by t.TempDir however this is not very portable. And I agree that on this example it adds no value.

[alexandear]

Hi, thank you for the quick and meaningful reply.

I believe this would be a good solution:

disable the check of os.TempDir by default, this can be enabled via configuration if needed

Thank you for providing the workaround for TestGetTempDir.