-
Notifications
You must be signed in to change notification settings - Fork 128
test: add integration tests for 'bootc switch --apply' #1420
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
base: main
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request adds a new integration test for bootc switch --apply
. The test creates a derived container image, switches to it, and verifies the system reboots into the new image with the correct kernel arguments.
The changes are logical and well-structured. I've found a critical issue with a filename typo in one of the tmf
files that would prevent the test from running. I've also pointed out a few minor improvements in the nushell test script, such as removing unused variables and an unnecessary bash -c
wrapper to improve code clarity and maintainability.
Once these points are addressed, the new test will be a valuable addition to the test suite.
b975721
to
06725a3
Compare
Hmm. Reading the docs, it doesn't seem like tmt supports rebooting using anything other than the
|
Yes see #1419 |
Hi @p5, I can't push to your diff --git i/tmt/tests/booted/test-image-upgrade-reboot.nu w/tmt/tests/booted/test-image-upgrade-reboot.nu
index e4e8498..6c5ff5a 100644
--- i/tmt/tests/booted/test-image-upgrade-reboot.nu
+++ w/tmt/tests/booted/test-image-upgrade-reboot.nu
@@ -39,7 +39,7 @@ COPY usr/ /usr/
podman build -t localhost/bootc-derived .
# Now, switch into the new image
- bootc switch --apply --transport containers-storage localhost/bootc-derived
+ tmt-reboot -c "bootc switch --apply --transport containers-storage localhost/bootc-derived"
# We cannot perform any other checks here since the system will be automatically rebooted
} |
Signed-off-by: Robert Sturla <[email protected]>
06725a3
to
25d7d49
Compare
Thanks for the pointer @henrywang ! Your patch has been applied and squashed into the existing commit. The integration tests seemingly are passing, so think this is ready for some reviews! This test should:
Edit: I'm not actually sure that this is working as we intended. |
podman build -t localhost/bootc-derived . | ||
|
||
# Now, switch into the new image | ||
tmt-reboot -c "bootc switch --apply --transport containers-storage localhost/bootc-derived" |
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.
This doesn't seem to work - perhaps TMT reboots at the end of the command if the command itself doesn't?
Maybe we do something like:
tmt-reboot -c "bootc switch --apply --transport containers-storage localhost/bootc-derived && exit 1"
In theory, if the --apply
flag works, the exit 1
should never be reached and the tests continue as expected.
If the exit 1
is hit, it should mean --apply
didn't initiate a reboot, and the test will rightfully fail.
Thoughts?
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.
According to https://tmt.readthedocs.io/en/stable/stories/features.html#reboot-during-test and code https://github.com/teemtee/tmt/blob/main/tmt/steps/scripts/tmt-reboot, the tmt-reboot will not reboot the system if -c
used.
I've not had chance to setup a local environment for tmt, so hoping I'm able to test in this PR.
This uses the existing
test-20-local-upgrade.fmf
with a lot of the unneeded tests stripped out. Might move this into that test, we'll see.If all goes well, tests will fail to begin with. Then when I rebase the fixes, they pass.