From ddbcdbb2d02a46eb0392e00a2b2d8e246cc8e3de Mon Sep 17 00:00:00 2001 From: Sean McGinnis Date: Tue, 24 Feb 2015 09:24:52 -0600 Subject: [PATCH] Remove use of contextlib.nested The contextlib.nested call has been deprecated in Python 2.7. This causes DeprecationWarning messages in the unit tests. There are also known issues with contextlib.nested that were addressed by the native support for multiple "with" variables. For instance, if the first object is created but the second one throws an exception, the first object's __exit__ is never called. Since Cinder no longer supports 2.6 we can remove the use of these contextlib.nested calls. Added hacking check to catch if any new instances are added to the codebase. Note: line continuation markers (e.g. '\') had to be used or syntax errors were thrown. While using parentheses is the preferred way for multiple line statements it is not a requirement. Partial-Bug: 1428424 Change-Id: I7bb7d201d31ff239be3402fb64e5f202ede019b0 --- HACKING.rst | 1 + cinder/hacking/checks.py | 10 +++ cinder/tests/compute/test_nova.py | 9 +-- cinder/tests/test_backup_ceph.py | 28 ++++--- cinder/tests/test_cmd.py | 37 ++++----- cinder/tests/test_hacking.py | 6 ++ cinder/tests/test_quobyte.py | 121 +++++++++++++++--------------- cinder/tests/test_smbfs.py | 18 ++--- cinder/tests/test_volume.py | 9 +-- 9 files changed, 118 insertions(+), 121 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index bca0ccff7..750d93047 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -14,6 +14,7 @@ Cinder Specific Commandments - [N324] Enforce no use of LOG.audit messages. LOG.info should be used instead. - [N327] assert_called_once is not a valid Mock method. - [N333] Ensure that oslo namespaces are used for namespaced libraries. +- [N339] Prevent use of deprecated contextlib.nested. General diff --git a/cinder/hacking/checks.py b/cinder/hacking/checks.py index cb27afe0b..972748658 100644 --- a/cinder/hacking/checks.py +++ b/cinder/hacking/checks.py @@ -138,6 +138,15 @@ def check_oslo_namespace_imports(logical_line): yield(0, msg) +def check_no_contextlib_nested(logical_line): + msg = ("N339: contextlib.nested is deprecated. With Python 2.7 and later " + "the with-statement supports multiple nested objects. See https://" + "docs.python.org/2/library/contextlib.html#contextlib.nested " + "for more information.") + if "with contextlib.nested" in logical_line: + yield(0, msg) + + def factory(register): register(no_vi_headers) register(no_translate_debug_logs) @@ -146,3 +155,4 @@ def factory(register): register(check_no_log_audit) register(check_assert_called_once) register(check_oslo_namespace_imports) + register(check_no_contextlib_nested) diff --git a/cinder/tests/compute/test_nova.py b/cinder/tests/compute/test_nova.py index f655ee5a2..8bc50e9d4 100644 --- a/cinder/tests/compute/test_nova.py +++ b/cinder/tests/compute/test_nova.py @@ -12,8 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -import contextlib - import mock from cinder.compute import nova @@ -102,11 +100,10 @@ class NovaApiTestCase(test.TestCase): self.ctx = context.get_admin_context() def test_update_server_volume(self): - with contextlib.nested( - mock.patch.object(nova, 'novaclient'), + with mock.patch.object(nova, 'novaclient') as mock_novaclient, \ mock.patch.object(self.novaclient.volumes, - 'update_server_volume') - ) as (mock_novaclient, mock_update_server_volume): + 'update_server_volume') as \ + mock_update_server_volume: mock_novaclient.return_value = self.novaclient self.api.update_server_volume(self.ctx, 'server_id', diff --git a/cinder/tests/test_backup_ceph.py b/cinder/tests/test_backup_ceph.py index d2d0785b7..f65f3a487 100644 --- a/cinder/tests/test_backup_ceph.py +++ b/cinder/tests/test_backup_ceph.py @@ -14,7 +14,6 @@ # under the License. """ Tests for Ceph backup service.""" -import contextlib import hashlib import os import tempfile @@ -481,10 +480,9 @@ class BackupCephTestCase(test.TestCase): self.mock_rbd.RBD.list = mock.Mock() self.mock_rbd.RBD.list.return_value = [backup_name] - with contextlib.nested( - mock.patch.object(self.service, 'get_backup_snaps'), - mock.patch.object(self.service, '_rbd_diff_transfer') - ) as (_unused, mock_rbd_diff_transfer): + with mock.patch.object(self.service, 'get_backup_snaps'), \ + mock.patch.object(self.service, '_rbd_diff_transfer') as \ + mock_rbd_diff_transfer: def mock_rbd_diff_transfer_side_effect(src_name, src_pool, dest_name, dest_pool, src_user, src_conf, @@ -495,10 +493,11 @@ class BackupCephTestCase(test.TestCase): # Raise a pseudo exception.BackupRBDOperationFailed. mock_rbd_diff_transfer.side_effect \ = mock_rbd_diff_transfer_side_effect - with contextlib.nested( - mock.patch.object(self.service, '_full_backup'), - mock.patch.object(self.service, '_try_delete_base_image') - ) as (_unused, mock_try_delete_base_image): + + with mock.patch.object(self.service, '_full_backup'), \ + mock.patch.object(self.service, + '_try_delete_base_image') as \ + mock_try_delete_base_image: def mock_try_delete_base_image_side_effect(backup_id, volume_id, base_name): @@ -556,12 +555,11 @@ class BackupCephTestCase(test.TestCase): self.mock_rbd.RBD.list = mock.Mock() self.mock_rbd.RBD.list.return_value = [backup_name] - with contextlib.nested( - mock.patch.object(self.service, 'get_backup_snaps'), - mock.patch.object(self.service, '_rbd_diff_transfer'), - mock.patch.object(self.service, '_full_backup'), - mock.patch.object(self.service, '_backup_metadata') - ) as (_unused1, _u2, _u3, mock_backup_metadata): + with mock.patch.object(self.service, 'get_backup_snaps'), \ + mock.patch.object(self.service, '_rbd_diff_transfer'), \ + mock.patch.object(self.service, '_full_backup'), \ + mock.patch.object(self.service, '_backup_metadata') as \ + mock_backup_metadata: def mock_backup_metadata_side_effect(backup): raise exception.BackupOperationError(_('mock')) diff --git a/cinder/tests/test_cmd.py b/cinder/tests/test_cmd.py index afe44ab7a..5c18be973 100644 --- a/cinder/tests/test_cmd.py +++ b/cinder/tests/test_cmd.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import contextlib import datetime import StringIO import sys @@ -706,16 +705,14 @@ class TestCinderRtstoolCmd(test.TestCase): mock.sentinel.iser_enabled) def _test_create_rtslib_error_network_portal(self, ip): - with contextlib.nested( - mock.patch('rtslib.NetworkPortal'), - mock.patch('rtslib.LUN'), - mock.patch('rtslib.TPG'), - mock.patch('rtslib.FabricModule'), - mock.patch('rtslib.Target'), - mock.patch('rtslib.BlockStorageObject'), - mock.patch('rtslib.root.RTSRoot') - ) as (network_portal, lun, tpg, fabric_module, target, - block_storage_object, rts_root): + with mock.patch('rtslib.NetworkPortal') as network_portal, \ + mock.patch('rtslib.LUN') as lun, \ + mock.patch('rtslib.TPG') as tpg, \ + mock.patch('rtslib.FabricModule') as fabric_module, \ + mock.patch('rtslib.Target') as target, \ + mock.patch('rtslib.BlockStorageObject') as \ + block_storage_object, \ + mock.patch('rtslib.root.RTSRoot') as rts_root: root_new = mock.MagicMock(storage_objects=mock.MagicMock()) rts_root.return_value = root_new block_storage_object.return_value = mock.sentinel.so_new @@ -766,16 +763,14 @@ class TestCinderRtstoolCmd(test.TestCase): self._test_create_rtslib_error_network_portal('::0') def _test_create(self, ip): - with contextlib.nested( - mock.patch('rtslib.NetworkPortal'), - mock.patch('rtslib.LUN'), - mock.patch('rtslib.TPG'), - mock.patch('rtslib.FabricModule'), - mock.patch('rtslib.Target'), - mock.patch('rtslib.BlockStorageObject'), - mock.patch('rtslib.root.RTSRoot') - ) as (network_portal, lun, tpg, fabric_module, target, - block_storage_object, rts_root): + with mock.patch('rtslib.NetworkPortal') as network_portal, \ + mock.patch('rtslib.LUN') as lun, \ + mock.patch('rtslib.TPG') as tpg, \ + mock.patch('rtslib.FabricModule') as fabric_module, \ + mock.patch('rtslib.Target') as target, \ + mock.patch('rtslib.BlockStorageObject') as \ + block_storage_object, \ + mock.patch('rtslib.root.RTSRoot') as rts_root: root_new = mock.MagicMock(storage_objects=mock.MagicMock()) rts_root.return_value = root_new block_storage_object.return_value = mock.sentinel.so_new diff --git a/cinder/tests/test_hacking.py b/cinder/tests/test_hacking.py index 8457b0720..365152b31 100644 --- a/cinder/tests/test_hacking.py +++ b/cinder/tests/test_hacking.py @@ -163,3 +163,9 @@ class HackingTestCase(test.TestCase): "from oslo.log import foo")))) self.assertEqual(0, len(list(checks.check_oslo_namespace_imports( "from oslo_log import bar")))) + + def test_no_contextlib_nested(self): + self.assertEqual(1, len(list(checks.check_no_contextlib_nested( + "with contextlib.nested(")))) + self.assertEqual(0, len(list(checks.check_no_contextlib_nested( + "with foo as bar")))) diff --git a/cinder/tests/test_quobyte.py b/cinder/tests/test_quobyte.py index 90b73f7e8..d18646697 100644 --- a/cinder/tests/test_quobyte.py +++ b/cinder/tests/test_quobyte.py @@ -15,7 +15,6 @@ # under the License. """Unit tests for the Quobyte driver module.""" -import contextlib import errno import os import StringIO @@ -126,11 +125,9 @@ class QuobyteDriverTestCase(test.TestCase): drv.local_path(volume)) def test_mount_quobyte_should_mount_correctly(self): - with contextlib.nested( - mock.patch.object(self._driver, '_execute'), + with mock.patch.object(self._driver, '_execute') as mock_execute, \ mock.patch('cinder.volume.drivers.quobyte.QuobyteDriver' - '.read_proc_mount'), - ) as (mock_execute, mock_open): + '.read_proc_mount') as mock_open: # Content of /proc/mount (not mounted yet). mock_open.return_value = StringIO.StringIO( "/dev/sda5 / ext4 rw,relatime,data=ordered 0 0") @@ -152,11 +149,9 @@ class QuobyteDriverTestCase(test.TestCase): [mkdir_call, mount_call, getfattr_call], any_order=False) def test_mount_quobyte_already_mounted_detected_seen_in_proc_mount(self): - with contextlib.nested( - mock.patch.object(self._driver, '_execute'), + with mock.patch.object(self._driver, '_execute') as mock_execute, \ mock.patch('cinder.volume.drivers.quobyte.QuobyteDriver' - '.read_proc_mount'), - ) as (mock_execute, mock_open): + '.read_proc_mount') as mock_open: # Content of /proc/mount (already mounted). mock_open.return_value = StringIO.StringIO( "quobyte@%s %s fuse rw,nosuid,nodev,noatime,user_id=1000" @@ -179,12 +174,10 @@ class QuobyteDriverTestCase(test.TestCase): Because _mount_quobyte gets called with ensure=True, the error will be suppressed and logged instead. """ - with contextlib.nested( - mock.patch.object(self._driver, '_execute'), + with mock.patch.object(self._driver, '_execute') as mock_execute, \ mock.patch('cinder.volume.drivers.quobyte.QuobyteDriver' - '.read_proc_mount'), - mock.patch('cinder.volume.drivers.quobyte.LOG') - ) as (mock_execute, mock_open, mock_LOG): + '.read_proc_mount') as mock_open, \ + mock.patch('cinder.volume.drivers.quobyte.LOG') as mock_LOG: # Content of /proc/mount (empty). mock_open.return_value = StringIO.StringIO() mock_execute.side_effect = [None, putils.ProcessExecutionError( @@ -209,11 +202,9 @@ class QuobyteDriverTestCase(test.TestCase): test_mount_quobyte_should_suppress_and_log_already_mounted_error but with ensure=False. """ - with contextlib.nested( - mock.patch.object(self._driver, '_execute'), + with mock.patch.object(self._driver, '_execute') as mock_execute, \ mock.patch('cinder.volume.drivers.quobyte.QuobyteDriver' - '.read_proc_mount') - ) as (mock_execute, mock_open): + '.read_proc_mount') as mock_open: mock_open.return_value = StringIO.StringIO() mock_execute.side_effect = [ None, # mkdir @@ -312,10 +303,10 @@ class QuobyteDriverTestCase(test.TestCase): def test_ensure_share_mounted(self): """_ensure_share_mounted simple use case.""" - with contextlib.nested( - mock.patch.object(self._driver, '_get_mount_point_for_share'), - mock.patch.object(self._driver, '_mount_quobyte') - ) as (mock_get_mount_point, mock_mount): + with mock.patch.object(self._driver, '_get_mount_point_for_share') as \ + mock_get_mount_point, \ + mock.patch.object(self._driver, '_mount_quobyte') as \ + mock_mount: drv = self._driver drv._ensure_share_mounted(self.TEST_QUOBYTE_VOLUME) @@ -578,16 +569,19 @@ class QuobyteDriverTestCase(test.TestCase): volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume_filename) info_file = volume_path + '.info' - with contextlib.nested( - mock.patch.object(self._driver, '_ensure_share_mounted'), - mock.patch.object(self._driver, '_local_volume_dir'), - mock.patch.object(self._driver, 'get_active_image_from_info'), - mock.patch.object(self._driver, '_execute'), - mock.patch.object(self._driver, '_local_path_volume'), - mock.patch.object(self._driver, '_local_path_volume_info') - ) as (mock_ensure_share_mounted, mock_local_volume_dir, - mock_active_image_from_info, mock_execute, - mock_local_path_volume, mock_local_path_volume_info): + with mock.patch.object(self._driver, '_ensure_share_mounted') as \ + mock_ensure_share_mounted, \ + mock.patch.object(self._driver, '_local_volume_dir') as \ + mock_local_volume_dir, \ + mock.patch.object(self._driver, + 'get_active_image_from_info') as \ + mock_active_image_from_info, \ + mock.patch.object(self._driver, '_execute') as \ + mock_execute, \ + mock.patch.object(self._driver, '_local_path_volume') as \ + mock_local_path_volume, \ + mock.patch.object(self._driver, '_local_path_volume_info') as \ + mock_local_path_volume_info: mock_local_volume_dir.return_value = self.TEST_MNT_POINT mock_active_image_from_info.return_value = volume_filename mock_local_path_volume.return_value = volume_path @@ -812,15 +806,16 @@ class QuobyteDriverTestCase(test.TestCase): volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume['name']) image_meta = {'id': '10958016-e196-42e3-9e7f-5d8927ae3099'} - with contextlib.nested( - mock.patch.object(drv, 'get_active_image_from_info'), - mock.patch.object(drv, '_local_volume_dir'), - mock.patch.object(image_utils, 'qemu_img_info'), - mock.patch.object(image_utils, 'upload_volume'), - mock.patch.object(image_utils, 'create_temporary_file') - ) as (mock_get_active_image_from_info, mock_local_volume_dir, - mock_qemu_img_info, mock_upload_volume, - mock_create_temporary_file): + with mock.patch.object(drv, 'get_active_image_from_info') as \ + mock_get_active_image_from_info, \ + mock.patch.object(drv, '_local_volume_dir') as \ + mock_local_volume_dir, \ + mock.patch.object(image_utils, 'qemu_img_info') as \ + mock_qemu_img_info, \ + mock.patch.object(image_utils, 'upload_volume') as \ + mock_upload_volume, \ + mock.patch.object(image_utils, 'create_temporary_file') as \ + mock_create_temporary_file: mock_get_active_image_from_info.return_value = volume['name'] mock_local_volume_dir.return_value = self.TEST_MNT_POINT @@ -854,16 +849,18 @@ class QuobyteDriverTestCase(test.TestCase): volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume['name']) image_meta = {'id': '10958016-e196-42e3-9e7f-5d8927ae3099'} - with contextlib.nested( - mock.patch.object(drv, 'get_active_image_from_info'), - mock.patch.object(drv, '_local_volume_dir'), - mock.patch.object(image_utils, 'qemu_img_info'), - mock.patch.object(image_utils, 'convert_image'), - mock.patch.object(image_utils, 'upload_volume'), - mock.patch.object(image_utils, 'create_temporary_file') - ) as (mock_get_active_image_from_info, mock_local_volume_dir, - mock_qemu_img_info, mock_convert_image, mock_upload_volume, - mock_create_temporary_file): + with mock.patch.object(drv, 'get_active_image_from_info') as \ + mock_get_active_image_from_info, \ + mock.patch.object(drv, '_local_volume_dir') as \ + mock_local_volume_dir, \ + mock.patch.object(image_utils, 'qemu_img_info') as \ + mock_qemu_img_info, \ + mock.patch.object(image_utils, 'convert_image') as \ + mock_convert_image, \ + mock.patch.object(image_utils, 'upload_volume') as \ + mock_upload_volume, \ + mock.patch.object(image_utils, 'create_temporary_file') as \ + mock_create_temporary_file: mock_get_active_image_from_info.return_value = volume['name'] mock_local_volume_dir.return_value = self.TEST_MNT_POINT @@ -900,16 +897,18 @@ class QuobyteDriverTestCase(test.TestCase): volume_filename = 'volume-%s' % self.VOLUME_UUID image_meta = {'id': '10958016-e196-42e3-9e7f-5d8927ae3099'} - with contextlib.nested( - mock.patch.object(drv, 'get_active_image_from_info'), - mock.patch.object(drv, '_local_volume_dir'), - mock.patch.object(image_utils, 'qemu_img_info'), - mock.patch.object(image_utils, 'convert_image'), - mock.patch.object(image_utils, 'upload_volume'), - mock.patch.object(image_utils, 'create_temporary_file') - ) as (mock_get_active_image_from_info, mock_local_volume_dir, - mock_qemu_img_info, mock_convert_image, mock_upload_volume, - mock_create_temporary_file): + with mock.patch.object(drv, 'get_active_image_from_info') as \ + mock_get_active_image_from_info, \ + mock.patch.object(drv, '_local_volume_dir') as \ + mock_local_volume_dir, \ + mock.patch.object(image_utils, 'qemu_img_info') as \ + mock_qemu_img_info, \ + mock.patch.object(image_utils, 'convert_image') as \ + mock_convert_image, \ + mock.patch.object(image_utils, 'upload_volume') as \ + mock_upload_volume, \ + mock.patch.object(image_utils, 'create_temporary_file') as \ + mock_create_temporary_file: mock_get_active_image_from_info.return_value = volume['name'] mock_local_volume_dir.return_value = self.TEST_MNT_POINT diff --git a/cinder/tests/test_smbfs.py b/cinder/tests/test_smbfs.py index aa1dd69fe..b7faf812a 100644 --- a/cinder/tests/test_smbfs.py +++ b/cinder/tests/test_smbfs.py @@ -12,7 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -import contextlib import copy import os @@ -321,10 +320,9 @@ class SmbFsTestCase(test.TestCase): return_value=mock.Mock(file_format=image_format)) drv._delete = mock.Mock() - with contextlib.nested( - mock.patch.object(image_utils, 'resize_image'), - mock.patch.object(image_utils, 'convert_image')) as ( - fake_resize, fake_convert): + with mock.patch.object(image_utils, 'resize_image') as fake_resize, \ + mock.patch.object(image_utils, 'convert_image') as \ + fake_convert: if extend_failed: self.assertRaises(exception.ExtendVolumeError, drv._extend_volume, @@ -473,13 +471,9 @@ class SmbFsTestCase(test.TestCase): mock.sentinel.block_size) exc = None - with contextlib.nested( - mock.patch.object(image_utils, - 'fetch_to_volume_format'), - mock.patch.object(image_utils, - 'qemu_img_info')) as ( - fake_fetch, - fake_qemu_img_info): + with mock.patch.object(image_utils, 'fetch_to_volume_format') as \ + fake_fetch, mock.patch.object(image_utils, 'qemu_img_info') as \ + fake_qemu_img_info: if wrong_size_after_fetch: exc = exception.ImageUnacceptable diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index cf66d7fbf..9e304f86c 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -18,7 +18,6 @@ Tests for Volume Code. """ -import contextlib import datetime import os import shutil @@ -1618,12 +1617,10 @@ class VolumeTestCase(BaseVolumeTestCase): 'key2': 'value2'} } - with contextlib.nested( - mock.patch.object(cinder.volume.volume_types, - 'get_volume_type_qos_specs'), + with mock.patch.object(cinder.volume.volume_types, + 'get_volume_type_qos_specs') as type_qos, \ mock.patch.object(cinder.tests.fake_driver.FakeISCSIDriver, - 'initialize_connection') - ) as (type_qos, driver_init): + 'initialize_connection') as driver_init: type_qos.return_value = dict(qos_specs=qos_values) driver_init.return_value = {'data': {}} qos_specs_expected = {'key1': 'value1', -- 2.45.2