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

Jump to bottom

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

Closed
rogpeppe opened this issue Jan 29, 2019 · 8 comments
Closed

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

rogpeppe opened this issue Jan 29, 2019 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented Jan 29, 2019 •
edited

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 0a99049, 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.
@gopherbot gopherbot added this to the Unreleased milestone Jan 29, 2019
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 29, 2019
@heschi
Copy link
Contributor

heschi 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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@heschi
Copy link
Contributor

heschi 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
Copy link

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

@ianthehat
Copy link

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?

@heschi
Copy link
Contributor

heschi 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?

@SteelPhase SteelPhase mentioned this issue Jun 4, 2019
Closed
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@gopherbot
Copy link

Change https://golang.org/cl/213221 mentions this issue: internal/imports: load test exports of package under test

@golang golang locked and limited conversation to collaborators Jan 13, 2021
@rsc rsc unassigned ianthehat and heschi Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@bradfitz @rogpeppe @ianthehat @gopherbot @heschi