Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: panics duplicate information #71517

Closed
neild opened this issue Jan 31, 2025 · 6 comments
Closed

testing: panics duplicate information #71517

neild opened this issue Jan 31, 2025 · 6 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@neild
Copy link
Contributor

neild commented Jan 31, 2025

Consider a test which panics:

func Test(t *testing.T) {
	panic("oops")
}

The testing package produces this output for this test:

--- FAIL: Test (0.00s)
panic: oops [recovered]
	panic: oops

goroutine 3 [running]:
...

Note that the panic value ("oops") is printed twice, because the testing package recovers the panic, performs cleanup, and then re-panics with the same value:
https://go.googlesource.com/go/+/refs/tags/go1.23.5/src/testing/testing.go#1632

This isn't all that much of a problem when the panic message is short, but it can get confusing if the message is long. If the formatted panic value spans multiple lines, the interior panic is indented inconsistently:

--- FAIL: Test (0.00s)
panic: this
	is
	a
	long
	panic [recovered]
	panic: this
	is
	a
	long
	panic

This can get exceedingly verbose if the panic message happens to contain stack information itself. (Perhaps that's an argument against putting stack information in panic messages.)

I'm not sure if this is a general problem with how we print panics which reuse the same value, or a problem with the testing package in particular. Perhaps the testing package should use a short, consistent message when re-panicking, since the original panic value will be printed when the panic terminates the program:

--- FAIL: Test (0.00s)
panic: oops [recovered]
	panic: panic in Test

Or perhaps runtime.printpanics should detect the case where a panic value has been reused:

--- FAIL: Test (0.00s)
panic: oops [recovered, reraised]

goroutine 34 [running]:
...
@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Jan 31, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/645915 mentions this issue: testing: avoid duplicate panic text when tests panic

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/645916 mentions this issue: runtime: don't duplicate reraised panic values in printpanics

@mpx
Copy link
Contributor

mpx commented Feb 2, 2025

IIUC, this issue could also be avoided if it was possible to only recover a panic for the specific expected type (Eg, #50424 (comment)).

The extra frames in the panic output routinely hurt debugability in these situations.

@neild
Copy link
Contributor Author

neild commented Feb 4, 2025

Recovering a specific type of panic doesn't help with this case. The testing package deliberately recovers panics of all types so it can perform cleanup before re-raising the panic; there is no specific panic that it's looking for.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/647495 mentions this issue: cmd/go: adjust testsuite to add reraised panic message

gopherbot pushed a commit that referenced this issue Feb 6, 2025
A couple of tests generate different output due to CL 645916
for issue #71517.

Fixes #71593
Fixes #71594

Change-Id: Ifaeff4e9de8d881202bd9e6394c9b9cff8959596
Reviewed-on: https://go-review.googlesource.com/c/go/+/647495
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
@dmitshur dmitshur added this to the Go1.25 milestone Feb 7, 2025
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants