Merge lp:~mandel/ubuntuone-dev-tools/squid-errors into lp:ubuntuone-dev-tools

[Manuel de la Peña]

- Read the stdout and stderr pipes when we cannot launch squid to give more information to the user (LP: #985004).

- Read the stdout and stderr pipes when we cannot launch squid to give more information to the user (LP: #985004).

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[dobey (dobey)]

77 + ex = self.assertRaises(squid.SquidLaunchError,
78 + self.runner.start_service)

In the diff, this second line is spaced over very far to the right. Why is that?

12 + msg = 'Could not start squid: %s, %s' % (output, err)

I don't think this formatting is correct. As stdout and stderr may contain multiple lines, this formatting will reduce readability slightly. I think you should do "\n%s\n%s" instead of "%s, %s" here.

review: Needs Fixing

[Sidnei da Silva (sidnei)]

Erm, what's wrong with StringIO? If you look at the source, it works just like your Pipe() object, except it appends to a list instead of using += for string concatenation, which is about an order of magnitude faster.

review: Needs Fixing

[lp:~mandel/ubuntuone-dev-tools/squid-errors]

65. By Manuel de la Peña on 2012-04-20

Use StringIO for the Pipe and made a better exception message.

66. By Manuel de la Peña on 2012-04-20

Use a more tranditional indentation.

67. By Manuel de la Peña on 2012-04-20

New lines, new linesvim ubuntuone/devtools/services/tests/test_squid.py

68. By Manuel de la Peña on 2012-04-20

Added spaces.

[dobey (dobey)]

review: Approve

[Sidnei da Silva (sidnei)]

review: Approve

=== modified file 'ubuntuone/devtools/services/squid.py'
--- ubuntuone/devtools/services/squid.py	2012-03-30 17:44:03 +0000
+++ ubuntuone/devtools/services/squid.py	2012-04-20 17:04:20 +0000
@@ -244,7 +244,12 @@
                               stderr=subprocess.PIPE)
         store_proxy_settings(self.settings)
         if not self._is_squid_running():
-            raise SquidLaunchError('Could not start squid.')
+            # grab the stdout and stderr to provide more information
+            output = sp.stdout.read()
+            err = sp.stderr.read()
+            msg = 'Could not start squid:\nstdout:\n%s\nstderr\n%s' % (
+                output, err)
+            raise SquidLaunchError(msg)
         self.squid_pid = sp.pid
         self.running = True
 

=== modified file 'ubuntuone/devtools/services/tests/test_squid.py'
--- ubuntuone/devtools/services/tests/test_squid.py	2012-03-30 17:44:03 +0000
+++ ubuntuone/devtools/services/tests/test_squid.py	2012-04-20 17:04:20 +0000
@@ -30,6 +30,8 @@
 import json
 import os
 
+from StringIO import StringIO
+
 from twisted.internet import defer
 
 from ubuntuone.devtools.testcases import BaseTestCase
@@ -232,6 +234,14 @@
         self._assert_missing_binary('htpasswd')
 
 
+class Pipe(StringIO):
+    """A read write pipe."""
+
+    def read(self):
+        """Read the data."""
+        return self.getvalue()
+
+
 class FakeSubprocess(object):
     """Fake the subprocess module."""
 
@@ -240,10 +250,12 @@
         """Create a new instance."""
         self.called = []
         self.PIPE = 'PIPE'
+        self.stdout = Pipe()
+        self.stderr = Pipe()
 
-    def Popen(self, args, stdout=None, stderr=None):
+    def Popen(self, args, **kwargs):
         """Fake Popen."""
-        self.called.append(('Popen', args, stdout, stderr))
+        self.called.append(('Popen', args, kwargs))
         return self
     # pylint: enable=C0103
 
@@ -345,3 +357,21 @@
                      squid_temp=squid._get_squid_temp_path(self.tmpdir)))
         self.assertTrue(expected_parameters, self.template.called[0])
         self.assertEqual(2, self.called.count(('fake_get_port',)))
+
+    def test_start_error(self):
+        """Test that we do raise an exception correctly."""
+        # set the error in the pipes
+        out = 'Normal out'
+        err = 'Error goes here'
+        self.subprocess.stdout.write(out)
+        self.subprocess.stderr.write(err)
+        no_op = lambda _: None
+        for func in ('_generate_auth_file', '_generate_config_file',
+                     '_generate_swap'):
+            self.patch(self.runner, func, no_op)
+        self.patch(squid, 'store_proxy_settings', no_op)
+        self.patch(self.runner, '_is_squid_running', lambda: False)
+        ex = self.assertRaises(squid.SquidLaunchError,
+                               self.runner.start_service)
+        self.assertTrue(out in ex.message)
+        self.assertTrue(err in ex.message)