internal/testfiles: consolidate to CopyToTmp

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]>

Author: timothy-king

go/analysis/internal/checker/checker_test.go

@@ -299,10 +299,11 @@ hello from other
 `
 
 	// Expand archive into tmp tree.
-	tmpdir := t.TempDir()
-	if err := testfiles.ExtractTxtar(tmpdir, txtar.Parse([]byte(src))); err != nil {
+	fs, err := txtar.FS(txtar.Parse([]byte(src)))
+	if err != nil {
 		t.Fatal(err)
 	}
+	tmpdir := testfiles.CopyToTmp(t, fs)
 
 	ran := false
 	a := &analysis.Analyzer{

go/analysis/passes/loopclosure/loopclosure_test.go

@@ -26,13 +26,11 @@ func Test(t *testing.T) {
 func TestVersions22(t *testing.T) {
 	testenv.NeedsGo1Point(t, 22)
 
-	txtar := filepath.Join(analysistest.TestData(), "src", "versions", "go22.txtar")
-	dir := testfiles.ExtractTxtarFileToTmp(t, txtar)
+	dir := testfiles.ExtractTxtarFileToTmp(t, filepath.Join(analysistest.TestData(), "src", "versions", "go22.txtar"))
 	analysistest.Run(t, dir, loopclosure.Analyzer, "golang.org/fake/versions")
 }
 
 func TestVersions18(t *testing.T) {
-	txtar := filepath.Join(analysistest.TestData(), "src", "versions", "go18.txtar")
-	dir := testfiles.ExtractTxtarFileToTmp(t, txtar)
+	dir := testfiles.ExtractTxtarFileToTmp(t, filepath.Join(analysistest.TestData(), "src", "versions", "go18.txtar"))
 	analysistest.Run(t, dir, loopclosure.Analyzer, "golang.org/fake/versions")
 }

go/analysis/unitchecker/separate_test.go

@@ -82,10 +82,11 @@ func MyPrintf(format string, args ...any) {
 `
 
 	// Expand archive into tmp tree.
