Merge lp:~rvb/maas-test/display-maas-version into lp:maas-test

[Raphaël Badin]

Display the version of MAAS to be installed.

It would be nice to also display the source of the package but this is a start and should be good enough for now.

[Julian Edwards (julian-edwards)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review: approve

Looks good, thanks for doing this very useful change. Just a couple
of small comments:

> === modified file 'maastest/tests/test_maasfixture.py' ---
> maastest/tests/test_maasfixture.py 2013-12-18 12:04:41 +0000 +++
> maastest/tests/test_maasfixture.py 2014-01-08 16:22:42 +0000 @@
> -335,6 +335,44 @@ check_call=True), )
>
> + def test_install_maas_logs_maas_version(self): +
> fixture = self.make_kvm_fixture() + maas_fixture =
> self.make_maas_fixture(fixture) + version =
> self.getUniqueString() + mock_get_maas_version =
> mock.MagicMock(return_value=version) +
> self.patch(maas_fixture, 'get_maas_version',
> mock_get_maas_version)

Because the mock is returned from patch(), you can avoid importing
MagickMock and just do:

   self.patch(maas_fixture, 'get_maas_version').return_value = version

> + mock_logging = mock.MagicMock() +
> self.patch(maasfixture.logging, 'info', mock_logging)

And this can be:

   mock_logging = self.patch(maasfixture.logging, 'info')

> + maas_fixture.install_maas() + + self.assertEqual( +
> ( + [mock.call()], + [ +
> mock.call("Installing MAAS (version %s)..." % version), +
> mock.call("Done installing MAAS."), + ]), +
> (mock_get_maas_version.mock_calls, mock_logging.mock_calls))

I think this is definitely a case where having more than one assertion
is better than compounding them into a single call. This is extremely
hard to read and digest. And ease of reading trumps everything else!

> + + def test_get_maas_version_returns_maas_version(self): +
> fixture = self.make_kvm_fixture() + maas_fixture =
> self.make_maas_fixture(fixture) + version =
> self.getUniqueString() + policy = "Candidate: %s\n" %
> version + self.patch(fixture, 'run_command',
> mock.MagicMock( + return_value=(0, policy, b'stderr')))
> + + extracted_version = maas_fixture.get_maas_version() + +
> fixture.run_command.assert_has_call( + mock.call( +
> maas_fixture.kvm_fixture._make_apt_get_install_command( +
> ['language-pack-en']), + check_call=True), +
> )

You could make this easier to read by assigning a variable to the
result of mock.call() and using that in the assert_has_call.

Cheers!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLN8WUACgkQWhGlTF8G/HcjiQCffd/lSuhJw3fupbpVCefJLGuQ
eSEAn1YZRK6NYjQxqqARnVYeifwSGl+/
=CJXi
-----END PGP SIGNATURE-----

review: Approve

[lp:~rvb/maas-test/display-maas-version]

104. By Raphaël Badin on 2014-01-09

Review fixes.

[Raphaël Badin (rvb)]

 > Because the mock is returned from patch(), you can avoid importing
> MagickMock and just do:
>
>[...]

Well, actually, the mock object is *not* returned by patch(…). In MAAS it is, see src/maastesting/testcase.py:MAASTestCase.patch. But in maas-test, we don't have that utility

>
> > + maas_fixture.install_maas() + + self.assertEqual( +
> > ( + [mock.call()], + [ +
> > mock.call("Installing MAAS (version %s)..." % version), +
> > mock.call("Done installing MAAS."), + ]), +
> > (mock_get_maas_version.mock_calls, mock_logging.mock_calls))
>
> I think this is definitely a case where having more than one assertion
> is better than compounding them into a single call. This is extremely
> hard to read and digest. And ease of reading trumps everything else!

All right, you know I'm a sucker for compound assertions but in this instance, you're definitely right.

> [...]
> You could make this easier to read by assigning a variable to the
> result of mock.call() and using that in the assert_has_call.

Actually, I've refactored that code a bit and it's now a bit simpler.

Thanks for the review!

[Julian Edwards (julian-edwards)]

On 09/01/14 18:40, Raphaël Badin wrote:
> > Because the mock is returned from patch(), you can avoid importing
>> MagickMock and just do:
>>
>> [...]
>
> Well, actually, the mock object is *not* returned by patch(…). In MAAS it is, see src/maastesting/testcase.py:MAASTestCase.patch. But in maas-test, we don't have that utility

Ah! That's probably worth adding as a small change next time you're in
this code then.

> Thanks for the review!

NP.

=== modified file 'maastest/maasfixture.py'
--- maastest/maasfixture.py	2014-01-08 17:31:43 +0000
+++ maastest/maasfixture.py	2014-01-09 08:39:45 +0000
@@ -99,14 +99,21 @@
         # Install the English language pack first.  If this is not done
         # before postgres is installed, on some systems it won't be able to
         # set up its main cluster.
+        maas_version = self.get_maas_version()
+        logging.info("Installing MAAS (version %s)..." % maas_version)
         # TODO: Is there no better way to ensure this, e.g. through a
         # dependency?
-        logging.info("Installing MAAS...")
         self.kvm_fixture.install_packages(['language-pack-en'])
         # Now we can install maas (which also installs postgres).
         self.kvm_fixture.install_packages(['maas', 'maas-dhcp', 'maas-dns'])
         logging.info("Done installing MAAS.")
 
+    def get_maas_version(self):
+        """Return the version of MAAS that is to be installed."""
+        _, policy, _ = self.kvm_fixture.run_command(
+            ['apt-cache', 'policy', 'maas'], check_call=True)
+        return utils.extract_package_version(policy)
+
     def configure_default_maas_url(self):
         """Set `DEFAULT_MAAS_URL` in the virtual machine.
 
 
=== modified file 'maastest/tests/test_maasfixture.py'
--- maastest/tests/test_maasfixture.py	2014-01-08 17:31:43 +0000
+++ maastest/tests/test_maasfixture.py	2014-01-09 08:39:45 +0000
@@ -335,6 +335,37 @@
                 check_call=True),
             )
 
+    def test_install_maas_logs_maas_version(self):
+        fixture = self.make_kvm_fixture()
+        maas_fixture = self.make_maas_fixture(fixture)
+        version = self.getUniqueString()
+        mock_get_maas_version = mock.MagicMock(return_value=version)
+        self.patch(maas_fixture, 'get_maas_version', mock_get_maas_version)
+        mock_logging = mock.MagicMock()
+        self.patch(maasfixture.logging, 'info', mock_logging)
+
+        maas_fixture.install_maas()
+
+        self.assertEqual([mock.call()], mock_get_maas_version.mock_calls)
+        self.assertEqual(
+            [
+                mock.call("Installing MAAS (version %s)..." % version),
+                mock.call("Done installing MAAS."),
+            ],
+            mock_logging.mock_calls)
+
+    def test_get_maas_version_returns_maas_version(self):
+        fixture = self.make_kvm_fixture()
+        maas_fixture = self.make_maas_fixture(fixture)
+        version = self.getUniqueString()
+        policy = "Candidate: %s\n" % version
+        self.patch(fixture, 'run_command', mock.MagicMock(
+            return_value=(0, policy, b'stderr')))
+
+        extracted_version = maas_fixture.get_maas_version()
+
+        self.assertEqual(version, extracted_version)
+
     def test_install_maas_installs_language_pack_followed_by_maas(self):
         # We need to do it in this order, or postgres (and other things) may
         # break during installation.
 
=== modified file 'maastest/tests/test_utils.py'
--- maastest/tests/test_utils.py	2014-01-08 17:49:09 +0000
+++ maastest/tests/test_utils.py	2014-01-09 08:39:45 +0000
@@ -412,3 +412,21 @@
         stream.foo_bar_baz = mock.MagicMock(return_value=some_string)
         caching_stream = utils.CachingOutputStream(stream)
         self.assertEqual(some_string, caching_stream.foo_bar_baz())
+
+
+class TestExtractPackageVersion(testtools.TestCase):
+
+    def test_extracts_package_version(self):
+        version = '1.4+bzr1693+dfsg-0ubuntu2.2'
+        policy = dedent("""
+            maas:
+              Installed: (none)
+              Candidate: %s
+              Version table:
+                 1.4+bzr1693+dfsg-0ubuntu2.2 0
+                    500 http://example.com/ubuntu/ saucy/main amd64 Packages
+        """) % version
+        self.assertEqual(version, utils.extract_package_version(policy))
+
+    def test_returns_None_if_policy_is_unparseable(self):
+        self.assertIsNone(utils.extract_package_version("unparsable"))
 
=== modified file 'maastest/utils.py'
--- maastest/utils.py	2014-01-09 07:19:20 +0000
+++ maastest/utils.py	2014-01-09 08:39:45 +0000
@@ -25,11 +25,12 @@
     ]
 
 
+import inspect
 from io import BytesIO
-import inspect
 import logging
 from pipes import quote
 import platform
+import re
 from subprocess import (
     PIPE,
     Popen,
@@ -256,6 +257,20 @@
         return True
 
 
+def extract_package_version(policy):
+    """Get the version of the package to be installed from policy settings.
+
+    The given policy is assumed to be of the form returned by running:
+    `apt-cache policy <packagename>`.  This method returns None if the
+    given policy string cannot be parsed.
+    """
+    match = re.search('Candidate: (.*)\n', policy)
+    if match is not None:
+        return match.group(1)
+    else:
+        return None
+
+
 def virtualization_type():
     """Returns the name of the virtualization type of this machine.