From 2c7daa644acd499d44366a16c10012487b2abdf8 Mon Sep 17 00:00:00 2001 From: Samuel Berthe Date: Sun, 5 Jul 2026 17:18:31 +0200 Subject: [PATCH 1/3] perf(error): check SourceFragmentsHidden before formatting sources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ToMap, LogValue and formatVerbose evaluated the condition if sources := o.Sources(); sources != "" && !SourceFragmentsHidden left-to-right: Sources() formatted the full source-fragment output - reading source files from disk and building per-line strings - and the result was then discarded, because SourceFragmentsHidden is true by default. Checking the flag first skips all of that work in the default configuration; behavior with SourceFragmentsHidden=false is unchanged. Also replaces fmt.Sprintf with direct concatenation in the per-line / per-block builders (getSourceFromFrame, framesToStacktraceBlocks, framesToSourceBlocks), benefiting the non-hidden path too. benchstat vs main (darwin/arm64, Apple M3, -count=10 -cpu=1): | before3.txt | after3.txt | | sec/op | sec/op vs base | ToMap 3.494u ± 5% 1.631u ± 15% -53.31% (p=0.002 n=10) LogValue 3.499u ± 4% 1.478u ± 8% -57.77% (p=0.000 n=10) geomean 3.496u 1.553u -55.60% | before3.txt | after3.txt | | B/op | B/op vs base | ToMap 5.214Ki ± 0% 2.789Ki ± 0% -46.51% (p=0.000 n=10) LogValue 5.357Ki ± 0% 2.745Ki ± 0% -48.76% (p=0.000 n=10) geomean 5.285Ki 2.767Ki -47.65% | before3.txt | after3.txt | | allocs/op | allocs/op vs base | ToMap 63.00 ± 0% 28.00 ± 0% -55.56% (p=0.000 n=10) LogValue 61.00 ± 0% 24.00 ± 0% -60.66% (p=0.000 n=10) geomean 61.99 25.92 -58.18% --- error.go | 23 +++++++++++++++++------ sources.go | 5 ++--- stacktrace.go | 8 ++------ 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/error.go b/error.go index 7f6b043..cf486de 100644 --- a/error.go +++ b/error.go @@ -680,8 +680,13 @@ func (o OopsError) LogValue() slog.Value { //nolint:gocyclo attrs = append(attrs, slog.String("stacktrace", stacktrace)) } - if sources := o.Sources(); sources != "" && !SourceFragmentsHidden { - attrs = append(attrs, slog.String("sources", sources)) + // Check the flag before calling Sources(): formatting source fragments + // reads source files from disk and builds the full output string, all of + // which would be discarded here when fragments are hidden (the default). + if !SourceFragmentsHidden { + if sources := o.Sources(); sources != "" { + attrs = append(attrs, slog.String("sources", sources)) + } } return slog.GroupValue(attrs...) @@ -780,8 +785,11 @@ func (o OopsError) ToMap() map[string]any { //nolint:gocyclo payload["stacktrace"] = stacktrace } - if sources := o.Sources(); sources != "" && !SourceFragmentsHidden { - payload["sources"] = sources + // See LogValue: only format source fragments when they will be used. + if !SourceFragmentsHidden { + if sources := o.Sources(); sources != "" { + payload["sources"] = sources + } } return payload @@ -910,8 +918,11 @@ func (o *OopsError) formatVerbose() string { //nolint:gocyclo _, _ = fmt.Fprintf(&output, "Stacktrace:\n%s\n", stacktrace) } - if sources := o.Sources(); sources != "" && !SourceFragmentsHidden { - _, _ = fmt.Fprintf(&output, "Sources:\n%s\n", sources) + // See LogValue: only format source fragments when they will be used. + if !SourceFragmentsHidden { + if sources := o.Sources(); sources != "" { + _, _ = fmt.Fprintf(&output, "Sources:\n%s\n", sources) + } } return output.String() diff --git a/sources.go b/sources.go index bfbee56..d552f34 100644 --- a/sources.go +++ b/sources.go @@ -1,8 +1,8 @@ package oops import ( - "fmt" "os" + "strconv" "strings" "sync" ) @@ -166,8 +166,7 @@ func getSourceFromFrame(frame oopsStacktraceFrame) []string { line := lines[i] - // Format the line with line number - message := fmt.Sprintf("%d\t%s", i+1, line) + message := strconv.Itoa(i+1) + "\t" + line output = append(output, message) // Add visual indicator for the error line diff --git a/stacktrace.go b/stacktrace.go index a0f6a36..1b38c3d 100644 --- a/stacktrace.go +++ b/stacktrace.go @@ -1,7 +1,6 @@ package oops import ( - "fmt" "reflect" "runtime" "slices" @@ -386,10 +385,7 @@ func framesToStacktraceBlocks(blocks []outputBlock) []string { firstFrame = false } - stacktraceStr := strings.Join(frameLines, "\n") - block := fmt.Sprintf("%s\n%s", msg, stacktraceStr) - - output = append(output, block) + output = append(output, msg+"\n"+strings.Join(frameLines, "\n")) } slices.Reverse(output) @@ -404,7 +400,7 @@ func framesToSourceBlocks(blocks []outputBlock) []string { header, body := st.Source() if b.msg != "" { - header = fmt.Sprintf("%s\n%s", b.msg, header) + header = b.msg + "\n" + header } if header != "" && len(body) > 0 { From 17739bb3c3968504bbcb688fded88427cc899561 Mon Sep 17 00:00:00 2001 From: Samuel Berthe Date: Sun, 5 Jul 2026 17:32:04 +0200 Subject: [PATCH 2/3] test(error): cover the SourceFragmentsHidden=false path The previous commit gated Sources() behind SourceFragmentsHidden in ToMap, LogValue and formatVerbose, but no existing test ever set SourceFragmentsHidden=false, leaving oopsStacktrace.Source() and framesToSourceBlocks at 0% coverage and the new gated branches partially untested. Adds: - TestOopsSourceFragmentsVisible / TestOopsSourceFragmentsHiddenByDefault: verify ToMap, LogValue and formatVerbose include/omit "sources" depending on the flag. - TestOopsStacktraceSource: direct coverage of oopsStacktrace.Source(). - TestFramesToSourceBlocks: direct coverage of framesToSourceBlocks, including the no-body-skips-block and multi-block-reversal cases. Coverage: 90.8% -> 94.0%. --- oops_test.go | 58 +++++++++++++++++++++++++++++ stacktrace_test.go | 92 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) diff --git a/oops_test.go b/oops_test.go index bda738d..975a38c 100644 --- a/oops_test.go +++ b/oops_test.go @@ -1756,3 +1756,61 @@ func TestOopsJoinNilHandling(t *testing.T) { result2 := Join(nil, nil) is.NoError(result2) } + +// TestOopsSourceFragmentsVisible verifies that ToMap, LogValue and +// formatVerbose include the "sources" entry when SourceFragmentsHidden is +// false, and that its content matches OopsError.Sources(). +func TestOopsSourceFragmentsVisible(t *testing.T) { //nolint:paralleltest + // Modifies the global SourceFragmentsHidden flag — do not run in parallel. + is := assert.New(t) + + original := SourceFragmentsHidden + SourceFragmentsHidden = false + defer func() { SourceFragmentsHidden = original }() + + err := New("boom") + oopsErr := err.(OopsError) + + sources := oopsErr.Sources() + is.NotEmpty(sources, "Sources() should be non-empty when SourceFragmentsHidden is false") + + m := oopsErr.ToMap() + is.Equal(sources, m["sources"]) + + group := oopsErr.LogValue().Group() + var got string + var found bool + for _, a := range group { + if a.Key == "sources" { + found = true + got = a.Value.String() + } + } + is.True(found, "expected a 'sources' attr in LogValue when SourceFragmentsHidden is false") + is.Equal(sources, got) + + verbose := oopsErr.formatVerbose() + is.Contains(verbose, "Sources:\n"+sources) +} + +// TestOopsSourceFragmentsHiddenByDefault verifies that ToMap, LogValue and +// formatVerbose omit "sources" when SourceFragmentsHidden is true (the +// default), even though the error carries a resolvable stacktrace. +func TestOopsSourceFragmentsHiddenByDefault(t *testing.T) { + t.Parallel() + is := assert.New(t) + + is.True(SourceFragmentsHidden, "this test assumes the package default") + + err := New("boom") + oopsErr := err.(OopsError) + + _, hasSources := oopsErr.ToMap()["sources"] + is.False(hasSources) + + for _, a := range oopsErr.LogValue().Group() { + is.NotEqual("sources", a.Key) + } + + is.NotContains(oopsErr.formatVerbose(), "Sources:") +} diff --git a/stacktrace_test.go b/stacktrace_test.go index a3c9a2d..9bcf637 100644 --- a/stacktrace_test.go +++ b/stacktrace_test.go @@ -176,6 +176,98 @@ func TestOopsStacktraceString(t *testing.T) { } } +func TestOopsStacktraceSource(t *testing.T) { + t.Parallel() + + _, currentFile, currentLine, _ := runtime.Caller(0) + + t.Run("empty_frames", func(t *testing.T) { + t.Parallel() + is := assert.New(t) + + st := &oopsStacktrace{span: "test"} + header, body := st.Source() + is.Empty(header) + is.Empty(body) + }) + + t.Run("with_frames", func(t *testing.T) { //nolint:paralleltest + t.Cleanup(func() { + mutex.Lock() + cache = map[string][]string{} + mutex.Unlock() + }) + is := assert.New(t) + + frame := oopsStacktraceFrame{file: currentFile, line: currentLine, function: "TestOopsStacktraceSource"} + st := &oopsStacktrace{span: "test", frames: []oopsStacktraceFrame{frame}} + + header, body := st.Source() + is.Equal(frame.String(), header) + is.NotEmpty(body) + }) +} + +func TestFramesToSourceBlocks(t *testing.T) { + t.Parallel() + + _, currentFile, currentLine, _ := runtime.Caller(0) + + t.Run("no_blocks", func(t *testing.T) { + t.Parallel() + is := assert.New(t) + + is.Empty(framesToSourceBlocks(nil)) + }) + + t.Run("message_and_frames_produce_a_block", func(t *testing.T) { //nolint:paralleltest + t.Cleanup(func() { + mutex.Lock() + cache = map[string][]string{} + mutex.Unlock() + }) + is := assert.New(t) + + frame := oopsStacktraceFrame{file: currentFile, line: currentLine, function: "TestFramesToSourceBlocks"} + blocks := []outputBlock{{msg: "boom", frames: []oopsStacktraceFrame{frame}}} + + out := framesToSourceBlocks(blocks) + is.Len(out, 1) + is.Contains(out[0], "boom") + is.Contains(out[0], frame.String()) + }) + + t.Run("unresolvable_frame_produces_no_block", func(t *testing.T) { + t.Parallel() + is := assert.New(t) + + frame := oopsStacktraceFrame{file: "/nonexistent/path/file.go", line: 1, function: "x"} + blocks := []outputBlock{{msg: "boom", frames: []oopsStacktraceFrame{frame}}} + + is.Empty(framesToSourceBlocks(blocks)) + }) + + t.Run("multiple_blocks_are_reversed", func(t *testing.T) { //nolint:paralleltest + t.Cleanup(func() { + mutex.Lock() + cache = map[string][]string{} + mutex.Unlock() + }) + is := assert.New(t) + + frame := oopsStacktraceFrame{file: currentFile, line: currentLine, function: "TestFramesToSourceBlocks"} + blocks := []outputBlock{ + {msg: "first", frames: []oopsStacktraceFrame{frame}}, + {msg: "second", frames: []oopsStacktraceFrame{frame}}, + } + + out := framesToSourceBlocks(blocks) + is.Len(out, 2) + is.Contains(out[0], "second") + is.Contains(out[1], "first") + }) +} + func TestOopsStacktraceFrameString(t *testing.T) { t.Parallel() From d2050d381747116c99f41164f9c70b034df761eb Mon Sep 17 00:00:00 2001 From: Samuel Berthe Date: Sun, 5 Jul 2026 17:39:34 +0200 Subject: [PATCH 3/3] test(error): run SourceFragmentsHidden tests sequentially TestOopsSourceFragmentsHiddenByDefault asserts on the package-level SourceFragmentsHidden default, which TestOopsSourceFragmentsVisible temporarily flips. Both must run outside the parallel pool so the flip-and-restore in the latter cannot interleave with the former assumption; add the paralleltest lint suppression that comes with opting out of t.Parallel() for that reason. --- oops_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/oops_test.go b/oops_test.go index 975a38c..7b9e572 100644 --- a/oops_test.go +++ b/oops_test.go @@ -1796,8 +1796,10 @@ func TestOopsSourceFragmentsVisible(t *testing.T) { //nolint:paralleltest // TestOopsSourceFragmentsHiddenByDefault verifies that ToMap, LogValue and // formatVerbose omit "sources" when SourceFragmentsHidden is true (the // default), even though the error carries a resolvable stacktrace. -func TestOopsSourceFragmentsHiddenByDefault(t *testing.T) { - t.Parallel() +// +// Not parallel: asserts on the SourceFragmentsHidden default, which +// TestOopsSourceFragmentsVisible flips for its own duration. +func TestOopsSourceFragmentsHiddenByDefault(t *testing.T) { //nolint:paralleltest is := assert.New(t) is.True(SourceFragmentsHidden, "this test assumes the package default")