-
Notifications
You must be signed in to change notification settings - Fork 886
chore: avoid concurrent usage of t.FailNow #1683
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
Conversation
What about for cases where the test can't continue if a call fails? |
I'd expect we'd need a separate channel to signal |
For reference, here's what failed in the first lint run (not sure why it didn't show up in the runner output):
Edit: these are fixed now, only show up on Linux |
I imagine we might get some weird logs from the test not stopping at the require in some cases, but using assert makes more sense in a lot of those go routines 👍 |
Is this the cause of weird test output occasionally? (can't find a sample, but I feel like you understand what I mean) |
I think it's a contributing factor. At this stage I think it's basically a case of eliminating all of the possible sources of flakes to find that really annoying one, like that old phone you dropped down a sofa somewhere and has an alarm set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
Idea (for another PR): Considering we have this widespread pattern of go func()
ing in tests where the goroutine may outlive the test, perhaps we could consider some kind of test helper that synchronizes the goroutine/cancellation to the test via t.Cleanup
.
Usage could look like this (ctx will be cancelled at the end of test and test exit delayed until func exits):
testhelper.Go(t, func(ctx context.Context) {
err := cmd.ExecuteContext(ctx)
(require|assert).NoError(err)
})
Package name is obv. bad.. not sure what a good name would be. testutil
, ctest
, testy
... 😄
Yeah this is deffo a good |
* chore: golangci: add linter rule to report usage of t.FailNow inside goroutines * chore: avoid t.FailNow in goroutines to appease the race detector
Context
https://pkg.go.dev/testing#T.FailNow
Problem
We make heavy use of
github.com/stretchr/testify/require
in our unit test code. However, one "gotcha" about every method in therequire
package is that it will callt.FailNow
under the hood if any assertion fails. This is unsafe to do outside of the main testing goroutine. There's actually an open issue about this behaviour.For example, the following code can trigger a data race:
Solution
require
with the equivalentassert
function (github.com/stretchr/testify/assert
)github.com/stretchr/testify/assert.*
ort.FailNow
(Explicitly tagging @Emyrk as our resident Ruleguard laywer)