Merge lp:~blake-rouse/maas/fix-bug-1324966 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Fix invalid attribute references in the VirshSSH class. Added more test for the VirshSSH class.

The VirshSSH class is referencing attributes that were renamed during the code review process. The test did not cover that section of the code, so it went by unnoticed.

I have fixed the attributes and added test's to cover those attributes, and other methods in the VirshSSH class.

[Raphaël Badin (rvb)]

Looks good. Couple of comments inline.

review: Approve

[Blake Rouse (blake-rouse)]

Thanks for the quick review. I made the recommended changes.

[Julian Edwards (julian-edwards)]

On 03/06/14 01:00, Raphaël Badin wrote:
> It's not always possible but we usually try to make the name of each test a bit more readable than this one it. This might seem a detail but it's really useful when reviewing tons of tests. In this instance I'd rename the test into "test_login_with_sshey". This is true for most of the tests in this branch.

I'd go a bit further with that. The test function names should
generally be of the format:

test_doing_foo_results_in_bar

IOW, test_login_with_sshkey should be
test_login_with_sshkey_attempts_connection, for example.

=== modified file 'src/provisioningserver/custom_hardware/tests/test_virsh.py'
--- src/provisioningserver/custom_hardware/tests/test_virsh.py	2014-05-21 17:03:00 +0000
+++ src/provisioningserver/custom_hardware/tests/test_virsh.py	2014-06-02 15:21:14 +0000
@@ -55,11 +55,122 @@
 class TestVirshSSH(MAASTestCase):
     """Tests for `VirshSSH`."""
 
+    def configure_virshssh_pexpect(self, inputs=None):
+        """Configures the VirshSSH class to use 'cat' process
+        for testing instead of the actual virsh."""
+        conn = virsh.VirshSSH(timeout=1)
+        self.addCleanup(conn.close)
+        self.patch(conn, '_execute')
+        conn._spawn('cat')
+        if inputs is not None:
+            for line in inputs:
+                conn.sendline(line)
+        return conn
+
     def configure_virshssh(self, output):
         self.patch(virsh.VirshSSH, 'run').return_value = output
         return virsh.VirshSSH()
 
-    def test_virssh_mac_addresses_returns_list(self):
+    def test_login_prompt(self):
+        virsh_outputs = [
+            'virsh # '
+        ]
+        conn = self.configure_virshssh_pexpect(virsh_outputs)
+        self.assertTrue(conn.login(poweraddr=None))
+
+    def test_login_with_sshkey(self):
+        virsh_outputs = [
+            "The authenticity of host '127.0.0.1' can't be established.",
+            "ECDSA key fingerprint is "
+            "00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff.",
+            "Are you sure you want to continue connecting (yes/no)? ",
+        ]
+        conn = self.configure_virshssh_pexpect(virsh_outputs)
+        mock_sendline = self.patch(conn, 'sendline')
+        conn.login(poweraddr=None)
+        self.assertThat(mock_sendline, MockCalledOnceWith('yes'))
+
+    def test_login_with_password(self):
+        virsh_outputs = [
+            "ubuntu@%s's password: " % factory.getRandomIPAddress(),
+        ]
+        conn = self.configure_virshssh_pexpect(virsh_outputs)
+        fake_password = factory.make_name('password')
+        mock_sendline = self.patch(conn, 'sendline')
+        conn.login(poweraddr=None, password=fake_password)
+        self.assertThat(mock_sendline, MockCalledOnceWith(fake_password))
+
+    def test_login_missing_password(self):
+        virsh_outputs = [
+            "ubuntu@%s's password: " % factory.getRandomIPAddress(),
+        ]
+        conn = self.configure_virshssh_pexpect(virsh_outputs)
+        mock_close = self.patch(conn, 'close')
+        self.assertFalse(conn.login(poweraddr=None, password=None))
+        mock_close.assert_called()
+
+    def test_login_invalid(self):
+        virsh_outputs = [
+            factory.getRandomString(),
+        ]
+        conn = self.configure_virshssh_pexpect(virsh_outputs)
+        mock_close = self.patch(conn, 'close')
+        self.assertFalse(conn.login(poweraddr=None))
+        mock_close.assert_called()
+
+    def test_logout(self):
+        conn = self.configure_virshssh_pexpect()
+        mock_sendline = self.patch(conn, 'sendline')
+        mock_close = self.patch(conn, 'close')
+        conn.logout()
+        self.assertThat(mock_sendline, MockCalledOnceWith('quit'))
+        mock_close.assert_called()
+
+    def test_prompt(self):
+        virsh_outputs = [
+            'virsh # '
+        ]
+        conn = self.configure_virshssh_pexpect(virsh_outputs)
+        self.assertTrue(conn.prompt())
+
+    def test_invalid_prompt(self):
+        virsh_outputs = [
+            factory.getRandomString()
+        ]
+        conn = self.configure_virshssh_pexpect(virsh_outputs)
+        self.assertFalse(conn.prompt())
+
+    def test_run(self):
+        cmd = ['list', '--all', '--name']
+        expected = ' '.join(cmd)
+        names = [factory.make_name('machine') for _ in range(3)]
+        conn = self.configure_virshssh_pexpect()
+        conn.before = '\n'.join([expected] + names)
+        mock_sendline = self.patch(conn, 'sendline')
+        mock_prompt = self.patch(conn, 'prompt')
+        output = conn.run(cmd)
+        self.assertThat(mock_sendline, MockCalledOnceWith(expected))
+        mock_prompt.assert_called()
+        self.assertEqual('\n'.join(names), output)
+
+    def test_list(self):
+        names = [factory.make_name('machine') for _ in range(3)]
+        conn = self.configure_virshssh('\n'.join(names))
+        expected = conn.list()
+        self.assertItemsEqual(names, expected)
+
+    def test_get_state(self):
+        state = factory.make_name('state')
+        conn = self.configure_virshssh(state)
+        expected = conn.get_state('')
+        self.assertEqual(state, expected)
+
+    def test_get_state_error(self):
+        conn = self.configure_virshssh('error')
+        expected = conn.get_state('')
+        self.assertEqual(None, expected)
+
+    def test_mac_addresses_returns_list(self):
         macs = [factory.getRandomMACAddress() for _ in range(2)]
         output = SAMPLE_IFLIST % (macs[0], macs[1])
         conn = self.configure_virshssh(output)
@@ -67,14 +178,14 @@
         for i in range(2):
             self.assertEqual(macs[i], expected[i])
 
-    def test_virssh_get_arch_returns_valid(self):
+    def test_get_arch_returns_valid(self):
         arch = factory.make_name('arch')
         output = SAMPLE_DUMPXML % arch
         conn = self.configure_virshssh(output)
         expected = conn.get_arch('')
         self.assertEqual(arch, expected)
 
-    def test_virssh_get_arch_returns_valid_fixed(self):
+    def test_get_arch_returns_valid_fixed(self):
         arch = random.choice(virsh.ARCH_FIX.keys())
         fixed_arch = virsh.ARCH_FIX[arch]
         output = SAMPLE_DUMPXML % arch

=== modified file 'src/provisioningserver/custom_hardware/virsh.py'
--- src/provisioningserver/custom_hardware/virsh.py	2014-05-21 17:03:00 +0000
+++ src/provisioningserver/custom_hardware/virsh.py	2014-06-02 15:21:14 +0000
@@ -52,8 +52,9 @@
         PROMPT_PASSWORD,
         PROMPT,
         PROMPT_DENIED,
+        PROMPT_CLOSED,
         pexpect.TIMEOUT,
-        PROMPT_CLOSED
+        pexpect.EOF,
     ]
 
     I_PROMPT = PROMPTS.index(PROMPT)
@@ -65,24 +66,27 @@
             None, timeout=timeout, maxread=maxread)
         self.name = '<virssh>'
 
+    def _execute(self, poweraddr):
+        """Spawns the pexpect command."""
+        cmd = 'virsh --connect %s' % poweraddr
+        self._spawn(cmd)
+
     def login(self, poweraddr, password=None):
         """Starts connection to virsh."""
-        cmd = 'virsh --connect %s' % poweraddr
-        super(VirshSSH, self)._spawn(cmd)
-
+        self._execute(poweraddr)
         i = self.expect(self.PROMPTS, timeout=10)
         if i == self.I_PROMPT_SSHKEY:
             # New certificate, lets always accept but if
             # it changes it will fail to login.
             self.sendline("yes")
-            i = self.expect(self.EXPECT_PROMPTS)
+            i = self.expect(self.PROMPTS)
         elif i == self.I_PROMPT_PASSWORD:
             # Requesting password, give it if available.
             if password is None:
                 self.close()
                 return False
             self.sendline(password)
-            i = self.expect(self.EXPECT_PROMPTS)
+            i = self.expect(self.PROMPTS)
 
         if i != self.I_PROMPT:
             # Something bad happened, either disconnect,
@@ -100,7 +104,7 @@
         """Waits for virsh prompt."""
         if timeout is None:
             timeout = self.timeout
-        i = self.expect([self.VIRSH_PROMPT, pexpect.TIMEOUT], timeout=timeout)
+        i = self.expect([self.PROMPT, pexpect.TIMEOUT], timeout=timeout)
         if i == 1:
             return False
         return True