From 68de5508d374c910cc0bf066b8f65cd5395115b1 Mon Sep 17 00:00:00 2001 From: Yury Smolsky Date: Tue, 22 May 2018 22:37:40 +0300 Subject: [PATCH] cmd/vet: eliminate use of Perl in tests This change uses errorCheck and wantedErrors functions copied from the test/run.go to eliminate use of the test/errchk perl script. Tests' error messages that contained full filenames were changed to have base filenames because the errorCheck function processes output from "go vet" in the same way. Fixes #20032. Change-Id: Ieb7be67c2d7281b9648171c698398449b7e2d4dd Reviewed-on: https://go-review.googlesource.com/114176 Run-TryBot: Yury Smolsky TryBot-Result: Gobot Gobot Reviewed-by: Alan Donovan --- src/cmd/vet/testdata/atomic.go | 2 +- src/cmd/vet/testdata/shadow.go | 8 +- src/cmd/vet/testdata/structtag.go | 18 +- src/cmd/vet/vet_test.go | 285 +++++++++++++++++++++++++----- 4 files changed, 255 insertions(+), 58 deletions(-) diff --git a/src/cmd/vet/testdata/atomic.go b/src/cmd/vet/testdata/atomic.go index 8b587567c7..69730b4e6f 100644 --- a/src/cmd/vet/testdata/atomic.go +++ b/src/cmd/vet/testdata/atomic.go @@ -43,7 +43,7 @@ func AtomicTests() { { // A variable declaration creates a new variable in the current scope. - x := atomic.AddUint64(&x, 1) // ERROR "declaration of .x. shadows declaration at testdata/atomic.go:16" + x := atomic.AddUint64(&x, 1) // ERROR "declaration of .x. shadows declaration at atomic.go:16" // Re-declaration assigns a new value. x, w := atomic.AddUint64(&x, 1), 10 // ERROR "direct assignment to atomic value" diff --git a/src/cmd/vet/testdata/shadow.go b/src/cmd/vet/testdata/shadow.go index 3b61137b87..c55cb2772a 100644 --- a/src/cmd/vet/testdata/shadow.go +++ b/src/cmd/vet/testdata/shadow.go @@ -17,7 +17,7 @@ func ShadowRead(f *os.File, buf []byte) (err error) { _ = err } if f != nil { - _, err := f.Read(buf) // ERROR "declaration of .err. shadows declaration at testdata/shadow.go:13" + _, err := f.Read(buf) // ERROR "declaration of .err. shadows declaration at shadow.go:13" if err != nil { return err } @@ -25,8 +25,8 @@ func ShadowRead(f *os.File, buf []byte) (err error) { _ = i } if f != nil { - x := one() // ERROR "declaration of .x. shadows declaration at testdata/shadow.go:14" - var _, err = f.Read(buf) // ERROR "declaration of .err. shadows declaration at testdata/shadow.go:13" + x := one() // ERROR "declaration of .x. shadows declaration at shadow.go:14" + var _, err = f.Read(buf) // ERROR "declaration of .err. shadows declaration at shadow.go:13" if x == 1 && err != nil { return err } @@ -46,7 +46,7 @@ func ShadowRead(f *os.File, buf []byte) (err error) { if shadowTemp := shadowTemp; true { // OK: obviously intentional idiomatic redeclaration var f *os.File // OK because f is not mentioned later in the function. // The declaration of x is a shadow because x is mentioned below. - var x int // ERROR "declaration of .x. shadows declaration at testdata/shadow.go:14" + var x int // ERROR "declaration of .x. shadows declaration at shadow.go:14" _, _, _ = x, f, shadowTemp } // Use a couple of variables to trigger shadowing errors. diff --git a/src/cmd/vet/testdata/structtag.go b/src/cmd/vet/testdata/structtag.go index c87e42f5d0..ce21e803c8 100644 --- a/src/cmd/vet/testdata/structtag.go +++ b/src/cmd/vet/testdata/structtag.go @@ -44,40 +44,40 @@ type AnonymousXML struct{} type DuplicateJSONFields struct { JSON int `json:"a"` - DuplicateJSON int `json:"a"` // ERROR "struct field DuplicateJSON repeats json tag .a. also at testdata/structtag.go:46" + DuplicateJSON int `json:"a"` // ERROR "struct field DuplicateJSON repeats json tag .a. also at structtag.go:46" IgnoredJSON int `json:"-"` OtherIgnoredJSON int `json:"-"` OmitJSON int `json:",omitempty"` OtherOmitJSON int `json:",omitempty"` - DuplicateOmitJSON int `json:"a,omitempty"` // ERROR "struct field DuplicateOmitJSON repeats json tag .a. also at testdata/structtag.go:46" + DuplicateOmitJSON int `json:"a,omitempty"` // ERROR "struct field DuplicateOmitJSON repeats json tag .a. also at structtag.go:46" NonJSON int `foo:"a"` DuplicateNonJSON int `foo:"a"` Embedded struct { DuplicateJSON int `json:"a"` // OK because its not in the same struct type } - AnonymousJSON `json:"a"` // ERROR "struct field AnonymousJSON repeats json tag .a. also at testdata/structtag.go:46" + AnonymousJSON `json:"a"` // ERROR "struct field AnonymousJSON repeats json tag .a. also at structtag.go:46" XML int `xml:"a"` - DuplicateXML int `xml:"a"` // ERROR "struct field DuplicateXML repeats xml tag .a. also at testdata/structtag.go:60" + DuplicateXML int `xml:"a"` // ERROR "struct field DuplicateXML repeats xml tag .a. also at structtag.go:60" IgnoredXML int `xml:"-"` OtherIgnoredXML int `xml:"-"` OmitXML int `xml:",omitempty"` OtherOmitXML int `xml:",omitempty"` - DuplicateOmitXML int `xml:"a,omitempty"` // ERROR "struct field DuplicateOmitXML repeats xml tag .a. also at testdata/structtag.go:60" + DuplicateOmitXML int `xml:"a,omitempty"` // ERROR "struct field DuplicateOmitXML repeats xml tag .a. also at structtag.go:60" NonXML int `foo:"a"` DuplicateNonXML int `foo:"a"` Embedded struct { DuplicateXML int `xml:"a"` // OK because its not in the same struct type } - AnonymousXML `xml:"a"` // ERROR "struct field AnonymousXML repeats xml tag .a. also at testdata/structtag.go:60" + AnonymousXML `xml:"a"` // ERROR "struct field AnonymousXML repeats xml tag .a. also at structtag.go:60" Attribute struct { XMLName xml.Name `xml:"b"` NoDup int `xml:"b"` // OK because XMLName above affects enclosing struct. Attr int `xml:"b,attr"` // OK because 0 is valid. - DupAttr int `xml:"b,attr"` // ERROR "struct field DupAttr repeats xml attribute tag .b. also at testdata/structtag.go:76" - DupOmitAttr int `xml:"b,omitempty,attr"` // ERROR "struct field DupOmitAttr repeats xml attribute tag .b. also at testdata/structtag.go:76" + DupAttr int `xml:"b,attr"` // ERROR "struct field DupAttr repeats xml attribute tag .b. also at structtag.go:76" + DupOmitAttr int `xml:"b,omitempty,attr"` // ERROR "struct field DupOmitAttr repeats xml attribute tag .b. also at structtag.go:76" - AnonymousXML `xml:"b,attr"` // ERROR "struct field AnonymousXML repeats xml attribute tag .b. also at testdata/structtag.go:76" + AnonymousXML `xml:"b,attr"` // ERROR "struct field AnonymousXML repeats xml attribute tag .b. also at structtag.go:76" } } diff --git a/src/cmd/vet/vet_test.go b/src/cmd/vet/vet_test.go index f654d4679e..ecb4ce1295 100644 --- a/src/cmd/vet/vet_test.go +++ b/src/cmd/vet/vet_test.go @@ -6,12 +6,17 @@ package main_test import ( "bytes" + "errors" "fmt" "internal/testenv" + "io/ioutil" + "log" "os" "os/exec" "path/filepath" + "regexp" "runtime" + "strconv" "strings" "sync" "testing" @@ -19,7 +24,7 @@ import ( const ( dataDir = "testdata" - binary = "testvet.exe" + binary = "./testvet.exe" ) // We implement TestMain so remove the test binary when all is done. @@ -29,16 +34,6 @@ func TestMain(m *testing.M) { os.Exit(result) } -func MustHavePerl(t *testing.T) { - switch runtime.GOOS { - case "plan9", "windows": - t.Skipf("skipping test: perl not available on %s", runtime.GOOS) - } - if _, err := exec.LookPath("perl"); err != nil { - t.Skipf("skipping test: perl not found in path") - } -} - var ( buildMu sync.Mutex // guards following built = false // We have built the binary. @@ -55,7 +50,6 @@ func Build(t *testing.T) { t.Skip("cannot run on this environment") } testenv.MustHaveGoBuild(t) - MustHavePerl(t) cmd := exec.Command(testenv.GoToolPath(t), "build", "-o", binary) output, err := cmd.CombinedOutput() if err != nil { @@ -67,23 +61,19 @@ func Build(t *testing.T) { } func Vet(t *testing.T, files []string) { - errchk := filepath.Join(runtime.GOROOT(), "test", "errchk") flags := []string{ - "./" + binary, "-printfuncs=Warn:1,Warnf:1", "-all", "-shadow", } - cmd := exec.Command(errchk, append(flags, files...)...) - if !run(cmd, t) { - t.Fatal("vet command failed") - } + cmd := exec.Command(binary, append(flags, files...)...) + errchk(cmd, files, t) } -// Run this shell script, but do it in Go so it can be run by "go test". -// go build -o testvet -// $(GOROOT)/test/errchk ./testvet -shadow -printfuncs='Warn:1,Warnf:1' testdata/*.go testdata/*.s -// rm testvet +// TestVet is equivalent to running this: +// go build -o ./testvet +// errorCheck the output of ./testvet -shadow -printfuncs='Warn:1,Warnf:1' testdata/*.go testdata/*.s +// rm ./testvet // // TestVet tests self-contained files in testdata/*.go. @@ -95,7 +85,6 @@ func TestVet(t *testing.T) { Build(t) t.Parallel() - // errchk ./testvet gos, err := filepath.Glob(filepath.Join(dataDir, "*.go")) if err != nil { t.Fatal(err) @@ -128,17 +117,14 @@ func TestVet(t *testing.T) { func TestVetPrint(t *testing.T) { Build(t) - errchk := filepath.Join(runtime.GOROOT(), "test", "errchk") + file := filepath.Join("testdata", "print.go") cmd := exec.Command( - errchk, - "go", "vet", "-vettool=./"+binary, + "go", "vet", "-vettool="+binary, "-printf", "-printfuncs=Warn:1,Warnf:1", - "testdata/print.go", + file, ) - if !run(cmd, t) { - t.Fatal("vet command failed") - } + errchk(cmd, []string{file}, t) } func TestVetAsm(t *testing.T) { @@ -155,7 +141,6 @@ func TestVetAsm(t *testing.T) { } t.Parallel() - // errchk ./testvet Vet(t, append(gos, asms...)) } @@ -181,23 +166,20 @@ func TestVetDirs(t *testing.T) { } } -func run(c *exec.Cmd, t *testing.T) bool { +func errchk(c *exec.Cmd, files []string, t *testing.T) { output, err := c.CombinedOutput() - if err != nil { + if _, ok := err.(*exec.ExitError); !ok { t.Logf("vet output:\n%s", output) t.Fatal(err) } - // Errchk delights by not returning non-zero status if it finds errors, so we look at the output. - // It prints "BUG" if there is a failure. - if !c.ProcessState.Success() { - t.Logf("vet output:\n%s", output) - return false + fullshort := make([]string, 0, len(files)*2) + for _, f := range files { + fullshort = append(fullshort, f, filepath.Base(f)) } - ok := !bytes.Contains(output, []byte("BUG")) - if !ok { - t.Logf("vet output:\n%s", output) + err = errorCheck(string(output), false, fullshort...) + if err != nil { + t.Errorf("error check failed: %s", err) } - return ok } // TestTags verifies that the -tags argument controls which files to check. @@ -214,7 +196,7 @@ func TestTags(t *testing.T) { "-v", // We're going to look at the files it examines. "testdata/tagtest", } - cmd := exec.Command("./"+binary, args...) + cmd := exec.Command(binary, args...) output, err := cmd.CombinedOutput() if err != nil { t.Fatal(err) @@ -234,10 +216,225 @@ func TestTags(t *testing.T) { func TestVetVerbose(t *testing.T) { t.Parallel() Build(t) - cmd := exec.Command("./"+binary, "-v", "-all", "testdata/cgo/cgo3.go") + cmd := exec.Command(binary, "-v", "-all", "testdata/cgo/cgo3.go") out, err := cmd.CombinedOutput() if err != nil { t.Logf("%s", out) t.Error(err) } } + +// All declarations below were adapted from test/run.go. + +// errorCheck matches errors in outStr against comments in source files. +// For each line of the source files which should generate an error, +// there should be a comment of the form // ERROR "regexp". +// If outStr has an error for a line which has no such comment, +// this function will report an error. +// Likewise if outStr does not have an error for a line which has a comment, +// or if the error message does not match the . +// The syntax is Perl but its best to stick to egrep. +// +// Sources files are supplied as fullshort slice. +// It consists of pairs: full path to source file and it's base name. +func errorCheck(outStr string, wantAuto bool, fullshort ...string) (err error) { + var errs []error + out := splitOutput(outStr, wantAuto) + // Cut directory name. + for i := range out { + for j := 0; j < len(fullshort); j += 2 { + full, short := fullshort[j], fullshort[j+1] + out[i] = strings.Replace(out[i], full, short, -1) + } + } + + var want []wantedError + for j := 0; j < len(fullshort); j += 2 { + full, short := fullshort[j], fullshort[j+1] + want = append(want, wantedErrors(full, short)...) + } + for _, we := range want { + var errmsgs []string + if we.auto { + errmsgs, out = partitionStrings("", out) + } else { + errmsgs, out = partitionStrings(we.prefix, out) + } + if len(errmsgs) == 0 { + errs = append(errs, fmt.Errorf("%s:%d: missing error %q", we.file, we.lineNum, we.reStr)) + continue + } + matched := false + n := len(out) + for _, errmsg := range errmsgs { + // Assume errmsg says "file:line: foo". + // Cut leading "file:line: " to avoid accidental matching of file name instead of message. + text := errmsg + if i := strings.Index(text, " "); i >= 0 { + text = text[i+1:] + } + if we.re.MatchString(text) { + matched = true + } else { + out = append(out, errmsg) + } + } + if !matched { + errs = append(errs, fmt.Errorf("%s:%d: no match for %#q in:\n\t%s", we.file, we.lineNum, we.reStr, strings.Join(out[n:], "\n\t"))) + continue + } + } + + if len(out) > 0 { + errs = append(errs, fmt.Errorf("Unmatched Errors:")) + for _, errLine := range out { + errs = append(errs, fmt.Errorf("%s", errLine)) + } + } + + if len(errs) == 0 { + return nil + } + if len(errs) == 1 { + return errs[0] + } + var buf bytes.Buffer + fmt.Fprintf(&buf, "\n") + for _, err := range errs { + fmt.Fprintf(&buf, "%s\n", err.Error()) + } + return errors.New(buf.String()) +} + +func splitOutput(out string, wantAuto bool) []string { + // gc error messages continue onto additional lines with leading tabs. + // Split the output at the beginning of each line that doesn't begin with a tab. + // lines are impossible to match so those are filtered out. + var res []string + for _, line := range strings.Split(out, "\n") { + line = strings.TrimSuffix(line, "\r") // normalize Windows output + if strings.HasPrefix(line, "\t") { + res[len(res)-1] += "\n" + line + } else if strings.HasPrefix(line, "go tool") || strings.HasPrefix(line, "#") || !wantAuto && strings.HasPrefix(line, "") { + continue + } else if strings.TrimSpace(line) != "" { + res = append(res, line) + } + } + return res +} + +// matchPrefix reports whether s starts with file name prefix followed by a :, +// and possibly preceded by a directory name. +func matchPrefix(s, prefix string) bool { + i := strings.Index(s, ":") + if i < 0 { + return false + } + j := strings.LastIndex(s[:i], "/") + s = s[j+1:] + if len(s) <= len(prefix) || s[:len(prefix)] != prefix { + return false + } + if s[len(prefix)] == ':' { + return true + } + return false +} + +func partitionStrings(prefix string, strs []string) (matched, unmatched []string) { + for _, s := range strs { + if matchPrefix(s, prefix) { + matched = append(matched, s) + } else { + unmatched = append(unmatched, s) + } + } + return +} + +type wantedError struct { + reStr string + re *regexp.Regexp + lineNum int + auto bool // match line + file string + prefix string +} + +var ( + errRx = regexp.MustCompile(`// (?:GC_)?ERROR (.*)`) + errAutoRx = regexp.MustCompile(`// (?:GC_)?ERRORAUTO (.*)`) + errQuotesRx = regexp.MustCompile(`"([^"]*)"`) + lineRx = regexp.MustCompile(`LINE(([+-])([0-9]+))?`) +) + +// wantedErrors parses expected errors from comments in a file. +func wantedErrors(file, short string) (errs []wantedError) { + cache := make(map[string]*regexp.Regexp) + + src, err := ioutil.ReadFile(file) + if err != nil { + log.Fatal(err) + } + for i, line := range strings.Split(string(src), "\n") { + lineNum := i + 1 + if strings.Contains(line, "////") { + // double comment disables ERROR + continue + } + var auto bool + m := errAutoRx.FindStringSubmatch(line) + if m != nil { + auto = true + } else { + m = errRx.FindStringSubmatch(line) + } + if m == nil { + continue + } + all := m[1] + mm := errQuotesRx.FindAllStringSubmatch(all, -1) + if mm == nil { + log.Fatalf("%s:%d: invalid errchk line: %s", file, lineNum, line) + } + for _, m := range mm { + replacedOnce := false + rx := lineRx.ReplaceAllStringFunc(m[1], func(m string) string { + if replacedOnce { + return m + } + replacedOnce = true + n := lineNum + if strings.HasPrefix(m, "LINE+") { + delta, _ := strconv.Atoi(m[5:]) + n += delta + } else if strings.HasPrefix(m, "LINE-") { + delta, _ := strconv.Atoi(m[5:]) + n -= delta + } + return fmt.Sprintf("%s:%d", short, n) + }) + re := cache[rx] + if re == nil { + var err error + re, err = regexp.Compile(rx) + if err != nil { + log.Fatalf("%s:%d: invalid regexp \"%#q\" in ERROR line: %v", file, lineNum, rx, err) + } + cache[rx] = re + } + prefix := fmt.Sprintf("%s:%d", short, lineNum) + errs = append(errs, wantedError{ + reStr: rx, + re: re, + prefix: prefix, + auto: auto, + lineNum: lineNum, + file: short, + }) + } + } + + return +} -- 2.48.1