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.

Sign up for GitHub

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

Jump to bottom

some reserve-mb adjusts #35

Merged
merged 2 commits into from
Oct 20, 2023
Merged

some reserve-mb adjusts #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84baed2
gui add reserved mem range remind
iasunsea May 9, 2023
dc73730
check 'reserve-mb' number with kickstart
iasunsea May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions com_redhat_kdump/gui/spokes/kdump.glade
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,17 @@
<property name="top-attach">0</property>
</packing>
</child>
<child>
<object class="GtkLabel" id="rangeReservedMemMBLabel">
<property name="visible">True</property>
<property name="can-focus">False</property>
<property name="halign">start</property>
</object>
<packing>
<property name="left-attach">2</property>
<property name="top-attach">0</property>
</packing>
</child>
<child>
<object class="GtkLabel" id="totalMemLabel">
<property name="visible">True</property>
Expand Down
4 changes: 3 additions & 1 deletion com_redhat_kdump/gui/spokes/kdump.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ def initialize(self):
self._manualButton = self.builder.get_object("manualButton")
self._fadumpButton = self.builder.get_object("fadumpCheck")
self._toBeReservedSpin = self.builder.get_object("toBeReservedSpin")
self._rangeReservedMemMBLabel = self.builder.get_object("rangeReservedMemMBLabel")
self._totalMemMB = self.builder.get_object("totalMemMB")
self._usableMemMB = self.builder.get_object("usableMemMB")
self._autoWarn = self.builder.get_object("autoReservationWarning")
Expand All @@ -92,6 +93,7 @@ def initialize(self):
adjustment = Gtk.Adjustment(lower, lower, upper, step, step, 0)
self._toBeReservedSpin.set_adjustment(adjustment)
self._toBeReservedSpin.set_value(lower)
self._rangeReservedMemMBLabel.set_text(" (%d - %d MB)" % (lower, upper))

# Connect a callback to the PropertiesChanged signal.
storage = STORAGE.get_proxy()
Expand Down Expand Up @@ -142,7 +144,7 @@ def apply(self):
if self._autoButton.get_active():
self._proxy.ReservedMemory = "auto"
else:
self._proxy.ReservedMemory = "%dM" % self._toBeReservedSpin.get_value_as_int()
self._proxy.ReservedMemory = "%d" % self._toBeReservedSpin.get_value_as_int()
self._proxy.FadumpEnabled = self._fadumpButton.get_active()

# This hub have been visited, use should now be aware of the crypted devices issue
Expand Down
11 changes: 10 additions & 1 deletion com_redhat_kdump/service/kdump.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def __init__(self):

self._reserved_memory = "auto"
self.reserved_memory_changed = Signal()
self._lower, self._upper, self._step = getMemoryBounds()

def publish(self):
"""Publish the DBus objects."""
Expand Down Expand Up @@ -78,14 +79,22 @@ def fadump_enabled(self, value):
self.fadump_enabled_changed.emit()
log.debug("Fadump enabled is set to '%s'.", value)

def check_reserved_memory(self, value):
if value != "auto":
if int(value) > self._upper:
value = str(int(self._upper))
if int(value) < self._lower:
value = str(int(self._lower))
return value

@property
def reserved_memory(self):
"""Amount of memory in MB to reserve for kdump."""
return self._reserved_memory

@reserved_memory.setter
def reserved_memory(self, value):
self._reserved_memory = value
self._reserved_memory = self.check_reserved_memory(value)
self.reserved_memory_changed.emit()
log.debug("Reserved memory is set to '%s'.", value)

Expand Down
12 changes: 11 additions & 1 deletion test/unit_tests/test_interface.py
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@iasunsea iasunsea May 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
from unittest.case import TestCase
from unittest.mock import patch
from unittest.mock import Mock

from com_redhat_kdump import common
from .mock import MockBuiltinRead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MockBuiltinRead is no longer used.

from com_redhat_kdump.constants import KDUMP
from com_redhat_kdump.service.kdump import KdumpService
from com_redhat_kdump.service.kdump_interface import KdumpInterface
Expand Down Expand Up @@ -48,7 +50,15 @@ def test_fadump_enabled(self):
self._check_properties_changed("FadumpEnabled", True)
self.assertEqual(self._interface.FadumpEnabled, True)

def test_reserved_memory(self):
@patch("com_redhat_kdump.service.kdump.KdumpService.check_reserved_memory", return_value="256")
def test_reserved_memory(self, _mock_read):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self._interface.ReservedMemory = "256"
self._check_properties_changed("ReservedMemory", "256")
self.assertEqual(self._interface.ReservedMemory, "256")

@patch("com_redhat_kdump.common.getMemoryBounds", return_value=(500, 800, 1))
def test_check_reserved_memory(self, _mock_read):
self._service._lower , self._service._upper, self._service._step = common.getMemoryBounds()
self.assertEqual(self._service.check_reserved_memory("900"), "800")
self.assertEqual(self._service.check_reserved_memory("400"), "500")
self.assertEqual(self._service.check_reserved_memory("600"), "600")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

7 changes: 6 additions & 1 deletion test/unit_tests/test_kickstart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
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


Expand Down Expand Up @@ -68,7 +70,10 @@ def test_ks_disable(self):
%end
""")

def test_ks_reserve_mb(self):
@patch("com_redhat_kdump.common.getMemoryBounds", return_value=(160, 800, 1))
def test_ks_reserve_mb(self, _mock_read):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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("""

self._service._lower , self._service._upper, self._service._step = common.getMemoryBounds()
self.assertEqual((self._service._lower , self._service._upper), (160, 800))
self._check_ks_input("""
%addon com_redhat_kdump --enable --reserve-mb=256
%end
Expand Down
Loading