From: Sean McGinnis <sean_mcginnis@dell.com>
Date: Tue, 24 Feb 2015 15:24:52 +0000 (-0600)
Subject: Remove use of contextlib.nested
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=ddbcdbb2d02a46eb0392e00a2b2d8e246cc8e3de;p=openstack-build%2Fcinder-build.git

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

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',