Peter Weinberger (Gerrit)

unread,

Nov 26, 2025, 9:26:00 AM (7 days ago) Nov 26

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Peter Weinberger has uploaded the change for review

Commit message

gopls/internal/marker: make checkDiffs less fragile

WIP, NOT READY FOR REVIEW

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 do correctly
convert the before code into the after code.

A subsequent CL will replace myers.ComputeEdit.

Change-Id: Iabf6659a646bd69730d8ae19b1d46885a4903d90

Change diff

diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go
index 47ca885..afd45e1 100644
--- a/gopls/internal/test/marker/marker_test.go
+++ b/gopls/internal/test/marker/marker_test.go
@@ -27,6 +27,7 @@
 	"runtime"
 	"slices"
 	"sort"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -1461,7 +1462,12 @@
 // checkDiffs computes unified diffs for each changed file, and compares with
 // the diff content stored in the given golden directory.
 func checkDiffs(mark marker, changed map[string][]byte, golden *Golden) {
-	diffs := make(map[string]string)
+	var _ marker
+	var _ markerTestRun
+	var _ markerTest
+	log.SetFlags(log.Lshortfile)
+	log.Printf("%d, %s", len(changed), mark.run.test.name)
+
 	for name, after := range changed {
 		before := mark.run.env.FileContent(name)
 		// TODO(golang/go#64023): switch back to diff.Strings.
@@ -1481,23 +1487,23 @@
 			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)
 		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 {
+		if want, 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 {
+			checkUnified(string(want), before, string(after))
 		}
 	}
+
 	// Report unmet expectations.
 	for filename := range golden.data {
 		if _, ok := changed[filename]; !ok {
@@ -1508,6 +1514,68 @@
 	}
 }
 
+var atregexp = regexp.MustCompile(`@@ -(\d+)(,(\d+))? \+(\d+)(,(\d+))? @@`)
+
+// PJW: useful doc comment
+func checkUnified(diffs, b, a string) error {
+	before := strings.Split(b, "\n")
+	after := strings.Split(a, "\n")
+	unif := strings.Split(diffs, "\n")
+	type hunk struct {
+		fromLine, fromCount, toLine, toCount int
+	}
+	parseAts := func(l string) hunk {
+		m := atregexp.FindStringSubmatch(l) // 1, 3, 4, 6
+		fromL, err1 := strconv.Atoi(m[1])
+		toL, err2 := strconv.Atoi(m[4])
+		if err1 != nil || err2 != nil {
+			log.Printf("missing line numbers in %q", l)
+			return hunk{}
+		}
+		var ans hunk
+		ans.fromLine = fromL
+		ans.toLine = toL
+		log.Printf("%q", m)
+		return ans
+	}
+	var got []string
+	left := 0
+	var blob hunk
+	for _, l := range unif {
+		if len(l) == 0 {
+			continue // done
+		}
+		switch l[0] {
+		case '@':
+			blob = parseAts(l)
+			for ; left < blob.fromLine-1; left++ {
+				got = append(got, before[left])
+			}
+		case '+':
+			got = append(got, l[1:])
+		case '-':
+			left++
+		case ' ':
+			log.Fatalf("weird line %q", l)
+		default:
+			log.Fatalf("impossible unified %q", diffs)
+		}
+	}
+	for ; left < len(before); left++ {
+		got = append(got, before[left])
+	}
+	if strings.Join(got, "\n") != strings.Join(after, "\n") {
+		for i := 0; i < len(got) && i < len(after); i++ {
+			if got[i] != after[i] {
+				log.Printf("differ i=%d, got[i]=%q, after[i]=%q", i, got[i], after[i])
+				break
+			}
+		}
+		log.Print("oops")
+	}
+	return nil
+}
+
 // ---- marker functions ----
 
 // TODO(rfindley): consolidate documentation of these markers. They are already

Change information

Files:

M gopls/internal/test/marker/marker_test.go

Change size: M

Delta: 1 file changed, 79 insertions(+), 11 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Peter Weinberger (Gerrit)

unread,

Dec 2, 2025, 2:41:06 PM (13 hours ago) Dec 2

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Peter Weinberger uploaded new patchset

Peter Weinberger uploaded patch set #3 to this change.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Peter Weinberger (Gerrit)

unread,

Dec 2, 2025, 3:59:59 PM (12 hours ago) Dec 2

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Peter Weinberger uploaded new patchset

Peter Weinberger uploaded patch set #4 to this change.

Following approvals got outdated and were removed:

TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Peter Weinberger (Gerrit)

unread,

Dec 2, 2025, 4:59:12 PM (11 hours ago) Dec 2

to goph...@pubsubhelper.golang.org, Alan Donovan, Go LUCI, golang-co...@googlegroups.com

Attention needed from Alan Donovan

Peter Weinberger added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Peter Weinberger . resolved

feel free to assign to someone else.

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

Dec 2, 2025, 9:02:05 PM (7 hours ago) Dec 2

to Peter Weinberger, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com

Attention needed from Peter Weinberger

Alan Donovan added 5 comments

Commit Message

Line 21, Patchset 4 (Latest):A subsequent CL will replace myers.ComputeEdit.

Alan Donovan . resolved

nice

File gopls/internal/test/marker/marker_test.go

Line 1463, Patchset 4 (Latest):// converts the orginal oontents into the changed contents.

Alan Donovan . unresolved

contents

Line 1467, Patchset 4 (Parent): // TODO(golang/go#64023): switch back to diff.Strings.

Alan Donovan . resolved

nice

Line 1473, Patchset 4 (Latest): // Trim the unified header from diffs, as it is unnecessary and repetitive.


		// (an artifact of ToUnified)
		difflines := strings.Split(d, "\n")

		var got string
		if len(difflines) >= 2 && strings.HasPrefix(difflines[1], "+++") {

			got = strings.Join(difflines[2:], "\n")
		} else {

			got = d // "" in packagedecl.txt:147
		}

Alan Donovan . unresolved

Do you know why the existing code checked for `+++` at line 1 here? I would expect that some diffs might start with `---`, or have both with the `---` appearing first, followed by an arbitrary number of lines.

Line 1505, Patchset 4 (Latest):func checkUnified(diffs, bef, aft string) error {

Alan Donovan . unresolved

The bulk of this logic seems like it should belong adjacent to ToUnified, perhaps called ApplyUnified, returning the got string. That way the producer and consumer are more likely to stay in sync. A few tests would be good too.

Open in Gerrit

Related details

Attention is currently required from:

Peter Weinberger

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[tools] gopls/internal/marker: make checkDiffs less fragile

Peter Weinberger (Gerrit)

Peter Weinberger has uploaded the change for review

Commit message

Change diff

Change information

Related details

Peter Weinberger (Gerrit)

Peter Weinberger uploaded new patchset

Related details

Peter Weinberger (Gerrit)

Peter Weinberger uploaded new patchset

Related details

Peter Weinberger (Gerrit)

Peter Weinberger added 1 comment

Related details

Alan Donovan (Gerrit)

Alan Donovan added 5 comments

Related details