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

Proposed by Manuel de la Peña on 2012-04-20
Status: Merged
Approved by: Sidnei da Silva on 2012-04-20
Approved revision: 68
Merged at revision: 66
Proposed branch: lp:~mandel/ubuntuone-dev-tools/squid-errors
Merge into: lp:ubuntuone-dev-tools
Diff against target: 82 lines (+38/-3)
2 files modified
ubuntuone/devtools/services/squid.py (+6/-1)
ubuntuone/devtools/services/tests/test_squid.py (+32/-2)
To merge this branch: bzr merge lp:~mandel/ubuntuone-dev-tools/squid-errors
Reviewer Review Type Date Requested Status
Sidnei da Silva (community) Approve on 2012-04-20
dobey (community) Approve on 2012-04-20
Diego Sarmentero (community) 2012-04-20 Approve on 2012-04-20
Review via email: mp+102825@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Diego Sarmentero (diegosarmentero) wrote :

+1

review: Approve
dobey (dobey) wrote :

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) wrote :

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
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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntuone/devtools/services/squid.py'
2--- ubuntuone/devtools/services/squid.py 2012-03-30 17:44:03 +0000
3+++ ubuntuone/devtools/services/squid.py 2012-04-20 17:04:20 +0000
4@@ -244,7 +244,12 @@
5 stderr=subprocess.PIPE)
6 store_proxy_settings(self.settings)
7 if not self._is_squid_running():
8- raise SquidLaunchError('Could not start squid.')
9+ # grab the stdout and stderr to provide more information
10+ output = sp.stdout.read()
11+ err = sp.stderr.read()
12+ msg = 'Could not start squid:\nstdout:\n%s\nstderr\n%s' % (
13+ output, err)
14+ raise SquidLaunchError(msg)
15 self.squid_pid = sp.pid
16 self.running = True
17
18
19=== modified file 'ubuntuone/devtools/services/tests/test_squid.py'
20--- ubuntuone/devtools/services/tests/test_squid.py 2012-03-30 17:44:03 +0000
21+++ ubuntuone/devtools/services/tests/test_squid.py 2012-04-20 17:04:20 +0000
22@@ -30,6 +30,8 @@
23 import json
24 import os
25
26+from StringIO import StringIO
27+
28 from twisted.internet import defer
29
30 from ubuntuone.devtools.testcases import BaseTestCase
31@@ -232,6 +234,14 @@
32 self._assert_missing_binary('htpasswd')
33
34
35+class Pipe(StringIO):
36+ """A read write pipe."""
37+
38+ def read(self):
39+ """Read the data."""
40+ return self.getvalue()
41+
42+
43 class FakeSubprocess(object):
44 """Fake the subprocess module."""
45
46@@ -240,10 +250,12 @@
47 """Create a new instance."""
48 self.called = []
49 self.PIPE = 'PIPE'
50+ self.stdout = Pipe()
51+ self.stderr = Pipe()
52
53- def Popen(self, args, stdout=None, stderr=None):
54+ def Popen(self, args, **kwargs):
55 """Fake Popen."""
56- self.called.append(('Popen', args, stdout, stderr))
57+ self.called.append(('Popen', args, kwargs))
58 return self
59 # pylint: enable=C0103
60
61@@ -345,3 +357,21 @@
62 squid_temp=squid._get_squid_temp_path(self.tmpdir)))
63 self.assertTrue(expected_parameters, self.template.called[0])
64 self.assertEqual(2, self.called.count(('fake_get_port',)))
65+
66+ def test_start_error(self):
67+ """Test that we do raise an exception correctly."""
68+ # set the error in the pipes
69+ out = 'Normal out'
70+ err = 'Error goes here'
71+ self.subprocess.stdout.write(out)
72+ self.subprocess.stderr.write(err)
73+ no_op = lambda _: None
74+ for func in ('_generate_auth_file', '_generate_config_file',
75+ '_generate_swap'):
76+ self.patch(self.runner, func, no_op)
77+ self.patch(squid, 'store_proxy_settings', no_op)
78+ self.patch(self.runner, '_is_squid_running', lambda: False)
79+ ex = self.assertRaises(squid.SquidLaunchError,
80+ self.runner.start_service)
81+ self.assertTrue(out in ex.message)
82+ self.assertTrue(err in ex.message)

Subscribers

People subscribed via source and target branches

to all changes: