Merge lp:~jtv/maas/test-dhcp-config-with-proper-params into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Make DHCP configuration tests at various levels randomise their DHCP configuration variables.

These changes were salvaged from another branch which I had to abandon because the approach didn't pan out. Some of them will probably smooth the way for IPv6 tests; others are just Better™.

Jeroen

[Graham Binns (gmb)]

Looks good. You've got what looks like some code duplication in there, but nothing too heinous.

review: Approve

=== added directory 'src/provisioningserver/dhcp/testing'
=== added file 'src/provisioningserver/dhcp/testing/__init__.py'
=== added file 'src/provisioningserver/dhcp/testing/config.py'
--- src/provisioningserver/dhcp/testing/config.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/dhcp/testing/config.py	2014-08-22 10:23:17 +0000
@@ -0,0 +1,40 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test helpers related to DHCP configuration."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'make_subnet_config',
+    ]
+
+from maastesting.factory import factory
+from netaddr import IPAddress
+
+
+def make_subnet_config(network=None):
+    """Return complete DHCP configuration dict for a subnet."""
+    if network is None:
+        network = factory.getRandomNetwork()
+    ip_low, ip_high = factory.make_ip_range(network)
+    return {
+        'interface': factory.make_name('eth', sep=''),
+        'subnet': unicode(IPAddress(network.first)),
+        'subnet_mask': unicode(network.netmask),
+        'subnet_cidr': unicode(network.cidr),
+        'broadcast_ip': unicode(network.broadcast),
+        'dns_servers': unicode(factory.pick_ip_in_network(network)),
+        'ntp_server': unicode(factory.pick_ip_in_network(network)),
+        'domain_name': '%s.example.com' % factory.make_name('domain'),
+        'router_ip': unicode(factory.pick_ip_in_network(network)),
+        'ip_range_low': unicode(ip_low),
+        'ip_range_high': unicode(ip_high),
+        }

=== modified file 'src/provisioningserver/dhcp/tests/test_config.py'
--- src/provisioningserver/dhcp/tests/test_config.py	2014-08-22 02:42:35 +0000
+++ src/provisioningserver/dhcp/tests/test_config.py	2014-08-22 10:23:17 +0000
@@ -24,6 +24,7 @@
 from maastesting.factory import factory
 from provisioningserver.boot import BootMethodRegistry
 from provisioningserver.dhcp import config
+from provisioningserver.dhcp.testing.config import make_subnet_config
 from provisioningserver.testing.testcase import PservTestCase
 import tempita
 from testtools.matchers import (
@@ -50,26 +51,15 @@
 
 
 def make_sample_params():
-    """Produce a dict of sample parameters.
-
-    The sample provides all parameters used by the DHCP template.
-    """
-    return dict(
-        omapi_key="random",
-        dhcp_subnets=[dict(
-            subnet="10.0.0.0",
-            interface="eth0",
-            subnet_mask="255.0.0.0",
-            subnet_cidr="10.0.0.0/8",
-            broadcast_ip="10.255.255.255",
-            dns_servers="10.1.0.1 10.1.0.2",
-            ntp_server="8.8.8.8",
-            domain_name="example.com",
-            router_ip="10.0.0.2",
-            ip_range_low="10.0.0.3",
-            ip_range_high="10.0.0.254",
-            )]
-        )
+    """Return a dict of arbitrary DHCP configuration parameters."""
+    if factory.pick_bool():
+        network = factory.getRandomNetwork()
+    else:
+        network = factory.make_ipv6_network()
+    return {
+        'omapi_key': factory.make_name('key'),
+        'dhcp_subnets': [make_subnet_config(network)],
+        }
 
 
 class TestGetConfig(PservTestCase):

=== modified file 'src/provisioningserver/dhcp/tests/test_writer.py'
--- src/provisioningserver/dhcp/tests/test_writer.py	2014-03-28 04:36:31 +0000
+++ src/provisioningserver/dhcp/tests/test_writer.py	2014-08-22 10:23:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `provisioningserver.dhcp.writer`."""
@@ -24,8 +24,12 @@
 import sys
 
 from maastesting import root
+from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
+from mock import Mock
 from provisioningserver.dhcp import writer
+from provisioningserver.dhcp.testing.config import make_subnet_config
+from provisioningserver.utils.fs import read_text_file
 from testtools.matchers import (
     ContainsAll,
     MatchesStructure,
@@ -35,37 +39,78 @@
 class TestScript(MAASTestCase):
     """Test the DHCP configuration writer."""
 
-    test_args = (
-        '--subnet', 'subnet',
-        '--interface', 'eth0',
-        '--subnet-mask', 'subnet-mask',
-        '--broadcast-ip', 'broadcast-ip',
-        '--dns-servers', 'dns-servers',
-        '--ntp-server', 'ntp-server',
-        '--domain-name', 'domain-name',
-        '--router-ip', 'router-ip',
-        '--ip-range-low', 'ip-range-low',
-        '--ip-range-high', 'ip-range-high',
-        '--omapi-key', 'omapi-key',
-        )
+    def make_args(self, network=None):
+        """Create a fake parameter for `run`, based on `network`."""
+        settings = make_subnet_config(network)
+        args = Mock()
+        args.outfile = None
+        args.omapi_key = factory.make_name('key')
+        args.subnet = settings['subnet']
+        args.interface = settings['interface']
+        args.subnet_mask = settings['subnet_mask']
+        args.broadcast_ip = settings['broadcast_ip']
+        args.dns_servers = settings['dns_servers']
+        args.ntp_server = settings['ntp_server']
+        args.domain_name = settings['domain_name']
+        args.router_ip = settings['router_ip']
+        args.ip_range_low = settings['ip_range_low']
+        args.ip_range_high = settings['ip_range_high']
+        return args
 
     def test_script_executable(self):
-        script = ["%s/bin/maas-provision" % root, "generate-dhcp-config"]
-        script.extend(self.test_args)
+        args = self.make_args()
+        script = [
+            "%s/bin/maas-provision" % root,
+            "generate-dhcp-config",
+            '--subnet', args.subnet,
+            '--interface', args.interface,
+            '--subnet-mask', args.subnet_mask,
+            '--broadcast-ip', args.broadcast_ip,
+            '--dns-servers', args.dns_servers,
+            '--ntp-server', args.ntp_server,
+            '--domain-name', args.domain_name,
+            '--router-ip', args.router_ip,
+            '--ip-range-low', args.ip_range_low,
+            '--ip-range-high', args.ip_range_high,
+            '--omapi-key', args.omapi_key,
+            ]
+
         cmd = Popen(
             script, stdout=PIPE, env=dict(PYTHONPATH=":".join(sys.path)))
         output, err = cmd.communicate()
+
         self.assertEqual(0, cmd.returncode, err)
-        contains_all_params = ContainsAll(
-            ['subnet', 'subnet-mask', 'broadcast-ip',
-             'omapi-key', 'dns-servers', 'ntp-server', 'domain-name',
-             'router-ip', 'ip-range-low', 'ip-range-high'])
-        self.assertThat(output, contains_all_params)
+
+        self.assertThat(output, ContainsAll([
+            args.subnet,
+            args.subnet_mask,
+            args.broadcast_ip,
+            args.omapi_key,
+            args.dns_servers,
+            args.ntp_server,
+            args.domain_name,
+            args.router_ip,
+            args.ip_range_low,
+            args.ip_range_high,
+            ]))
 
     def test_arg_setup(self):
+        test_args = (
+            '--subnet', 'subnet',
+            '--interface', 'eth0',
+            '--subnet-mask', 'subnet-mask',
+            '--broadcast-ip', 'broadcast-ip',
+            '--dns-servers', 'dns-servers',
+            '--ntp-server', 'ntp-server',
+            '--domain-name', 'domain-name',
+            '--router-ip', 'router-ip',
+            '--ip-range-low', 'ip-range-low',
+            '--ip-range-high', 'ip-range-high',
+            '--omapi-key', 'omapi-key',
+            )
         parser = ArgumentParser()
         writer.add_arguments(parser)
-        args = parser.parse_args(self.test_args)
+        args = parser.parse_args(test_args)
         self.assertThat(
             args, MatchesStructure.byEquality(
                 subnet='subnet',
@@ -82,46 +127,44 @@
 
     def test_run(self):
         self.patch(sys, "stdout", BytesIO())
-        parser = ArgumentParser()
-        writer.add_arguments(parser)
-        args = parser.parse_args(self.test_args)
+        args = self.make_args(factory.getRandomNetwork())
+
         writer.run(args)
+
         output = sys.stdout.getvalue()
         contains_all_params = ContainsAll([
-            'subnet',
-            'interface',
-            'subnet-mask',
-            'broadcast-ip',
-            'omapi-key',
-            'dns-servers',
-            'ntp-server',
-            'domain-name',
-            'router-ip',
-            'ip-range-low',
-            'ip-range-high',
+            args.subnet,
+            args.interface,
+            args.subnet_mask,
+            args.broadcast_ip,
+            args.omapi_key,
+            args.dns_servers,
+            args.ntp_server,
+            args.domain_name,
+            args.router_ip,
+            args.ip_range_low,
+            args.ip_range_high,
             ])
         self.assertThat(output, contains_all_params)
 
     def test_run_save_to_file(self):
-        parser = ArgumentParser()
-        writer.add_arguments(parser)
-        outfile = os.path.join(self.make_dir(), "outfile.txt")
-        args = parser.parse_args(
-            self.test_args + ("--outfile", outfile))
+        args = self.make_args()
+        args.outfile = os.path.join(self.make_dir(), "outfile.txt")
+
         writer.run(args)
-        with open(outfile, "rb") as stream:
-            output = stream.read()
-        contains_all_params = ContainsAll([
-            'subnet',
-            'interface',
-            'subnet-mask',
-            'broadcast-ip',
-            'omapi-key',
-            'dns-servers',
-            'ntp-server',
-            'domain-name',
-            'router-ip',
-            'ip-range-low',
-            'ip-range-high',
-            ])
-        self.assertThat(output, contains_all_params)
+
+        self.assertThat(
+            read_text_file(args.outfile),
+            ContainsAll([
+                args.subnet,
+                args.interface,
+                args.subnet_mask,
+                args.broadcast_ip,
+                args.omapi_key,
+                args.dns_servers,
+                args.ntp_server,
+                args.domain_name,
+                args.router_ip,
+                args.ip_range_low,
+                args.ip_range_high,
+                ]))

=== modified file 'src/provisioningserver/dhcp/writer.py'
--- src/provisioningserver/dhcp/writer.py	2014-08-21 13:27:59 +0000
+++ src/provisioningserver/dhcp/writer.py	2014-08-22 10:23:17 +0000
@@ -74,7 +74,7 @@
 
 def run(args):
     """Generate a DHCP server configuration, and write it to stdout."""
-    params = vars(args)
+    params = vars(args).copy()
     omapi_key = params.pop('omapi_key')
     outfile = params.pop('outfile')
     kwargs = {

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2014-08-21 13:27:59 +0000
+++ src/provisioningserver/tests/test_tasks.py	2014-08-22 10:23:17 +0000
@@ -54,6 +54,7 @@
     config,
     leases,
     )
+from provisioningserver.dhcp.testing.config import make_subnet_config
 from provisioningserver.dns.config import (
     celery_conf,
     MAAS_NAMED_CONF_NAME,
@@ -189,23 +190,10 @@
 
     def make_dhcp_config_params(self):
         """Fake up a dict of dhcp configuration parameters."""
-        param_names = [
-            'interface',
-            'subnet',
-            'subnet_mask',
-            'broadcast_ip',
-            'dns_servers',
-            'domain_name',
-            'router_ip',
-            'ip_range_low',
-            'ip_range_high',
-        ]
         return {
-            'dhcp_subnets': [
-                {param: factory.make_string() for param in param_names}
-            ],
-            'omapi_key': factory.make_string(),
-        }
+            'omapi_key': factory.make_name('key'),
+            'dhcp_subnets': [make_subnet_config()],
+            }
 
     def test_upload_dhcp_leases(self):
         self.patch(