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
build: test cleanup wrt wg.Done #5746
Comments
That benchmark code is broken, Fatalf should not be called from a goroutine, only the testing goroutine. If you want to propose a fix for this, please follow the contribution guidelines here, http://golang.org/doc/contribute.html. Labels changed: added priority-later, removed priority-triage. Status changed to HelpWanted. |
if the Fatalf can't be called from a goroutine, how to report the other goroutine Fatal infomation? I look over the `testing.T` and `testing.T` source code, the `{t|b}.Fatalf` is thread safe. can we use `defer wg.Done()` to fix these issue(net/rpc/server_test.go [1])? for p := 0; p < procs; p++ { go func() { defer wg.Done() reply := new(Reply) for atomic.AddInt32(&N, -1) >= 0 { err := client.Call("Arith.Add", args, reply) if err != nil { b.Fatalf("rpc error: Add: expected no error but got string %q", err.Error()) } if reply.C != args.A+args.B { b.Fatalf("rpc error: Add: expected %d got %d", reply.C, args.A+args.B) } } }() } --- [1] http://golang.org/src/pkg/net/rpc/server_test.go?s=12968:13320#L548 |
the patch has been sent as https://golang.org/cl/10454043/ |
Change https://golang.org/cl/212920 mentions this issue: |
…ines Adds an analyzer to report an error if any tests or benchmarks have any *Fatal, FailNow, Skip* misuses in goroutines which are forbidden by the package testing, since those functions terminate the entire benchmark/test yet ideally one goroutine exiting shouldn't affect the entire benchmark/test. This first pass only works for plain goroutines and doesn't yet work with b.RunParallel. That'll be added in a subsequent CL after this one is reviewed and merged. Updates golang/go#5746 Change-Id: Ia47e5c9fd96ceced1ae9834b94f529f6ae2edaaa Reviewed-on: https://go-review.googlesource.com/c/tools/+/212920 Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com>
Thank you for this bug report @chai2010 and everyone for responding! I examined this issue last week and sent CL 212920 for a go vet analysis pass that will be added in Go1.15 and has been merge into x/tools currently. It also found 3 misuses in the standard library as per this CL 213097 I am going to close this issue since it was more of a question that was answered in the replies, it is also now answered in the documents for testing.T and testing.B but also as mentioned above, go vet will start flagging those cases in >=Go1.15. Thanks y'all and happy New Year! |
Change https://golang.org/cl/235677 mentions this issue: |
Add "go/analysis/passes/testinggoroutine" from x/tools and vendor its source in. This pass will catch misuses of: * testing.T.Fail* * testing.T.Fatal* * testing.T.Skip* inside goroutines explicitly started by the go keyword. The pass was implemented in CL 212920. While here, found 2 misuses in: * database/sql/sql_test.go * runtime/syscall_windows_test.go and fixed them in CL 235527. Fixes #5746 Change-Id: I1740ad3f1d677bb5d78dc5d8d66bac6ec287a2b1 Reviewed-on: https://go-review.googlesource.com/c/go/+/235677 Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Emmanuel Odeke <emmanuel@orijtech.com>
The text was updated successfully, but these errors were encountered: