some reserve-mb adjusts

[iasunsea]

we need to check reserve-mb number is in range of the lower and upper when we use ks command.
we know the range with tui, and we also can see the range with the gui

[VladimirSlavik]

@coiby um, ping?

[coiby]

@VladimirSlavik Thanks for the reminder!

@iasunsea Thanks for the PR! Unfortunately, it fails some tests,

$ make runpylint
***Running pylint checks***
pylint com_redhat_kdump -E 2> /dev/null
************* Module com_redhat_kdump.gui.spokes.kdump
com_redhat_kdump/gui/spokes/kdump.py:94:8: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:95:8: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:110:16: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:115:8: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:147:49: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:218:32: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
make: *** [Makefile:93: runpylint] Error 2


$ make unittest
***Running unittests checks***
pytest -vv test/unit_tests
==================================================================================== test session starts ====================================================================================
platform linux -- Python 3.11.3, pytest-7.1.3, pluggy-1.0.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /home/coiby/kexec-tools/kdump-anaconda-addon
collected 29 items                                                                                                                                                                          

test/unit_tests/test_common.py::KdumpCommonTestCase::test_memory_bound_aarch64 PASSED                                                                                                 [  3%]
test/unit_tests/test_common.py::KdumpCommonTestCase::test_memory_bound_ppc64 PASSED                                                                                                   [  6%]
test/unit_tests/test_common.py::KdumpCommonTestCase::test_memory_bound_x86 PASSED                                                                                                     [ 10%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_fadump_enabled PASSED                                                                             [ 13%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_get_default_crashkernel PASSED                                                                    [ 17%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_get_default_crashkernel_empty PASSED                                                              [ 20%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_get_default_crashkernel_file_not_found PASSED                                                     [ 24%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_crashkernel_auto PASSED                                                                     [ 27%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_crashkernel_auto_fallback PASSED                                                            [ 31%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_crashkernel_auto_fallback_legacy PASSED                                                     [ 34%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_disabled PASSED                                                                             [ 37%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_enabled PASSED                                                                              [ 41%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_installation_kdump_disabled PASSED                                                                              [ 44%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_installation_kdump_enabled PASSED                                                                               [ 48%]
test/unit_tests/test_interface.py::KdumpInterfaceTestCase::test_fadump_enabled PASSED                                                                                                 [ 51%]
test/unit_tests/test_interface.py::KdumpInterfaceTestCase::test_kdump_enabled PASSED                                                                                                  [ 55%]
test/unit_tests/test_interface.py::KdumpInterfaceTestCase::test_reserved_memory FAILED                                                                                                [ 58%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_default PASSED                                                                                                     [ 62%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_disable PASSED                                                                                                     [ 65%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_enable PASSED                                                                                                      [ 68%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_enablefadump PASSED                                                                                                [ 72%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_auto PASSED                                                                                                [ 75%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_aarch64_with_low FAILED                                                                                 [ 79%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_aarch64_with_normal FAILED                                                                              [ 82%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_aarch64_with_up FAILED                                                                                  [ 86%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_invalid PASSED                                                                                          [ 89%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_x86_with_low FAILED                                                                                     [ 93%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_x86_with_normal FAILED                                                                                  [ 96%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_x86_with_up FAILED                                                                                      [100%]

========================================================================================= FAILURES ==========================================================================================
________________________________________________________________________ KdumpInterfaceTestCase.test_reserved_memory ________________________________________________________________________

self = <test.unit_tests.test_interface.KdumpInterfaceTestCase testMethod=test_reserved_memory>

    def test_reserved_memory(self):
        self._interface.ReservedMemory = "256"
>       self._check_properties_changed("ReservedMemory", "256")

test/unit_tests/test_interface.py:53: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test/unit_tests/test_interface.py:35: in _check_properties_changed
    self._callback.assert_called_once_with(
/usr/lib64/python3.11/unittest/mock.py:945: in assert_called_once_with
    return self.assert_called_with(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <PropertiesChangedCallback id='140662632707024'>, args = ('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256'}, []), kwargs = {}
expected = call('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256'}, []), actual = call('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256M'}, [])
_error_message = <function NonCallableMock.assert_called_with.<locals>._error_message at 0x7fee92acd4e0>, cause = None

    def assert_called_with(self, /, *args, **kwargs):
        """assert that the last call was made with the specified arguments.
    
        Raises an AssertionError if the args and keyword args passed in are
        different to the last call to the mock."""
        if self.call_args is None:
            expected = self._format_mock_call_signature(args, kwargs)
            actual = 'not called.'
            error_message = ('expected call not found.\nExpected: %s\nActual: %s'
                    % (expected, actual))
            raise AssertionError(error_message)
    
        def _error_message():
            msg = self._format_mock_failure_message(args, kwargs)
            return msg
        expected = self._call_matcher(_Call((args, kwargs), two=True))
        actual = self._call_matcher(self.call_args)
        if actual != expected:
            cause = expected if isinstance(expected, Exception) else None
>           raise AssertionError(_error_message()) from cause
E           AssertionError: expected call not found.
E           Expected: mock('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256'}, [])
E           Actual: mock('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256M'}, [])
...

[iasunsea]

@coiby i have amend the code, please take a C C

[coiby]

Thanks for improving kdump-anaconda-addon! I've finished the review and please check the comments.

[coiby]

reserved_memory is architecture-independent so there is no need to create fixtures for each architecture. You shall be able to simplify the tests.

[iasunsea]

https://github.com/rhinstaller/kdump-anaconda-addon/blob/master/com_redhat_kdump/common.py#L71-L82, it make diffrent architecture lowerBound , so to test diffrent architecture is need.

[coiby]

Thanks for correcting me! But I think my point still stands. Although different architectures have different lowerBound and minUsable values, you only need to test one arch if choose a proper testing strategy. If you partition ReservedMemory by a) ReservedMemory <= lowerBound b) b) lowerBound < ReservedMemory < upperBound ReservedMemory >= upperBound, you only need to mock one arch.

[coiby]

I don't see no any need to create fixtures for each architecture here either. And if reserved_memory has already been thoroughly tested, maybe one or two test cases are sufficient?

[iasunsea]

ok, i will simplify the tests of test_kickstart.py

[iasunsea]

@coiby i have amend the code, please again.

[iasunsea]

i have saw the failed， i will change it.

[iasunsea]

pls do the check

[coiby]

@iasunsea One unit test still fails. Btw, I'll change the setting so you have the permission to trigger the workflow. Or you can run the tests locally by make test.

[iasunsea]

@iasunsea One unit test still fails. Btw, I'll change the setting so you have the permission to trigger the workflow. Or you can run the tests locally by make test.

ok， i think make test is good for me to do check.

[iasunsea]

it‘s ok, pls check

[coiby]

I don't think you should change this test or test/unit_tests/test_interface.py at all. All tests here in test/unit_tests/test_interface.py are to make sure PropertiesChanged signal will be emitted and then the corresponding callback functions will catch it.

[iasunsea]

Because different plat or different machine with test the “_lower“ ”_upper” will have different value. And when we just want to make sure PropertiesChanged will be emitted, we just control "check_reserved_memory" return value.

[coiby]

Maybe you can put the above code inside a standalone function and unit-test this function (choose three values, one < lowerBound, one between lowerBound and upperBound and one > upperBound) instead? I think this shall make the test easier and there is also no need to change test/unit_tests/test_kickstart.py.

[iasunsea]

different plat or different machine with test the “_lower“ ”_upper” will have different value. the set value oftest/unit_tests/test_kickstart.py must between the “_lower“ ”_upper” ， this is the pr we want to do. And i have change to control "getMemoryBounds" to give numeric value.

[iasunsea]

@coiby it‘s ok, pls.
I have to tell the tests in diffrent palt the up and low are diffrent, but the test must have the same result. so the tests will have some limitations.

[coiby]

It seems there is no need to change test_reserved_memory, or am I mistaken?

[coiby]

For this test, you should patch com_redhat_kdump.service.kdump.getMemoryBounds instead because you should patch a function where it's used (imported) instead of where it's defined. Though unittest doesn't support parameterized tests, you can still simplify the code a bit,

    @patch("com_redhat_kdump.service.kdump.getMemoryBounds", return_value = (500, 800, 1))
    def test_check_reserved_memory(self, mocker):
        service1 = KdumpService()
        for memory, reserved in [("900", "800"), ("400", "500"), ("600", "600")]:
            self.assertEqual(service1.check_reserved_memory(memory), reserved)

[coiby]

MockBuiltinRead is no longer used.

[coiby]

For this test, I would suggest to patch for setup instead,

diff --git a/test/unit_tests/test_kickstart.py b/test/unit_tests/test_kickstart.py
index 4f063de..66817ac 100644
--- a/test/unit_tests/test_kickstart.py
+++ b/test/unit_tests/test_kickstart.py
@@ -2,13 +2,13 @@ from textwrap import dedent
 from unittest.case import TestCase
 from unittest.mock import patch
 from com_redhat_kdump import common
-from .mock import MockBuiltinRead
 from com_redhat_kdump.service.kdump import KdumpService
 
 
 class KdumpKickstartTestCase(TestCase):
 
-    def setUp(self):
+    @patch("com_redhat_kdump.service.kdump.getMemoryBounds", return_value=(160, 800, 1))
+    def setUp(self, mocker):
         # Show unlimited diff.
         self.maxDiff = None
 
@@ -70,10 +70,7 @@ class KdumpKickstartTestCase(TestCase):
         %end
         """)
 
-    @patch("com_redhat_kdump.common.getMemoryBounds", return_value=(160, 800, 1))
-    def test_ks_reserve_mb(self, _mock_read):
-        self._service._lower , self._service._upper, self._service._step = common.getMemoryBounds()
-        self.assertEqual((self._service._lower , self._service._upper), (160, 800))
+    def test_ks_reserve_mb(self):
         self._check_ks_input("""

[iasunsea]

it's your turn, pls close this pr and commit a new one.

[coiby]

I've merged this PR and created a separate commit 8f61cc1 ("test: patch where an object is looked up") to fix the mocking issue. Thank you for your contribution to this project!