Merge pull request #4080 from apostasie/tigron-4-require

[Tigron]: Testing logs user experience

Author: AkihiroSuda

mod/tigron/.golangci.yml

@@ -37,6 +37,7 @@ linters:
     - cyclop            # provided by revive
     - exhaustruct       # does not serve much of a purpose
     - errcheck          # provided by revive
+    - errchkjson        # forces handling of json err (eg: prevents _), which is too much
     - forcetypeassert   # provided by revive
     - funlen            # provided by revive
     - gocognit          # provided by revive
@@ -65,7 +66,7 @@ linters:
           arguments: [60]
         - name: function-length
           # Default is 50, 75
-          arguments: [80, 180]
+          arguments: [80, 200]
         - name: cyclomatic
           # Default is 10
           arguments: [30]
@@ -93,11 +94,17 @@ linters:
           files:
             - $all
           allow:
+            # Allowing go standard library and tigron itself
             - $gostd
             - github.com/containerd/nerdctl/mod/tigron
+            # We use creack as our base for pty
             - github.com/creack/pty
+            # Used for display width computation in internal/formatter
+            - golang.org/x/text/width
+            # errgroups and term (make raw) are used by internal/pipes
             - golang.org/x/sync
             - golang.org/x/term
+            # EXPERIMENTAL: for go routines leakage detection
             - go.uber.org/goleak
     staticcheck:
       checks:

mod/tigron/expect/comparators.go

