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

source.CallExprArgs fails to open target source file when built with -trimpath #277

Open
aaron42net opened this issue Oct 16, 2023 · 7 comments
Labels

Comments

@aaron42net
Copy link

Similar to #274, the CI system I use has multiple concurrent builds and tests running in the same container, so it does source checkouts in temporary paths. This requires the use of go test -trimpath to avoid invalidating the cache every time the path changes.

Unfortunately, this also fails with "unable to parse source file" from source.CallExprArgs:

$ go test -trimpath .
--- FAIL: TestFoo (0.00s)
    foo_test.go:10: failed to parse source file foo.module/foo_test.go: open foo.module/foo_test.go: no such file or directory
FAIL
FAIL	foo.module	0.106s
FAIL

Instead of the expected useful output:

$ go test .
--- FAIL: TestFoo (0.00s)
    foo_test.go:10: assertion failed: "a" is not "b": some message
FAIL
FAIL	foo.module	0.106s
FAIL

Here's some simple reproduction code; drop these into an empty directory and run go mod tidy. The foo_test.go:

package foo

import (
	"testing"

	"gotest.tools/v3/assert"
)

func TestFoo(t *testing.T) {
	assert.Check(t, "a"=="b", "some message")
}

And go.mod:

module foo.module

replace foo.module => ./

go 1.19

require gotest.tools/v3 v3.5.1

require github.com/google/go-cmp v0.5.9 // indirect

The fix for #274 at least shows some message now, but after even more error messages. Would you be willing to accept a PR with a partial fix for -trimpath too?

Go tests are documented to be run from the same directory as the test binary, so it might be possible to infer a relative directory from that. Suggestions welcome for better ideas.

@dnephin
Copy link
Member

dnephin commented Oct 17, 2023

Thank you for the bug report! I think this would be good to fix.

Similar to the bazel fix, would you be able to run the test with some environment variable set to the path of the root of the project. assert could use that value to construct an absolute path.

@cstrahan
Copy link
Contributor

@aaron42net @dnephin I'll see if I can come up with a PR. Admittedly, I'm hoping that this fix and my bazel fix will motivate a new patch level version release soon-ish 😅.

@dnephin
Copy link
Member

dnephin commented Oct 28, 2023

@cstrahan Thank you for offering to fix this!

The env var to specify the GO module directory seems like the safe and easy option. Thinking through some more:

Go tests are documented to be run from the same directory as the test binary, so it might be possible to infer a relative directory from that.

Unfortunately I don't think it's safe to assume the directory is still the same directory as the package under test because some tests may run os.Chdir.

If tests are run in a git checkout, one option would be to use git rev-parse --show-toplevel to find the root of the repo. That is probably not reliable because the go module is not necessarily at the root of the repo. Probably not the best option.

Maybe we can do something using go list -m -f '{{.Dir}}' ? That should give us the the absolute path to the module root, but again fails if the os.Chdir is outside of the module root. That go list command can accept a package name, so if we have a way to lookup the package name being tested we could use go list -m -f '{{.Dir}}' <pkgname> to get the module directory. runtime.Callers doesn't work to get the package name (that's the problem we're trying to solve), and anything in reflect would need a reference to a type in the package, and I don't think we have that.

So env var might be the only reliable option. If we detect a relative path for filename and no env var, at least we can provide a good warning that tells someone they should set the variable to the go module root directory.

@dnephin dnephin added the bug label Oct 28, 2023
@cstrahan
Copy link
Contributor

cstrahan commented Nov 6, 2023

The env var to specify the GO module directory seems like the safe and easy option. Thinking through some more:
[....]

Unfortunately I don't think it's safe to assume the directory is still the same directory as the package under test because some tests may run os.Chdir.

@dnephin Perhaps we could do both? That is, if we're comfortable with capturing the current working directory in a static initializer. I would think the odds that another static initializer would os.Chdir would be really, really low. So perhaps we use the env var (if present), and fallback to an attempt to use the captured working directory, and if that fails we print a message suggesting use the env var.

How does that sound? And, do you have any suggest for env var name?

@dnephin
Copy link
Member

dnephin commented Nov 7, 2023

Good point! Capturing the working directory from an init function seems good to me. I think it's fine to go with that approach for now. We can add an env var later if we find cases where the working directory isn't correct.

@aaron42net
Copy link
Author

aaron42net commented Nov 26, 2023

I had not wanted to use environment variables here, because I thought they would bust the go test cache and require the test binary to be re-built and re-run, but apparently that magic is set up as part of m.Run(). So as long as they are only accessed from init(), it should be fine.

But I'd also prefer to not need to pass environment in the common case. At least in the large monorepo I help maintain, there are zero instances of os.Chdir() in init().

@jeremydonahue
Copy link

Any update? We are still seeing this issue.

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

No branches or pull requests

4 participants