Skip to content

Conversation

@mvo5
Copy link
Contributor

@mvo5 mvo5 commented Nov 26, 2025

[draft as this is potentially controversial]

This is a tiny wrapper around the build/boot tests scripts to run them via pytest. Why another layer of indirection you ask?

  1. it makes it easy to enumerate all tests one can run via pytest --collect-only (708!)
  2. it makes it easy to run selected tests: pytest -k rhel-10.2
  3. it provides a uniform way to that is the same for all pytest projects
  4. with a sufficiently small number of tests (via -k) we could do --parallel
  5. it abstracts away from the underlying scripts, we can (potentially) change how we run this without changing the higher layers
  6. (new) the (py)tests are split into build_only and build_and_boot testcases now, this makes it trivial to use pytest --collect-only -k build_only (or `-k build_and_boot) and see that we do 443 build tests and 265 boot tests.

Now that tests are easy to run locally it seems like a really nice feature.

[edit: Another thing I would like to do is make the tests that are skipped currently more visible, either via pytest.skip() when ensure_can_run_qemu_test() returns can exception or by having a declaratve skip-test list so that we can easily see what is currently skipped for what reason, right now the set of boot tests that we claim to do is a bit bigger than what we actually do (as we do not e.g. minimal-raw as it has no cloud-init]

@bcl
Copy link
Contributor

bcl commented Dec 2, 2025

I like it! Tests don't pass though :( Also I think there is a typo in the 2nd commit where you say it splits it into build and boot or boot, I think you mean build and boot or build. I'd also split the gen-manifest change into a commit by itself instead of mixing it with the pytest change but that's a minor 'complaint'.

@mvo5 mvo5 force-pushed the images-pytest branch 3 times, most recently from 02afaed to 37a5bb0 Compare December 2, 2025 10:17
@mvo5 mvo5 marked this pull request as ready for review December 2, 2025 16:39
@mvo5 mvo5 requested review from a team, achilleas-k and thozza as code owners December 2, 2025 16:39
@mvo5 mvo5 requested a review from croissanne December 2, 2025 16:39
We need a `-print-only` option so that we can see what
gen-manifests would generate. This will be input for a pytest
based framework (but seems generally be useful).

This also tweaks the outputs so that any status message now
goes to stderr so that one can parse stdout for the list
of image manifests that would be generated.
@mvo5 mvo5 force-pushed the images-pytest branch 2 times, most recently from a066047 to 8b482e6 Compare December 2, 2025 20:36
@supakeen
Copy link
Member

supakeen commented Dec 3, 2025

From the title I was kinda against this but after seeing this in action I feel it's nice. There's a little bug though:

FAILED test/test_build_integration.py::test_build_and_boot[rhel-10.0-x86_64-image-installer-jq-only] - NameError: name 'image_type' is not defined

mvo5 added 4 commits December 3, 2025 10:43
This is a tiny wrapper around the build/boot tests scripts
to run them via pytest. Why another layer of indirection you ask?

1. it makes it easy to enumerate all tests one can run via
   `pytest --collect-only`
2. it makes it easy to run selected tests: `pytest -k rhel-10.2`
3. it provides a uniform way to that is the same for all pytest
   projects
4. with a sufficiently small number of tests (via -k) we could do
   --parallel
5. it abstracts away from the underlying scripts, we can
   (potentially) change how we run this without changing the
   higher layers

Now that tests are easy to run locally it seems like a really
nice feature.
This skips the build and boot tests for non-root users, they
are currently required to run as root. A nice side effect is
that the normal workflow of running pytest to run the "normal"
tests is not interrupted. We could of course also use pytest
and a custom "marker" like "@pytest.mark.integration" if
this commit is considered too loose.
This makes it nicer to use `pytest -k build_and_boot` to
find only our build and boot tests.
We need to excude the integration tests in the python-test
workflow. So add a custom pytest marker. We could also
move the integration tests into their own dir, I have no
strong opinion.
Copy link
Contributor

@lzap lzap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that I can still use scripts directly. Nice.

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a nice idea. Thanks.

@thozza thozza added this pull request to the merge queue Dec 3, 2025
Merged via the queue into osbuild:main with commit b19fb77 Dec 3, 2025
23 checks passed
mvo5 added a commit to mvo5/images that referenced this pull request Dec 8, 2025
Tiny followup for osbuild#2045
that adds some documentation under test/README.md with examples
how the pytest wrapping can be useful.
mvo5 added a commit to achilleas-k/images that referenced this pull request Dec 9, 2025
In osbuild#2045 the script
was converted to use pytest. However this does not work
for ostree configs. A ostree config must be rewritten so
that it points to an actual ostree commit and server.

This is done in the `setup_dependencies()` script that then
creates a different config/config_name. As this is different
from the configs we get via `gen-manifests --dry-run` pytest
cannot be used here.

So for now be pragmatic and just revert
https://github.com/osbuild/images/pull/2045/files#diff-8a0342654218957707e9769f498c86df3f070ee601f5b059f77b30d4d1f50d1eL18

We probably want get back to this eventually and make this
setup part of pytest.
github-merge-queue bot pushed a commit that referenced this pull request Dec 10, 2025
In #2045 the script
was converted to use pytest. However this does not work
for ostree configs. A ostree config must be rewritten so
that it points to an actual ostree commit and server.

This is done in the `setup_dependencies()` script that then
creates a different config/config_name. As this is different
from the configs we get via `gen-manifests --dry-run` pytest
cannot be used here.

So for now be pragmatic and just revert
https://github.com/osbuild/images/pull/2045/files#diff-8a0342654218957707e9769f498c86df3f070ee601f5b059f77b30d4d1f50d1eL18

We probably want get back to this eventually and make this
setup part of pytest.
mvo5 added a commit to mvo5/images that referenced this pull request Dec 10, 2025
Tiny followup for osbuild#2045
that adds some documentation under test/README.md with examples
how the pytest wrapping can be useful.
github-merge-queue bot pushed a commit that referenced this pull request Dec 10, 2025
Tiny followup for #2045
that adds some documentation under test/README.md with examples
how the pytest wrapping can be useful.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants