Skip to content
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: remove some defer cleanup in favor of test.Cleanup() #43340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

gotest.tools v3.0.1 and up support Go's native test.Cleanup(), (see gotestyourself/gotest.tools#180) which means that manually calling the cleanup functions in a defer is no longer needed.

Some of these could probably be replaced by Go's native t.TempDir(), but keeping that for a follow-up exercise.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review area/testing kind/refactor PR's that refactor, or clean-up code labels Mar 6, 2022
@thaJeztah thaJeztah marked this pull request as draft March 6, 2022 22:44
@thaJeztah

This comment was marked as resolved.

client/client_test.go Outdated Show resolved Hide resolved
@thaJeztah thaJeztah marked this pull request as ready for review March 7, 2022 14:02
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nervously LGTM 😅

(I can see that this is probably correct, and should be fine, but it still feels conceptually wrong to remove the defer deleteTempFiles() bits 🙈 I'll get over it)

@thaJeztah
Copy link
Member Author

Yeah, so t.CleanUp() is the new defer (for tests), and there's a neat t.TempDir(). Overall, I think that makes sense (and it could be useful if Go does some smart things with it when reporting test-results and/or timing); that said #43346 (still WIP) shows that some tests were not happy (not 100% clear yet why; could be I missed something, or it's possible that it's revealing some mistakes in tests (some runaway container / processes?).

gotest.tools v3.0.1 and up support Go's native test.Cleanup(), which
means that manually calling the cleanup functions in a defer is no
longer needed.

Some of these could probably be replaced by Go's native `t.TempDir()`,
but keeping that for a follow-up exercise.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing kind/refactor PR's that refactor, or clean-up code status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants