From ca8f4be2a84d37319183cf9511d1791068a2e2fd Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Thu, 23 May 2019 14:17:31 +0100 Subject: [PATCH] Move selective patching of open() to nova.test for reuse Several existing tests patch open() to fake the contents of a file outside the test virtualenv, whilst avoiding interfering with reading and writing of other files inside the test virtualenv. Currently they do it by duplicating logic. Furthermore, in the near future, more tests (specifically, some SEV tests) will want to do the same selective patching, and similarly will need to avoid impacting reads of other files within the test virtualenv, e.g. placement-policy.yaml. So create new patch_open() context manager / decorator in nova.test for selectively patching open based on the path parameter, and reuse this for existing tests. Also add unit tests for all these cases. mock >= 3.0.0 is required because configparser.RawConfigParser._read() uses enumerate() to iterate over the lines of the (mocked) /etc/nova/release config file, and this uses __iter__() under the hood which was not supported via mock_open until a bug was fixed and backported to the external mock library for 3.0.0: https://bugs.python.org/issue21258 https://bugs.python.org/issue32933 https://github.com/testing-cabal/mock/commit/73f6eed0d6867299fa2543b88a07cd8f12198361 Change-Id: I19f49c923d2c41eab0c7b4cab28c50498dc07046 --- lower-constraints.txt | 2 +- nova/test.py | 61 +++++++++++++++++++++ nova/tests/unit/pci/test_utils.py | 3 +- nova/tests/unit/test_test.py | 57 +++++++++++++++++++ nova/tests/unit/test_versions.py | 19 ++----- nova/tests/unit/virt/libvirt/test_driver.py | 19 +------ test-requirements.txt | 2 +- 7 files changed, 128 insertions(+), 35 deletions(-) diff --git a/lower-constraints.txt b/lower-constraints.txt index 885dd92e0e75..aa1d8ac52dab 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -53,7 +53,7 @@ Mako==1.0.7 MarkupSafe==1.0 mccabe==0.2.1 microversion-parse==0.2.1 -mock==2.0.0 +mock==3.0.0 monotonic==1.4 mox3==0.20.0 msgpack==0.5.6 diff --git a/nova/test.py b/nova/test.py index 0bca282d3b79..048d0a2e0c27 100644 --- a/nova/test.py +++ b/nova/test.py @@ -48,6 +48,7 @@ from oslo_versionedobjects import fixture as ovo_fixture from oslotest import mock_fixture from oslotest import moxstubout import six +from six.moves import builtins import testtools from nova.compute import resource_tracker @@ -394,6 +395,17 @@ class TestCase(testtools.TestCase): """ return patch_exists(patched_path, result) + @staticmethod + def patch_open(patched_path, read_data): + """Provide a static method version of patch_open() which is easier to + use as a context manager within a test method via: + + def test_something(self): + with self.patch_open(path, "fake contents of file"): + ... + """ + return patch_open(patched_path, read_data) + def flags(self, **kw): """Override flag variables for a test.""" group = kw.pop('group', None) @@ -823,3 +835,52 @@ def patch_exists(patched_path, result): with mock.patch.object(os.path, "exists") as mock_exists: mock_exists.side_effect = fake_exists yield mock_exists + + +@contextlib.contextmanager +def patch_open(patched_path, read_data): + """Selectively patch open() so that if it's called with patched_path, + return a mock which makes it look like the file contains + read_data. Calls with any other path are passed through to the + real open() function. + + Either import and use as a decorator, or use the + nova.TestCase.patch_open() static method as a context manager. + + Currently it is *not* recommended to use this if any of the + following apply: + + - The code under test will attempt to write to patched_path. + + - You want to patch via decorator *and* make assertions about how the + mock is called (since using it in the decorator form will not make + the mock available to your code). + + - You want the faked file contents to be determined + programmatically (e.g. by matching substrings of patched_path). + + - You expect open() to be called multiple times on the same path + and return different file contents each time. + + Additionally within unit tests which only test a very limited code + path, it may be possible to ensure that the code path only invokes + open() once, in which case it's slightly overkill to do + selective patching based on the path. In this case something like + like this may be more appropriate: + + @mock.patch(six.moves.builtins, 'open') + def test_my_code(self, mock_open): + ... + mock_open.assert_called_once_with(path) + """ + real_open = builtins.open + m = mock.mock_open(read_data=read_data) + + def selective_fake_open(path, *args, **kwargs): + if path == patched_path: + return m(patched_path) + return real_open(path, *args, **kwargs) + + with mock.patch.object(builtins, 'open') as mock_open: + mock_open.side_effect = selective_fake_open + yield m diff --git a/nova/tests/unit/pci/test_utils.py b/nova/tests/unit/pci/test_utils.py index 6675be5c2471..c1105bd226e8 100644 --- a/nova/tests/unit/pci/test_utils.py +++ b/nova/tests/unit/pci/test_utils.py @@ -108,8 +108,7 @@ class GetFunctionByIfnameTestCase(test.NoDBTestCase): totalvf_path = "/sys/class/net/%s/device/%s" % (ifname, utils._SRIOV_TOTALVFS) mock_readlink.return_value = '../../../0000:00:00.1' - with mock.patch.object( - builtins, 'open', mock.mock_open(read_data='4')) as mock_open: + with self.patch_open(totalvf_path, '4') as mock_open: address, physical_function = utils.get_function_by_ifname('eth0') self.assertEqual(address, '0000:00:00.1') self.assertTrue(physical_function) diff --git a/nova/tests/unit/test_test.py b/nova/tests/unit/test_test.py index 633158b5ae61..1ef3b311e905 100644 --- a/nova/tests/unit/test_test.py +++ b/nova/tests/unit/test_test.py @@ -17,6 +17,8 @@ """Tests for the testing base code.""" import os.path +import tempfile +import uuid import mock from oslo_log import log as logging @@ -374,3 +376,58 @@ class PatchExistsTestCase(test.NoDBTestCase): # Check non-patched parameters self.assertTrue(os.path.exists(os.path.dirname(__file__))) self.assertFalse(os.path.exists('non-existent/file')) + + +class PatchOpenTestCase(test.NoDBTestCase): + fake_contents = "These file contents don't really exist" + + def _test_patched_open(self): + """Test that a selectively patched open can fake the contents of a + file while still allowing normal, real file operations. + """ + self.assertFalse(os.path.exists('fake_file')) + + with open('fake_file') as f: + self.assertEqual(self.fake_contents, f.read()) + + # Test we can still open and read this file from within the + # same context. NOTE: We have to make sure we open the .py + # file not the corresponding .pyc file. + with open(__file__.rstrip('c')) as f: + this_file_contents = f.read() + self.assertIn("class %s(" % self.__class__.__name__, + this_file_contents) + self.assertNotIn("magic concatenated" "string", + this_file_contents) + + # Test we can still create, write to, and then read from a + # temporary file, from within the same context. + tmp = tempfile.NamedTemporaryFile() + tmp_contents = str(uuid.uuid1()) + with open(tmp.name, 'w') as f: + f.write(tmp_contents) + with open(tmp.name) as f: + self.assertEqual(tmp_contents, f.read()) + + return tmp.name + + def test_with_patch_open(self): + """Test that "with patch_open" can fake the contents of a file + without changing other file operations, and that calls can + be asserted on the mocked method. + """ + with self.patch_open('fake_file', self.fake_contents) as mock_open: + tmp_name = self._test_patched_open() + + # Test we can make assertions about how the mock_open was called. + self.assertIn(mock.call('fake_file'), mock_open.mock_calls) + # The mock_open should get bypassed for non-patched path values: + self.assertNotIn(mock.call(__file__), mock_open.mock_calls) + self.assertNotIn(mock.call(tmp_name), mock_open.mock_calls) + + @test.patch_open('fake_file', fake_contents) + def test_patch_open_decorator(self): + """Test that @patch_open can fake the contents of a file + without changing other file operations. + """ + self._test_patched_open() diff --git a/nova/tests/unit/test_versions.py b/nova/tests/unit/test_versions.py index 9a2ecfda0b54..86522de2566f 100644 --- a/nova/tests/unit/test_versions.py +++ b/nova/tests/unit/test_versions.py @@ -13,8 +13,6 @@ # under the License. from oslo_config import cfg -import six -from six.moves import builtins from nova import test from nova import version @@ -32,9 +30,13 @@ class VersionTestCase(test.NoDBTestCase): self.assertEqual("5.5.5.5-g9ec3421", version.version_string_with_package()) + @test.patch_open("/etc/nova/release", read_data= + "[Nova]\n" + "vendor = ACME Corporation\n" + "product = ACME Nova\n" + "package = 1337\n") def test_release_file(self): version.loaded = False - real_open = builtins.open real_find_file = cfg.CONF.find_file def fake_find_file(self, name): @@ -42,17 +44,6 @@ class VersionTestCase(test.NoDBTestCase): return "/etc/nova/release" return real_find_file(self, name) - def fake_open(path, *args, **kwargs): - if path == "/etc/nova/release": - data = """[Nova] -vendor = ACME Corporation -product = ACME Nova -package = 1337""" - return six.StringIO(data) - - return real_open(path, *args, **kwargs) - - self.stub_out('six.moves.builtins.open', fake_open) self.stub_out('oslo_config.cfg.ConfigOpts.find_file', fake_find_file) self.assertEqual(version.vendor_string(), "ACME Corporation") diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index b43ec4221f05..27c1f6983736 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -55,7 +55,6 @@ from oslo_utils import units from oslo_utils import uuidutils from oslo_utils import versionutils import six -from six.moves import builtins from six.moves import range from nova.api.metadata import base as instance_metadata @@ -18273,24 +18272,10 @@ class TestGuestConfigSysinfoSerialOS(test.NoDBTestCase): self._test_get_guest_config_sysinfo_serial(self.theuuid) @test.patch_exists("/etc/machine-id", True) + @test.patch_open("/etc/machine-id", theuuid) def test_get_guest_config_sysinfo_serial_auto_os(self): self.flags(sysinfo_serial="auto", group="libvirt") - - real_open = builtins.open - with mock.patch.object(builtins, "open") as mock_open: - theuuid = "56b40135-a973-4eb3-87bb-a2382a3e6dbc" - - def fake_open(filename, *args, **kwargs): - if filename == "/etc/machine-id": - h = mock.MagicMock() - h.read.return_value = theuuid - h.__enter__.return_value = h - return h - return real_open(filename, *args, **kwargs) - - mock_open.side_effect = fake_open - - self._test_get_guest_config_sysinfo_serial(theuuid) + self._test_get_guest_config_sysinfo_serial(self.theuuid) def test_get_guest_config_sysinfo_serial_unique(self): self.flags(sysinfo_serial="unique", group="libvirt") diff --git a/test-requirements.txt b/test-requirements.txt index 318863b7ccff..60f8fb3acd5e 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -7,7 +7,7 @@ contextlib2>=0.5.5;python_version<'3.0' # PSF License coverage!=4.4,>=4.0 # Apache-2.0 ddt>=1.0.1 # MIT fixtures>=3.0.0 # Apache-2.0/BSD -mock>=2.0.0 # BSD +mock>=3.0.0 # BSD mox3>=0.20.0 # Apache-2.0 psycopg2>=2.7 # LGPL/ZPL PyMySQL>=0.7.6 # MIT License