testing: monkeypatch system_info call in unit tests (SC-533) by TheRealFalcon · Pull Request #1117 · canonical/cloud-init

TheRealFalcon · 2021-11-19T21:07:15Z

Proposed Commit Message

testing: monkeypatch system_info call in unit tests

system_info can make calls that read or write from the filesystem, which
should require special mocking. It is also decorated with 'lru_cache',
which means test authors often don't realize they need to be mocking.
Also, we don't actually want the results from the user's local
machine, so monkeypatching it across all tests should be reasonable.

Additionally, moved some of 'system_info` into a helper function to
reduce the surface area of the monkeypatch, added tests for the new
function (and fixed a bug as a result), and removed related mocks that
should be no longer needed.

Additional Context

Test Steps

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

TheRealFalcon · 2021-11-19T21:08:08Z

+            "variant": "ubuntu"
+        }
+
+    util.system_info = my_system_info


main change is here

TheRealFalcon · 2021-11-19T21:09:09Z

+        ({'system': 'linux', 'dist': ('debian',)}, 'debian'),
+        ({'system': 'linux', 'dist': ('eurolinux',)}, 'eurolinux'),
+        ({'system': 'linux', 'dist': ('fedora',)}, 'fedora'),
+        ({'system': 'linux', 'dist': ('openEuler',)}, 'openeuler'),


Discovered a bug here. We were checking for openEuler after .lower() had been called

TheRealFalcon · 2021-11-19T21:10:13Z

-    @mock.patch("cloudinit.util.get_linux_distro")
-    def test_sysconfig_available_uses_variant_mapping(self, m_distro, m_avail):
+    @mock.patch("cloudinit.util.system_info")
+    def test_sysconfig_available_uses_variant_mapping(self, m_info, m_avail):


This test was checking some of the "variant" determining behavior, so I moved the bit of code into it's own function and am testing that separately now.

Thx for the explanation here.

system_info can make calls that read or write from the filesystem, which should require special mocking. It is also decorated with 'lru_cache', which means test author's often don't realize they need to be mocking. Additionally, we don't actually want the results from the user's local machine, so monkeypatching it across all tests should be reasonable. Additionally, moved some of 'system_info` into a helper function to reduce the surface area of the monkeypatch, added tests for the new function (and fixed a bug as a result), and removed related mocks that should be no longer needed.

blackboxsw

Thanks for this @TheRealFalcon think we have a couple others too to drop as well?

diff --git a/cloudinit/analyze/tests/test_boot.py b/cloudinit/analyze/tests/test_boot.py
index 37788214..6b3afb5e 100644
--- a/cloudinit/analyze/tests/test_boot.py
+++ b/cloudinit/analyze/tests/test_boot.py
@@ -12,12 +12,8 @@ class TestDistroChecker(CiTestCase):
     def test_blank_distro(self):
         self.assertEqual(err_code, dist_check_timestamp())
 
-    @mock.patch('cloudinit.util.system_info', return_value={'dist': ('', '',
-                                                                     '')})
-    @mock.patch('cloudinit.util.get_linux_distro', return_value=('', '', ''))
     @mock.patch('cloudinit.util.is_FreeBSD', return_value=True)
-    def test_freebsd_gentoo_cant_find(self, m_sys_info,
-                                      m_get_linux_distro, m_is_FreeBSD):
+    def test_freebsd_gentoo_cant_find(self, m_is_FreeBSD):
         self.assertEqual(err_code, dist_check_timestamp())
 
     @mock.patch('cloudinit.subp.subp', return_value=(0, 1))
diff --git a/tests/unittests/test_distros/test_netconfig.py b/tests/unittests/test_distros/test_netconfig.py
index 4c65644d..e4eba179 100644
--- a/tests/unittests/test_distros/test_netconfig.py
+++ b/tests/unittests/test_distros/test_netconfig.py
@@ -241,8 +241,6 @@ class TestNetCfgDistroBase(FilesystemMockingTestCase):
     def setUp(self):
         super(TestNetCfgDistroBase, self).setUp()
         self.add_patch('cloudinit.util.system_is_snappy', 'm_snappy')
-        self.add_patch('cloudinit.util.system_info', 'm_sysinfo')
-        self.m_sysinfo.return_value = {'dist': ('Distro', '99.1', 'Codename')}
 
     def _get_distro(self, dname, renderers=None):
         cls = distros.fetch(dname)
diff --git a/tests/unittests/test_distros/test_user_data_normalize.py b/tests/unittests/test_distros/test_user_data_normalize.py
index fa48410a..50c86942 100644
--- a/tests/unittests/test_distros/test_user_data_normalize.py
+++ b/tests/unittests/test_distros/test_user_data_normalize.py
@@ -26,8 +26,6 @@ class TestUGNormalize(TestCase):
     def setUp(self):
         super(TestUGNormalize, self).setUp()
         self.add_patch('cloudinit.util.system_is_snappy', 'm_snappy')
-        self.add_patch('cloudinit.util.system_info', 'm_sysinfo')
-        self.m_sysinfo.return_value = {'dist': ('Distro', '99.1', 'Codename')}
 
     def _make_distro(self, dtype, def_user=None):
         cfg = dict(settings.CFG_BUILTIN)
diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
index b9e2ba57..46365009 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -37,8 +37,6 @@ class TestNtp(FilesystemMockingTestCase):
         self.new_root = self.tmp_dir()
         self.add_patch('cloudinit.util.system_is_snappy', 'm_snappy')
         self.m_snappy.return_value = False
-        self.add_patch('cloudinit.util.system_info', 'm_sysinfo')
-        self.m_sysinfo.return_value = {'dist': ('Distro', '99.1', 'Codename')}
         self.new_root = self.reRoot()
         self._get_cloud = par

blackboxsw · 2021-11-19T23:09:19Z

+def _get_variant(info):
    system = info['system'].lower()
-    var = 'unknown'
+    variant = 'unknown'


Yeah thanks on readability.

blackboxsw · 2021-11-22T18:12:49Z

-    @mock.patch("cloudinit.util.get_linux_distro")
-    def test_sysconfig_available_uses_variant_mapping(self, m_distro, m_avail):
+    @mock.patch("cloudinit.util.system_info")
+    def test_sysconfig_available_uses_variant_mapping(self, m_info, m_avail):


Thx for the explanation here.

that expects distro version number.

TheRealFalcon · 2021-11-22T20:10:31Z

Thanks @blackboxsw . Updated based on comments.

blackboxsw

approved pending flake8

TheRealFalcon commented Nov 19, 2021

View reviewed changes

TheRealFalcon force-pushed the system-info-testing branch from 096cd6d to e7ae6ef Compare November 19, 2021 21:36

TheRealFalcon changed the title ~~testing: monkeypatch system_info call in unit tests~~ testing: monkeypatch system_info call in unit tests (SC-533) Nov 19, 2021

blackboxsw requested changes Nov 22, 2021

View reviewed changes

This was referenced Nov 22, 2021

WIP: Fix mock in broken netconfig test #1116

Closed

Add Strict Metaschema Validation #1101

Merged

Review comments. Also updated monkeypatch for code

a972a56

that expects distro version number.

blackboxsw approved these changes Nov 22, 2021

View reviewed changes

flake8

f6c54e4

TheRealFalcon merged commit 31daf66 into canonical:main Nov 22, 2021

TheRealFalcon deleted the system-info-testing branch November 22, 2021 22:56

ubuntu-server-builder mentioned this pull request May 12, 2023

Release 22.1 #3948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing: monkeypatch system_info call in unit tests (SC-533)#1117

testing: monkeypatch system_info call in unit tests (SC-533)#1117
TheRealFalcon merged 3 commits into
canonical:mainfrom
TheRealFalcon:system-info-testing

TheRealFalcon commented Nov 19, 2021 •

edited

Loading

Uh oh!

TheRealFalcon Nov 19, 2021

Uh oh!

TheRealFalcon Nov 19, 2021

Uh oh!

TheRealFalcon Nov 19, 2021

Uh oh!

blackboxsw Nov 22, 2021

Uh oh!

blackboxsw left a comment

Uh oh!

blackboxsw Nov 19, 2021

Uh oh!

blackboxsw Nov 22, 2021

Uh oh!

TheRealFalcon commented Nov 22, 2021

Uh oh!

blackboxsw left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

TheRealFalcon commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Commit Message

Additional Context

Test Steps

Checklist:

Uh oh!

TheRealFalcon Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

blackboxsw Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

blackboxsw Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

blackboxsw Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon commented Nov 22, 2021

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TheRealFalcon commented Nov 19, 2021 •

edited

Loading