Merge lp:~jtv/maas/test-multi-zonegenerator into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

More testing for ZoneGenerator.

Turns out much of ZoneGenerator, a nontrivial class, was untested — along with some of its helpers. All we had was three top-level tests. Here I add some.

Also, _get_forward_zone_configs was confusing because of some forced symmetry with _get_reverse_zones. Basically the nodegroups it received as an argument were ignored; only the domain names were used. Passing it just the domain names makes that much clearer, even if it means a bit less symmetry.

Jeroen

[Gavin Panella (allenap)]

[1]

=== modified file 'src/maasserver/dns.py'
[...]

Everything in here is change for the sake of change. If you must, land
it, but I'm not sure there was any lack of clarity before, and there
certainly isn't any more now.

[2]

+class TestLazyDict(TestCase):

This is cool. Thanks for adding it; I was the one who didn't test it the
first time around. Same goes for ZoneGenerator, except for the top-level
tests.

[3]

+    def test_holds_one_value_per_key(self):

Not sure what this demonstrates. lazydict inherits from dict, so this
isn't surprising behaviour.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks. I promise that the changes in dns.py were not just change for the sake of change. They were all things that made me go "huh?" and, in the case of the parameter to _get_forward_zones, saved me a lot of testing because the problem became so much simpler.

[Julian Edwards (julian-edwards)]

On Tuesday 28 Jan 2014 10:47:24 you wrote:
> +class TestLazyDict(TestCase):
> + """Tests for `lazydict`."""

Just a small comment - it would be super useful in these test case docstrings
to describe the overall desired behaviour of the object under test and what
the test cases seek to achieve.

I have no idea* what a lazydict is and the test doesn't help me understand
that.

*Actually I do, but I'm playing Devil's Advocate :)

[Jeroen T. Vermeulen (jtv)]

Not a bad idea, though we'd have to explore the boundaries between what should go in the class docstring and what goes in the test docstring... As it is, the test docstring really only serves as a place to put the proper spelling of the thing that's being tested, without the CamelCase form that the class format imposes.

=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py	2014-01-27 11:35:09 +0000
+++ src/maasserver/dns.py	2014-01-28 10:46:14 +0000
@@ -169,29 +169,26 @@
         self.serial = serial
 
     @staticmethod
-    def _get_forward_nodegroups(nodegroups):
-        """Return a set of all forward nodegroups.
-
-        This is the set of all managed nodegroups with the same domain as the
-        domain of any of the given nodegroups.
+    def _filter_dns_managed(nodegroups):
+        """Return the subset of `nodegroups` for which we manage DNS."""
+        return set(filter(is_dns_managed, nodegroups))
+
+    @staticmethod
+    def _get_forward_nodegroups(domains):
+        """Return the set of forward nodegroups for the given `domains`.
+
+        These are all nodegroups with any of the given domains.
         """
-        forward_domains = {nodegroup.name for nodegroup in nodegroups}
-        forward_nodegroups = NodeGroup.objects.filter(name__in=forward_domains)
-        return {
-            nodegroup for nodegroup in forward_nodegroups
-            if is_dns_managed(nodegroup)
-            }
+        return ZoneGenerator._filter_dns_managed(
+            NodeGroup.objects.filter(name__in=domains))
 
     @staticmethod
     def _get_reverse_nodegroups(nodegroups):
-        """Return a set of all reverse nodegroups.
+        """Return the set of reverse nodegroups among `nodegroups`.
 
         This is the subset of the given nodegroups that are managed.
         """
-        return {
-            nodegroup for nodegroup in nodegroups
-            if is_dns_managed(nodegroup)
-            }
+        return ZoneGenerator._filter_dns_managed(nodegroups)
 
     @staticmethod
     def _get_mappings():
@@ -235,7 +232,8 @@
                     get_domain(nodegroup), serial=serial, network=network)
 
     def __iter__(self):
-        forward_nodegroups = self._get_forward_nodegroups(self.nodegroups)
+        forward_nodegroups = self._get_forward_nodegroups(
+            {nodegroup.name for nodegroup in self.nodegroups})
         reverse_nodegroups = self._get_reverse_nodegroups(self.nodegroups)
         mappings = self._get_mappings()
         networks = self._get_networks()

=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py	2014-01-27 12:42:51 +0000
+++ src/maasserver/tests/test_dns.py	2014-01-28 10:46:14 +0000
@@ -15,7 +15,6 @@
 __all__ = []
 
 
