Makefile: Partially fix ACTOR tests and enable them in containers #1403

matejmatuska · 2025-07-09T16:58:45Z

The ACTOR variable for running tests of a single actor has long been unusable, git bisect led me to dc3abf6 as the commit where it was (first?) broken due to the same model module being defined in both el7toel8 and el8toel9 repos.

This patch unfortunately doesn't fix that, but works around it by enabling the utils/actor_path.py script to search in specific repositories. These are passed from the Makefile via REPOSITORIES variable. A warning is also printed when attempting to run tests with ACTOR without REPOSITORIES.

As the Makefile rules for containerized tests already use repositories, the ACTOR variable is just passed along.

NOTE: the code in actor_path.py is ugly and uses private APIs, however that's nothing new :).

ALSO FIXED:

Since b6e84f7 the sanity check runs in each repository and doesn't respect $(REPOSITORIES). This breaks the sanity-check when it's run with a version of Python which is incompatible with the code in particular repository.

For example with python3.6 and el9toel10 repo, python crashes with the following because python3.6 doesn't understand the type hint:

  File "/payload/repos/system_upgrade/el9toel10/actors/mysql/scanmysql/libraries/scanmysql.py", line 35, in <module>
    def _check_incompatible_config() -> set[str]:
TypeError: 'type' object is not subscriptable

Yes, there is also dev_test_no_lint which is still faster, but currently doesn't work in containers.

github-actions · 2025-07-09T16:58:54Z

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the leapp-repository contribution and development guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

review please @oamg/developers to notify leapp developers of the review request
/packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
However, here are additional useful commands for packit:

/packit test to re-run manually the default tests
/packit retest-failed to re-run failed tests manually
/packit test oamg/leapp#42 to run tests with leapp builds for the leapp PR#42 (default is latest upstream - main - build)

Note that first time contributors cannot run tests automatically - they need to be started by a reviewer.

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

/packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
/packit test --labels beaker-minimal-8.10to9.4,kernel-rt-8.10to9.4 to schedule kernel-rt and beaker-minimal test sets for 8.10->9.4 upgrade path

See other labels for particular jobs defined in the .packit.yaml file.

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

Makefile

MichalHe

Code looks OK, I have provided only a minor suggestion regarding the comment. I will give Approve once I run it a couple of times.

Makefile

tomasfratrik · 2025-08-07T08:16:31Z

I have tried few tests in containers, and they worked. So lgtm.

pirat89

This change breaks the execution for linters at this moment. See, it's trying to use python3.6 for el9toel10 actors which are python3.9 compatible only:

Process Process-13:
Traceback (most recent call last):
  File "/usr/lib64/python3.6/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/usr/lib64/python3.6/multiprocessing/process.py", line 93, in run
    self._target(*self._args, **self._kwargs)
  File "/payload/tut/lib/python3.6/site-packages/leapp/repository/actor_definition.py", line 32, in inspect_actor
    definition.load()
  File "/payload/tut/lib/python3.6/site-packages/leapp/repository/actor_definition.py", line 199, in load
    self._module = load_module(importer, name)
  File "/payload/tut/lib/python3.6/site-packages/leapp/compat.py", line 86, in load_module
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/payload/repos/system_upgrade/el9toel10/actors/mysql/scanmysql/actor.py", line 2, in <module>
    from leapp.libraries.actor import scanmysql
  File "/payload/repos/system_upgrade/el9toel10/actors/mysql/scanmysql/libraries/scanmysql.py", line 35, in <module>
    def _check_incompatible_config() -> set[str]:
TypeError: 'type' object is not subscriptable
2025-08-08 15:50:53.225 ERROR    PID: 952 leapp.repository.system_upgrade_el9toel10: Process inspecting actor in actors/mysql/scanmysql failed with 1
Inspection of actor in actors/mysql/scanmysql failed
make: *** [Makefile:381: test_no_lint] Error 1

matejmatuska · 2025-08-11T12:15:47Z

This change breaks the execution for linters at this moment. See, it's trying to use python3.6 for el9toel10 actors which are python3.9 compatible only:

Process Process-13:
Traceback (most recent call last):
  File "/usr/lib64/python3.6/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/usr/lib64/python3.6/multiprocessing/process.py", line 93, in run
    self._target(*self._args, **self._kwargs)
  File "/payload/tut/lib/python3.6/site-packages/leapp/repository/actor_definition.py", line 32, in inspect_actor
    definition.load()
  File "/payload/tut/lib/python3.6/site-packages/leapp/repository/actor_definition.py", line 199, in load
    self._module = load_module(importer, name)
  File "/payload/tut/lib/python3.6/site-packages/leapp/compat.py", line 86, in load_module
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/payload/repos/system_upgrade/el9toel10/actors/mysql/scanmysql/actor.py", line 2, in <module>
    from leapp.libraries.actor import scanmysql
  File "/payload/repos/system_upgrade/el9toel10/actors/mysql/scanmysql/libraries/scanmysql.py", line 35, in <module>
    def _check_incompatible_config() -> set[str]:
