Merge lp:~jtv/maas-test/extract-fixtures into lp:maas-test

[Jeroen T. Vermeulen]

Move proxy/kvm fixtures from test-case class to main().

This is needed in order to be able to perform a second check for unexpected DHCP servers inside the virtual machine. As long as the virtual machine is a fixture in the test case, we can only perform that check inside the test case, where a failure gets misinterpreted as a test failure. When I tried that with a DHCP server running on my network, deliberately causing it to fail, the code even tried to report the fact to Launchpad!

The proxy comes along with the KVMFixture, because the latter needs to know the former. Now, only the MAAS fixture is left inside the test case. That makes a bit more sense, I suppose — a failure to set up the MAAS might be worth reporting. But the purpose of this branch is not to clean up in general, but to pave the way for the extra DHCP check. It will be very easy with these changes in place.

The import style in main.py started getting in the way of unit-testing: it gets awkward to patch() items that are only imported inside a function. But as far as I can see, by far the greatest benefit of late imports was to maintain responsiveness when the user calls maas-test with --help, or makes some basic mistake in usage. I resolved that by creating a new, dedicated little module for the entry point. That's where most of the late imports now happen; once you're past command-line parsing it's OK to do a bit more work.

Jeroen

[Graham Binns (gmb)]

You're a brilliant — if slightly disturbing — man and this is good stuff.

It does have conflicts, but then so does great drama. Fix those and all is happiness and ponies.

review: Approve

[lp:~jtv/maas-test/extract-fixtures]

121. By Jeroen T. Vermeulen on 2014-01-10

Merge trunk, resolve conflict.

[Gavin Panella (allenap)]

A few small things that I noticed in passing. The first and last ought
to be fixed, but you were just moving the code so they can just go on
the bug list.

[1]

+        else:
+            # The default case: start polipo and use that as our caching
+            # proxy.
+            proxy_fixture = LocalProxyFixture()
+            proxy_url = proxy_fixture.get_url()
+
+        if proxy_url != '':
+            os.putenv('http_proxy', proxy_url)
+            os.putenv('https_proxy', proxy_url)
+    except:
+        if proxy_fixture is not None:
+            # We have a proxy fixture set up, but we're not going to return.
+            # That means nobody else is responsible for cleaning it up.
+            proxy_fixture.cleanUp()

LocalProxyFixture.setUp() is not called here, so there won't be a
running fixture or much chance of an exception. I assume it's called
elsewhere?

[2]

Regarding [1], it seems strange to put the try...except around the whole
condition. How about:

    if args.http_proxy is not None:
        ...
    else:
        # The default case: start polipo and use that as our caching
        # proxy.
        proxy_fixture = LocalProxyFixture()
        try:
            proxy_fixture.setUp()
        except:
            proxy_fixture.cleanUp()
            raise
        else:
            proxy_url = proxy_fixture.get_url()

    if proxy_url != '':
        os.putenv('http_proxy', proxy_url)
        os.putenv('https_proxy', proxy_url)

?

Otoh, if LocalProxyFixture.setUp() is called elsewhere then there's no
need for the try...except at all.

[3]

+        elif args.disable_cache:
+            # The user has passed --disable-cache, so don't start polipo
+            # and don't try to use an external proxy.
+            proxy_url = ''
+            logging.info("Caching disabled.")
...
+        if proxy_url != '':
+            os.putenv('http_proxy', proxy_url)
+            os.putenv('https_proxy', proxy_url)

If http_proxy is set in the calling environment, but --disable-cache
passed to maas-test, this will leave the calling environment's
http_proxy setting unaltered, which is slightly suprising I think.

How about:

        if proxy_url == '':
            os.unsetenv('http_proxy')
            os.unsetenv('https_proxy')
        else:
            os.putenv('http_proxy', proxy_url)
            os.putenv('https_proxy', proxy_url)

?

review: Needs Fixing

=== modified file 'maastest/cases.py'
--- maastest/cases.py	2014-01-10 08:43:44 +0000
+++ maastest/cases.py	2014-01-10 15:28:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test whether or not a node is compatible with MAAS."""
@@ -16,16 +16,12 @@
 
 import httplib
 import json
-import logging
-import os
 import sys
 
 from fixtures import Fixture
 from maastest import utils
-from maastest.kvmfixture import KVMFixture
 from maastest.maas_enums import NODE_STATUS
 from maastest.maasfixture import MAASFixture
-from maastest.proxyfixture import LocalProxyFixture
 import testresources
 import testtools
 from testtools.testcase import gather_details
@@ -54,8 +50,16 @@
     """Base class for testing machine compatibility with MAAS."""
 
     @classmethod
-    def configure(cls, args):
+    def configure(cls, args, machine, proxy_url):
+        """Set test parameters.
+
+        These are class variables, but they are set only on ad-hoc subclasses,
+        not on `TestMAAS` itself.  So this class stays in its original state.
+        """
+        assert cls != TestMAAS, "TestMAAS.configure called on TestMAAS."
         cls.args = args
+        cls.machine = machine
+        cls.proxy_url = proxy_url
 
     fixtures = ClassFixture()
 
@@ -63,39 +67,8 @@
     def setUpClass(cls):
         super(TestMAAS, cls).setUpClass()
 
-        # TODO: series and architecture should be script parameters.
-        architecture = utils.determine_vm_architecture()
-
-        if cls.args.http_proxy is not None:
-            # The user has passed an external proxy; don't start polipo,
-            # just use that proxy for everything.
-            proxy_url = cls.args.http_proxy
-            logging.info("Using external proxy %s." % proxy_url)
-            os.environ['http_proxy'] = cls.args.http_proxy
-            os.environ['https_proxy'] = cls.args.https_proxy
-        elif cls.args.disable_cache:
-            # The user has passed --disable-cache, so don't start polipo
-            # and don't try to use an external proxy.
-            proxy_url = ''
-            logging.info("Caching disabled.")
-        else:
-            # The default case: start polipo and use that as our caching
-            # proxy.
-            proxy = LocalProxyFixture()
-            cls.fixtures.useFixture(proxy)
-            proxy_url = proxy.get_url()
-            os.environ['http_proxy'] = proxy_url
-            os.environ['https_proxy'] = proxy_url
-
-        cls.machine = KVMFixture(
-            series=cls.args.maas_series, architecture=architecture,
-            proxy_url=proxy_url, direct_interface=cls.args.interface,
-            archives=cls.args.archive)
-        cls.fixtures.addCleanup(delattr, cls, "machine")
-        cls.fixtures.useFixture(cls.machine)
-
         cls.maas = MAASFixture(
-            cls.machine, proxy_url=proxy_url, series=cls.args.series,
+            cls.machine, proxy_url=cls.proxy_url, series=cls.args.series,
             architecture=cls.args.architecture)
         cls.fixtures.addCleanup(delattr, cls, "maas")
         cls.fixtures.useFixture(cls.maas)

=== modified file 'maastest/main.py'
--- maastest/main.py	2014-01-10 13:42:34 +0000
+++ maastest/main.py	2014-01-10 15:28:21 +0000
@@ -14,10 +14,12 @@
     "main",
     ]
 
+import logging
+import os
 import sys
 
-# Most imports are done as needed in main() to improve responsiveness
-# when maas-test is run interactively, especially when --help is passed.
+from maastest import utils
+from maastest.proxyfixture import LocalProxyFixture
 
 
 class RETURN_CODES:
@@ -52,8 +54,6 @@
 
     :raise ProgramFailure: if the program is not running as root.
     """
-    # Importing locally for responsiveness.
-    import os
     if os.geteuid() != 0:
         raise ProgramFailure(
             RETURN_CODES.NOT_ROOT, "This test must be run as root.  Use sudo.")
