From 4cba6c703f68a7c1718e589feaeb2530d7812fbf Mon Sep 17 00:00:00 2001 From: Changkun Ou Date: Sat, 19 Sep 2020 23:32:12 +0200 Subject: [PATCH] testing: send t.signal only if there is no panic If a signal is sent to t.signal before the panic is triggered, a panicking test may end up with "warning: no tests to run" because the tRunner that invokes the test in t.Run calls runtime.Goexit on panic, which causes the panicking test not be recorded in runTests. Send the signal if and only if there is no panic. Fixes #41479 Change-Id: I812f1303bfe02c443a1902732e68d21620d6672e Reviewed-on: https://go-review.googlesource.com/c/go/+/256098 Run-TryBot: Emmanuel Odeke TryBot-Result: Go Bot Reviewed-by: Ian Lance Taylor Reviewed-by: Emmanuel Odeke Trust: Emmanuel Odeke Trust: Bryan C. Mills --- .../go/testdata/script/test_cleanup_failnow.txt | 14 ++++++++++++++ src/testing/testing.go | 10 ++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/cmd/go/testdata/script/test_cleanup_failnow.txt b/src/cmd/go/testdata/script/test_cleanup_failnow.txt index 5ad4185fc1..0737a93db2 100644 --- a/src/cmd/go/testdata/script/test_cleanup_failnow.txt +++ b/src/cmd/go/testdata/script/test_cleanup_failnow.txt @@ -1,11 +1,25 @@ # For issue 41355 [short] skip +# This test could fail if the testing package does not wait until +# a panicking test does the panic. Turn off multithreading, GC, and +# async preemption to increase the probability of such a failure. +env GOMAXPROCS=1 +env GOGC=off +env GODEBUG=asyncpreempt=off + +# If the test exits with 'no tests to run', it means the testing package +# implementation is incorrect and does not wait until a test panic. +# If the test exits with '(?s)panic: die.*panic: die', it means +# the testing package did an extra panic for a panicking test. + ! go test -v cleanup_failnow/panic_nocleanup_test.go +! stdout 'no tests to run' stdout '(?s)panic: die \[recovered\].*panic: die' ! stdout '(?s)panic: die \[recovered\].*panic: die.*panic: die' ! go test -v cleanup_failnow/panic_withcleanup_test.go +! stdout 'no tests to run' stdout '(?s)panic: die \[recovered\].*panic: die' ! stdout '(?s)panic: die \[recovered\].*panic: die.*panic: die' diff --git a/src/testing/testing.go b/src/testing/testing.go index d86354093a..a44c0a0749 100644 --- a/src/testing/testing.go +++ b/src/testing/testing.go @@ -1091,10 +1091,16 @@ func tRunner(t *T, fn func(t *T)) { // complete even if a cleanup function calls t.FailNow. See issue 41355. didPanic := false defer func() { - t.signal <- signal - if err != nil && !didPanic { + if didPanic { + return + } + if err != nil { panic(err) } + // Only report that the test is complete if it doesn't panic, + // as otherwise the test binary can exit before the panic is + // reported to the user. See issue 41479. + t.signal <- signal }() doPanic := func(err interface{}) { -- 2.48.1