tools/bumper: Refactor using DeferCleanup #1978
Conversation
kubevirt-bot
added
release-note-none
|
/cherry-pick release-0.97 |
|
@RamLavi: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| defer func(path string) { | ||
| Expect(os.RemoveAll(path)).To(Succeed()) | ||
| }(gitCnaoRepo.gitRepo.localDir) | ||
| DeferCleanup(os.RemoveAll, gitCnaoRepo.gitRepo.localDir) |
There was a problem hiding this comment.
Aint DeferCleanup should be used on BeforeAll and such ?
It meant to auto create a corresponding AfterAll block for example,
Here it feels that defer is more suitable,
unless the docs etc says otherwise
There was a problem hiding this comment.
No, that ok. See the question here https://gophers.slack.com/archives/CQQ50BBNW/p173627601851229, and the answer here https://gophers.slack.com/archives/CQQ50BBNW/p1736281169698839
| defer func(path string) { | ||
| Expect(os.RemoveAll(path)).To(Succeed()) | ||
| }(gitCnaoRepo.gitRepo.localDir) | ||
| DeferCleanup(os.RemoveAll, gitCnaoRepo.gitRepo.localDir) |
There was a problem hiding this comment.
I think that now, after moving to DeferCleanup, it's possible, and so it is proffered, to check the error; e.g.
| DeferCleanup(os.RemoveAll, gitCnaoRepo.gitRepo.localDir) | |
| DeferCleanup(func(){ | |
| Expect(os.RemoveAll(gitCnaoRepo.gitRepo.localDir)).To(Succeed()) | |
| }) |
There was a problem hiding this comment.
I would say better to keep it simple
There was a problem hiding this comment.
I would say that in go, you must check errors
| defer func(path string) { | ||
| Expect(os.RemoveAll(path)).To(Succeed()) | ||
| }(gitCnaoRepo.gitRepo.localDir) | ||
| DeferCleanup(os.RemoveAll, gitCnaoRepo.gitRepo.localDir) |
| defer func(path string) { | ||
| Expect(os.RemoveAll(path)).To(Succeed()) | ||
| }(gitCnaoRepo.gitRepo.localDir) | ||
| DeferCleanup(os.RemoveAll, gitCnaoRepo.gitRepo.localDir) |
| defer func(path string) { | ||
| Expect(os.RemoveAll(path)).To(Succeed()) | ||
| }(gitCnaoRepo.gitRepo.localDir) | ||
| DeferCleanup(os.RemoveAll, gitCnaoRepo.gitRepo.localDir) |
|
Not related to this PR does this require some retry on either core / test ? or just an uninteresting flake ? Thanks |
|
Nit: the PR desc should be under the "What this PR does / why we need it:" line |
I'm monitoring it as a flake (see on kmp) and also trying to investigate further. |
Signed-off-by: Ram Lavi <ralavi@redhat.com>
Currently the DeferCleanup is performed on every unit test. Move it to be more centralized - where the repo is first created. Signed-off-by: Ram Lavi <ralavi@redhat.com>
The tests creates a temporary test directory on every run. Removing it to prevent leftover folders. Signed-off-by: Ram Lavi <ralavi@redhat.com>
RamLavi
force-pushed
the
fix_deferDuringDescribe_table
branch
from
c57f313 to
81abf78
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
@nunnatsa PTAL |
| @@ -28,6 +28,11 @@ var _ = Describe("Testing internal git CNAO Repo", func() { | |||
| githubApi = newFakeGithubApi(repoDir) | |||
|
|
|||
| gitCnaoRepo = newFakeGitCnaoRepo(githubApi, repoDir, &component{}, expectedTagCommitMap) | |||
|
|
|||
| DeferCleanup(func() { | |||
| Expect(os.RemoveAll(gitCnaoRepo.gitRepo.localDir)).To(Succeed()) | |||
There was a problem hiding this comment.
Do you need both of them? Isn't the first dir under the 2nd?
There was a problem hiding this comment.
I though about keeping them both but maybe it's an overkill.
What this PR does / why we need it:
This PR is refactoring the bumper unit tests to use DeferCleanup, following a discussion in anotherPR
Special notes for your reviewer:
Release note: