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

x/tools/cmd/goimports: does not understand identifiers defined by internal test files #29979

Open
rogpeppe opened this issue Jan 29, 2019 · 7 comments

Comments

@rogpeppe
Copy link
Contributor

commented Jan 29, 2019

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

$ go version
go version devel +4b3f04c63b Thu Jan 10 18:15:48 2019 +0000 linux/amd64

As of commit 0a99049195aff55f007fc4dfd48e3ec2b4d5f602, goimports does not seem to consider the local package as a candidate for an external test.

Here's a testscript example that demonstrates the issue:

exec goimports -w a_test.go
cmp a_test.go a_test.go-goimports

-- go.mod --
module example.com/a
-- a_test.go --
package a_test

func TestX() {
	a.X()
	a.Y()
}
-- a.go --
package a

func X() {
}
-- export_test.go --
package a

func Y() {
}
-- a_test.go-goimports --
package a_test

import "example.com/a"

func TestX() {
	a.X()
	a.Y()
}

I would expect running goimports on a.go to add an import of example.com/a.

@heschik

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

The test script has a typo -- it's running on a.go, not a_test.go. Once I fix that it passes on my machine. If it doesn't on yours can you add -v and show the output?

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

@heschik Thanks, you're right! I'll have to investigate more. I've got a larger example that definitely fails in this way, but I obviously need to be a bit more careful isolating the circumstances in which it's happening.

@rogpeppe rogpeppe changed the title x/tools/cmd/goimports: does not choose local test package as import x/tools/cmd/goimports: does not understand identifiers defined by internal test files Jan 30, 2019

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

@heschik The test script now (hopefully!) demonstrates the issue correctly.

@heschik

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Ah. Yes. That is unsurprising in retrospect.

@ianthehat: Without looking I'd bet a small sum that it's because of this line in loadExports:

https://github.com/golang/tools/blob/cb89afadce946e09f295886d955e7135642e195c/imports/fix.go#L980

It should probably do the same thing as parseOtherFiles.

@ianthehat

This comment has been minimized.

Copy link

commented Feb 4, 2019

So I think https://go-review.googlesource.com/c/tools/+/160999 correctly tests the problem, now I just need to fix it...

@ianthehat

This comment has been minimized.

Copy link

commented Feb 4, 2019

I guess the question is how fussy do we need to be about this.
Should we try hard to include tests files only when viewing from an xtest (going to be very hard), or not worry about test files adding public symbols that might lead to an incorrect match?
If we start adding in the test files, we then also need to drop them again if their package declaration is for an xtest package rather than an internal test file.
Basically I am not sure what the best fix really looks like... thoughts?

@heschik

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Hmm, I guess this is more complicated than I had initially given it credit for. Perhaps the best thing would be to directly special-case the package under test? Like, in fixImportsDefault, if the filename ends with _test, call loadExports on srcDir in a special mode and add that directly to the candidate list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.