@@ -65,8 +65,6 @@
     :raise ProgramFailure: if the CPU lacks virtualisation support, or support
         has been disabled in firmware.
     """
-    # Importing locally for responsiveness.
-    from maastest import utils
     if not utils.check_kvm_ok():
         raise ProgramFailure(
             RETURN_CODES.NO_KVM_EXTENSIONS,
@@ -79,9 +77,6 @@
     For acceptable performance, maas-test should be run on a physical system.
     This function warns, but does not fail, if the system is virtual.
     """
-    # Importing locally for responsiveness.
-    import logging
-    from maastest import utils
     virt_type = utils.virtualization_type()
     if virt_type is not None:
         logging.info(
@@ -111,13 +106,68 @@
             % (interface, ', '.join(sorted(dhcp_servers))))
 
 
-def main(argv=None):
-    from maastest.parser import prepare_parser
-    args = prepare_parser().parse_args(argv)
-
+def set_up_proxy(args):
+    """Obtain or set up an http/https proxy, as appropriate.
+
+    The command-line arguments may disable this, or make it use an existing
+    proxy, or require us to set up our own.  This function will do whichever
+    fits, and set up the process environment as needed.
+
+    :return: A tuple of (proxy URL, proxy fixture).  The fixture may be `None`.
+        If it is not `None`, it will need to be cleaned up later.  The proxy
+        URL may be the empty string if no proxy is to be used.
+    """
+    proxy_fixture = None
+    try:
+        if args.http_proxy is not None:
+            # The user has passed an external proxy; don't start polipo,
+            # just use that proxy for everything.
+            proxy_url = args.http_proxy
+            logging.info("Using external proxy %s." % proxy_url)
+        elif args.disable_cache:
+            # The user has passed --disable-cache, so don't start polipo
+            # and don't try to use an external proxy.
+            proxy_url = ''
+            logging.info("Caching disabled.")
+        else:
+            # The default case: start polipo and use that as our caching
+            # proxy.
+            proxy_fixture = LocalProxyFixture()
+            proxy_url = proxy_fixture.get_url()
+
+        if proxy_url != '':
+            os.putenv('http_proxy', proxy_url)
+            os.putenv('https_proxy', proxy_url)
+    except:
+        if proxy_fixture is not None:
+            # We have a proxy fixture set up, but we're not going to return.
+            # That means nobody else is responsible for cleaning it up.
+            proxy_fixture.cleanUp()
+        raise
+
+    return proxy_url, proxy_fixture
+
+
+def create_vm(args, proxy_url=None):
+    """Create the virtual machine.
+
+    :param args: Arguments object as returned by the arguments parser.
+    :param proxy_url: Optional http/https proxy URL for the VM to use, as
+        returned by `set_up_proxy`.
+    :return: Virtual-machine fixture.  Clean it up after use.
+    """
     # Importing locally for responsiveness.
-    from maastest import utils
-    import logging
+    from maastest.kvmfixture import KVMFixture
+
+    # TODO: series and architecture should be script parameters.
+    architecture = utils.determine_vm_architecture()
+    return KVMFixture(
+        series=args.maas_series, architecture=architecture,
+        proxy_url=proxy_url, direct_interface=args.interface,
+        archives=args.archive)
+
+
+def main(args):
     # We tie everything onto one output stream so that we can capture
     # things for reporting.
     output_stream = utils.CachingOutputStream(sys.stdout)
@@ -125,53 +175,52 @@
         level=logging.DEBUG, format='%(asctime)s %(levelname)s %(message)s',
         stream=output_stream)
 
+    proxy_fixture = None
+    machine_fixture = None
     try:
         check_for_root()
         check_for_virtualisation_support()
         check_against_virtual_host()
         check_against_dhcp_servers(args.interface)
+
+        proxy_url, proxy_fixture = set_up_proxy(args)
+        machine_fixture = create_vm(args, proxy_url)
+
+        from maastest.cases import TestOneNode
+
+        class ConfiguredTestMAAS(TestOneNode):
+            """A configured version of TestInteractiveOneNode.
+
+            ConfiguredTestMAAS is a TestMAAS use to configure it by calling
+            cls.configure.  We need to configure the class itself and not
+            the instance because the KVMFixture TestMAAS instanciates and needs
+            to configure is created at the class level.
+            """
+        ConfiguredTestMAAS.configure(
+            args, machine=machine_fixture, proxy_url=proxy_url)
+
+        from maastest.console import run_console
+
+        result = run_console(ConfiguredTestMAAS, output_stream)
+
+        from maastest import report
+        encoded_results = (
+            output_stream.cache.getvalue().encode('utf-8'))
+        if not args.no_reporting:
+            report.write_test_results(encoded_results)
+
+        if not args.no_reporting and not args.log_results_only:
+            report.report_test_results(encoded_results, result.wasSuccessful())
+        if result.wasSuccessful():
+            return RETURN_CODES.SUCCESS
+        else:
+            return RETURN_CODES.TEST_FAILED
+
     except ProgramFailure as e:
         logging.error(e)
         return e.return_code
-
-    from maastest.cases import TestOneNode
-
-    class ConfiguredTestMAAS(TestOneNode):
-        """A configured version of TestInteractiveOneNode.
-
-        ConfiguredTestMAAS is a TestMAAS use to configure it by calling
-        cls.configure.  We need to configure the class itself and not
-        the instance because the KVMFixture TestMAAS instanciates and needs
-        to configure is created at the class level.
-        """
-    ConfiguredTestMAAS.configure(args)
-
-    from maastest.console import run_console
-    # This try/except is a band-aid to make sure the fixtures gets cleaned
-    # up when a KeyboardInterrupt exception is raised.
-    # This does miss out on test-level and module-level tearDowns.
-    try:
-        result = run_console(ConfiguredTestMAAS, output_stream)
-    except KeyboardInterrupt:
-        ConfiguredTestMAAS.tearDownClass()
-        raise
-    from maastest import report
-
-    encoded_results = (
-        output_stream.cache.getvalue().encode('utf-8'))
-    if not args.no_reporting:
-        report.write_test_results(encoded_results)
-
-    if not args.no_reporting and not args.log_results_only:
-        report.report_test_results(
-            encoded_results, result.wasSuccessful())
-    if result.wasSuccessful():
-        return RETURN_CODES.SUCCESS
-    else:
-        return RETURN_CODES.TEST_FAILED
-
-
-if __name__ == '__main__':
-    returncode = main()
-    if returncode is not None:
-        sys.exit(returncode)
+    finally:
+        if proxy_fixture is not None:
+            proxy_fixture.cleanUp()
+        if machine_fixture is not None:
+            machine_fixture.cleanUp()

=== added file 'maastest/script.py'
--- maastest/script.py	1970-01-01 00:00:00 +0000
+++ maastest/script.py	2014-01-10 15:28:21 +0000
@@ -0,0 +1,31 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Main script entry point for maas-test."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+import sys
+
+from maastest.parser import prepare_parser
+
+
+def entry_point():
+    args = prepare_parser().parse_args()
+
+    # If the user passed --help, we've already exited.  This is where we can
+    # import the rest of the code without hurting responsiveness for that case.
+    from maastest.main import main
+    return_code = main(args)
+
+    if return_code is not None:
+        sys.exit(return_code)

=== modified file 'maastest/tests/test_main.py'
--- maastest/tests/test_main.py	2013-12-17 07:46:12 +0000
+++ maastest/tests/test_main.py	2014-01-10 15:28:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `maastest.main`."""
@@ -32,6 +32,13 @@
 
 class TestTestMAAS(testtools.TestCase):
 
+    def make_args(self, interface, interactive=True):
+        """Create a fake arguments object."""
+        args = mock.MagicMock()
+        args.interface = interface
+        args.interactive = interactive
+        return args
+
     def patch_logging(self):
         """Shut up logging for the duration of this test."""
         # Quick and dirty.  Replace with something nicer if appropriate.
@@ -69,7 +76,7 @@
         self.patch(
             detect_dhcp, 'probe_dhcp', mock.MagicMock(return_value={server}))
 
-        return_value = main.main([interface, '--interactive'])
+        return_value = main.main(self.make_args(interface))
 
         self.assertEqual(
             main.RETURN_CODES.UNEXPECTED_DHCP_SERVER, return_value)
@@ -80,6 +87,81 @@
         self.patch_virtualization_type()
         interface = make_interface_name()
 
-        return_value = main.main([interface, '--interactive'])
+        return_value = main.main(self.make_args(interface))
 
         self.assertEqual(main.RETURN_CODES.NOT_ROOT, return_value)
+
+
+def make_proxy_url():
+    return 'http://%d.example.com:%d' % (randint(1, 999999), randint(1, 65535))
+
+
+class TestSetUpProxy(testtools.TestCase):
+
+    def setUp(self):
+        super(TestSetUpProxy, self).setUp()
+        self.patch(main, 'LocalProxyFixture', mock.MagicMock())
+        self.patch(os, 'putenv', mock.MagicMock())
+
+    def make_args(self, http_proxy=None, disable_cache=False):
+        """Create a fake arguments object with the given parameters."""
+        args = mock.MagicMock()
+        args.http_proxy = http_proxy
+        args.disable_cache = disable_cache
+        return args
+
+    def test_returns_empty_if_caching_disabled(self):
+        self.assertEqual(
+            ('', None),
+            main.set_up_proxy(self.make_args(disable_cache=True)))
+
+    def test_returns_existing_proxy_if_set(self):
+        proxy = make_proxy_url()
+        self.assertEqual(
+            (proxy, None),
+            main.set_up_proxy(self.make_args(http_proxy=proxy)))
+
+    def test_creates_proxy_if_appropriate(self):
+        proxy_url, proxy_fixture = main.set_up_proxy(self.make_args())
+        self.assertIsNotNone(proxy_fixture)
+        self.addCleanup(proxy_fixture.cleanUp)
+        self.assertEqual(proxy_fixture.get_url(), proxy_url)
+
+    def test_cleans_up_proxy_on_failure(self):
+        class DeliberateFailure(Exception):
+            """Deliberately induced error for testing."""
+
+        self.patch(
+            main.LocalProxyFixture.return_value, 'get_url',
+            mock.MagicMock(side_effect=DeliberateFailure))
+
+        self.assertRaises(
+            DeliberateFailure,
+            main.set_up_proxy, self.make_args())
+
+        self.assertEqual(
+            [mock.call()],
+            main.LocalProxyFixture.return_value.cleanUp.mock_calls)
+
+    def test_sets_env_to_existing_proxy(self):
+        proxy = make_proxy_url()
+        main.set_up_proxy(self.make_args(http_proxy=proxy))
+        self.assertItemsEqual(
+            [
+                mock.call('http_proxy', proxy),
+                mock.call('https_proxy', proxy),
+            ],
+            os.putenv.mock_calls)
+
+    def test_sets_env_to_created_proxy(self):
+        proxy_url, _ = main.set_up_proxy(self.make_args())
+        self.assertItemsEqual(
+            [
+                mock.call('http_proxy', proxy_url),
+                mock.call('https_proxy', proxy_url),
+            ],
+            os.putenv.mock_calls)
+
+    def test_leaves_env_unchanged_if_caching_disabled(self):
+        main.set_up_proxy(self.make_args(disable_cache=True))
+        self.assertEqual([], os.putenv.mock_calls)

=== modified file 'setup.py'
--- setup.py	2013-11-13 18:14:48 +0000
+++ setup.py	2014-01-10 15:28:21 +0000
@@ -1,5 +1,5 @@
 #!/usr/bin/env python
-# Copyright 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Distutils installer for maas-test."""
@@ -34,7 +34,7 @@
         "compatible with MAAS."),
     entry_points={
         "console_scripts": [
-            "maas-test = maastest.main:main",
+            "maas-test = maastest.script:entry_point",
             ],
         },
     )