Improve test coverage for prepare plugins in image mode#4719
Improve test coverage for prepare plugins in image mode#4719
prepare plugins in image mode#4719Conversation
There was a problem hiding this comment.
Code Review
This pull request significantly improves test coverage for prepare plugins in image mode by refactoring existing tests and adding new ones for bootc-based guests. It also implements missing methods in the Bootc package manager to support these tests. The changes are well-structured and the new test infrastructure is a valuable addition. I have one suggestion to simplify some of the new shell helper functions for better readability.
65f03c7 to
022036b
Compare
| if [ "$PROVISION_HOW" = "container" ] && rlIsFedora 43 && is_fedora_43 "$image"; then | ||
| if ([ "$PROVISION_HOW" = "container" ] && rlIsFedora 43 && is_fedora_43 "$image") || is_image_mode "$image"; then |
There was a problem hiding this comment.
Why should a special is_image_mode here be done?
There was a problem hiding this comment.
The original condition was container-only. Image mode was added because it supports local RPM installation via a different code path (Containerfile COPY + RUN dnf install), which is worth testing separately from repo-based installation.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Well, I am not sure where the confusion is, but the below tests run only against a container provisioner now. I want to run them on image mode, because they handling is special there and it is worth testing. Also, one of the cases tested should be testing the case we fixed in https://github.com/teemtee/tmt/pull/4598/files.
There was a problem hiding this comment.
Then put it next to the PROVSION_HOW check, not globally or-ed
There was a problem hiding this comment.
it would not match rlIsFedora 43 && is_fedora_43 "$image" ...
There was a problem hiding this comment.
it would not match
rlIsFedora 43 && is_fedora_43 "$image"...
Yes, I'm aware, and that's a separate issue to be resolved for the exact same reasons.
There was a problem hiding this comment.
I am lost what do you want here, please provide a snippet. We want to run this:
- one of the container images (fedora 43 now)
- on image mode
There was a problem hiding this comment.
- Have the test run solely on
rlIsFedora 43 && is_fedora_43 "$image"check (or a better equivalent one that we should add)- The
rlIsFedorain here does not seem right, it would make more sense to gate it on the packit/plan side, in line with Move/plans/provision/containerinto its own Packit job #4400 - The time-saved is miniscule when you compare https://artifacts.dev.testing-farm.io/c209da29-bb97-43e7-9636-bbe55a46db04/ with https://artifacts.dev.testing-farm.io/e5345cba-e5d8-4f95-be6c-b7cb4639b2a1/
- The
[[ "$PROVISION_HOW" = "container" || is_image_mode "$image" ]]- The comments indicate that both branches are intentional and should be kept as-is in a future refactoring
Overall the intent in this section is unclear and the change looks more like slapping a duct-tape on top of a pile of duct-tapes. If we come back to re-organizing this test, how should this be split, what is meaningful to run, etc., it would make our future selves jobs easier.
|
@gemini-code-assist please review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for 'image-mode' guests within the TMT framework. Key changes include updating .packit.yaml and plans/provision/virtual.fmf to define a new provision-virtual-image-mode job and FMF plan, along with adding specific image definitions and helper functions in tests/images.sh. The testing infrastructure has been significantly adapted across tests/prepare/install/test.sh, tests/prepare/recommend/test.sh, and tests/prepare/shell/test.sh to accommodate image-mode specific behaviors like Containerfile builds, image switches, and reboot persistence. Core package management logic for image-mode guests, including COPR repository handling and package installation, has been implemented in tmt/package_managers/bootc.py. Several old test files related to bootc and CentOS Stream 10 have been removed.
0a72ea2 to
221d6c3
Compare
| if [ "$PROVISION_HOW" = "container" ] && rlIsFedora 43 && is_fedora_43 "$image"; then | ||
| if ([ "$PROVISION_HOW" = "container" ] && rlIsFedora 43 && is_fedora_43 "$image") || is_image_mode "$image"; then |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Fixed the
Assisted-by: Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
|
Btw, what do we do with the diverging naming conventions |
I would opt calling this |
Did it create a confusion? Isn't the whole point of |
Remove standalone `tests/prepare/bootc/` and extend existing plugin tests (`prepare/install`, `prepare/shell`, `prepare/recommend`) with image mode coverage on CentOS Stream 10 and Fedora 44 bootc images. - `enable_copr()` — installs copr plugin via Containerfile (since `/usr` is read-only), then runs `dnf copr enable` on the live system to write `.repo` to `/etc`. - `create_repository()`, `install_repository()` — delegate to `aux_engine` to run on the live system. Not directly tested, will be exercised when `prepare/artifact` tests are extended. Existing tests gain `IS_IMAGE_MODE` handling and run against image mode images when triggered by the new `/plans/provision/virtual/image-mode` sub-plan. Reboot persistence is verified through a dedicated `reboot-persistence` inner plan that installs packages, reboots via `tmt-reboot`, and re-verifies. A single reboot test is sufficient — all install variants (repo, local RPM, COPR, etc.) go through the same bootc mechanism (Containerfile rebuild → `bootc switch` → reboot), so if packages survive a reboot in one case, they survive in all. - `prepare/artifact`, `prepare/require` — currently `provision-container` only - `prepare/ansible`, `prepare/feature/epel,fips,profile` — need Ansible support in image mode (#4636) - `prepare/distgit` — test refactoring needed Pull Request Checklist * [x] implement the feature * [x] extend the test coverage Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
This reverts commit 05d59b0.
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Co-authored-by: Cristian Le <git@lecris.dev>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Script from remote url is broken, see #4785 Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Incorporate the image mode testing into the plugins' tests and remove the separate
bootcspecific testing.Running the plugins test exercises all the cases as in package mode and provides good confidence about the
image mode testing support.
The testing is done against F44 and CS10 image mode cloud base images provided by Testing Farm.
Run the testing in a separate
/provision/virtual-image-modeplan, because the testing will similartime to run as
/provision/virtual.Fixes #4606
Pull Request Checklist
Assisted-by: Claude Code