diff --git a/go/analysis/internal/checker/checker_test.go b/go/analysis/internal/checker/checker_test.go index 46c74d723..a73a0967c 100644 --- a/go/analysis/internal/checker/checker_test.go +++ b/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{ diff --git a/go/analysis/passes/loopclosure/loopclosure_test.go b/go/analysis/passes/loopclosure/loopclosure_test.go index 03b810700..9b32c1495 100644 --- a/go/analysis/passes/loopclosure/loopclosure_test.go +++ b/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") } diff --git a/go/analysis/unitchecker/separate_test.go b/go/analysis/unitchecker/separate_test.go index 00c5aec70..827ca4c36 100644 --- a/go/analysis/unitchecker/separate_test.go +++ b/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{ diff --git a/internal/drivertest/driver_test.go b/internal/drivertest/driver_test.go index be5cd78e6..c1e3729a2 100644 --- a/internal/drivertest/driver_test.go +++ b/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) } diff --git a/internal/testfiles/testdata/versions/sub.test/sub.go b/internal/testfiles/testdata/versions/sub.test/sub.go.test similarity index 100% rename from internal/testfiles/testdata/versions/sub.test/sub.go rename to internal/testfiles/testdata/versions/sub.test/sub.go.test diff --git a/internal/testfiles/testfiles.go b/internal/testfiles/testfiles.go index c8a2bd947..ec5fc7f69 100644 --- a/internal/testfiles/testfiles.go +++ b/internal/testfiles/testfiles.go @@ -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) + } + for _, r := range rename { + old, new, found := strings.Cut(r, ",") + if !found { + t.Fatalf("rename directive %q does not contain delimiter %q", r, ",") + } + oldpath := filepath.Join(dstdir, old) + newpath := filepath.Join(dstdir, new) + if err := os.Rename(oldpath, newpath); err != nil { + 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 - } - if strings.HasSuffix(path, ".test") { - rename = append(rename, path) - } - 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) + fs, err := txtar.FS(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) - if err != nil { - t.Fatal(err) - } - return dir + return CopyToTmp(t, fs) } diff --git a/internal/testfiles/testfiles_test.go b/internal/testfiles/testfiles_test.go index d9563260e..67951a097 100644 --- a/internal/testfiles/testfiles_test.go +++ b/internal/testfiles/testfiles_test.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) + } -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") - } + if want := 4; got != want { + t.Errorf("Got %d diagnostics. wanted %d", got, want) } } + +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) +}