Skip to content
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

Allow to run test at build time #2574

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

schaefi
Copy link
Collaborator

@schaefi schaefi commented Jun 18, 2024

This change runs the unit tests at build time of the package. It increases the build time a lot because the test run is expensive. I created this pull request as a draft and open for a conversation if we want this. The Debian package maintainer @glaubitz runs the test at build time

@schaefi schaefi self-assigned this Jun 18, 2024
@@ -664,6 +670,11 @@ make -C doc man
# Build application wheel
%{__python3} -m build --no-isolation --wheel

# Run tests
pip install --break-system-packages dist/kiwi-%{version}-py3-none-any.whl
Copy link
Member

Choose a reason for hiding this comment

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

What's with "breaking system packages"? Also pip can't work in Logo or OBS like this.

Why not just ensure test dependencies are specified as BuildRequires?

Copy link
Collaborator Author

@schaefi schaefi Jun 19, 2024

Choose a reason for hiding this comment

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

Ok so here is the story:

running pytest without an installed kiwi fails at a million places because kiwi cannot be found as a module. One would think setting PYTHONPATH properly would fix this but it simply does not work. @glaubitz can tell you a looong frustrating story about this.

Long story short simply installing kiwi via pip into the users pip cache fixes all of this trouble and allows python and pytest to find kiwi. It's also the same approach when installing kiwi from pip. As you moved kiwi to poetry and we have standard wheel package without any extra script code or magic, the install of the .whl is a stable way to make kiwi available to the system, no matter if it's an obs worker or a chroot or a debian gbp chroot, or any other system

Also pip can't work in Logo or OBS like this

This is not correct because the way pip is used here will not pull in any dependencies. All dependencies are satisfied by the proper BuildRequires as you can see it from the patch here in this PR

So the call of pip will see all deps resolved, and will simply install the .whl in a way that python can consume it.

What's with "breaking system packages"

SUSE doesn't want users to use pip and want to force them to install packages. Because of that reason the default behavior is that the tool tells you that you should install a package and if you think this is not correct use the --break-system-packages option. That's why I have to call it that way. I'm not sure if this option is generally available in pip but if not we need to differentiate the call between the distros

Overall I was also hesitant to call pip to make kiwi available to python but with the standard .whl package and all deps resolved via the packaging it seems the simplest and most stable way to allow to run the tests against the kiwi code from source. Setting up a venv via poetry or other tools always failed because that pulls in all deps from the network which does not work in obs and other buildsystems.

I have spent many hours and tried many different paths. So far this few lines at least worked local and in obs.

Copy link
Collaborator Author

@schaefi schaefi Jun 19, 2024

Choose a reason for hiding this comment

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

I'm still not sure if we want to run the tests with every package build, but @glaubitz had a good point when he said that we will not find issues due to python dependency updates done in the distributions. We ensure that no upstream release reached pypi without passing all tests, but all those tests run as github action on some Ubuntu system fetching dependencies from pip and not from the distribution. So it probably makes sense to let the tests run at package build time too

Copy link
Member

Choose a reason for hiding this comment

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

How did you try to run the tests before this pip method?

Copy link
Member

@Conan-Kudo Conan-Kudo Jun 19, 2024

Choose a reason for hiding this comment

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

In Fedora, we have a %pytest macro that's defined effectively like so:

PATH="%{buildroot}%{_bindir}:$PATH" PYTHONPATH="${PYTHONPATH:-%{buildroot}%{python3_sitearch}:%{buildroot}%{python3_sitelib}}" PYTHONDONTWRITEBYTECODE=1 pytest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That does not work for me

osc build --clean openSUSE_Tumbleweed x86_64

[   31s] + cd test/unit
[   31s] + PATH=/home/abuild/rpmbuild/BUILDROOT/python-kiwi-10.0.22-0.x86_64/usr/bin:/usr/local/bin:/usr/bin:/bin
[   31s] + PYTHONPATH='/home/abuild/rpmbuild/BUILDROOT/python-kiwi-10.0.22-0.x86_64%{pyth
[   31s] on3_sitearch:/home/abuild/rpmbuild/BUILDROOT/python-kiwi-10.0.22-0.x86_64/usr/lib/python3.11/site-packages}'
[   31s] + PYTHONDONTWRITEBYTECODE=1
[   31s] + pytest
[   33s] ============================= test session starts ==============================
[   33s] platform linux -- Python 3.11.9, pytest-7.4.4, pluggy-1.3.0
[   33s] rootdir: /home/abuild/rpmbuild/BUILD/kiwi-10.0.22
[   33s] configfile: setup.cfg
[   33s] collected 0 items / 180 errors
[   33s] 
[   33s] ==================================== ERRORS ====================================
[   33s] ________________ ERROR collecting test/unit/api_helper_test.py _________________
[   33s] ImportError while importing test module '/home/abuild/rpmbuild/BUILD/kiwi-10.0.22/test/unit/api_helper_test.py'.
[   33s] Hint: make sure your test modules/packages have valid Python names.
[   33s] Traceback:
[   33s] /usr/lib64/python3.11/importlib/__init__.py:126: in import_module
[   33s]     return _bootstrap._gcd_import(name[level:], package, level)
[   33s] api_helper_test.py:3: in <module>
[   33s]     from kiwi.api_helper import (
[   33s] E   ModuleNotFoundError: No module named 'kiwi'

... and a lot more of these

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How did you try to run the tests before this pip method?

we did not run them in the package build so far. If you have enabled that in your Fedora spec file I would be really interested in how you make it to work. As I said I tried for days without any success and @glaubitz also didn't found a method that worked for Debian

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a %check section to cover this part and added a comment to the spec file why the pip way was the only way to install into userland such that importlib finds the kiwi namespace.

I still could not find another way. @Conan-Kudo if you have a better idea please advise

@Conan-Kudo Conan-Kudo marked this pull request as draft July 9, 2024 12:32
@schaefi schaefi force-pushed the run_tests_during_package_build branch from 8ad9b24 to 87e2e8c Compare October 23, 2024 13:41
@schaefi schaefi force-pushed the run_tests_during_package_build branch from 87e2e8c to e29ab4a Compare October 23, 2024 15:05
For full test coverage the anymarkup module must be installed.
However the module is an optional module for kiwi and only
required when running tests to reach the 100% code coverage.
Unfortunately not all distros provide anymarkup which is the
reason why I limited the %check execution to only TW for the
moment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants