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

Fix mocks that cause tests to fail outside of containers #1276

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

matejmatuska
Copy link
Member

@matejmatuska matejmatuska commented Aug 9, 2024

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

Copy link

github-actions bot commented Aug 9, 2024

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.
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 - master - 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.

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
Copy link
Member

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!

@pirat89 pirat89 added this to the 8.10/9.5 milestone Aug 9, 2024
@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Aug 9, 2024
@pirat89
Copy link
Member

pirat89 commented Aug 9, 2024

@matejmatuska on my machine, there is still one test failing:

================================================================================================================== FAILURES ==================================================================================================================
________________________________________________________________________________________________________________ test_nm_conn ________________________________________________________________________________________________________________

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f6ac3dbb1d0>

    @pytest.mark.skipif(not nmconnscanner.libnm_available, reason="NetworkManager g-ir not installed")
    def test_nm_conn(monkeypatch):
        """
        Check a basic keyfile
        """
    
        monkeypatch.setattr(nmconnscanner.os, "listdir", _listdir_nm_conn)
        monkeypatch.setattr(api, "produce", produce_mocked())
        monkeypatch.setattr(nmconnscanner.GLib.KeyFile, "load_from_file", _load_from_file)
        nmconnscanner.process()
    
        assert api.produce.called == 1
        assert len(api.produce.model_instances) == 1
        nm_conn = api.produce.model_instances[0]
        assert isinstance(nm_conn, NetworkManagerConnection)
        assert nm_conn.filename == "/etc/NetworkManager/system-connections/conn1.nmconnection"
        assert len(nm_conn.settings) == 3
        assert nm_conn.settings[0].name == "connection"
>       assert len(nm_conn.settings[0].properties) == 4
E       assert 3 == 4
E         +3
E         -4

repos/system_upgrade/el8toel9/actors/networkmanagerconnectionscanner/tests/unit_test_networkmanagerconnectionscanner.py:78: AssertionError

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.

Copy link
Member

@pirat89 pirat89 left a 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).

@pirat89 pirat89 merged commit a8c5664 into oamg:master Aug 9, 2024
19 of 20 checks passed
@matejmatuska matejmatuska deleted the fix-mocks branch December 13, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants