launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/test_dns into lp:maas with lp:~jtv/maas/maasserver-testcase-docstrings as a prerequisite.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/maas/test_dns/+merge/121599

Why did test_dns fail all over the place when run alone, but not as part of a full test run?  The answer turned out to be that some test that ran earlier enabled DHCP management in the config, and this database change was never reverted between tests.  The DNS tests implicitly relied on the setting.

I at least mentioned this to Julian.  My solution — which I have neglected to pre-imp, being so ecstatic to have found the problem in the first place, and being temporarily without colleagues to pre-imp with — is to create a fixture that manages Config items.  The fixture lets you mess with this kind of configuration all you want during the test, and at the end restores the configuration's natural state.  To wit, none.  There should be no initial configuration: that's what default values are for.  So to keep things simple (and fast) the fixture just erases all configuration settings.

The naming was a bit awkward, since we have numerous disparate config modules.  They're all over the place, and they don't have much in common beyond the name.  We even have a ConfigFixture that's got nothing to do with the Config model.

With this branch landed, we can avoid future test-isolation problems on config items.  But we'll have to remember to use the new fixture whenever a test changes the config.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/test_dns/+merge/121599
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/test_dns into lp:maas.

=== added file 'src/maasserver/testing/configmodel.py'
--- src/maasserver/testing/configmodel.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/testing/configmodel.py	2012-08-28 12:29:24 +0000
@@ -0,0 +1,49 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Fixture to support (and undo) :class:`Config` changes in tests.
+
+Named to avoid confusion between the numerous unrelated "config" modules
+and Config classes.  This one pertains to `maasserver.models.Config`.
+"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'ConfigModelFixture',
+    ]
+
+from fixtures import Fixture
+from maasserver.models import Config
+
+
+def wipe_config():
+    """Reset all configuration.
+
+    There shouldn't be any initial configuration, apart from defaults.
+    """
+    Config.objects.all().delete()
+
+
+class ConfigModelFixture(Fixture):
+    """Allow tests to meddle with configuration; undo at end of test."""
+
+    def __init__(self, **kwargs):
+        """Create (but do not yet set up) a configuration fixture.
+
+        Pass any settings that you want to set right now as keyword arguments.
+        However it's also fine to set them later, while the fixture is in use.
+        """
+        super(ConfigModelFixture, self).__init__()
+        self.custom_settings = kwargs
+
+    def setUp(self):
+        super(ConfigModelFixture, self).setUp()
+        self.addCleanup(wipe_config)
+        for key, value in self.custom_settings.items():
+            Config.objects.set_config(key, value)

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-27 10:37:14 +0000
+++ src/maasserver/tests/test_api.py	2012-08-28 12:29:24 +0000
@@ -71,6 +71,7 @@
     reload_object,
     reload_objects,
     )
+from maasserver.testing.configmodel import ConfigModelFixture
 from maasserver.testing.factory import factory
 from maasserver.testing.oauthclient import OAuthAuthenticatedClient
 from maasserver.testing.testcase import (
@@ -2416,16 +2417,6 @@
         get_worker_user(), token=nodegroup.api_token)
 
 
-def enable_dhcp_management():
-    """Turn MAAS DHCP management on."""
-    Config.objects.set_config('manage_dhcp', True)
-
-
-def disable_dhcp_management():
-    """Turn MAAS DHCP management on, or off."""
-    Config.objects.set_config('manage_dhcp', False)
-
-
 class TestNodeGroupAPI(APITestCase):
 
     resources = (
@@ -2452,7 +2443,7 @@
         self.assertEqual(httplib.NOT_FOUND, response.status_code)
 
     def test_update_leases_processes_empty_leases_dict(self):
-        enable_dhcp_management()
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         nodegroup = factory.make_node_group()
         factory.make_dhcp_lease(nodegroup=nodegroup)
         client = make_worker_client(nodegroup)
@@ -2487,7 +2478,7 @@
                 for lease in DHCPLease.objects.filter(nodegroup=nodegroup)])
 
     def test_update_leases_stores_leases_even_if_not_managing_dhcp(self):
-        disable_dhcp_management()
+        self.useFixture(ConfigModelFixture(manage_dhcp=False))
         nodegroup = factory.make_node_group()
         lease = factory.make_random_leases()
         client = make_worker_client(nodegroup)
@@ -2505,7 +2496,7 @@
             for lease in DHCPLease.objects.filter(nodegroup=nodegroup)])
 
     def test_update_leases_adds_new_leases_on_worker(self):
-        enable_dhcp_management()
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         nodegroup = factory.make_node_group()
         client = make_worker_client(nodegroup)
         self.patch(Omshell, 'create', FakeMethod())
@@ -2525,7 +2516,7 @@
             Omshell.create.extract_args())
 
     def test_update_leases_does_not_add_old_leases(self):
-        enable_dhcp_management()
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         nodegroup = factory.make_node_group()
         client = make_worker_client(nodegroup)
         self.patch(tasks, 'add_new_dhcp_host_map', FakeMethod())

=== modified file 'src/maasserver/tests/test_commands_config_master_dhcp.py'
--- src/maasserver/tests/test_commands_config_master_dhcp.py	2012-08-24 10:28:16 +0000
+++ src/maasserver/tests/test_commands_config_master_dhcp.py	2012-08-28 12:29:24 +0000
@@ -15,14 +15,12 @@
 from optparse import OptionValueError
 
 from django.core.management import call_command
-from mock import Mock
 from maasserver.management.commands.config_master_dhcp import name_option
-from maasserver.models import (
-    Config,
-    NodeGroup,
-    )
+from maasserver.models import NodeGroup
+from maasserver.testing.configmodel import ConfigModelFixture
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
+from mock import Mock
 from provisioningserver import tasks
 from testtools.matchers import MatchesStructure
 
@@ -136,7 +134,7 @@
         master = NodeGroup.objects.ensure_master()
         self.patch(NodeGroup, 'set_up_dhcp', Mock())
         settings = make_dhcp_settings()
-        Config.objects.set_config('manage_dhcp', True)
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         call_command('config_master_dhcp', **settings)
         self.assertEqual(1, master.set_up_dhcp.call_count)
 
@@ -144,6 +142,6 @@
         master = NodeGroup.objects.ensure_master()
         self.patch(NodeGroup, 'set_up_dhcp', Mock())
         settings = make_dhcp_settings()
-        Config.objects.set_config('manage_dhcp', False)
+        self.useFixture(ConfigModelFixture(manage_dhcp=False))
         call_command('config_master_dhcp', **settings)
         self.assertEqual(0, master.set_up_dhcp.call_count)

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2012-08-21 10:33:21 +0000
+++ src/maasserver/tests/test_dhcp.py	2012-08-28 12:29:24 +0000
@@ -13,7 +13,7 @@
 __all__ = []
 
 from maasserver.dhcp import is_dhcp_management_enabled
-from maasserver.models import Config
+from maasserver.testing.configmodel import ConfigModelFixture
 from maastesting.testcase import TestCase
 
 
@@ -23,5 +23,5 @@
         self.assertFalse(is_dhcp_management_enabled())
 
     def test_is_dhcp_management_enabled_follows_manage_dhcp_config(self):
-        Config.objects.set_config('manage_dhcp', True)
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         self.assertTrue(is_dhcp_management_enabled())

=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py	2012-08-10 13:10:53 +0000
+++ src/maasserver/tests/test_dns.py	2012-08-28 12:29:24 +0000
@@ -29,6 +29,7 @@
     DHCPLease,
     post_updates,
     )
+from maasserver.testing.configmodel import ConfigModelFixture
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maastesting.bindfixture import BINDServer
@@ -93,6 +94,7 @@
         self.assertRaises(dns.DNSException, dns.get_dns_server_address)
 
     def test_get_zone_creates_DNSZoneConfig(self):
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         nodegroup = factory.make_node_group()
         serial = random.randint(1, 100)
         zone = dns.get_zone(nodegroup, serial)
@@ -123,6 +125,7 @@
 
     def setUp(self):
         super(TestDNSConfigModifications, self).setUp()
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         self.bind = self.useFixture(BINDServer())
         self.patch(conf, 'DNS_CONFIG_DIR', self.bind.config.homedir)
 

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-08-28 12:29:23 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-08-28 12:29:24 +0000
@@ -12,11 +12,9 @@
 __metaclass__ = type
 __all__ = []
 
-from maasserver.models import (
-    Config,
-    NodeGroup,
-    )
+from maasserver.models import NodeGroup
 from maasserver.testing import reload_object
+from maasserver.testing.configmodel import ConfigModelFixture
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maasserver.worker_user import get_worker_user
@@ -163,15 +161,15 @@
         )
 
     def test_is_dhcp_enabled_returns_True_if_fully_set_up(self):
-        Config.objects.set_config('manage_dhcp', True)
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         self.assertTrue(factory.make_node_group().is_dhcp_enabled())
 
     def test_is_dhcp_enabled_returns_False_if_disabled(self):
-        Config.objects.set_config('manage_dhcp', False)
+        self.useFixture(ConfigModelFixture(manage_dhcp=False))
         self.assertFalse(factory.make_node_group().is_dhcp_enabled())
 
     def test_is_dhcp_enabled_returns_False_if_config_is_missing(self):
-        Config.objects.set_config('manage_dhcp', True)
+        self.useFixture(ConfigModelFixture(manage_dhcp=True))
         required_fields = [
             'subnet_mask', 'broadcast_ip', 'ip_range_low', 'ip_range_high']
         # Map each required field's name to a nodegroup that has just

=== modified file 'src/maasserver/tests/test_views_settings.py'
--- src/maasserver/tests/test_views_settings.py	2012-08-21 10:47:56 +0000
+++ src/maasserver/tests/test_views_settings.py	2012-08-28 12:29:24 +0000
@@ -27,12 +27,19 @@
     get_prefixed_form_data,
     reload_object,
     )
+from maasserver.testing.configmodel import ConfigModelFixture
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import AdminLoggedInTestCase
 
 
 class SettingsTest(AdminLoggedInTestCase):
 
+    def setUp(self):
+        super(SettingsTest, self).setUp()
+        # Allow tests to mess with configuration without affecting other
+        # tests.
+        self.useFixture(ConfigModelFixture())
+
     def test_settings_list_users(self):
         # The settings page displays a list of the users with links to view,
         # delete or edit each user. Note that the link to delete the the