TypeError: 'type' object is not subscriptable
2025-08-08 15:50:53.225 ERROR    PID: 952 leapp.repository.system_upgrade_el9toel10: Process inspecting actor in actors/mysql/scanmysql failed with 1
Inspection of actor in actors/mysql/scanmysql failed
make: *** [Makefile:381: test_no_lint] Error 1

It's broken on main due to the sanity check, which now runs in each repository and doesn't respect $(REPOSITORIES). This has been introduced in b6e84f7. I will make it respect $(REPOSITORIES). As can be seen, no tests are run, just the sanity check:

Running sanity-check in repos/system_upgrade/common/
Running sanity-check in repos/system_upgrade/el8toel9/
Running sanity-check in repos/system_upgrade/el9toel10/
Process Process-13:
Traceback (most recent call last):
  File "/usr/lib64/python3.6/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/usr/lib64/python3.6/multiprocessing/process.py", line 93, in run
    self._target(*self._args, **self._kwargs)
  File "/payload/tut/lib/python3.6/site-packages/leapp/repository/actor_definition.py", line 32, in inspect_actor
    definition.load()
  File "/payload/tut/lib/python3.6/site-packages/leapp/repository/actor_definition.py", line 199, in load
    self._module = load_module(importer, name)
  File "/payload/tut/lib/python3.6/site-packages/leapp/compat.py", line 86, in load_module
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/payload/repos/system_upgrade/el9toel10/actors/mysql/scanmysql/actor.py", line 2, in <module>
    from leapp.libraries.actor import scanmysql
  File "/payload/repos/system_upgrade/el9toel10/actors/mysql/scanmysql/libraries/scanmysql.py", line 35, in <module>
    def _check_incompatible_config() -> set[str]:
TypeError: 'type' object is not subscriptable
2025-08-08 15:50:53.225 ERROR    PID: 952 leapp.repository.system_upgrade_el9toel10: Process inspecting actor in actors/mysql/scanmysql failed with 1
Inspection of actor in actors/mysql/scanmysql failed
make: *** [Makefile:381: test_no_lint] Error 1
Script done.

matejmatuska · 2025-08-11T13:28:23Z

@pirat89 I fixed the sanity check in a new commit, tests now pass.

pirat89 · 2025-08-12T07:08:07Z

@matejmatuska you are right! thanks for fixing it. However, trying the functionality now, it seems broken when ACTOR is specified:

$ TEST_CONTAINER=rhel8 make test_container ACTOR=target_userspace_creator
make[1]: Entering directory '/home/pstodulk/special/leapp/leapp-repository'
make[1]: Leaving directory '/home/pstodulk/special/leapp/leapp-repository'
=== Running  in rhel8 container ===
Error: no container with name or ID "leapp-repo-tests-rhel8-cont" found: no such container
Error: no container with ID or name "leapp-repo-tests-rhel8-cont" found: no such container
5387fda00354ca96e81dd82ac0ae826691e452ee315f0bf2b15c0f254159bb6c
make[1]: Entering directory '/home/pstodulk/special/leapp/leapp-repository'
--- Clean repo ---
. tut/bin/activate; \
echo "--- Linting ... ---" && \
SEARCH_PATH="`${PYTHON_VENV:-python3.6} utils/actor_path.py targetuserspacecreator common,el8toel9`" && \
echo "Using search path '${SEARCH_PATH}'" && \
echo "--- Running pylint ---" && \
bash -c "[[ ! -z '${SEARCH_PATH}' ]] && find ${SEARCH_PATH} -name '*.py' | sort -u | xargs pylint -j0 " && \
echo "--- Running flake8 ---" && \
bash -c "[[ ! -z '${SEARCH_PATH}' ]] && flake8 ${SEARCH_PATH} "
--- Linting ... ---
ERROR: Unknown error: No repos configured? Try adding some with "snactor repo find"
ERROR: Possibly you need newer version of the leapp framework
ERROR:   e.g.: rm -rf .tut && make install-deps-fedora
make: *** [Makefile:343: lint] Error 1
make[1]: *** [Makefile:406: _test_container_ipu] Error 2
make[1]: Leaving directory '/home/pstodulk/special/leapp/leapp-repository'
leapp-repo-tests-rhel8-cont
leapp-repo-tests-rhel8-cont
TIP: If you do not see an error in the end of logs, scroll up. Multiple tests could be executed.
make: *** [Makefile:438: test_container] Error 1

Trying to specify REPOSITORIES manually did not help in my case (there is always space for PEBKAC however).

matejmatuska · 2025-08-14T13:41:50Z

