thom311/libnl

3.6.0: pytest is failing on python module

Closed this issue ยท 6 comments

I'm trying to package your projest as a rpm package.
Part of your project is python module.
So I'm using the typical PEP517 based build, install and test cycle used on building packages from non-root account.

  • python3 -sBm build -w --no-isolation
  • because I'm calling build with --no-isolation I'm using during all processes only locally installed modules
  • install .whl file in </install/prefix>
  • run pytest with PYTHONPATH pointing to sitearch and sitelib inside </install/prefix>

Here is pytest output:

+ cd python
+ PYTHONPATH=/home/tkloczko/rpmbuild/BUILDROOT/libnl3-3.6.0-2.fc35.x86_64/usr/lib64/python3.8/site-packages:/home/tkloczko/rpmbuild/BUILDROOT/libnl3-3.6.0-2.fc35.x86_64/usr/lib/python3.8/site-packages
+ /usr/bin/pytest -ra tests/test-create-bridge.py
=========================================================================== test session starts ============================================================================
platform linux -- Python 3.8.13, pytest-7.1.1, pluggy-1.0.0
rootdir: /home/tkloczko/rpmbuild/BUILD/libnl-3.6.0/python
collected 0 items / 1 error

================================================================================== ERRORS ==================================================================================
_______________________________________________________________ ERROR collecting tests/test-create-bridge.py _______________________________________________________________
/usr/lib/python3.8/site-packages/_pytest/python.py:608: in _importtestmodule
    mod = import_path(self.path, mode=importmode, root=self.config.rootpath)
/usr/lib/python3.8/site-packages/_pytest/pathlib.py:533: in import_path
    importlib.import_module(module_name)
/usr/lib64/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1014: in _gcd_import
    ???
<frozen importlib._bootstrap>:991: in _find_and_load
    ???
<frozen importlib._bootstrap>:975: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:671: in _load_unlocked
    ???
/usr/lib/python3.8/site-packages/_pytest/assertion/rewrite.py:159: in exec_module
    source_stat, co = _rewrite_test(fn, self.config)
/usr/lib/python3.8/site-packages/_pytest/assertion/rewrite.py:359: in _rewrite_test
    tree = ast.parse(source, filename=strfn)
/usr/lib64/python3.8/ast.py:47: in parse
    return compile(source, filename, mode, flags,
E     File "/home/tkloczko/rpmbuild/BUILD/libnl-3.6.0/python/tests/test-create-bridge.py", line 11
E       print testtap1
E             ^
E   SyntaxError: Missing parentheses in call to 'print'. Did you mean print(testtap1)?
========================================================================= short test summary info ==========================================================================
ERROR tests/test-create-bridge.py
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
============================================================================= 1 error in 0.16s =============================================================================

Probably it would be good to test that python code usin pytest-cov, pytest-flake and pytest-black. I would be not surprices if those extensions will show more issues.

I think the python module is very rudimentary, not tested, and gets very little attention. I regret enabling it for Fedora, because it will be hard to drop. On the other hand, having it (and having it in Fedora) means that somebody could improve it...

Your first issue here is that test-create-bridge.py is not python3 compatible (the print statement). We would have to python3-ize. Yes, using black would be good.

tests/test-create-bridge.py is also not really a unit test. Like most tests/test-*.c files. These make assumptions about your system, they often require CAP_NET_ADMIN priviledges and they modify the test system. Only recently I added tests/cksuite-all-netns.c, which creates a new namespace (thus does not require root nor does it modified the tested system) and that it really contains unit-tests (that is, tests that are also run by the github actions). For python, something similiar would be necessary.

As the name tests/test-* was already taken for not unit tests, the actual unit tests are called tests/check*. The tests/test-* binaries are only for testing compilation (and I guess, you could manually tweak and call that, which is obviously not "CI").

Anyway, patches welcome all improvements!!

tests/test-create-bridge.py is also not really a unit test. Like most tests/test-*.c files. These make assumptions about your system, they often require CAP_NET_ADMIN priviledges and they modify the test system. Only recently I added tests/cksuite-all-netns.c, which creates a new namespace (thus does not require root nor does it modified the tested system) and that it really contains unit-tests (that is, tests that are also run by the github actions). For python, something similiar would be necessary.

OK so I see where is the problem.
I'm building all my packages in LXC zones (created just to build single package). LXC by default strips down CAP_NET_ADMIN.
Q: how can I disable that one test unit? ๐Ÿค”

no, no. This is not a unit test, there is nothing to disable. Just don't run it.

You know .. with test suites is like with documentation.
Pparaphtrasing from gawk documentation: "With test suites is like with sex. Whn it is good it is very good. When it is bad. it is better than nothing" ๐Ÿ˜ƒ
This is why I've been thinking about remove somehow that single unit from execution ๐Ÿ˜„

I think the test suite should be run by make check, and not by searching the files for things that have test in the name. make check is supposed to run exactly the tests.

I agree, that the names tests/test-* and python/tests/test-* for not unit tests is bad and confusing.

I am gonna close this issue, because python/tests/test-* is not intended to run this way.

Having better unit tests would be welcome however. Or renaming non-unit-tests to not use the name "test-*". Patch welcome.