-from functools import partial
 from itertools import islice
 import socket
 
@@ -33,9 +32,11 @@
 from maasserver.models import (
     Config,
     node as node_module,
+    NodeGroupInterface,
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils import map_enum
 from maastesting.celery import CeleryFixture
 from maastesting.fakemethod import FakeMethod
 from mock import (
@@ -59,6 +60,7 @@
 from provisioningserver.testing.tests.test_bindfixture import dig_call
 from rabbitfixture.server import allocate_ports
 from testresources import FixtureResource
+from testtools import TestCase
 from testtools.matchers import (
     IsInstance,
     MatchesAll,
@@ -155,6 +157,37 @@
         self.assertEqual(nodegroups_with_expected_results, results)
 
 
+class TestLazyDict(TestCase):
+    """Tests for `lazydict`."""
+
+    def test_empty_initially(self):
+        self.assertEqual({}, dns.lazydict(Mock()))
+
+    def test_populates_on_demand(self):
+        value = factory.make_name('value')
+        value_dict = dns.lazydict(lambda key: value)
+        key = factory.make_name('key')
+        retrieved_value = value_dict[key]
+        self.assertEqual(value, retrieved_value)
+        self.assertEqual({key: value}, value_dict)
+
+    def test_remembers_elements(self):
+        value_dict = dns.lazydict(lambda key: factory.make_name('value'))
+        key = factory.make_name('key')
+        self.assertEqual(value_dict[key], value_dict[key])
+
+    def test_holds_one_value_per_key(self):
+        value_dict = dns.lazydict(lambda key: key)
+        key1 = factory.make_name('key')
+        key2 = factory.make_name('key')
+
+        value1 = value_dict[key1]
+        value2 = value_dict[key2]
+
+        self.assertEqual((key1, key2), (value1, value2))
+        self.assertEqual({key1: key1, key2: key2}, value_dict)
+
+
 class TestDNSConfigModifications(MAASServerTestCase):
 
     resources = (
@@ -434,10 +467,126 @@
 class TestZoneGenerator(MAASServerTestCase):
     """Tests for :class:x`dns.ZoneGenerator`."""
 
-    # Factory to return an accepted nodegroup with a managed interface.
-    make_node_group = partial(
-        factory.make_node_group, status=NODEGROUP_STATUS.ACCEPTED,
-        management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+    def make_node_group(self, **kwargs):
+        """Create an accepted nodegroup with a managed interface."""
+        return factory.make_node_group(
+            status=NODEGROUP_STATUS.ACCEPTED,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS, **kwargs)
+
+    def test_get_forward_nodegroups_returns_empty_for_unknown_domain(self):
+        self.assertEqual(
+            set(),
+            dns.ZoneGenerator._get_forward_nodegroups(
+                factory.make_name('domain')))
+
+    def test_get_forward_nodegroups_returns_empty_for_no_domains(self):
+        self.assertEqual(set(), dns.ZoneGenerator._get_forward_nodegroups([]))
+
+    def test_get_forward_nodegroups_returns_dns_managed_nodegroups(self):
+        domain = factory.make_name('domain')
+        nodegroup = self.make_node_group(name=domain)
+        self.assertEqual(
+            {nodegroup},
+            dns.ZoneGenerator._get_forward_nodegroups([domain]))
+
+    def test_get_forward_nodegroups_includes_multiple_domains(self):
+        nodegroups = [self.make_node_group() for _ in range(3)]
+        self.assertEqual(
+            set(nodegroups),
+            dns.ZoneGenerator._get_forward_nodegroups(
+                [nodegroup.name for nodegroup in nodegroups]))
+
+    def test_get_forward_nodegroups_ignores_non_dns_nodegroups(self):
+        domain = factory.make_name('domain')
+        managed_nodegroup = self.make_node_group(name=domain)
+        factory.make_node_group(
+            name=domain, status=NODEGROUP_STATUS.ACCEPTED,
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+        factory.make_node_group(
+            name=domain, status=NODEGROUP_STATUS.ACCEPTED,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
+        self.assertEqual(
+            {managed_nodegroup},
+            dns.ZoneGenerator._get_forward_nodegroups([domain]))
+
+    def test_get_forward_nodegroups_ignores_other_domains(self):
+        nodegroups = [self.make_node_group() for _ in range(2)]
+        self.assertEqual(
+            {nodegroups[0]},
+            dns.ZoneGenerator._get_forward_nodegroups([nodegroups[0].name]))
+
+    def test_get_forward_nodegroups_ignores_unaccepted_nodegroups(self):
+        domain = factory.make_name('domain')
+        nodegroups = {
+            status: factory.make_node_group(
+                status=status, name=domain,
+                management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+            for status in map_enum(NODEGROUP_STATUS).values()
+            }
+        self.assertEqual(
+            {nodegroups[NODEGROUP_STATUS.ACCEPTED]},
+            dns.ZoneGenerator._get_forward_nodegroups([domain]))
+
+    def test_get_reverse_nodegroups_returns_only_dns_managed_nodegroups(self):
+        nodegroups = {
+            management: factory.make_node_group(
+                status=NODEGROUP_STATUS.ACCEPTED, management=management)
+            for management in map_enum(NODEGROUPINTERFACE_MANAGEMENT).values()
+            }
+        self.assertEqual(
+            {nodegroups[NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS]},
+            dns.ZoneGenerator._get_reverse_nodegroups(nodegroups.values()))
+
+    def test_get_reverse_nodegroups_ignores_other_nodegroups(self):
+        nodegroups = [self.make_node_group() for _ in range(3)]
+        self.assertEqual(
+            {nodegroups[0]},
+            dns.ZoneGenerator._get_reverse_nodegroups(nodegroups[:1]))
+
+    def test_get_reverse_nodegroups_ignores_unaccepted_nodegroups(self):
+        nodegroups = {
+            status: factory.make_node_group(
+                status=status,
+                management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+            for status in map_enum(NODEGROUP_STATUS).values()
+            }
+        self.assertEqual(
+            {nodegroups[NODEGROUP_STATUS.ACCEPTED]},
+            dns.ZoneGenerator._get_reverse_nodegroups(nodegroups.values()))
+
+    def test_get_networks_returns_network(self):
+        nodegroup = self.make_node_group()
+        [interface] = nodegroup.get_managed_interfaces()
+        networks_dict = dns.ZoneGenerator._get_networks()
+        retrieved_interface = networks_dict[nodegroup]
+        self.assertEqual([interface.network], retrieved_interface)
+
+    def test_get_networks_returns_multiple_networks(self):
+        nodegroups = [self.make_node_group() for _ in range(3)]
+        networks_dict = dns.ZoneGenerator._get_networks()
+        for nodegroup in nodegroups:
+            [interface] = nodegroup.get_managed_interfaces()
+            self.assertEqual([interface.network], networks_dict[nodegroup])
+
+    def test_get_networks_returns_managed_networks(self):
+        nodegroups = [
+            factory.make_node_group(
+                status=NODEGROUP_STATUS.ACCEPTED, management=management)
+            for management in map_enum(NODEGROUPINTERFACE_MANAGEMENT).values()
+            ]
+        networks_dict = dns.ZoneGenerator._get_networks()
+        # Force lazydict to evaluate for all these nodegroups.
+        for nodegroup in nodegroups:
+            networks_dict[nodegroup]
+        self.assertEqual(
+            {
+                nodegroup: [
+                    interface.network
+                    for interface in nodegroup.get_managed_interfaces()
+                    ]
+                for nodegroup in nodegroups
+            },
+            networks_dict)
 
     def test_with_no_nodegroups_yields_nothing(self):
         self.assertEqual([], dns.ZoneGenerator(()).as_list())
@@ -451,6 +600,26 @@
                 (forward_zone("henry", "10/32"),
                  reverse_zone("henry", "10/32"))))
 
+    def test_two_managed_interfaces_yields_one_forward_two_reverse_zones(self):
+        # XXX JeroenVermeulen 2014-01-28 bug=1052339: This patch becomes
+        # unnecessary once we allow multiple managed interfaces per nodegroup.
+        self.patch(NodeGroupInterface, 'clean_management')
+        nodegroup = self.make_node_group()
+        factory.make_node_group_interface(
+            nodegroup=nodegroup,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+        [interface1, interface2] = nodegroup.get_managed_interfaces()
+
+        expected_zones = [
+            forward_zone(
+                nodegroup.name, interface1.network, interface2.network),
+            reverse_zone(nodegroup.name, interface1.network),
+            reverse_zone(nodegroup.name, interface2.network),
+            ]
+        self.assertThat(
+            dns.ZoneGenerator([nodegroup]).as_list(),
+            MatchesListwise(expected_zones))
+
     def test_with_many_nodegroups_yields_many_zones(self):
         # This demonstrates ZoneGenerator in all-singing all-dancing mode.
         nodegroups = [