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: x/tools/testfiles: provide utilities for testing tools for Go code with modules #68408

Open
timothy-king opened this issue Jul 12, 2024 · 6 comments
Labels
Milestone

Comments

@timothy-king
Copy link
Contributor

timothy-king commented Jul 12, 2024

Updated July 30.

Proposal Details

The proposal is to move the existing internal x/tools/internal/testfiles.CopyToTmp function for testing Go code with go.mod files into a new exported package x/tools/testfiles. This function creates a temporary testing directory from an io/fs.FS and then potentially renaming files.

// CopyToTmp copies the files and directories in src to a new temporary testing
// directory dst, and returns dst on success.
//
// After copying the files, it processes each of the 'old,new,' rename
// directives in order. Each rename directive moves the relative path "old"
// to the relative path "new" within the directory.
//
// Renaming allows tests to hide files whose names have
// special meaning, such as "go.mod" files or "testdata" directories
// from the go command, or ill-formed Go source files from gofmt.
func CopyToTmp(t testing.TB, src fs.FS, rename ...string) string

The potential renaming allows for populating the new directory with files names significant to Go, such as go.mod, without checking in files with special names. For example if we copy the directory testdata:

	testdata/
	    go.mod.test
	    a/a.go
	    b/b.go

using `dir := testfiles.CopyToTmp(t, os.DirFS("testdata"), "go.mod.test,go.mod"), the resulting files will be:

	dir/
	    go.mod
	    a/a.go
	    b/b.go

This package was originally created to support x/tools/analysistest.Run. From it's documentation:

func Run(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result
Run applies an analysis to the packages denoted by the "go list" patterns.

It loads the packages from the specified directory using golang.org/x/tools/go/packages, runs the analysis on them, and checks that each analysis emits the expected diagnostics and facts [...] .

If the directory contains a go.mod file, Run treats it as the root of the Go module in which to work.

Adopting this proposal would resolve #37054, #46041, #53063, and #61336.

Note this proposal works well with both testdata/ directories using os.DirFS, and txtar archives using x/tools/txtar.FS (#44158).

@gopherbot gopherbot added this to the Proposal milestone Jul 12, 2024
@earthboundkid
Copy link
Contributor

earthboundkid commented Jul 14, 2024

See #62484 (os.CopyFS) and #44158 (txtar fs.FS).

@timothy-king
Copy link
Contributor Author

@earthboundkid Do you want to submit the CL you started for #44158? Or anything else I could do to help it get over the finish line? I think this would lead to a nice simplification to this proposal.

@earthboundkid
Copy link
Contributor

I have been too busy to work on #44158 and I'm not sure what the best way to implement ErrModified is, so if you want to take over, feel free. 😄

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/598996 mentions this issue: internal/testfiles: consolidate API

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Jul 24, 2024
gopherbot pushed a commit to golang/tools that referenced this issue Jul 26, 2024
Use the new txtar.FS function to consolidate API around fs.FS.
This has been simplified to two functions: ExtractTxtarFileToTmp,
and CopyToTmp. CopyToTmp is a combination replacement for CopyFS and
CopyDirToTmp. The main distinction is that it now takes an explicit
renaming map instead of implicitly removing ".test" extensions.

Updates golang/go#68408

Change-Id: I9558044ec4613835327c0b0a5e8d1cc8fe847d59
Reviewed-on: https://go-review.googlesource.com/c/tools/+/598996
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Tim King <[email protected]>
@timothy-king
Copy link
Contributor Author

Updated the proposal with the new consolidated API.

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

No branches or pull requests

4 participants