Don't suggest test context in test cleanup functions #9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See ldez#4 This is not a safe replacement and not usually a good suggestion since inside t.Cleanup functions the test context will already be cancelled so doing meaningful cleanup that uses context for timeout is not possible.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4.
What does this PR do?
Suggesting the replacement of
t.Context()forcontext.Background()inside a test'st.Cleanup()functions is usually not a good suggestion since inside t.Cleanup functions the test context will already be cancelled so doing meaningful cleanup that uses context for timeout is not possible.This PR detects the use of
t.Cleanupand prunes further inspection of the child-nodes (i.e. the cleanup function literal).Motivation
I'd like to enable the
usetestinglinter in codebases I maintain but we make wide use oft.Cleanupwith timeout contexts to close resources after a test ends in a limited amount of time (see the example in the added test case). Following this linter's suggestions causes such tests to fail because no time is allowed to clean up the resources (as the context returned byt.Context()is closed before cleanup functions are invoked)Additional Notes
Note that this fix cancels all checking within
t.Cleanupfunctions, not just forcontext.Background/context.Todo. Ideally withint.Cleanupfunctions we would still check for the other things this linter looks at, but I wasn't able to see a good way to do that given the structure of the linter.