From 567bb5a4fa4eb59a82ab01cceb6eae8fa4d01176 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Tue, 11 Aug 2020 16:42:32 +0000 Subject: [PATCH] go/analysis/analysistest: fix issue in RunWithSuggestedFixes on a package with tests The issue is described further in golang/go#40574. The fix is simply to do the suggested-fixes checking on each result separately. We still get duplicate messages, which is arguably the correct behavior but certainly not so bad, and we no longer get a bunch of spurious errors. Fixes golang/go#40574. Change-Id: I3540070e7ce2f20e65269f166b941e9c90436d01 GitHub-Last-Rev: 706ca751535dab820388fa25e7452f103c814c64 GitHub-Pull-Request: golang/tools#245 Reviewed-on: https://go-review.googlesource.com/c/tools/+/246737 Trust: Rebecca Stambler Run-TryBot: Rebecca Stambler gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Michael Matloob --- go/analysis/analysistest/analysistest.go | 145 ++++++++++-------- go/analysis/analysistest/analysistest_test.go | 19 ++- 2 files changed, 94 insertions(+), 70 deletions(-) diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index 5b22053d00..bb79cde893 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -97,12 +97,22 @@ type Testing interface { func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result { r := Run(t, dir, a, patterns...) - // file -> message -> edits - fileEdits := make(map[*token.File]map[string][]diff.TextEdit) - fileContents := make(map[*token.File][]byte) - - // Validate edits, prepare the fileEdits map and read the file contents. + // Process each result (package) separately, matching up the suggested + // fixes into a diff, which we will compare to the .golden file. We have + // to do this per-result in case a file appears in two packages, such as in + // packages with tests, where mypkg/a.go will appear in both mypkg and + // mypkg.test. In that case, the analyzer may suggest the same set of + // changes to a.go for each package. If we merge all the results, those + // changes get doubly applied, which will cause conflicts or mismatches. + // Validating the results separately means as long as the two analyses + // don't produce conflicting suggestions for a single file, everything + // should match up. for _, act := range r { + // file -> message -> edits + fileEdits := make(map[*token.File]map[string][]diff.TextEdit) + fileContents := make(map[*token.File][]byte) + + // Validate edits, prepare the fileEdits map and read the file contents. for _, diag := range act.Diagnostics { for _, sf := range diag.SuggestedFixes { for _, edit := range sf.TextEdits { @@ -142,78 +152,78 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns } } } - } - for file, fixes := range fileEdits { - // Get the original file contents. - orig, ok := fileContents[file] - if !ok { - t.Errorf("could not find file contents for %s", file.Name()) - continue - } - - // Get the golden file and read the contents. - ar, err := txtar.ParseFile(file.Name() + ".golden") - if err != nil { - t.Errorf("error reading %s.golden: %v", file.Name(), err) - continue - } - - if len(ar.Files) > 0 { - // one virtual file per kind of suggested fix - - if len(ar.Comment) != 0 { - // we allow either just the comment, or just virtual - // files, not both. it is not clear how "both" should - // behave. - t.Errorf("%s.golden has leading comment; we don't know what to do with it", file.Name()) + for file, fixes := range fileEdits { + // Get the original file contents. + orig, ok := fileContents[file] + if !ok { + t.Errorf("could not find file contents for %s", file.Name()) continue } - for sf, edits := range fixes { - found := false - for _, vf := range ar.Files { - if vf.Name == sf { - found = true - out := diff.ApplyEdits(string(orig), edits) - // the file may contain multiple trailing - // newlines if the user places empty lines - // between files in the archive. normalize - // this to a single newline. - want := string(bytes.TrimRight(vf.Data, "\n")) + "\n" - formatted, err := format.Source([]byte(out)) - if err != nil { - continue + // Get the golden file and read the contents. + ar, err := txtar.ParseFile(file.Name() + ".golden") + if err != nil { + t.Errorf("error reading %s.golden: %v", file.Name(), err) + continue + } + + if len(ar.Files) > 0 { + // one virtual file per kind of suggested fix + + if len(ar.Comment) != 0 { + // we allow either just the comment, or just virtual + // files, not both. it is not clear how "both" should + // behave. + t.Errorf("%s.golden has leading comment; we don't know what to do with it", file.Name()) + continue + } + + for sf, edits := range fixes { + found := false + for _, vf := range ar.Files { + if vf.Name == sf { + found = true + out := diff.ApplyEdits(string(orig), edits) + // the file may contain multiple trailing + // newlines if the user places empty lines + // between files in the archive. normalize + // this to a single newline. + want := string(bytes.TrimRight(vf.Data, "\n")) + "\n" + formatted, err := format.Source([]byte(out)) + if err != nil { + continue + } + if want != string(formatted) { + d := myers.ComputeEdits("", want, string(formatted)) + t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(fmt.Sprintf("%s.golden [%s]", file.Name(), sf), "actual", want, d)) + } + break } - if want != string(formatted) { - d := myers.ComputeEdits("", want, string(formatted)) - t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(fmt.Sprintf("%s.golden [%s]", file.Name(), sf), "actual", want, d)) - } - break + } + if !found { + t.Errorf("no section for suggested fix %q in %s.golden", sf, file.Name()) } } - if !found { - t.Errorf("no section for suggested fix %q in %s.golden", sf, file.Name()) + } else { + // all suggested fixes are represented by a single file + + var catchallEdits []diff.TextEdit + for _, edits := range fixes { + catchallEdits = append(catchallEdits, edits...) } - } - } else { - // all suggested fixes are represented by a single file - var catchallEdits []diff.TextEdit - for _, edits := range fixes { - catchallEdits = append(catchallEdits, edits...) - } + out := diff.ApplyEdits(string(orig), catchallEdits) + want := string(ar.Comment) - out := diff.ApplyEdits(string(orig), catchallEdits) - want := string(ar.Comment) - - formatted, err := format.Source([]byte(out)) - if err != nil { - continue - } - if want != string(formatted) { - d := myers.ComputeEdits("", want, string(formatted)) - t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(file.Name()+".golden", "actual", want, d)) + formatted, err := format.Source([]byte(out)) + if err != nil { + continue + } + if want != string(formatted) { + d := myers.ComputeEdits("", want, string(formatted)) + t.Errorf("suggested fixes failed for %s:\n%s", file.Name(), diff.ToUnified(file.Name()+".golden", "actual", want, d)) + } } } } @@ -320,7 +330,6 @@ func loadPackages(dir string, patterns ...string) ([]*packages.Package, error) { // specified by the contents of "// want ..." comments in the package's // source files, which must have been parsed with comments enabled. func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis.Diagnostic, facts map[types.Object][]analysis.Fact) { - type key struct { file string line int diff --git a/go/analysis/analysistest/analysistest_test.go b/go/analysis/analysistest/analysistest_test.go index 18c2f1eea9..e1ce857985 100644 --- a/go/analysis/analysistest/analysistest_test.go +++ b/go/analysis/analysistest/analysistest_test.go @@ -33,7 +33,8 @@ func TestTheTest(t *testing.T) { // which (by default) reports calls to functions named 'println'. findcall.Analyzer.Flags.Set("name", "println") - filemap := map[string]string{"a/b.go": `package main // want package:"found" + filemap := map[string]string{ + "a/b.go": `package main // want package:"found" func main() { // The expectation is ill-formed: @@ -108,7 +109,12 @@ func main() { // OK (facts and diagnostics on same line) func println(...interface{}) { println_TEST_() } // want println:"found" "call of println(...)" -`} +`, + "a/b_test.go": `package main + +// Test file shouldn't mess with things (issue #40574) +`, + } dir, cleanup, err := analysistest.WriteFiles(filemap) if err != nil { t.Fatal(err) @@ -128,6 +134,15 @@ func println(...interface{}) { println_TEST_() } // want println:"found" "call o `a/b.go:14:9: unexpected diagnostic: call of println(...)`, `a/b.go:11: no diagnostic was reported matching "wrong expectation text"`, `a/b.go:17: no diagnostic was reported matching "unsatisfied expectation"`, + // duplicate copies of each message from the test package (see issue #40574) + `a/b.go:5: in 'want' comment: unexpected ":"`, + `a/b.go:6: in 'want' comment: got String after foo, want ':'`, + `a/b.go:7: in 'want' comment: got EOF, want regular expression`, + `a/b.go:8: in 'want' comment: invalid char escape`, + `a/b.go:11:9: diagnostic "call of println(...)" does not match pattern "wrong expectation text"`, + `a/b.go:14:9: unexpected diagnostic: call of println(...)`, + `a/b.go:11: no diagnostic was reported matching "wrong expectation text"`, + `a/b.go:17: no diagnostic was reported matching "unsatisfied expectation"`, } if !reflect.DeepEqual(got, want) { t.Errorf("got:\n%s\nwant:\n%s",