test/e2e: dedup Before/AfterEach nodes #18544
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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 |
|
Nice simplification |
|
Big Gold Star for this one @Luap99 🌟 |
There was a problem hiding this comment.
I really love this, thank you! I'm concerned about the ordering of AfterEach(). (BeforeEach(), it seems clear that common runs before the test-specific one). Specifically, in tests where cleanup involves Unsetenv("CONTAINERS_CONF"), can you confirm that the ordering of Unsetenv()-Cleanup() is safe, or does not matter? Also, the Volumes thing below.
Thank you for taking this on!
It most cases it does not matter, however while working on ginkgov2 I found a case where it matters. When tests use an invalid CONTAINERS_CONF file which contains invalid options or maybe does not even exists podman will just error out. Thus the Cleanup() could fail in this case. Therefore the correct order is to always unsetenv before Cleanup(). |
|
given how many tests use CONTAINERS_CONF I could juts move it in the main AfterEach() block and let all test unset it. There shouldn't be a problem with that and it prevents other from forgetting to unset it. |
|
Seems fair. I did a very quick check, and don't see any places where |
|
This is a lot cleaner and looks safer, thank you! Thanks for documenting the execution order and for the link. A few typos in the commit message, not worth re-pushing for, but if a re-push is needed for anything else, please take a quick edit pass there. |
test/e2e/image_scp_test.go
Outdated
| processTestResult(f) | ||
|
|
||
| // make sure connections are not written to real user config on host | ||
| os.Setenv("CONTAINERS_CONF", filepath.Join(podmanTest.TempDir, "containersconf")) |
There was a problem hiding this comment.
Looks like you need to create this.
There was a problem hiding this comment.
Guess I deserve that for not running it locally first.
"defined"; "our"
"Each"; "function" |
There is no reason to define the same code every time in each file, just use global nodes. This diff should speak for itself. CleanupSecrets()/Volume() no longer call Cleanup() directly, as the global AfterEach node will always call Cleanup() this is no longer necessary. If one AfterEach() node fails it will still run the others. Also always unset the CONTAINERS_CONF env vars. This prevents people from forgetting to unset it. And fix the special CONTAINERS_CONF logic in the system connection tests, we do not want to preserve CONTAINERS_CONF anyway so just remove this logic. Ginkgo orders the BeforeEach and AfterEach nodes. They will be executed from the outer-most defined to inner-most. This means our global BeforeEach is always first. Only then the inner one (in the Describe() function in each file). For AfterEach it is inverted, from the inner to the outer. Also see https://onsi.github.io/ginkgo/#organizing-specs-with-container-nodes Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
/lgtm |
|
/hold cancel |
There is no reason to define the same code every time in each file, just use global nodes. This diff should speak for itself.
Does this PR introduce a user-facing change?