Merge lp:~gmb/lpsetup/death-to-doctests-in-subcommands into lp:lpsetup

[Graham Binns]

The doctests in the subcommands package have been turned into unit tests.

This branch removes all the subcommands doctests and turns them into unit tests. I'll tackle other doctests in separate branches.

[lp:~gmb/lpsetup/death-to-doctests-in-subcommands]

50. By Graham Binns on 2012-07-11

Fixed conflicts and lint.

[Brad Crittenden (bac)]

Looks good Graham.

On IRC we discussed:

1) ORing the expected file stat masks together and then using XOR with the actual mode to allow a single assertion.

2) Taking into account the new behavior of write_file_contents where it no longer added the \n.

Thanks,
Brad

review: Approve (code)

[lp:~gmb/lpsetup/death-to-doctests-in-subcommands]

51. By Graham Binns on 2012-07-11

Bitwise funk.

52. By Graham Binns on 2012-07-11

Test tweak.

=== modified file 'lpsetup/subcommands/inithost.py'
--- lpsetup/subcommands/inithost.py	2012-07-10 19:06:12 +0000
+++ lpsetup/subcommands/inithost.py	2012-07-11 11:50:23 +0000
@@ -53,39 +53,7 @@
 
 
 def write_file_contents(filename, contents, mode, header=None):
-    """Write `contents` to filename and chmod it to 0644.
-
-        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
-        >>> print open('/tmp/foo', 'r').read()
-        Hello, world
-        >>> os.remove('/tmp/foo')
-
-    If the file already exists, `mode` is set to 'w', and the file's
-    contents differ from `contents`, write_file_contents() raises an
-    Exception.
-
-        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
-        >>> write_file_contents(
-        ...     '/tmp/foo', 'Hello again, world', 'w') # doctest: +ELLIPSIS
-        Traceback (most recent call last):
-        Exception: File /tmp/foo already exists with different contents...
-        >>> os.remove('/tmp/foo')
-
-    If the existing file's contents don't differ from `contents`,
-    write_file_contents() will simply return.
-
-        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
-        >>> write_file_contents(
-        ...     '/tmp/foo', 'Hello, world', 'w') # doctest: +ELLIPSIS
-        >>> os.remove('/tmp/foo')
-
-    Passing a `mode` other than "a" or "w" will cause an Exception.
-
-        >>> write_file_contents(
-        ...     '/tmp/foo', 'Hello, world', 'w+') # doctest: +ELLIPSIS
-        Traceback (most recent call last):
-        AssertionError: Mode should be 'w' or 'a' only...
-    """
+    """Write `contents` to filename and chmod it to 0644."""
     assert mode in ('w', 'a'), "Mode should be 'w' or 'a' only."
     if mode == 'w' and os.path.exists(filename):
         with open(filename, 'r') as existing_file:
@@ -104,55 +72,7 @@
 
 def ensure_ssh_keys(ssh_dir, private_key, public_key, valid_ssh_keys,
                     ssh_key_path):
-    """Ensure that SSH is configured correctly for the user.
-
-    If valid SSH keys are passed to the function, they will be written
-    out to `ssh_key_path`
-
-        >>> public = "Public key"
-        >>> private = "Private key"
-        >>> ssh_dir = "/tmp"
-        >>> ensure_ssh_keys(ssh_dir, private, public, True, '/tmp/test_key')
-        >>> public_keyfile = open('/tmp/test_key.pub', 'r')
-        >>> print public_keyfile.read() # doctest: +ELLIPSIS
-        # This file created at ...
-        Public key
-        >>> private_keyfile = open('/tmp/test_key', 'r')
-        >>> print private_keyfile.read() # doctest: +ELLIPSIS
-        # This file created at ...
-        Private key
-
-    Two other files will have been generated: authorized_keys and known_hosts
-
-        >>> print open(
-        ...     '/tmp/authorized_keys', 'r').read() # doctest: +ELLIPSIS
-        # This file created at ...
-        Public key
-        >>> print open('/tmp/known_hosts', 'r').read() # doctest: +ELLIPSIS
-        # This file created at ...
-        bazaar.launchpad.net ...
-        >>> os.remove('/tmp/test_key')
-        >>> os.remove('/tmp/test_key.pub')
-        >>> os.remove('/tmp/authorized_keys')
-        >>> os.remove('/tmp/known_hosts')
-
-    If valid keys are not passed, new keys will be generated.
-
-        >>> ensure_ssh_keys(ssh_dir, None, None, False, '/tmp/test_key')
-        >>> os.path.exists('/tmp/test_key')
-        True
-        >>> os.path.exists('/tmp/test_key.pub')
-        True
-        >>> os.path.exists('/tmp/authorized_keys')
-        True
-        >>> os.path.exists('/tmp/known_hosts')
-        True
-        >>> os.remove('/tmp/test_key')
-        >>> os.remove('/tmp/test_key.pub')
-        >>> os.remove('/tmp/authorized_keys')
-        >>> os.remove('/tmp/known_hosts')
-
-    """
+    """Ensure that SSH is configured correctly for the user."""
     # Set up the user's ssh directory.  The ssh key must be associated
     # with the lpuser's Launchpad account.
     mkdirs(ssh_dir)

=== modified file 'lpsetup/tests/subcommands/test_inithost.py'
--- lpsetup/tests/subcommands/test_inithost.py	2012-07-09 21:18:21 +0000
+++ lpsetup/tests/subcommands/test_inithost.py	2012-07-11 11:50:23 +0000
@@ -4,6 +4,10 @@
 
 """Tests for the inithost sub command."""
 
+import os
+import shutil
+import stat
+import tempfile
 import unittest
 
 from lpsetup import handlers
