Skip to content

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

Merged
merged 3 commits into from
May 24, 2022
Merged

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented May 23, 2022

Context

https://pkg.go.dev/testing#T.FailNow

FailNow marks the function as having failed and stops its execution by calling runtime.Goexit
(which then runs all deferred calls in the current goroutine). Execution will continue at the
next test or benchmark. FailNow must be called from the goroutine running the test or
benchmark function, not from other goroutines created during the test. Calling FailNow
does not stop those other goroutines.

Problem

We make heavy use of github.com/stretchr/testify/require in our unit test code. However, one "gotcha" about every method in the require package is that it will call t.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:

func Test_SomeOperationThatMightFail(t *testing.T) {
    c := make(chan struct{})
    go func() { 
        err := someOperationThatMightFail()
        require.NoError(t, err)
        c <- struct{}{}
    }()
    <-c
}

Solution

  1. Replace existing concurrent usages of require with the equivalent assert function (github.com/stretchr/testify/assert)
  2. Add a Ruleguard rule to detect concurrent usage of github.com/stretchr/testify/assert.* or t.FailNow

(Explicitly tagging @Emyrk as our resident Ruleguard laywer)

@johnstcn johnstcn requested review from Emyrk and a team May 23, 2022 19:48
@johnstcn johnstcn self-assigned this May 23, 2022
@johnstcn johnstcn changed the title chore: avoid race conditions in unit test assertions chore: avoid concurrent usage of t.FailNow May 23, 2022
@coadler
Copy link
Contributor

coadler commented May 23, 2022

What about for cases where the test can't continue if a call fails?

@johnstcn
Copy link
Member Author

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 t.FailNow from the main goroutine, plus the relevant scaffolding.
However, I would expect the worst case here is that you end up with a NPE or an AIOBE, which should still cause the test to fail.

@johnstcn
Copy link
Member Author

johnstcn commented May 23, 2022

For reference, here's what failed in the first lint run (not sure why it didn't show up in the runner output):

provisioner/terraform/parse_test.go:36:3: ruleguard: Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834) (gocritic)
                require.NoError(t, err)
                ^
provisioner/terraform/provision_test.go:56:3: ruleguard: Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834) (gocritic)
                require.NoError(t, err)
                ^
coderd/database/pubsub_test.go:51:4: ruleguard: Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834) (gocritic)
                        require.NoError(t, err)
                        ^
make: *** [Makefile:56: lint] Error 1

Edit: these are fixed now, only show up on Linux

@Emyrk
Copy link
Member

Emyrk commented May 23, 2022

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 👍

@kylecarbs
Copy link
Member

Is this the cause of weird test output occasionally? (can't find a sample, but I feel like you understand what I mean)

@johnstcn
Copy link
Member Author

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.

Copy link
Member

@mafredri mafredri left a 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... 😄

@johnstcn
Copy link
Member Author

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 t.Helper candidate

@johnstcn johnstcn merged commit c2f74f3 into main May 24, 2022
@johnstcn johnstcn deleted the cj/ruleguard-testing-t-race branch May 24, 2022 07:58
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
* chore: golangci: add linter rule to report usage of t.FailNow inside goroutines
* chore: avoid t.FailNow in goroutines to appease the race detector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants