-
Notifications
You must be signed in to change notification settings - Fork 2.9k
test: Fix PODMAN_BATS_LEAK_CHECK #27826
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
Conversation
lsm5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as best I can tell. Clearly an improvement over main when run with PODMAN_BATS_LEAK_CHECK set.
@containers/podman-maintainers PTAL.
|
LGTM once comment addressed |
This variable is set by hack/bats and it fails if PODMAN_CMD is unset. Signed-off-by: Ricardo Branco <rbranco@suse.de>
9885e65 to
8d3ac1a
Compare
I was wondering why you didn't catch it earlier in your CI. |
|
Full verification run for latest change: https://openqa.opensuse.org/tests/5580232 |
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering why you didn't catch it earlier in your CI.
Because we don't run with this option set in CI, which is the reason why we have this option to begin with. The leak check is considered to "slow" so we don't do it. WE rather have the few minutes of CI speed up
LGTM
Wow. Our openQA jobs run for like ~2 hours on x86_64. Will check how much they take without this option and maybe unset it in slower arches like aarch64 & ppc64le. |
|
see 81c90f5 for the history, I noted 400s back then (on x86_64) and of course with the more test cases it is likely more now. However that is only them not run in parallel, in parallel we cannot do the check anyway. It is certainly good that we have someone testing with this option on regular to avoid broken or buggy tests. I agree that turning off on certain arches is fine. |
This variable is set by
hack/batsand it fails becausePODMAN_CMDis unset inbasic_teardown. I could manually reproduce as well when calling bats directly withPODMAN_BATS_LEAK_CHECK=1.Otherwise you get errors like this:
https://openqa-assets.opensuse.org/tests/5555887/file/podman-bats-user-local.tap.txt
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?