@@ -36,7 +40,7 @@
         '-v', private_key, '-b', public_key, '-S', ssh_key_name)
 
 
-class InithostTest(StepsBasedSubCommandTestMixin, unittest.TestCase):
+class InithostSmokeTest(StepsBasedSubCommandTestMixin, unittest.TestCase):
 
     sub_command_class = inithost.SubCommand
     expected_arguments = get_arguments()
@@ -51,3 +55,110 @@
         initialize_lxc_step,
         setup_apt_step,)
     needs_root = True
+
+
+class WriteFileContentsTestCase(unittest.TestCase):
+    """Tests for inithost.write_file_contents()."""
+
+    def setUp(self):
+        temp_file = tempfile.NamedTemporaryFile()
+        # Calling close() on the temp_file will delete it, but its name
+        # will live on...
+        temp_file.close()
+        self.temp_filename = temp_file.name
+
+    def test_write_file_contents_writes_file_contents(self):
+        # inithost.write_file_contents writes the supplied file contents
+        # to a given file using the mode provided and chmods the file to
+        # 0644 for the current user.
+        self.addCleanup(os.remove, self.temp_filename)
+        inithost.write_file_contents(self.temp_filename, 'Hello, world!', 'w')
+        with open(self.temp_filename, 'r') as the_file:
+            contents = the_file.read()
+        self.assertEqual('Hello, world!', contents)
+        file_mode = os.stat(self.temp_filename).st_mode
+        # The file permissions should be 0644. So, we do a bitwise
+        # dance...
+        expected_mode = (
+            stat.S_IWUSR | stat.S_IRUSR | stat.S_IRGRP | stat.S_IWGRP |
+            stat.S_IROTH | ~ stat.S_IXUSR | ~ stat.S_IWGRP |
+            ~ stat.S_IXGRP | ~ stat.S_IWOTH | ~ stat.S_IXOTH)
+        self.assertTrue(file_mode ^ expected_mode)
+
+    def test_write_file_contents_wont_overwrite_existing_files(self):
+        # If the file already exists, `mode` is set to 'w', and the file's
+        # contents differ from `contents`, write_file_contents() raises an
+        # Exception.
+        self.addCleanup(os.remove, self.temp_filename)
+        inithost.write_file_contents(self.temp_filename, 'Hello, world!', 'w')
+        self.assertRaises(
+            Exception, inithost.write_file_contents, self.temp_filename,
+            'Hello again!', 'w')
+
+    def test_write_file_contents_returns_if_file_contents_dont_change(self):
+        # If the existing file's contents don't differ from `contents`,
+        # write_file_contents() will simply return.
+        self.addCleanup(os.remove, self.temp_filename)
+        inithost.write_file_contents(self.temp_filename, 'Hello, world!', 'w')
+        # This second call shouldn't raise an exception.
+        inithost.write_file_contents(self.temp_filename, 'Hello, world!', 'w')
+
+    def test_write_file_contents_accepts_modes_a_or_w(self):
+        # Passing a `mode` other than "a" or "w" to write_file_contents()
+        # will cause an Exception.
+        self.assertRaises(
+            AssertionError, inithost.write_file_contents,
+            '/tmp/foo', 'Hello, world!', 'a+')
+
+
+class EnsureSSHKeysTestCase(unittest.TestCase):
+    """Tests for inithost.ensure_ssh_keys()."""
+
+    def setUp(self):
+        temp_file = tempfile.NamedTemporaryFile()
+        temp_file.close()
+        self.temp_filename = temp_file.name
+        self.ssh_dir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, self.ssh_dir)
+        self.addCleanup(os.remove, self.temp_filename)
+        self.addCleanup(os.remove, self.temp_filename + ".pub")
+
+    def test_ensure_ssh_keys_writes_to_ssh_key_path(self):
+        # If inithost.ensure_ssh_keys() is told that the keys it has
+        # been passed are valid, it will write them out to the provided
+        # ssh_key_path.
+        public_key = "Public"
+        private_key = "Private"
+        inithost.ensure_ssh_keys(
+            self.ssh_dir, private_key, public_key, True, self.temp_filename)
+        with open(self.temp_filename + '.pub', 'r') as pub_file:
+            self.assertIn(public_key, pub_file.read())
+        with open(self.temp_filename, 'r') as priv_file:
+            self.assertIn(private_key, priv_file.read())
+
+    def test_ensure_ssh_keys_generates_keys_if_not_passed(self):
+        # If SSH keys aren't passed to ensure_ssh_keys(), the function
+        # will generate some.
+        inithost.ensure_ssh_keys(
+            self.ssh_dir, None, None, False, self.temp_filename)
+        self.assertTrue(os.path.exists(self.temp_filename))
+        self.assertTrue(os.path.exists(self.temp_filename + ".pub"))
+
+    def test_ensure_ssh_keys_generates_other_files(self):
+        # ensure_ssh_keys() also generates an authorized_keys file and a
+        # known_hosts file in the ssh dir.
+        inithost.ensure_ssh_keys(
+            self.ssh_dir, None, None, False, self.temp_filename)
+        self.assertTrue(
+            os.path.exists(os.path.join(self.ssh_dir, 'authorized_keys')))
+        self.assertTrue(
+            os.path.exists(os.path.join(self.ssh_dir, 'known_hosts')))
+
+    def test_ensure_ssh_keys_creates_ssh_dir(self):
+        # If the ssh_dir passed to ensure_ssh_keys() doesn't exist, it
+        # will be created.
+        shutil.rmtree(self.ssh_dir)
+        inithost.ensure_ssh_keys(
+            self.ssh_dir, None, None, False, self.temp_filename)
+        self.assertTrue(os.path.exists(self.ssh_dir))
+        self.assertTrue(os.path.isdir(self.ssh_dir))