*_test.go: use t.TempDir, t.Setenv #2417
Conversation
Reviewer's Guide by SourceryThis PR refactors the test suite to use Ginkgo's No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
kolyshkin
force-pushed
the
use-t
branch
2 times, most recently
from
13669b8 to
96fb2c6
Compare
|
Like the direction here, a lot cleaner with these helpers |
kolyshkin
force-pushed
the
use-t
branch
3 times, most recently
from
8196af1 to
8c9ec13
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kolyshkin - I've reviewed your changes - here's some feedback:
Overall Comments:
- This looks like a good cleanup of test code, thanks for using
t.TempDirandt.Setenv!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Thanks, that seems fine here but as general thing there is some IMO unexpected behavior in ginkgo when mixing AfterEach() and DeferCleanup() (used behind the scenes by ginkgo here to register the cleanup functions for the tmpdir, etc...)
ref
containers/podman@3c0176b onsi/ginkgo#1360
I cannot see any case where this would matter here but I still would have a slight preference to migrate away from AfterEach() here and use DeferCleanup() directly where needed to perfom extra cleanup in the ginkgo tests
Using os.Setenv in tests is problematic, because the change is process-wise and other tests running in parallel might be affected. Also, a somewhat complicated cleanup is needed. Both issues are solved by using t.Setenv. This commit also uses t.TempDir, t.Cleanup, and t.Helper when it makes sense. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of using os.MkdirTemp in tests, use t.TempDir. In a few places where the parent directory is already create by t.TempDir, replace os.MkdirTemp with os.Mkdir. While at it, make sure to not a leak opened file descriptor returned by os.CreateTemp. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Done (as a separate third commit). |
In Ginkgo, there might be some unexpected behavior when mising AfterEach and DeferCleanup (see [1], [2]), so let's switch to DeferCleanup. [1]: containers/podman@3c0176b2d0dd3 [2]: onsi/ginkgo#1360 Reported-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kolyshkin, Luap99, sourcery-ai[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
FYI, this did indeed cause a test regression (flake). But only because the test was already badly written before: aaa527e |
Those are cases not reported by
usetestinglinter (most probably because it doesn't grok ginkgo tests).It's hard to separate the changes into smaller commits, thus it's somewhat harder to review.
Summary by Sourcery
Refactor test code to use Ginkgo's t.TempDir() and t.Setenv() methods for better test management and cleanup
Enhancements:
Tests: