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/go/analysis/analysistest: support modules #37054

Open
muirdm opened this issue Feb 5, 2020 · 21 comments
Open

x/tools/go/analysis/analysistest: support modules #37054

muirdm opened this issue Feb 5, 2020 · 21 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted 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

Comments

@muirdm
Copy link

muirdm commented Feb 5, 2020

Currently the analysistest helper only supports a GOPATH like tree. This is fine for stand alone analyses, but does not help much for analyses designed to run against a particular module. For example, you have an analysis that checks all code in your module correctly/safely interacts with a specific package in your module. Ideally you would be able to write an analysis test with a stub file that is able to load other packages/dependencies of the current module.

/cc @matloob @heschik

@gopherbot gopherbot added this to the Unreleased milestone Feb 5, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 5, 2020
@matloob
Copy link
Contributor

matloob commented Feb 5, 2020

Just to understand this a bit better, do you mean that the test code in the test tree can depend on an external module and download it?

@muirdm
Copy link
Author

muirdm commented Feb 5, 2020

do you mean that the test code in the test tree can depend on an external module and download it

I mean the test code behaves as if it is part of the module that contains the analyzer. I'm still getting used to modules, but I envisioned it working one of two ways:

  1. I don't use analysistest.WriteFiles, instead writing my own temporary test package inside my module. Then I run analysistest.Run on my test package, and any imports in my test package resolve in module mode. If I understand, this doesn't work because analysistest puts go/packages into GOPATH mode.
  2. I do use analysistest.WriteFiles and it creates a temporary module with a replace directive pointing back to my containing module.

@matloob
Copy link
Contributor

matloob commented Feb 5, 2020

Hm, I think Option 1 would create some problems because running the test would modify the module is contained in, and that could be very surprising.

Option 2 would be more desirable depending on how it works.

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 6, 2020
@bflad
Copy link

bflad commented Mar 24, 2020

Hi there! 👋 I'm not sure if this is an intended use case, but we love that we can write analysis.Analyzer for other Go modules using real dependencies in the testing code rather than stubbing them, which can be quick complex. This requires some careful dependency handling with vendoring and symlinks to make it work with analysistest.Run. Since files are copied between test runs its a little slow though. Full implementation: https://github.com/bflad/tfproviderlint / Example Analyzer: https://github.com/bflad/tfproviderlint/tree/master/passes/S037

It'd be neat if this could use something like Go 1.14's -modfile support or it just allowed using a go.mod in the testdata source. In our case, we would just point all our testing at the same Go Module file (contents at least).

@matloob
Copy link
Contributor

matloob commented Mar 24, 2020

I'd welcome a contribution to fix this if the code is simple enough. My preference is that there's no extra configuration other than the module file of the module the tests are contained in.

quasilyte added a commit to go-critic/go-critic that referenced this issue Mar 15, 2021
Instead of using a real `framework.Context` I had to use a
stub struct, see golang/go#37054
quasilyte added a commit to go-critic/go-critic that referenced this issue Mar 15, 2021
Instead of using a real `framework.Context` I had to use a
stub struct, see golang/go#37054
@firelizzard18
Copy link
Contributor

I am writing an analyzer that verifies that the correct packages are imported. I have files in testdata/ that import github.com/go-gl/gl/<version>/gl. go mod tidy does not add a reference to the module, github.com/go-gl/gl, and in fact it will remove a manually added reference. But when I run tests via analysistest.Run(t, analysistest.TestData(), pkgvet.Analyzer), I get the following error:

import_gl2.1.go:3:8: cannot find package "github.com/go-gl/gl/v4.6-core/gl" in any of:
        /usr/lib/go/src/github.com/go-gl/gl/v4.6-core/gl (from $GOROOT)
        REDACTED/src/go-pkgvet/testdata/src/github.com/go-gl/gl/v4.6-core/gl (from $GOPATH)

So in my case "analysistest: support modules" means it knowing how to look in the module cache for package sources.

@zhammer
Copy link

zhammer commented Dec 6, 2021

hey @tomarrell, can you explain how you resolved this by vendoring? tried grokking tomarrell/wrapcheck@5a99e6a but there's a lot there and don't see much mention of go mod.

i've run into this issue building analyzers & am not super familiar with vendoring. thanks :)

@joemiller
Copy link

joemiller commented Dec 7, 2021

@zhammer I'm not @tommarrel but here's how I resolved this:

https://github.com/planetscale/go-logkeycheck/tree/main/internal/logkeycheck/testdata

I did not vendor the entire set of dependencies into the repo (mostly due to personal preference). Instead, I created a make task (make src) that runs go mod vendor ; mv vendor src. This is called as a test dependency from the top-level makefile: https://github.com/planetscale/go-logkeycheck/blob/main/Makefile#L4-L5

EDIT: updated and fixed links

@rebornwwp
Copy link

Hi there! 👋 I'm not sure if this is an intended use case, but we love that we can write analysis.Analyzer for other Go modules using real dependencies in the testing code rather than stubbing them, which can be quick complex. This requires some careful dependency handling with vendoring and symlinks to make it work with analysistest.Run. Since files are copied between test runs its a little slow though. Full implementation: https://github.com/bflad/tfproviderlint / Example Analyzer: https://github.com/bflad/tfproviderlint/tree/master/passes/S037

It'd be neat if this could use something like Go 1.14's -modfile support or it just allowed using a go.mod in the testdata source. In our case, we would just point all our testing at the same Go Module file (contents at least).

Save my life. thanks a lot. 👍

@zhammer
Copy link

zhammer commented Dec 24, 2021

forgot to answer but thank you @joemiller! it’d be great if there were a slack/discord/community for folks writing golang codemods

@filipdukat
Copy link

Hey, any updates on this? I am writing analyzer that verifies if certain imports are used in a project. Including vendor folder in order to test the analyzer seems messy and I would like to avoid it, especially if I want to push the project to remote repository with CI.

@Antonboom

This comment was marked as spam.

@timothy-king
Copy link
Contributor

I do not believe there have been any updates on the Go team's side since #37054 (comment). A community contribution would still be welcomed.

@hyangah
Copy link
Contributor

hyangah commented May 25, 2022

Hi - I am proposing a new analyzer test run helper in #53063 - in the issue, my focus was to provide a replacement of Run. But reading the first couple of comments, I am not sure if my proposal is sufficient to meet all of the needs.

For populating test data (WriteFiles) in module-based structure, I found golang.org/x/tools/go/packages/packagestest.Export is flexible enough to capture different code layout. For proxy, or other operations, I also found golang.org/x/tools/internal/lsp/fake.Sandbox is quite powerful. I am pretty sure there are already third-party utilities that do something similar. Do we still want a WriteFiles-like utility from analysistest?

@karelbilek
Copy link

For anyone reading this - I hacked it around by using rogpeppe's testscript

https://github.com/rogpeppe/go-internal/tree/master/testscript

and manually adding the expect on all the lines. It's not as great as using analysistest, but I really want to use real go.mod without stubbing everything and figuring out how GOPATH works exactly again

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
@adonovan
Copy link
Member

Related: #46041

@romaindoumenc
Copy link

Closing in favor of #46041.

ldez added a commit to ldez/tools that referenced this issue Jun 26, 2023
Modules were explicitly disabled in analysistest. This lead to failures in tests where testdata required modules.

This change introduces new struct Options that contains configuration for a new func `RunWithOptions`.
`RunWithOptions` is called internally by `Run` and `RunWithSuggestedFixes`  with modules disabled for backward compatibility.
`RunWithOptions` can be run explicitly too, with a configuration allowing usage of modules.

Fixes golang/go#46041
Fixes golang/go#37054
ldez added a commit to ldez/tools that referenced this issue Jun 26, 2023
Modules were explicitly disabled in analysistest. This lead to failures in tests where testdata required modules.

This change introduces new struct Options that contains configuration for a new func `RunWithOptions`.
`RunWithOptions` is called internally by `Run` and `RunWithSuggestedFixes`  with modules disabled for backward compatibility.
`RunWithOptions` can be run explicitly too, with a configuration allowing usage of modules.

It also work with Go workspace.

Fixes golang/go#46041
Fixes golang/go#37054
ldez added a commit to ldez/tools that referenced this issue Jun 26, 2023
Modules were explicitly disabled in analysistest. This lead to failures in tests where testdata required modules.

This change introduces new struct Options that contains configuration for a new func `RunWithOptions`.
`RunWithOptions` is called internally by `Run` and `RunWithSuggestedFixes`  with modules disabled for backward compatibility.
`RunWithOptions` can be run explicitly too, with a configuration allowing usage of modules.

It also work with Go workspace.

Fixes golang/go#46041
Fixes golang/go#37054

Change-Id: I91a9365b46ea44afae79e78333720ea8c46489d7
@gopherbot
Copy link

Change https://go.dev/cl/506275 mentions this issue: go/analysis/analysistest: using modules

@gopherbot
Copy link

Change https://go.dev/cl/506355 mentions this issue: go/analysis/analysistest: sketch of packages.Load(testdata/go.mod)

@gopherbot
Copy link

Change https://go.dev/cl/506356 mentions this issue: go/analysis/analysistest: sketch of a way to support modules

@lambdanis
Copy link

lambdanis commented Jul 24, 2023

Making the testdata directory a separate Go module and vendoring dependencies under src/ seems an easy enough workaround for external import failures. However, I run into some trickiness with testdata consisting of multiple packages. To handle internal imports I used relative import paths, which are not supported by Go modules, so I had to ignore errors when vendoring:

go mod tidy -e && go mod vendor -e -o src/ && go mod verify

Anyway, it seems that a proper solution is incoming, looking forward to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted 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
Development

No branches or pull requests