mirror
/
go
mirror of https://github.com/golang/go.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
This commit is contained in:
Ben Kraft 2020-08-11 16:42:32 +00:00 committed by Rebecca Stambler
parent 90a82dd33a
commit 567bb5a4fa
2 changed files with 94 additions and 70 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

145
go/analysis/analysistest/analysistest.go
View File

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

19
go/analysis/analysistest/analysistest_test.go
View File

@ -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",