-	tmpdir := t.TempDir()
-	if err := testfiles.ExtractTxtar(tmpdir, txtar.Parse([]byte(src))); err != nil {
+	fs, err := txtar.FS(txtar.Parse([]byte(src)))
+	if err != nil {
 		t.Fatal(err)
 	}
+	tmpdir := testfiles.CopyToTmp(t, fs)
 
 	// Load metadata for the main package and all its dependencies.
 	cfg := &packages.Config{

internal/drivertest/driver_test.go

@@ -44,11 +44,15 @@ package m
 package lib
 `
 
-	dir := testfiles.ExtractTxtarToTmp(t, txtar.Parse([]byte(workspace)))
+	fs, err := txtar.FS(txtar.Parse([]byte(workspace)))
+	if err != nil {
+		t.Fatal(err)
+	}
+	dir := testfiles.CopyToTmp(t, fs)
 
 	// TODO(rfindley): on mac, this is required to fix symlink path mismatches.
 	// But why? Where is the symlink being evaluated in go/packages?
-	dir, err := filepath.EvalSymlinks(dir)
+	dir, err = filepath.EvalSymlinks(dir)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/testfiles/testdata/versions/sub.test/sub.go renamed to internal/testfiles/testdata/versions/sub.test/sub.go.test

@@ -17,20 +17,14 @@ import (
 	"golang.org/x/tools/txtar"
 )
 
-// CopyDirToTmp copies dir to a temporary test directory using
-// CopyTestFiles and returns the path to the test directory.
-func CopyDirToTmp(t testing.TB, srcdir string) string {
-	dst := t.TempDir()
-	if err := CopyFS(dst, os.DirFS(srcdir)); err != nil {
-		t.Fatal(err)
-	}
-	return dst
-}
-
-// CopyFS copies the files and directories in src to a
-// destination directory dst. Paths to files and directories
-// ending in a ".test" extension have the ".test" extension
-// removed. This allows tests to hide files whose names have
+// 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.
 //
@@ -41,42 +35,31 @@ func CopyDirToTmp(t testing.TB, srcdir string) string {
 //	    a/a.go
 //	    b/b.go
 //
-// The resulting files will be:
+// with the rename "go.mod.test,go.mod", the resulting files will be:
 //
 //	dst/
 //	    go.mod
 //	    a/a.go
 //	    b/b.go
-func CopyFS(dstdir string, src fs.FS) error {
+func CopyToTmp(t testing.TB, src fs.FS, rename ...string) string {
+	dstdir := t.TempDir()
+
 	if err := copyFS(dstdir, src); err != nil {
-		return err
+		t.Fatal(err)
 	}
-
-	// Collect ".test" paths in lexical order.
-	var rename []string
-	err := fs.WalkDir(os.DirFS(dstdir), ".", func(path string, d fs.DirEntry, err error) error {
-		if err != nil {
-			return err
+	for _, r := range rename {
+		old, new, found := strings.Cut(r, ",")
+		if !found {
+			t.Fatalf("rename directive %q does not contain delimiter %q", r, ",")
 		}
-		if strings.HasSuffix(path, ".test") {
-			rename = append(rename, path)
+		oldpath := filepath.Join(dstdir, old)
+		newpath := filepath.Join(dstdir, new)
+		if err := os.Rename(oldpath, newpath); err != nil {
+			t.Fatal(err)
 		}
-		return nil
-	})
-	if err != nil {
-		return err
 	}
 
-	// Rename the .test paths in reverse lexical order, e.g.
-	// in d.test/a.test renames a.test to d.test/a then d.test to d.
-	for i := len(rename) - 1; i >= 0; i-- {
-		oldpath := filepath.Join(dstdir, rename[i])
-		newpath := strings.TrimSuffix(oldpath, ".test")
-		if err != os.Rename(oldpath, newpath) {
-			return err
-		}
-	}
-	return nil
+	return dstdir
 }
 
 // Copy the files in src to dst.
@@ -106,46 +89,18 @@ func copyFS(dstdir string, src fs.FS) error {
 	})
 }
 
-// ExtractTxtar writes each archive file to the corresponding location beneath dir.
-//
-// TODO(adonovan): move this to txtar package, we need it all the time (#61386).
-func ExtractTxtar(dstdir string, ar *txtar.Archive) error {
-	for _, file := range ar.Files {
-		name := filepath.Join(dstdir, file.Name)
-		if err := os.MkdirAll(filepath.Dir(name), 0777); err != nil {
-			return err
-		}
-		if err := os.WriteFile(name, file.Data, 0666); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 // ExtractTxtarFileToTmp read a txtar archive on a given path,
 // extracts it to a temporary directory, and returns the
 // temporary directory.
-func ExtractTxtarFileToTmp(t testing.TB, archiveFile string) string {
-	ar, err := txtar.ParseFile(archiveFile)
+func ExtractTxtarFileToTmp(t testing.TB, file string) string {
+	ar, err := txtar.ParseFile(file)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	dir := t.TempDir()
-	err = ExtractTxtar(dir, ar)
-	if err != nil {
-		t.Fatal(err)
-	}
-	return dir
-}
-
-// ExtractTxtarToTmp extracts the given archive to a temp directory, and
-// returns that temporary directory.
-func ExtractTxtarToTmp(t testing.TB, ar *txtar.Archive) string {
-	dir := t.TempDir()
-	err := ExtractTxtar(dir, ar)
+	fs, err := txtar.FS(ar)
 	if err != nil {
 		t.Fatal(err)
 	}
-	return dir
+	return CopyToTmp(t, fs)
 }

internal/testfiles/testfiles.go

@@ -5,6 +5,7 @@
 package testfiles_test
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -14,16 +15,20 @@ import (
 	"golang.org/x/tools/internal/testenv"
 	"golang.org/x/tools/internal/testfiles"
 	"golang.org/x/tools/internal/versions"
+	"golang.org/x/tools/txtar"
 )
 
 func TestTestDir(t *testing.T) {
 	testenv.NeedsGo1Point(t, 22)
 
-	// TODO(taking): Expose a helper for this pattern?
-	// dir must contain a go.mod file to be picked up by Run().
-	// So this pattern or Join(TestDir(t, TestData()), "versions")  are
-	// probably what everyone will want.
-	dir := testfiles.CopyDirToTmp(t, filepath.Join(analysistest.TestData(), "versions"))
+	// Files are initially {go.mod.test,sub.test/sub.go.test}.
+	fs := os.DirFS(filepath.Join(analysistest.TestData(), "versions"))
+	tmpdir := testfiles.CopyToTmp(t, fs,
+		"go.mod.test,go.mod",                // After: {go.mod,sub.test/sub.go.test}
+		"sub.test/sub.go.test,sub.test/abc", // After: {go.mod,sub.test/abc}
+		"sub.test,sub",                      // After: {go.mod,sub/abc}
+		"sub/abc,sub/sub.go",                // After: {go.mod,sub/sub.go}
+	)
 
 	filever := &analysis.Analyzer{
 		Name: "filever",
@@ -37,18 +42,54 @@ func TestTestDir(t *testing.T) {
 			return nil, nil
 		},
 	}
-	analysistest.Run(t, dir, filever, "golang.org/fake/versions", "golang.org/fake/versions/sub")
+	res := analysistest.Run(t, tmpdir, filever, "golang.org/fake/versions", "golang.org/fake/versions/sub")
+	got := 0
+	for _, r := range res {
+		got += len(r.Diagnostics)
+	}
+
+	if want := 4; got != want {
+		t.Errorf("Got %d diagnostics. wanted %d", got, want)
+	}
 }
 
-func TestCopyTestFilesErrors(t *testing.T) {
-	tmp := t.TempDir() // a real tmp dir
-	for _, dir := range []string{
-		filepath.Join(analysistest.TestData(), "not_there"),    // dir does not exist
-		filepath.Join(analysistest.TestData(), "somefile.txt"), // not a dir
-	} {
-		err := testfiles.CopyFS(tmp, os.DirFS(dir))
-		if err == nil {
-			t.Error("Expected an error from CopyTestFiles")
-		}
+func TestTestDirErrors(t *testing.T) {
+	const input = `
+-- one.txt --
+one
+`
+	// Files are initially {go.mod.test,sub.test/sub.go.test}.
+	fs, err := txtar.FS(txtar.Parse([]byte(input)))
+	if err != nil {
+		t.Fatal(err)
 	}
+
+	directive := "no comma to split on"
+	intercept := &fatalIntercept{t, nil}
+	func() {
+		defer func() { // swallow panics from fatalIntercept.Fatal
+			if r := recover(); r != intercept {
+				panic(r)
+			}
+		}()
+		testfiles.CopyToTmp(intercept, fs, directive)
+	}()
+
+	got := fmt.Sprint(intercept.fatalfs)
+	want := `[rename directive "no comma to split on" does not contain delimiter ","]`
+	if got != want {
+		t.Errorf("CopyToTmp(%q) had the Fatal messages %q. wanted %q", directive, got, want)
+	}
+}
+
+// helper for TestTestDirErrors
+type fatalIntercept struct {
+	testing.TB
+	fatalfs []string
+}
+
+func (i *fatalIntercept) Fatalf(format string, args ...any) {
+	i.fatalfs = append(i.fatalfs, fmt.Sprintf(format, args...))
+	// Do not mark the test as failing, but fail early.
+	panic(i)
 }