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

proposal: testing: t.TempDir() should return actual path on macOS #56259

Closed
thaJeztah opened this issue Oct 16, 2022 · 8 comments
Closed

proposal: testing: t.TempDir() should return actual path on macOS #56259

thaJeztah opened this issue Oct 16, 2022 · 8 comments

Comments

@thaJeztah
Copy link
Contributor

On macOS, temp-directories are symlinked to /private/var, which may cause unexpected results when testing code that handles symlinks.

Taking the following example:

package main

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

func TestSymLink(t *testing.T) {
	tmpDir := t.TempDir()

	srcPath := filepath.Join(tmpDir, "/source")
	dstPath := filepath.Join(tmpDir, "/symlinked")
	if err := os.Mkdir(srcPath, 0o777); err != nil {
		t.Errorf("failed to create directory: %s", err)
	}
	if err := os.Symlink(srcPath, dstPath); err != nil {
		t.Errorf("failed to create symlink: %s", err)
	}

	t.Run("using original tmpDir", func(t *testing.T) {
		p, err := filepath.EvalSymlinks(dstPath)
		if err != nil {
			t.Error(err)
		}
		if p != srcPath {
			t.Errorf("expected %q, got %q", srcPath, p)
		}
	})

	t.Run("using actual tmpDir", func(t *testing.T) {
		// On macOS, tmp itself is symlinked, so resolve this one upfront.
		realTmpDir, err := filepath.EvalSymlinks(tmpDir)
		if err != nil {
			t.Fatal(err)
		}
		realSrcPath := filepath.Join(realTmpDir, "/source")

		p, err := filepath.EvalSymlinks(dstPath)
		if err != nil {
			t.Error(err)
		}
		if p != realSrcPath {
			t.Errorf("expected %q, got %q", realSrcPath, p)
		}
	})
}

Running the test shows that the "using original tmpDir" test fails;

go test -v .
=== RUN   TestSymLink
=== RUN   TestSymLink/using_original_tmpDir
    sys_test.go:131: expected "/var/folders/6f/tz5jf4nn1_n5jb0ctrrw5p2w0000gn/T/TestSymLink727570636/001/source", got "/private/var/folders/6f/tz5jf4nn1_n5jb0ctrrw5p2w0000gn/T/TestSymLink727570636/001/source"
=== RUN   TestSymLink/using_actual_tmpDir
--- FAIL: TestSymLink (0.00s)
    --- FAIL: TestSymLink/using_original_tmpDir (0.00s)
    --- PASS: TestSymLink/using_actual_tmpDir (0.00s)
FAIL

While this may be "expected", and it's possible to work around this, it's cumbersome (and easy to overlook) to have to resolve the actual path for t.TempDir() for tests that may expect the path returned to be the actual path.

Proposal

Make t.TempDir() resolve and return the actual path.

@gopherbot gopherbot added this to the Proposal milestone Oct 16, 2022
@bcmills
Copy link
Contributor

bcmills commented Oct 17, 2022

I dispute your use of the term “actual” in this proposal. In the presence of symlinks and hard-links, there may be more than one valid name for any given path — possibly even more than one valid name not involving symlinks! — and they are all in some sense “actual”.

In my experience, the approach that is the least prone to bugs is to always preserve the version of the path explicitly given in environment variables (such as $HOME, $PWD, $TMPDIR, and/or $GOROOT), even when that path passes through one or more links, because it is presenting the path as explicitly requested by the user's environment. And it looks like that is already the current behavior of (*testing.T).Tmpdir.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Oct 20, 2022
@apparentlymart
Copy link

apparentlymart commented Oct 27, 2022

FWIW, prior to the introduction of t.TempDir in the standard library we had a mostly-equivalent helper function in a codebase I routinely work on in which we went back and forth several times on how and whether it should handle this situation on macOS.

We initially had it working the way that t.TempDir currently works, returning a path starting with /tmp instead of /private/var.

We then encountered a similar situation to that discussed in the opening comment here: we had a test whose expected result included a file path and it was difficult for the test code to actually calculate what path it was "expecting" because something downstream was calculating a derived path and resolving the symlinks while doing so. So we changed our helper to itself call filepath.EvalSymlinks before returning its result, so that the test code would receive a path it could manipulate purely syntactically using the filepath package to calculate which path it was expecting.

However, we later had a test that wanted to make exactly the opposite assumption, and had no way to undo the EvalSymlinks to recover the original path.

Our ultimate conclusion was to make the helper behave equivalently to how t.TempDir does, and then have tests with special needs call filepath.EvalSymlinks themselves if necessary. This was the best approach for us because test code is empowered to call filepath.EvalSymlinks itself if it needs to, but a test cannot undo filepath.EvalSymlinks to recover the original direct path.

Based on that experience, I would recommend against making the exact change proposed here. It may be possible to reduce the boilerplate required in tests that do want a fully-evaluated path -- e.g. with a function which calls t.Fail if it doesn't succeed so that it isn't necessary to handle the error -- but I haven't personally found the boilerplate of calling filepath.EvalSymlinks and then checking the error to be a significant burden in the small number of tests where this has been necessary.


I will say that it is unfortunate that this quirk only typically applies on macOS systems. More than once we've had developers write tests on other platforms (including me, on Linux) which fail when run on macOS because of not considering this quirk. But each time that's happened it's only been a brief annoyance, since the boilerplate to deal with it is typically the same in each case, and it trips me up maybe once per year at most.

@ianlancetaylor
Copy link
Member

@apparentlymart Thanks for the note. It's very helpful.

@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Nov 16, 2022
@rsc
Copy link
Contributor

rsc commented Nov 30, 2022

Thanks for the comment @apparentlymart. It sounds like we should not make this change, because

(1) programs that want to match $TMPDIR can do that today by just using the result, but they can't do it at all if we apply EvalSymlinks.
(2) programs that want the exact physical path can call EvalSymlinks themselves today.

@rsc
Copy link
Contributor

rsc commented Dec 7, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals Dec 7, 2022
@thaJeztah
Copy link
Contributor Author

Oh, I see I never replied to earlier comments; yes, @apparentlymart brought up some good points, and I agree that changing would fix some cases, but not others.

The common thing is, just that symlinks are a pain, and unfortunately something we'll have to live with (and macOS implicitly using them for these just rubbing that in).

I'll close this proposal, but really appreciate all participating on this discussion, and the maintainers for considering it.

@rsc
Copy link
Contributor

rsc commented Dec 14, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc moved this from Likely Decline to Declined in Proposals Dec 14, 2022
@rsc rsc removed this from Proposals Dec 9, 2023
@golang golang locked and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants