From 9235ada8c2c250603dc5b299cc08bb7a982d4fc6 Mon Sep 17 00:00:00 2001 From: Johannes Erdfelt Date: Thu, 29 May 2014 04:56:22 -0700 Subject: [PATCH] Don't set CONF options directly in unit tests The values won't get changed back after the unit test is finished causing changes to leak out to other tests. The flags() method handles changing the option temporarily and resetting it after the test is done. Change-Id: I3173076e0f4ffbbc51f52a2bd37b98f88db3ddc1 --- nova/hacking/checks.py | 18 ++++++++++++++++++ nova/tests/compute/test_compute.py | 12 ++++++------ nova/tests/network/test_linux_net.py | 2 +- nova/tests/test_hacking.py | 21 +++++++++++++++++++++ nova/tests/test_utils.py | 2 +- nova/tests/test_wsgi.py | 2 +- nova/tests/virt/baremetal/test_pxe.py | 9 +++------ nova/tests/virt/libvirt/test_libvirt.py | 2 +- 8 files changed, 52 insertions(+), 16 deletions(-) diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index ac0c43ee9c..c35b18a76b 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -49,6 +49,7 @@ asse_equal_end_with_none_re = re.compile( r"(.)*assertEqual\((\w|\.|\'|\"|\[|\])+, None\)") asse_equal_start_with_none_re = re.compile( r"(.)*assertEqual\(None, (\w|\.|\'|\"|\[|\])+\)") +conf_attribute_set_re = re.compile(r"CONF\.[a-z0-9_.]+\s*=\s*\w") def import_no_db_in_virt(logical_line, filename): @@ -243,6 +244,22 @@ def no_translate_debug_logs(logical_line, filename): yield(0, "N319 Don't translate debug level logs") +def no_setting_conf_directly_in_tests(logical_line, filename): + """Check for setting CONF.* attributes directly in tests + + The value can leak out of tests affecting how subsequent tests run. + Using self.flags(option=value) is the preferred method to temporarily + set config options in tests. + + N320 + """ + if 'nova/tests/' in filename: + res = conf_attribute_set_re.match(logical_line) + if res: + yield (0, "N320: Setting CONF.* attributes directly in tests is " + "forbidden. Use self.flags(option=value) instead") + + def factory(register): register(import_no_db_in_virt) register(no_db_session_in_public_api) @@ -256,3 +273,4 @@ def factory(register): register(assert_equal_type) register(assert_equal_none) register(no_translate_debug_logs) + register(no_setting_conf_directly_in_tests) diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index 59e00be347..0b70e17b29 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -604,7 +604,7 @@ class ComputeVolumeTestCase(BaseTestCase): # None of the mocks should be called. self.mox.ReplayAll() - CONF.bandwidth_poll_interval = 0 + self.flags(bandwidth_poll_interval=0) self.compute._poll_bandwidth_usage(ctxt) self.mox.UnsetStubs() @@ -627,7 +627,7 @@ class ComputeVolumeTestCase(BaseTestCase): NotImplementedError) self.mox.ReplayAll() - CONF.bandwidth_poll_interval = 1 + self.flags(bandwidth_poll_interval=1) self.compute._poll_bandwidth_usage(ctxt) # A second call won't call the stubs again as the bandwidth # poll is now disabled @@ -662,7 +662,7 @@ class ComputeVolumeTestCase(BaseTestCase): # None of the mocks should be called. self.mox.ReplayAll() - CONF.volume_usage_poll_interval = 0 + self.flags(volume_usage_poll_interval=0) self.compute._poll_volume_usage(ctxt) self.mox.UnsetStubs() @@ -676,7 +676,7 @@ class ComputeVolumeTestCase(BaseTestCase): self.compute._get_host_volume_bdms(ctxt).AndReturn([]) self.mox.ReplayAll() - CONF.volume_usage_poll_interval = 10 + self.flags(volume_usage_poll_interval=10) self.compute._poll_volume_usage(ctxt) self.mox.UnsetStubs() @@ -692,7 +692,7 @@ class ComputeVolumeTestCase(BaseTestCase): self.compute._get_host_volume_bdms(ctxt).AndReturn([1, 2]) self.compute._update_volume_usage_cache(ctxt, [3, 4]) self.mox.ReplayAll() - CONF.volume_usage_poll_interval = 10 + self.flags(volume_usage_poll_interval=10) self.compute._poll_volume_usage(ctxt) self.mox.UnsetStubs() @@ -742,7 +742,7 @@ class ComputeVolumeTestCase(BaseTestCase): # Poll volume usage & then detach the volume. This will update the # total fields in the volume usage cache. - CONF.volume_usage_poll_interval = 10 + self.flags(volume_usage_poll_interval=10) self.compute._poll_volume_usage(self.context) # Check that a volume.usage and volume.attach notification was sent self.assertEqual(2, len(fake_notifier.NOTIFICATIONS)) diff --git a/nova/tests/network/test_linux_net.py b/nova/tests/network/test_linux_net.py index 016a9f75ab..380c09c8f4 100644 --- a/nova/tests/network/test_linux_net.py +++ b/nova/tests/network/test_linux_net.py @@ -593,7 +593,7 @@ class LinuxNetworkTestCase(test.NoDBTestCase): default_domain = CONF.dhcp_domain for domain in ('', default_domain): executes = [] - CONF.dhcp_domain = domain + self.flags(dhcp_domain=domain) linux_net.restart_dhcp(self.context, dev, network_ref) expected = ['env', 'CONFIG_FILE=%s' % jsonutils.dumps(CONF.dhcpbridge_flagfile), diff --git a/nova/tests/test_hacking.py b/nova/tests/test_hacking.py index ed7fbbfc6d..d4b689d8ab 100644 --- a/nova/tests/test_hacking.py +++ b/nova/tests/test_hacking.py @@ -123,3 +123,24 @@ class HackingTestCase(test.NoDBTestCase): self.assertEqual(len(list(checks.no_translate_debug_logs( "LOG.info(_('foo'))", "nova/scheduler/foo.py"))), 0) + + def test_no_setting_conf_directly_in_tests(self): + self.assertEqual(len(list(checks.no_setting_conf_directly_in_tests( + "CONF.option = 1", "nova/tests/test_foo.py"))), 1) + + self.assertEqual(len(list(checks.no_setting_conf_directly_in_tests( + "CONF.group.option = 1", "nova/tests/test_foo.py"))), 1) + + self.assertEqual(len(list(checks.no_setting_conf_directly_in_tests( + "CONF.option = foo = 1", "nova/tests/test_foo.py"))), 1) + + # Shouldn't fail with comparisons + self.assertEqual(len(list(checks.no_setting_conf_directly_in_tests( + "CONF.option == 'foo'", "nova/tests/test_foo.py"))), 0) + + self.assertEqual(len(list(checks.no_setting_conf_directly_in_tests( + "CONF.option != 1", "nova/tests/test_foo.py"))), 0) + + # Shouldn't fail since not in nova/tests/ + self.assertEqual(len(list(checks.no_setting_conf_directly_in_tests( + "CONF.option = 1", "nova/compute/foo.py"))), 0) diff --git a/nova/tests/test_utils.py b/nova/tests/test_utils.py index 9efed945f1..6969240cac 100644 --- a/nova/tests/test_utils.py +++ b/nova/tests/test_utils.py @@ -955,7 +955,7 @@ class GetImageFromSystemMetadataTestCase(test.NoDBTestCase): sys_meta = self.get_system_metadata() sys_meta["%soo1" % utils.SM_IMAGE_PROP_PREFIX] = "bar" - CONF.non_inheritable_image_properties = ["foo1"] + self.flags(non_inheritable_image_properties=["foo1"]) image = utils.get_image_from_system_metadata(sys_meta) diff --git a/nova/tests/test_wsgi.py b/nova/tests/test_wsgi.py index f6707be51f..c91c3ee05a 100644 --- a/nova/tests/test_wsgi.py +++ b/nova/tests/test_wsgi.py @@ -104,7 +104,7 @@ class TestWSGIServer(test.NoDBTestCase): self.assertEqual("test_app", server.name) def test_custom_max_header_line(self): - CONF.max_header_line = 4096 # Default value is 16384. + self.flags(max_header_line=4096) # Default value is 16384. nova.wsgi.Server("test_custom_max_header_line", None) self.assertEqual(CONF.max_header_line, eventlet.wsgi.MAX_HEADER_LINE) diff --git a/nova/tests/virt/baremetal/test_pxe.py b/nova/tests/virt/baremetal/test_pxe.py index 185d51e2b8..297bb4d01c 100644 --- a/nova/tests/virt/baremetal/test_pxe.py +++ b/nova/tests/virt/baremetal/test_pxe.py @@ -285,15 +285,12 @@ class PXEClassMethodsTestCase(BareMetalPXETestCase): flavor = utils.get_test_flavor() # Raises an exception when options are neither specified # on the instance nor in configuration file - CONF.baremetal.deploy_kernel = None - CONF.baremetal.deploy_ramdisk = None self.assertRaises(exception.NovaException, pxe.get_tftp_image_info, self.instance, flavor) # Test that other non-true values also raise an exception - CONF.baremetal.deploy_kernel = "" - CONF.baremetal.deploy_ramdisk = "" + self.flags(deploy_kernel='', deploy_ramdisk='', group='baremetal') self.assertRaises(exception.NovaException, pxe.get_tftp_image_info, self.instance, flavor) @@ -313,8 +310,8 @@ class PXEClassMethodsTestCase(BareMetalPXETestCase): # Here, we confirm both that all four values were set # and that the proper paths are getting set for all of them - CONF.baremetal.deploy_kernel = 'cccc' - CONF.baremetal.deploy_ramdisk = 'dddd' + self.flags(deploy_kernel='cccc', deploy_ramdisk='dddd', + group='baremetal') base = os.path.join(CONF.baremetal.tftp_root, self.instance['uuid']) res = pxe.get_tftp_image_info(self.instance, flavor) expected = { diff --git a/nova/tests/virt/libvirt/test_libvirt.py b/nova/tests/virt/libvirt/test_libvirt.py index 40b1a93539..468387ef38 100644 --- a/nova/tests/virt/libvirt/test_libvirt.py +++ b/nova/tests/virt/libvirt/test_libvirt.py @@ -9069,7 +9069,7 @@ class LibvirtVolumeSnapshotTestCase(test.TestCase): self.conn._volume_api, self.conn) def test_volume_snapshot_create(self, quiesce=True): - CONF.instance_name_template = 'instance-%s' + self.flags(instance_name_template='instance-%s') self.mox.StubOutWithMock(self.conn, '_lookup_by_name') self.mox.StubOutWithMock(self.conn, '_volume_api')