-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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: race detected in goroutine can report an error in an unnamed test #60083
Comments
cc @bcmills |
I think this is plausibly correct? The empty |
I tend to think that an empty |
I believe it is currently necessary in order for |
Can you expand on that? test2json currently doesn't seem to count race detector reports at all. |
Hi! While we're at it, let me point out a related "weird" pattern in race error messages for tests. Unlike other errors, these get propagated to all the ancestors of the subtest that generated the race, and the same hardcoder error message ( Just wanted to mention this before I forgot. It's a different problem (if we want to call it that) from the subject of this issue, in the sense that fixing any of them won't solve the other. Maybe it's worth filing another bug for this? |
@cip999 Thanks, I was just looking at that. |
The reason for that is how race errors are counted:
(By the way, for some reason this is done by storing the negative of the first number, adding the second number, and checking whether the result is greater than 0. I'm confused.) It shouldn't be difficult to fix it. |
Using the negative count and then adding to it is because the number of race detector errors is monotonically increasing. We want to do the right thing when we run multiple tests, and some have race errors. |
Yes, my confusion was about why not storing the first error count (not its negative) and comparing it to the second one. But I think I figured it out, it's to handle the counting for parallel tests. Anyway, I'll stop diverging from the main subject. :) |
Change https://go.dev/cl/494057 mentions this issue: |
Following up on this comment thread, I investigated the way race errors are handled and reported in parallel tests. There are a couple notable points.
[Incidentally, the semantics of Now, what happens with the proposed change? What I said in the comments isn't exactly true. Parallel tests will still be "deferred" to when race errors for the parent have been already reported, so the parent won't be affected at all. However, the parent's parent (and all previous ancestors) will be: if a race happens during parallel execution of two or more sub-sub-tests, the race count will be decremented by 2 or more (instead of 1). Then, if another race happens outside the subtests, it won't be detected. (Disclaimer: I haven't verified this, I'm too lazy to patch the CL...) Admittedly, sub-sub-tests are seldom a thing (if ever). Still, something that works "by chance" is not super solid. Edit: Just to clarify, a minimal working example of the scenario I describe should be the following: func makeRace() {
var x int
c := make(chan bool)
go func() {
x = 1
c <- true
}()
fmt.Println(x)
<-c
}
func TestFoo(t *testing.T) {
t.Run("subtest", func(t *testing.T) {
c := make(chan bool)
t.Run("first subsubtest", func(t *testing.T) {
t.Parallel()
<-c
makeRace() // All the signaling is to make sure that this happens inside both subsubtests.
c <- true
})
t.Run("second subsubtest", func(t *testing.T) {
t.Parallel()
c <- true
<-c
})
})
} |
CL is being reverted. |
Change https://go.dev/cl/501895 mentions this issue: |
Change https://go.dev/cl/506300 mentions this issue: |
Change https://go.dev/cl/506755 mentions this issue: |
While investigating #60083, I found a couple of bugs (notably #61034) that had slipped through code review in part because the concurrency patterns used in the testing package were too complex for me to fully reason about. This change adjusts those patterns to be more in line with current idioms, and to reduce the number of special cases that depend on details that should be orthogonal. (For example: the details of how we invoke the Cleanup functions should not depend on whether the test happened to run any parallel subtests.) In the process, this change fixes a handful of bugs: - Concurrent calls to Run (explicitly allowed by TestParallelSub) could previously drive the testcontext.running count negative, causing the number of running parallel tests to exceed the -parallel flag. - The -failfast flag now takes effect immediately on failure. It no longer delays until the test finishes, and no longer misses failures during cleanup (fixing #61034). - If a Cleanup function calls runtime.Goexit (typically via t.FailNow) during a panic, Cleanup functions from its parent tests are no longer skipped and buffered logs from its parent tests are now flushed. - The time reported for a test with subtests now includes the time spent running those subtests, regardless of whether they are parallel. (Previously, non-parallel subtests were included but parallel subtests were not.) - Calls to (*B).Run in iterations after the first are now diagnosed with a panic. (This diagnoses badly-behaved benchmarks: if Run is called during the first iteration, no subsequent iterations are supposed to occur.) Fixes #61034. Change-Id: I3797f6ef5210a3d2d5d6c2710d3f35c0219b02ea Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-linux-amd64-longtest-race,gotip-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/506755 Auto-Submit: Bryan Mills <bcmills@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Test case:
Running this test with
go test -race -test.v
printsNote the
=== NAME
near the end. A=== NAME
line should be followed by the name of the test. Omitting the name is confusing and can break code that is parsing the test output.The text was updated successfully, but these errors were encountered: