[go] testing: skip parallel tests when -failfast is enabled
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
testing: skip parallel tests when -failfast is enabled
The t.Parallel method does not interact well with the -failfast flag.
See golang/go#30522.
With `-failfast` enabled tests blocking on `t.Parallel()` can be skipped
after the first test failure.
This won't immediately kill any parallel tests which are already running
but it will prevent more tests from starting.
I've chosen to use `t.SkipNow()` when we detect that we want to failfast
after a parallel test wakes up.
This to me seemed like the cleanest solution and doesn't hide
what is happening.
Though arguably we could print a more detailed message?
Fixes #30522
Change-Id: Ic319c74af13a8e13040259b99f32a21ae0c7bcc4
GitHub-Last-Rev: 4b21ddd06df9c822db8855da3c0ce24eefbf3cb2
GitHub-Pull-Request: golang/go#53460
---
M src/cmd/go/testdata/script/test_fail_fast.txt
M src/testing/testing.go
2 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/src/cmd/go/testdata/script/test_fail_fast.txt b/src/cmd/go/testdata/script/test_fail_fast.txt
index 132ea70..51e8063 100644
--- a/src/cmd/go/testdata/script/test_fail_fast.txt
+++ b/src/cmd/go/testdata/script/test_fail_fast.txt
@@ -16,17 +16,26 @@
# mix with parallel tests
! go test ./failfast_test.go -run='TestFailingB|TestParallelFailingA' -failfast=true
-stdout -count=2 'FAIL - '
+stdout -count=1 'FAIL - '
! go test ./failfast_test.go -run='TestFailingB|TestParallelFailingA' -failfast=false
stdout -count=2 'FAIL - '
! go test ./failfast_test.go -run='TestFailingB|TestParallelFailing[AB]' -failfast=true
-stdout -count=3 'FAIL - '
+stdout -count=1 'FAIL - '
! go test ./failfast_test.go -run='TestFailingB|TestParallelFailing[AB]' -failfast=false
stdout -count=3 'FAIL - '
+# parallel subtests with timeout
+# the timeouts are necessary to avoid race conditions in the test case
+! go test -parallel=2 ./failfast_test.go -run='TestWithFailingParallelSubtestsA' -failfast=false
+stdout -count=3 'FAIL - '
+! go test -parallel=2 ./failfast_test.go -run='TestWithFailingParallelSubtestsA' -failfast=true
+stdout -count=2 'FAIL - '
+! go test -parallel=1 ./failfast_test.go -run='TestWithFailingParallelSubtestsA' -failfast=true
+stdout -count=1 'FAIL - '
+
# mix with parallel sub-tests
! go test ./failfast_test.go -run='TestFailingB|TestParallelFailing[AB]|TestParallelFailingSubtestsA' -failfast=true
-stdout -count=3 'FAIL - '
+stdout -count=1 'FAIL - '
! go test ./failfast_test.go -run='TestFailingB|TestParallelFailing[AB]|TestParallelFailingSubtestsA' -failfast=false
stdout -count=5 'FAIL - '
! go test ./failfast_test.go -run='TestParallelFailingSubtestsA' -failfast=true
@@ -55,7 +64,10 @@
package failfast
-import "testing"
+import (
+ "testing"
+ "time"
+)
func TestA(t *testing.T) {
// Edge-case testing, mixing unparallel tests too
@@ -81,6 +93,26 @@
t.Errorf("FAIL - %s", t.Name())
}
+func TestWithFailingParallelSubtestsA(t *testing.T) {
+ t.Run("SubtestsA1", func(t *testing.T) {
+ t.Parallel()
+ time.Sleep(100 * time.Millisecond)
+
+ t.Errorf("FAIL - %s", t.Name())
+ })
+ t.Run("SubtestsA2", func(t *testing.T) {
+ t.Parallel()
+ time.Sleep(100 * time.Millisecond)
+
+ t.Errorf("FAIL - %s", t.Name())
+ })
+ t.Run("SubtestsA3", func(t *testing.T) {
+ t.Parallel()
+
+ t.Errorf("FAIL - %s", t.Name())
+ })
+}
+
func TestParallelFailingSubtestsA(t *testing.T) {
t.Parallel()
t.Run("TestFailingSubtestsA1", func(t *testing.T) {
diff --git a/src/testing/testing.go b/src/testing/testing.go
index ec2d864..eafe890 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -1271,6 +1271,10 @@
<-t.parent.barrier // Wait for the parent test to complete.
t.context.waitParallel()
+ if shouldFailFast() {
+ t.SkipNow()
+ }
+
if t.chatty != nil {
t.chatty.Updatef(t.name, "=== CONT %s\n", t.name)
}
To view, visit change 413236. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Bryan Mills, Michael Matloob.
1 comment:
Patchset:
Thanks, will review for 1.20.
To view, visit change 413236. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Michael Matloob.
Patch set 1:Hold +1
1 comment:
File src/testing/testing.go:
if shouldFailFast() {
t.SkipNow()
}This looks like a backward-incompatible API change.
`SkipNow` “must be called from the goroutine running the test, not from other goroutines created during the test”. However, `Parallel` has no such restriction, and calls to `Parallel` can (correctly but rarely) occur in other goroutines.
For example, this change would cause the test function in https://go.dev/play/p/5t19ytbLVVE — which should always pass today! — to instead deadlock if `-failfast` is enabled and some other parallel test fails concurrently.
To view, visit change 413236. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Ian Lance Taylor abandoned this change.
To view, visit change 413236. To unsubscribe, or for help writing mail filters, visit settings.