@pirat89 It should be resolved now, I added a manual call to snactor repo find in the container which generates ~/.config/leapp/repos.json which is required by the utils/actor_path.py script to link leapp repositories.

PeterMocary

The code LGTM, and I found only one nitpick.

I've tested the functionality and found that running the following command results in make exiting with an error.

TEST_CONTAINER=el9 ACTOR=sssd_facts make test_container_no_lint

The RHEL9 container tests both upgrade paths, but the given actor is only in one of the paths, so the actor search in the el8toel9 repository fails.

=== Running test_no_lint in rhel9 container ===
Error: no container with name or ID "leapp-repo-tests-rhel9-cont" found: no such container
Error: no container with ID or name "leapp-repo-tests-rhel9-cont" found: no such container
03aa1116d0d66507995d8e369ef4ff960501f6af8491ec5b47ae549e24c4dc73
Registering repos/common
Registering repos/system_upgrade/common
Registering repos/system_upgrade/el8toel9
Registering repos/system_upgrade/el9toel10
make[2]: Entering directory '/home/pmocary/Repos/leapp-repository'
--- Clean repo ---
ERROR: No actor found for search "sssd_facts"
============= snactor sanity-check ipu ===============
. tut/bin/activate; \
snactor repo find --path repos/; \
for dir in $(echo common,el8toel9 | tr "," " "); do \
	echo "Running sanity-check in "repos/system_upgrade"/$dir"; \
	(cd "repos/system_upgrade"/$dir && snactor workflow sanity-check ipu); \
done
Registering repos/common
Registering repos/system_upgrade/common
Registering repos/system_upgrade/el8toel9
Registering repos/system_upgrade/el9toel10
Running sanity-check in repos/system_upgrade/common
Running sanity-check in repos/system_upgrade/el8toel9
==================== unit tests ======================
. tut/bin/activate; \
${PYTHON_VENV:-python3.6} -m pytest  ERROR:__read_error_messages_above_this_one_on_stderr__ 
[...]

In contrast, the RHEL8 container tests only the el8toel9 path, so giving it the same sssd_facts actor results in a similar actor-not-found error. Using an existing actor on this path worked as expected.

We should decide whether the RHEL9 container running both paths on python3.9 should fail on one when the actor is not found, or whether only one path that actually contains the actor should be run.

I'm ok with the current behavior. However, it's not immediately intuitive that there is a failing part while the tests were, in fact, executed correctly.

utils/actor_path.py

matejmatuska · 2025-10-16T17:31:20Z

Okay I pushed a fix, but it's ugly. At this point I think we should extract a lot of the containerized tests related code into a script or separate Makefile maybe. It's become a undebuggable mess.

The error is still being printed, but there is a line in yellow informing that the test is skipped.

matejmatuska · 2025-10-17T06:35:28Z

Incorrect quoting broke tests without ACTOR, fixed now.

matejmatuska · 2025-10-17T06:38:39Z

Makefile

+# the below commands need to be one shell invocation for the early exit to work;
+# note: need to store the paths into separate var as it here as it's lazily
+# evaluated on each use :), using ?= for the assignment does not help for
+# some reason


These have to be unindented, otherwise they are treated as bash not make comments and get echoed by make :/. Another solution is to prefix all of them with the '@'.

btw, if you use spaces instead of tabs, it will be also processed as comment. but rather do it this way, as otherwise it could happen that someone by mistake add spaces instead of tabs to a real code/recipe.

I didn't know that, but it makes sense. I will leave it as it is.

matejmatuska · 2025-10-17T12:45:37Z

Force pushed after resolving a conflict

Since b6e84f7, the sanity check, runs in each repository and doesn't respect $(REPOSITORIES). This breaks the sanity-check when it's run with a version of Python which is incompatible with the code in particular repository. For example with python3.6 and el9toel10 repo, python crashes with the following because python3.6 doesn't understand the type hint: File "/payload/repos/system_upgrade/el9toel10/actors/mysql/scanmysql/libraries/scanmysql.py", line 35, in <module> def _check_incompatible_config() -> set[str]: TypeError: 'type' object is not subscriptable

The ACTOR variable for running tests of a single actor has long been unusable, git bisect led me to dc3abf6 as the commit where it was (first?) broken due to the same model module being defined in both el7toel8 and el8toel9 repos. This patch unfortunately doesn't fix that, but works around it by enabling the utils/actor_path.py script to search in specific repositories. These are passed from the Makefile via REPOSITORIES variable. As the Makefile rules for containerized tests already use repositories, the ACTOR variable is just passed along. NOTE: the code in actor_path.py is ugly and uses private APIs, however that's nothing new :).

Using ACTOR withtout REPOSITORIES leads to a dead lock during actor discovery (likely due to the 'multipathconfcheck' actor). This patch adds a new make target that prevents the use of ACTOR without REPOSITORIES.

matejmatuska · 2025-10-29T12:41:38Z

Force pushed after resolving a conflict

Again, another conflict.

PeterMocary

The issue I mentioned in my above comment was fixed.

I tried the case where one wants to test an actor that exists only in one of the upgrade path repositories. The tests are now executed only for the repository which has the specified actor while also informing about the skipped unit tests run when actor could not be found in the repository.

Running the TEST_CONTAINER=el9 ACTOR=sssd_facts make test_container_no_lint
resulted in skipped unit tests for el8toel9 repository while for the el9toel10 the tests were run as expected because the actor sssd_facts exists there. The makefile did not report error at the end.

==================== unit tests ======================
Failed to find the sssd_facts actor in the common,el8toel9 repositories: ERROR:__read_error_messages_above_this_one_on_stderr__
Skipping unit tests, could not find the 'sssd_facts' actor in common,el8toel9 repositories

I've also tried it with a common actor, an actor that is only present on el8toel9 repository and with the test container el8. Everything worked as expected.

LGTM!

I also agree that the makefile should be improved, since now it is very hard to navigate.

matejmatuska force-pushed the makefile-actor-test-fix branch from c38d7f1 to f512a70 Compare July 10, 2025 09:11

matejmatuska marked this pull request as ready for review July 10, 2025 09:12

matejmatuska force-pushed the makefile-actor-test-fix branch 2 times, most recently from 849d7ba to c4a7827 Compare July 10, 2025 10:39

tomasfratrik reviewed Jul 10, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

MichalHe reviewed Jul 11, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

pirat89 added this to the 8.10/9.7 milestone Aug 8, 2025

matejmatuska force-pushed the makefile-actor-test-fix branch from f9a76d8 to 9cd1218 Compare August 8, 2025 15:46

pirat89 requested changes Aug 11, 2025

View reviewed changes

matejmatuska force-pushed the makefile-actor-test-fix branch from 9cd1218 to 6d1a327 Compare August 11, 2025 13:22

pirat89 modified the milestones: 8.10/9.7, 8.10/9.8 Aug 13, 2025

matejmatuska force-pushed the makefile-actor-test-fix branch from 0981667 to ad8bfb9 Compare August 14, 2025 13:42

matejmatuska requested a review from pirat89 October 15, 2025 14:33

PeterMocary reviewed Oct 16, 2025

View reviewed changes

utils/actor_path.py Show resolved Hide resolved

matejmatuska force-pushed the makefile-actor-test-fix branch 3 times, most recently from 8258e62 to 675e422 Compare October 17, 2025 06:34

matejmatuska commented Oct 17, 2025

View reviewed changes

matejmatuska force-pushed the makefile-actor-test-fix branch from 675e422 to 1dde784 Compare October 17, 2025 12:45

matejmatuska and others added 3 commits October 29, 2025 13:40

make: error uniformly when ACTOR without REPOSITORIES is used

ce388e3

Using ACTOR withtout REPOSITORIES leads to a dead lock during actor discovery (likely due to the 'multipathconfcheck' actor). This patch adds a new make target that prevents the use of ACTOR without REPOSITORIES.

Makefile: Skip tests with ACTOR when it's not present in REPOSITORIES

955daa2

matejmatuska force-pushed the makefile-actor-test-fix branch from 1dde784 to 955daa2 Compare October 29, 2025 12:41

PeterMocary reviewed Oct 30, 2025

View reviewed changes

Makefile: Partially fix ACTOR tests and enable them in containers #1403

Are you sure you want to change the base?

Makefile: Partially fix ACTOR tests and enable them in containers #1403

Uh oh!

Conversation

matejmatuska commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ALSO FIXED:

Uh oh!

github-actions bot commented Jul 9, 2025

Thank you for contributing to the Leapp project!

Uh oh!

Uh oh!

MichalHe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomasfratrik commented Aug 7, 2025

Uh oh!

pirat89 left a comment

Choose a reason for hiding this comment

Uh oh!

matejmatuska commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matejmatuska commented Aug 11, 2025

Uh oh!

pirat89 commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matejmatuska commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterMocary left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matejmatuska commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matejmatuska commented Oct 17, 2025

Uh oh!

matejmatuska Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

pirat89 Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

matejmatuska Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

matejmatuska commented Oct 17, 2025

Uh oh!

matejmatuska commented Oct 29, 2025

Uh oh!

PeterMocary left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matejmatuska commented Jul 9, 2025 •

edited

Loading

matejmatuska commented Aug 11, 2025 •

edited

Loading

pirat89 commented Aug 12, 2025 •

edited

Loading

matejmatuska commented Aug 14, 2025 •

edited

Loading

PeterMocary left a comment •

edited

Loading

matejmatuska commented Oct 16, 2025 •

edited

Loading

PeterMocary left a comment •

edited

Loading