gopls/internal/marker: make checkDiffs less fragile
The marker tests make over 500 calls to checkDiffs, which currently
works by computing line diffs between the before code and the after code
and comparing these with the unified diffs that are
the golden contents in the test .txt file.
This is fragile, as a different diff algorithm would produce
different diffs, and we would like to remove the dependency on
myers.ComputeEdit.
This CL instead just checks that the unified golden diffs correctly
convert the before code into the after code. (Note that this
is a complete test of the new code on all existing uses in marker
tests.)
A subsequent CL will replace myers.ComputeEdit.
Change-Id: Iabf6659a646bd69730d8ae19b1d46885a4903d90
Reviewed-on: https://go-review.googlesource.com/c/tools/+/724660
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go
index c5ff7a2..ec4e6af 100644
--- a/gopls/internal/test/marker/marker_test.go
+++ b/gopls/internal/test/marker/marker_test.go
@@ -1458,22 +1458,13 @@
}
}
-// checkDiffs computes unified diffs for each changed file, and compares with
-// the diff content stored in the given golden directory.
+// checkDiffs checks that the diff content stored in the given golden directory
+// converts the orginal contents into the changed contents.
+// (This logic is strange because hundreds of existing marker tests were created
+// containing a modified version of unified diffs.)
func checkDiffs(mark marker, changed map[string][]byte, golden *Golden) {
- diffs := make(map[string]string)
for name, after := range changed {
before := mark.run.env.FileContent(name)
- // TODO(golang/go#64023): switch back to diff.Strings.
- // The attached issue is only one obstacle to switching.
- // Another is that different diff algorithms produce
- // different results, so if we commit diffs in test
- // expectations, then we need to either (1) state
- // which diff implementation they use and never change
- // it, or (2) don't compare diffs, but instead apply
- // the "want" diff and check that it produces the
- // "got" output. Option 2 is more robust, as it allows
- // the test expectation to use any valid diff.
edits := myers.ComputeEdits(before, string(after))
d, err := diff.ToUnified("before", "after", before, edits, 0)
if err != nil {
@@ -1481,23 +1472,36 @@
log.Fatalf("internal error in diff.ToUnified: %v", err)
}
// Trim the unified header from diffs, as it is unnecessary and repetitive.
+ // (an artifact of ToUnified, but not stored in the marker files)
difflines := strings.Split(d, "\n")
+ var got string
if len(difflines) >= 2 && strings.HasPrefix(difflines[1], "+++") {
- diffs[name] = strings.Join(difflines[2:], "\n")
+ got = strings.Join(difflines[2:], "\n")
} else {
- diffs[name] = d
+ got = d // "" in packagedecl.txt:147
}
- }
- // Check changed files match expectations.
- for filename, got := range diffs {
- if want, ok := golden.Get(mark.T(), filename, []byte(got)); !ok {
+ // the call to Get is so that the -update flag will update the test.
+ // normally it just returns 'got'.
+ if tdiffs, ok := golden.Get(mark.T(), name, []byte(got)); !ok {
mark.errorf("%s: unexpected change to file %s; got diff:\n%s",
- mark.note.Name, filename, got)
- } else if got != string(want) {
- mark.errorf("%s: wrong diff for %s:\n\ngot:\n%s\n\nwant:\n%s\n",
- mark.note.Name, filename, got, want)
+ mark.note.Name, name, got)
+ return
+ } else {
+ // restore the ToUnifed header lines deleted above
+ // before calling ApplyUnified
+ diffsFromTest := "--- \n+++ \n" + string(tdiffs)
+ want, err := diff.ApplyUnified(diffsFromTest, before)
+ if err != nil {
+ mark.errorf("%s: ApplyUnified(%q,%q) failed: %v",
+ mark.note.Name, before, after, err)
+ }
+ if want != string(after) {
+ mark.errorf("%s: got\n%q expected\n%q",
+ mark.note.Name, want, string(after))
+ }
}
}
+
// Report unmet expectations.
for filename := range golden.data {
if _, ok := changed[filename]; !ok {
diff --git a/internal/diff/unified.go b/internal/diff/unified.go
index cfbda61..df06a68 100644
--- a/internal/diff/unified.go
+++ b/internal/diff/unified.go
@@ -7,6 +7,8 @@
import (
"fmt"
"log"
+ "regexp"
+ "strconv"
"strings"
)
@@ -249,3 +251,51 @@
}
return b.String()
}
+
+// ApplyUnified applies the unified diffs.
+func ApplyUnified(udiffs, bef string) (string, error) {
+ before := strings.Split(bef, "\n")
+ unif := strings.Split(udiffs, "\n")
+ var got []string
+ left := 0
+ // parse and apply the unified diffs
+ for _, l := range unif {
+ if len(l) == 0 {
+ continue // probably the last line (from Split)
+ }
+ switch l[0] {
+ case '@': // The @@ line
+ m := atregexp.FindStringSubmatch(l)
+ fromLine, err := strconv.Atoi(m[1])
+ if err != nil {
+ return "", fmt.Errorf("missing line number in %q", l)
+ }
+ // before is a slice, so0-based; fromLine is 1-based
+ for ; left < fromLine-1; left++ {
+ got = append(got, before[left])
+ }
+ case '+': // add this line
+ if strings.HasPrefix(l, "+++ ") {
+ continue
+ }
+ got = append(got, l[1:])
+ case '-': // delete this line
+ if strings.HasPrefix(l, "--- ") {
+ continue
+ }
+ left++
+ case ' ':
+ return "", fmt.Errorf("unexpected line %q", l)
+ default:
+ return "", fmt.Errorf("impossible unified %q", udiffs)
+ }
+ }
+ // copy any remaining lines
+ for ; left < len(before); left++ {
+ got = append(got, before[left])
+ }
+ return strings.Join(got, "\n"), nil
+}
+
+// The first number in the @@ lines is the line number in the 'before' data
+var atregexp = regexp.MustCompile(`@@ -(\d+).* @@`)
