-
Notifications
You must be signed in to change notification settings - Fork 148
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
Fix mocks that cause tests to fail outside of containers #1276
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp 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,
See other labels for particular jobs defined in the 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. |
The run function was mocked instead of `leapp.libraries.common.grub.get_grub_devices`, making test fail when ran directly on the host, not in containers, because gru2-probe failed there as expected. Left a couple of TODOs to improve error handling in the grub library. Jira: OAMG-11647
The setup_target_rhui_access_if_needed() function is not covered by tests and makes tests fail outside of containers because it calls commands (api.run) which are not mocked. Jira: OAMG-11647
@@ -36,7 +36,7 @@ def blk_dev_from_partition(partition): | |||
api.current_logger().warning( | |||
'Could not get parent device of {} partition'.format(partition) | |||
) | |||
raise StopActorExecution() | |||
raise StopActorExecution() # TODO: return some meaningful value/error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 ➕ that's bug introduced basically when the private actor's library has been moved to shared library instead. Checking the library, it should be fixed everywhere, but let's keep it for another PR. Good catch!
@matejmatuska on my machine, there is still one test failing:
Based on the skip condition, it seems it's still reading something from the system. In container it does not appear as the test is skipped. Update: realized I executed it with python3.12 which could possibly affect the test too as the actor is supposed to be working just for python3.6. However, in case of running it under python3.6, the test is never executed as the required dependencies are missing - so it's skipped always - unless you have installed pygobject, etc... for python3.6 interpreter. then you can executed for python3.6. we should check that separately with the author as it's possible the actor or test is all the time broken as well and we just didn't now about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. discovere one additional problem with nmconnscanner tests but keeping that for separate PR in future (see my comment).
See commit messages for details.
To reproduce you can use, for example:
PYTHON_VENV=python3.6 REPOSITORIES='common,el8toel9' make install-deps-fedora test_no_lint