@@ -30,59 +30,59 @@ import (
 
 // All can be used as a parameter for expected.Output to group a set of comparators.
 func All(comparators ...test.Comparator) test.Comparator {
-	return func(stdout, info string, t *testing.T) {
+	return func(stdout, _ string, t *testing.T) {
 		t.Helper()
 
 		for _, comparator := range comparators {
-			comparator(stdout, info, t)
+			comparator(stdout, "", t)
 		}
 	}
 }
 
 // Contains can be used as a parameter for expected.Output and ensures a comparison string is found contained in the
 // output.
 func Contains(compare string) test.Comparator {
-	return func(stdout, info string, t *testing.T) {
+	return func(stdout, _ string, t *testing.T) {
 		t.Helper()
-		assertive.Contains(assertive.WithFailLater(t), stdout, compare, info)
+		assertive.Contains(assertive.WithFailLater(t), stdout, compare, "Inspecting output (contains)")
 	}
 }
 
 // DoesNotContain is to be used for expected.Output to ensure a comparison string is NOT found in the output.
 func DoesNotContain(compare string) test.Comparator {
-	return func(stdout, info string, t *testing.T) {
+	return func(stdout, _ string, t *testing.T) {
 		t.Helper()
-		assertive.DoesNotContain(assertive.WithFailLater(t), stdout, compare, info)
+		assertive.DoesNotContain(assertive.WithFailLater(t), stdout, compare, "Inspecting output (does not contain)")
 	}
 }
 
 // Equals is to be used for expected.Output to ensure it is exactly the output.
 func Equals(compare string) test.Comparator {
-	return func(stdout, info string, t *testing.T) {
+	return func(stdout, _ string, t *testing.T) {
 		t.Helper()
-		assertive.IsEqual(assertive.WithFailLater(t), stdout, compare, info)
+		assertive.IsEqual(assertive.WithFailLater(t), stdout, compare, "Inspecting output (equals)")
 	}
 }
 
 // Match is to be used for expected.Output to ensure we match a regexp.
 func Match(reg *regexp.Regexp) test.Comparator {
-	return func(stdout, info string, t *testing.T) {
+	return func(stdout, _ string, t *testing.T) {
 		t.Helper()
-		assertive.Match(assertive.WithFailLater(t), stdout, reg, info)
+		assertive.Match(assertive.WithFailLater(t), stdout, reg, "Inspecting output (match)")
 	}
 }
 
 // JSON allows to verify that the output can be marshalled into T, and optionally can be further verified by a provided
 // method.
 func JSON[T any](obj T, verifier func(T, string, tig.T)) test.Comparator {
-	return func(stdout, info string, t *testing.T) {
+	return func(stdout, _ string, t *testing.T) {
 		t.Helper()
 
 		err := json.Unmarshal([]byte(stdout), &obj)
-		assertive.ErrorIsNil(assertive.WithFailLater(t), err, "failed to unmarshal JSON from stdout")
+		assertive.ErrorIsNil(assertive.WithSilentSuccess(t), err, "Unmarshalling JSON from stdout must succeed")
 
 		if verifier != nil && err == nil {
-			verifier(obj, info, t)
+			verifier(obj, "Inspecting output (JSON)", t)
 		}
 	}
 }

mod/tigron/go.mod

@@ -5,8 +5,9 @@ go 1.23.0
 require (
 	github.com/creack/pty v1.1.24
 	go.uber.org/goleak v1.3.0
-	golang.org/x/sync v0.12.0
+	golang.org/x/sync v0.13.0
 	golang.org/x/term v0.30.0
+	golang.org/x/text v0.24.0
 )
 
 require golang.org/x/sys v0.31.0 // indirect

mod/tigron/go.sum

@@ -8,11 +8,13 @@ github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PK
 github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
 go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
-golang.org/x/sync v0.12.0 h1:MHc5BpPuC30uJk597Ri8TV3CNZcTLu6B6z4lJy+g6Jw=
-golang.org/x/sync v0.12.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
+golang.org/x/sync v0.13.0 h1:AauUjRAJ9OSnvULf/ARrrVywoJDy0YS2AwQ98I37610=
+golang.org/x/sync v0.13.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
 golang.org/x/sys v0.31.0 h1:ioabZlmFYtWhL+TRYpcnNlLwhyxaM9kWTDEmfnprqik=
 golang.org/x/sys v0.31.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k=
 golang.org/x/term v0.30.0 h1:PQ39fJZ+mfadBm0y5WlL4vlM7Sx1Hgf13sMIY2+QS9Y=
 golang.org/x/term v0.30.0/go.mod h1:NYYFdzHoI5wRh/h5tDMdMqCqPJZEuNqVR5xJLd/n67g=
+golang.org/x/text v0.24.0 h1:dd5Bzh4yt5KYA8f9CJHCP4FB4D51c2c6JvN37xJJkJ0=
+golang.org/x/text v0.24.0/go.mod h1:L8rBsPeo2pSS+xqN0d5u2ikmjtmoJbDBT1b7nHvFCdU=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

mod/tigron/internal/assertive/assertive.go

@@ -31,12 +31,12 @@ import (
 	"github.com/containerd/nerdctl/mod/tigron/tig"
 )
 
-// TODO: once debugging output will be cleaned-up, reintroduce hexdump.
-
 const (
+	markLineLength           = 20
 	expectedSuccessDecorator = "✅️ does verify:\t\t"
-	expectedFailDecorator    = "❌ does not verify:\t"
+	expectedFailDecorator    = "❌ FAILED!\t\t"
 	receivedDecorator        = "👀 testing:\t\t"
+	annotationDecorator      = "🖊️"
 	hyperlinkDecorator       = "🔗"
 )
 
@@ -163,8 +163,8 @@ func True(testing tig.T, comp bool, msg ...string) bool {
 
 // WithFailLater will allow an assertion to not fail the test immediately.
 // Failing later is necessary when asserting inside go routines, and also if you want many
-// successive asserts to all
-// evaluate instead of stopping at the first failing one.
+// successive asserts to all evaluate instead of stopping at the first failing one.
+// FIXME: it should be possible to have both WithFailLater and WithSilentSuccess at the same time.
 func WithFailLater(t tig.T) tig.T {
 	return &failLater{
 		t,
@@ -205,49 +205,76 @@ func evaluate(testing tig.T, isSuccess bool, actual, expected any, msg ...string
 func decorate(testing tig.T, isSuccess bool, actual, expected any, msg ...string) {
 	testing.Helper()
 
-	header := "\t"
+	if _, ok := testing.(*silentSuccess); !isSuccess || !ok {
+		head := strings.Repeat("<", markLineLength)
+		footer := strings.Repeat(">", markLineLength)
+		header := "\t"
 
-	hyperlink := getTopFrameFile()
-	if hyperlink != "" {
-		msg = append([]string{hyperlink + "\n"}, msg...)
-	}
+		custom := fmt.Sprintf("\t%s %s", annotationDecorator, strings.Join(msg, "\n"))
 
-	msg = append(msg, fmt.Sprintf("\t%s`%v`", receivedDecorator, actual))
+		msg = append([]string{"", head}, custom)
 
-	if isSuccess {
-		msg = append(msg,
-			fmt.Sprintf("\t%s%v", expectedSuccessDecorator, expected),
-		)
-	} else {
-		msg = append(msg,
-			fmt.Sprintf("\t%s%v", expectedFailDecorator, expected),
-		)
-	}
+		msg = append([]string{getTopFrameFile()}, msg...)
 
-	if _, ok := testing.(*silentSuccess); !isSuccess || !ok {
-		testing.Log(header + strings.Join(msg, "\n") + "\n")
+		msg = append(msg, fmt.Sprintf("\t%s`%v`", receivedDecorator, actual))
+
+		if isSuccess {
+			msg = append(msg,
+				fmt.Sprintf("\t%s%v", expectedSuccessDecorator, expected),
+			)
+		} else {
+			msg = append(msg,
+				fmt.Sprintf("\t%s%v", expectedFailDecorator, expected),
+			)
+		}
+
+		testing.Log(header + strings.Join(msg, "\n") + "\n" + footer + "\n")
 	}
 }
 
+// XXX FIXME #expert
+// Because of how golang testing works, the upper frame is the one from where t.Run is being called,
+// as (presumably) the passed function is starting with its own stack in a go routine.
+// In the case of subtests, t.Run being called from inside Tigron will make it so that the top frame
+// is case.go around line 233 (where we call Command.Run(), which is the one calling assertive).
+// To possibly address this:
+// plan a. just drop entirely OSC8 links and source extracts and trash all of this
+// plan b. get the top frame from the root test, and pass it to subtests on a custom property, the somehow into here
+// plan c. figure out a hack to call t.Run from the test file without ruining the Tigron UX
+// Dereference t.Run? Return a closure to be called from the top? w/o disabling inlining in the right place?
+// Short term, blacklisting /tigron (and /nerdtest) will at least prevent the wrong links from appearing in the output.
 func getTopFrameFile() string {
-	// Get the frames.
+	// Get the frames. Skip the first two frames - current one and caller.
 	//nolint:mnd // Whatever mnd...
-	pc := make([]uintptr, 20)
+	pc := make([]uintptr, 40)
 	//nolint:mnd // Whatever mnd...
 	n := runtime.Callers(2, pc)
 	callersFrames := runtime.CallersFrames(pc[:n])
 
-	var file string
+	var (
+		file       string
+		lineNumber int
+		frame      runtime.Frame
+	)
 
-	var lineNumber int
+	more := true
+	for more {
+		frame, more = callersFrames.Next()
 
-	var frame runtime.Frame
-	for range 20 {
-		frame, _ = callersFrames.Next()
+		// Once we are in the go main stack, bail out
 		if !strings.Contains(frame.Function, "/") {
 			break
 		}
 
+		// XXX see note above
+		if strings.Contains(frame.File, "/tigron") {
+			continue
+		}
+
+		if strings.Contains(frame.File, "/nerdtest") {
+			continue
+		}
+
 		file = frame.File
 		lineNumber = frame.Line
 	}
@@ -282,6 +309,6 @@ func getTopFrameFile() string {
 	return hyperlinkDecorator + " " + (&formatter.OSC8{
 		Text:     line,
 		Location: "file://" + file,
-		Line:     frame.Line,
+		Line:     lineNumber,
 	}).String()
 }