Merge lp:~fwereade/pyjuju/cobbler-zk-connect-error-messages into lp:~fwereade/pyjuju/cobbler-zk-connect
- cobbler-zk-connect-error-messages
- Merge into cobbler-zk-connect
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | William Reade | ||||
Approved revision: | 339 | ||||
Merged at revision: | 339 | ||||
Proposed branch: | lp:~fwereade/pyjuju/cobbler-zk-connect-error-messages | ||||
Merge into: | lp:~fwereade/pyjuju/cobbler-zk-connect | ||||
Diff against target: |
1156 lines (+192/-174) 43 files modified
ensemble/agents/provision.py (+4/-4) ensemble/agents/tests/test_provision.py (+3/-3) ensemble/agents/tests/test_unit.py (+4/-4) ensemble/control/status.py (+3/-3) ensemble/control/tests/test_status.py (+0/-2) ensemble/errors.py (+7/-19) ensemble/formula/bundle.py (+3/-3) ensemble/formula/provider.py (+1/-1) ensemble/formula/tests/test_bundle.py (+8/-4) ensemble/formula/tests/test_config.py (+6/-4) ensemble/formula/tests/test_repository.py (+4/-4) ensemble/hooks/tests/test_arguments.py (+2/-2) ensemble/hooks/tests/test_cli.py (+5/-2) ensemble/hooks/tests/test_executor.py (+1/-1) ensemble/hooks/tests/test_invoker.py (+1/-1) ensemble/lib/tests/test_schema.py (+1/-1) ensemble/machine/tests/test_unit_deployment.py (+6/-4) ensemble/machine/unit.py (+1/-1) ensemble/providers/common/connect.py (+2/-2) ensemble/providers/common/findzookeepers.py (+1/-1) ensemble/providers/common/tests/test_connect.py (+5/-7) ensemble/providers/common/tests/test_findzookeepers.py (+27/-2) ensemble/providers/common/tests/test_utils.py (+2/-5) ensemble/providers/common/utils.py (+3/-1) ensemble/providers/ec2/__init__.py (+3/-2) ensemble/providers/ec2/launch.py (+2/-3) ensemble/providers/ec2/securitygroup.py (+6/-7) ensemble/providers/ec2/tests/common.py (+0/-10) ensemble/providers/ec2/tests/test_connect.py (+5/-7) ensemble/providers/ec2/tests/test_getmachines.py (+2/-3) ensemble/providers/ec2/tests/test_launch.py (+3/-4) ensemble/providers/ec2/tests/test_provider.py (+4/-1) ensemble/providers/ec2/tests/test_securitygroup.py (+6/-6) ensemble/providers/orchestra/cobbler.py (+5/-5) ensemble/providers/orchestra/tests/test_cobbler.py (+14/-9) ensemble/providers/orchestra/tests/test_connect.py (+4/-4) ensemble/providers/orchestra/tests/test_launch.py (+14/-6) ensemble/providers/orchestra/tests/test_provider.py (+4/-2) ensemble/state/service.py (+2/-1) ensemble/state/tests/test_security.py (+1/-1) ensemble/state/tests/test_service.py (+9/-5) ensemble/tests/test_errors.py (+6/-16) ensemble/unit/tests/test_formula.py (+2/-1) |
||||
To merge this branch: | bzr merge lp:~fwereade/pyjuju/cobbler-zk-connect-error-messages | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jim Baker (community) | Approve | ||
Gustavo Niemeyer (community) | Approve | ||
Review via email: mp+72033@code.launchpad.net |
Commit message
Description of the change
Fixed errors as requested in https:/
Also made many assertIn error message checks into assertEquals; improved some error messages; reworked ProviderInterac
- 333. By William Reade
-
merge parent
- 334. By William Reade
-
merge parent
- 335. By William Reade
-
merge parent
- 336. By William Reade
-
merge parent
William Reade (fwereade) wrote : | # |
[error messages]
The only solution I can see to all these issues is to eliminate all error message prefixes, and to write out the full error message for every exception type at every call site... for example, I thought you didn't like the "Colon-joined sentence fragment: Style of error message", but you're asking me to reinstate it in [2] and [3]. WRT [1], people were already using the "simple" form incorrectly, and the same fix applies.
[docstrings]
Reverting; opened lp:831805
William Reade (fwereade) wrote : | # |
> [error messages]
>
> "Colon-joined sentence fragment: Style of error message", but you're asking me to
> reinstate it in [2] and [3].
On further inspection, FormulaError(
- 337. By William Reade
-
merge parent
- 338. By William Reade
-
fix ProviderInterac
tionErrors per review - 339. By William Reade
-
merge parent
Gustavo Niemeyer (niemeyer) wrote : | # |
> On further inspection, FormulaError(
> really does appear to be the standard, because it results in a capitalised
> error message without weird interspersed caps, and I'll be leaving them as-is.
> If I've misunderstood I'll do a separate cleanup branch.
Sounds good then. Thanks for checking rather than taking the crackful
comment blindly. :-)
Jim Baker (jimbaker) wrote : | # |
+1, overall this looks good. Moving to assertEquals is definitely an improvement as well. There are a number of spots where you should consider using %r instead of %s where file names and potentially other strings might be Unicode. There is one case that certainly should be done that way:
def __str__(self):
- return "Error processing '%s': %s." % (self.path, self.message)
+ return "Error processing '%s': %s" % (self.path, self.message)
Jim Baker (jimbaker) wrote : | # |
Interestingly I cannot mark the branch as being approved now that we have hit our quorum of reviewers. Is this because it's stacked? Anyway, someone should take this action if at all possible!
William Reade (fwereade) wrote : | # |
> Interestingly I cannot mark the branch as being approved now that we have hit
> our quorum of reviewers. Is this because it's stacked? Anyway, someone should
> take this action if at all possible!
Feels funny approving my own branch :). I'll make your changes and get it in shortly. Thanks for the review :).
- 340. By William Reade
-
merge parent
- 341. By William Reade
-
fixes per review
Preview Diff
1 | === modified file 'ensemble/agents/provision.py' |
2 | --- ensemble/agents/provision.py 2011-08-12 19:28:37 +0000 |
3 | +++ ensemble/agents/provision.py 2011-08-26 13:09:07 +0000 |
4 | @@ -4,7 +4,7 @@ |
5 | from zookeeper import NoNodeException |
6 | |
7 | from ensemble.environment.config import EnvironmentsConfig |
8 | -from ensemble.errors import MachinesNotFound, ProviderInteractionError |
9 | +from ensemble.errors import MachinesNotFound, ProviderError |
10 | from ensemble.lib.twistutils import concurrent_execution_guard |
11 | from ensemble.state.errors import ( |
12 | MachineStateNotFound, ServiceStateNotFound, ServiceUnitStateNotFound, |
13 | @@ -178,7 +178,7 @@ |
14 | # map of instance_id -> machine |
15 | try: |
16 | provider_machines = yield self.provider.get_machines() |
17 | - except ProviderInteractionError: |
18 | + except ProviderError: |
19 | log.exception("Cannot get machine list") |
20 | return |
21 | |
22 | @@ -192,7 +192,7 @@ |
23 | machine_state_id, provider_machines) |
24 | except (StateChanged, |
25 | MachineStateNotFound, |
26 | - ProviderInteractionError): |
27 | + ProviderError): |
28 | log.exception("Cannot process machine %s", machine_state_id) |
29 | continue |
30 | instance_ids.append(instance_id) |
31 | @@ -204,7 +204,7 @@ |
32 | machine = provider_machines[instance_id] |
33 | try: |
34 | yield self.provider.shutdown_machine(machine) |
35 | - except ProviderInteractionError: |
36 | + except ProviderError: |
37 | log.exception("Cannot shutdown machine %s", instance_id) |
38 | continue |
39 | |
40 | |
41 | === modified file 'ensemble/agents/tests/test_provision.py' |
42 | --- ensemble/agents/tests/test_provision.py 2011-08-11 05:59:30 +0000 |
43 | +++ ensemble/agents/tests/test_provision.py 2011-08-26 13:09:07 +0000 |
44 | @@ -313,7 +313,7 @@ |
45 | |
46 | mock_provider = self.mocker.patch(self.agent.provider) |
47 | mock_provider.start_machine({"machine-id": 0}) |
48 | - self.mocker.result(fail(ProviderInteractionError(OSError("Bad")))) |
49 | + self.mocker.result(fail(ProviderInteractionError())) |
50 | |
51 | mock_provider.start_machine({"machine-id": 1}) |
52 | self.mocker.passthrough() |
53 | @@ -339,7 +339,7 @@ |
54 | mock_provider = self.mocker.patch(self.agent.provider) |
55 | |
56 | mock_provider.shutdown_machine(MATCH_MACHINE) |
57 | - self.mocker.result(fail(ProviderInteractionError("Bad"))) |
58 | + self.mocker.result(fail(ProviderInteractionError())) |
59 | |
60 | mock_provider.shutdown_machine(MATCH_MACHINE) |
61 | self.mocker.passthrough() |
62 | @@ -368,7 +368,7 @@ |
63 | |
64 | mock_provider = self.mocker.patch(self.agent.provider) |
65 | mock_provider.get_machines() |
66 | - self.mocker.result(fail(ProviderInteractionError("Bad"))) |
67 | + self.mocker.result(fail(ProviderInteractionError())) |
68 | |
69 | mock_provider.get_machines() |
70 | self.mocker.passthrough() |
71 | |
72 | === modified file 'ensemble/agents/tests/test_unit.py' |
73 | --- ensemble/agents/tests/test_unit.py 2011-07-29 14:42:04 +0000 |
74 | +++ ensemble/agents/tests/test_unit.py 2011-08-26 13:09:07 +0000 |
75 | @@ -118,10 +118,10 @@ |
76 | e = self.assertRaises(EnsembleError, |
77 | self.agent.configure, |
78 | options) |
79 | - self.assertIn( |
80 | - ("--unit-name must be provided in the command line," |
81 | - " or $ENSEMBLE_UNIT_NAME in the environment"), |
82 | - str(e)) |
83 | + self.assertEquals( |
84 | + str(e), |
85 | + "--unit-name must be provided in the command line, or " |
86 | + "$ENSEMBLE_UNIT_NAME in the environment") |
87 | |
88 | def test_agent_unit_name_cli_extraction(self): |
89 | """The unit agent can parse its unit-name from the cli. |
90 | |
91 | === modified file 'ensemble/control/status.py' |
92 | --- ensemble/control/status.py 2011-07-20 23:35:48 +0000 |
93 | +++ ensemble/control/status.py 2011-08-26 13:09:07 +0000 |
94 | @@ -7,7 +7,7 @@ |
95 | from twisted.internet.defer import inlineCallbacks, returnValue |
96 | import yaml |
97 | |
98 | -from ensemble.errors import ProviderInteractionError |
99 | +from ensemble.errors import ProviderError |
100 | from ensemble.environment.errors import EnvironmentsConfigError |
101 | from ensemble.state.errors import UnitRelationStateNotFound |
102 | from ensemble.state.formula import FormulaStateManager |
103 | @@ -264,10 +264,10 @@ |
104 | try: |
105 | pm = yield machine_provider.get_machine(instance_id) |
106 | m["dns-name"] = pm.dns_name |
107 | - except ProviderInteractionError: |
108 | + except ProviderError: |
109 | # The provider doesn't have machine information |
110 | log.error("Machine provider information missing: machine %s" % ( |
111 | - machine_state.id)) |
112 | + machine_state.id)) |
113 | |
114 | machine_data[machine_state.id] = m |
115 | |
116 | |
117 | === modified file 'ensemble/control/tests/test_status.py' |
118 | --- ensemble/control/tests/test_status.py 2011-08-08 21:51:54 +0000 |
119 | +++ ensemble/control/tests/test_status.py 2011-08-26 13:09:07 +0000 |
120 | @@ -458,7 +458,6 @@ |
121 | self.assertEqual(state["machines"][7]["instance-id"], |
122 | "pending") |
123 | |
124 | - |
125 | @inlineCallbacks |
126 | def test_render_yaml(self): |
127 | yield self.build_topology() |
128 | @@ -610,7 +609,6 @@ |
129 | |
130 | self.assertIn("namespace:dummy-1", result) |
131 | |
132 | - |
133 | def test_render_dot_bad_clustering(self): |
134 | """Test around Bug #792448. |
135 | |
136 | |
137 | === modified file 'ensemble/errors.py' |
138 | --- ensemble/errors.py 2011-08-17 17:26:19 +0000 |
139 | +++ ensemble/errors.py 2011-08-26 13:09:07 +0000 |
140 | @@ -48,7 +48,7 @@ |
141 | self.path = path |
142 | |
143 | def __str__(self): |
144 | - return "File was not found: %s" % (self.path,) |
145 | + return "File was not found: %r" % (self.path,) |
146 | |
147 | |
148 | class FormulaError(EnsembleError): |
149 | @@ -59,7 +59,7 @@ |
150 | self.message = message |
151 | |
152 | def __str__(self): |
153 | - return "Error processing '%s': %s." % (self.path, self.message) |
154 | + return "Error processing %r: %s" % (self.path, self.message) |
155 | |
156 | |
157 | class FormulaInvocationError(EnsembleError): |
158 | @@ -70,8 +70,8 @@ |
159 | self.exit_code = exit_code |
160 | |
161 | def __str__(self): |
162 | - return "Error processing '%s': exit code %s." % (self.path, |
163 | - self.exit_code) |
164 | + return "Error processing %r: exit code %s." % ( |
165 | + self.path, self.exit_code) |
166 | |
167 | |
168 | class FileAlreadyExists(EnsembleError): |
169 | @@ -84,7 +84,7 @@ |
170 | self.path = path |
171 | |
172 | def __str__(self): |
173 | - return "File already exists, won't overwrite: %s" % (self.path,) |
174 | + return "File already exists, won't overwrite: %r" % (self.path,) |
175 | |
176 | |
177 | class InvalidEnsembleHeaderValue(EnsembleError): |
178 | @@ -101,7 +101,7 @@ |
179 | |
180 | def __str__(self): |
181 | return ("Expected a YAML file with an 'ensemble: %s' " |
182 | - "header, found 'ensemble: %s': %s" |
183 | + "header, found 'ensemble: %s': %r" |
184 | % (self.expected, self.found, self.path)) |
185 | |
186 | |
187 | @@ -130,12 +130,6 @@ |
188 | class EnvironmentPending(NoConnection): |
189 | """Raised when the ensemble environment is not accessible.""" |
190 | |
191 | - def __init__(self, info="no details available"): |
192 | - self._info = info |
193 | - |
194 | - def __str__(self): |
195 | - return "Ensemble environment is not accessible: %s" % self._info |
196 | - |
197 | |
198 | class ProviderError(EnsembleError): |
199 | """Raised when an exception occurs in a provider.""" |
200 | @@ -154,13 +148,7 @@ |
201 | |
202 | |
203 | class ProviderInteractionError(ProviderError): |
204 | - |
205 | - def __init__(self, error): |
206 | - self.error = error |
207 | - |
208 | - def __str__(self): |
209 | - return "ProviderError: Interaction with machine provider failed: %r" \ |
210 | - % self.error |
211 | + """Raised when an unexpected error occurs interacting with a provider""" |
212 | |
213 | |
214 | class CannotTerminateMachine(EnsembleError): |
215 | |
216 | === modified file 'ensemble/formula/bundle.py' |
217 | --- ensemble/formula/bundle.py 2011-06-05 20:19:18 +0000 |
218 | +++ ensemble/formula/bundle.py 2011-08-26 13:09:07 +0000 |
219 | @@ -18,13 +18,13 @@ |
220 | try: |
221 | zf = ZipFile(path, 'r') |
222 | except BadZipfile, exc: |
223 | - raise FormulaError(path, "Must be a ZIP-file (%s)." % str(exc)) |
224 | + raise FormulaError(path, "must be a zip file (%s)" % exc) |
225 | |
226 | metadata = MetaData() |
227 | if "metadata.yaml" not in zf.namelist(): |
228 | raise FormulaError(path, |
229 | - "Archive does not contain required file: " |
230 | - "\"metadata.yaml\".") |
231 | + "archive does not contain required file " |
232 | + "\"metadata.yaml\"") |
233 | |
234 | content = zf.read("metadata.yaml") |
235 | metadata.parse(content) |
236 | |
237 | === modified file 'ensemble/formula/provider.py' |
238 | --- ensemble/formula/provider.py 2010-11-02 16:15:23 +0000 |
239 | +++ ensemble/formula/provider.py 2011-08-26 13:09:07 +0000 |
240 | @@ -25,5 +25,5 @@ |
241 | from .directory import FormulaDirectory |
242 | return FormulaDirectory(specification) |
243 | |
244 | - raise FormulaError(specification, "Unable to process %s into a formula" % ( |
245 | + raise FormulaError(specification, "unable to process %s into a formula" % ( |
246 | specification)) |
247 | |
248 | === modified file 'ensemble/formula/tests/test_bundle.py' |
249 | --- ensemble/formula/tests/test_bundle.py 2011-05-06 04:45:17 +0000 |
250 | +++ ensemble/formula/tests/test_bundle.py 2011-08-26 13:09:07 +0000 |
251 | @@ -38,8 +38,10 @@ |
252 | try: |
253 | FormulaBundle(filename) |
254 | except FormulaError, exc: |
255 | - self.assertIn(filename, str(exc)) |
256 | - self.assertIn("zip", str(exc).lower()) |
257 | + self.assertEquals( |
258 | + str(exc), |
259 | + ("Error processing %r: " % filename) + |
260 | + "must be a zip file (File is not a zip file)") |
261 | return |
262 | |
263 | self.fail("Expected formula error.") |
264 | @@ -53,8 +55,10 @@ |
265 | try: |
266 | FormulaBundle(filename) |
267 | except FormulaError, exc: |
268 | - self.assertIn(filename, str(exc)) |
269 | - self.assertIn("metadata.yaml", str(exc).lower()) |
270 | + self.assertEquals( |
271 | + str(exc), |
272 | + ("Error processing %r: " % filename) + |
273 | + "archive does not contain required file \"metadata.yaml\"") |
274 | return |
275 | |
276 | self.fail("Expected formula error.") |
277 | |
278 | === modified file 'ensemble/formula/tests/test_config.py' |
279 | --- ensemble/formula/tests/test_config.py 2011-08-24 00:18:27 +0000 |
280 | +++ ensemble/formula/tests/test_config.py 2011-08-26 13:09:07 +0000 |
281 | @@ -102,7 +102,7 @@ |
282 | |
283 | error = self.assertRaises(ServiceConfigError, |
284 | self.config.validate, dict(username="1234")) |
285 | - self.assertIn(str(error), "Invalid value for username: 1234") |
286 | + self.assertEquals(str(error), "Invalid value for username: 1234") |
287 | |
288 | data = self.config.validate(dict(username="12345678")) |
289 | self.assertEqual(data, {"username": "12345678", "title": "My Title"}) |
290 | @@ -114,15 +114,17 @@ |
291 | error = self.assertRaises(ServiceConfigError, |
292 | self.config.parse, |
293 | local_configuration) |
294 | - self.assertIn("options.fails.validator: required value not found", |
295 | - str(error)) |
296 | + self.assertEquals( |
297 | + str(error), |
298 | + "Invalid options specification: options.fails.validator: " |
299 | + "required value not found") |
300 | |
301 | def test_validate_types(self): |
302 | self.config.parse(sample_configuration) |
303 | |
304 | error = self.assertRaises(ServiceConfigError, |
305 | self.config.validate, {"skill-level": "NaN"}) |
306 | - self.assertIn(str(error), "Invalid value for skill-level: NaN") |
307 | + self.assertEquals(str(error), "Invalid value for skill-level: NaN") |
308 | |
309 | data = self.config.validate({"skill-level": "9001"}) |
310 | # its over 9000! |
311 | |
312 | === modified file 'ensemble/formula/tests/test_repository.py' |
313 | --- ensemble/formula/tests/test_repository.py 2011-07-15 12:41:01 +0000 |
314 | +++ ensemble/formula/tests/test_repository.py 2011-08-26 13:09:07 +0000 |
315 | @@ -85,11 +85,11 @@ |
316 | self.assertTrue(formula.metadata) |
317 | |
318 | def test_find_formula_by_name_fails(self): |
319 | - exc = self.assertRaises( |
320 | + error = self.assertRaises( |
321 | FormulaNotFound, self.repository1.find, "missing") |
322 | - self.assertIn("missing", str(exc)) |
323 | - self.assertIn(self.unbundled_repo_path, str(exc), |
324 | - "Repository path not found in the error message.") |
325 | + self.assertEquals(str(error), |
326 | + "Formula 'missing' not found in repository %s" |
327 | + % self.unbundled_repo_path) |
328 | |
329 | def test_find_formula_with_multiple_versions_returns_latest(self): |
330 | # Copy the repository out of the codebase, so that we can hack it. |
331 | |
332 | === modified file 'ensemble/hooks/tests/test_arguments.py' |
333 | --- ensemble/hooks/tests/test_arguments.py 2011-06-11 02:37:35 +0000 |
334 | +++ ensemble/hooks/tests/test_arguments.py 2011-08-26 13:09:07 +0000 |
335 | @@ -52,8 +52,8 @@ |
336 | err = self.capture_stream("stderr") |
337 | os.environ.pop("ENSEMBLE_AGENT_SOCKET", None) |
338 | error = self.failUnlessRaises(SystemExit, cli.parse_args) |
339 | - self.assertEquals("No ENSEMBLE_AGENT_SOCKET/-s option found", |
340 | - str(error)) |
341 | + self.assertEquals(str(error), |
342 | + "No ENSEMBLE_AGENT_SOCKET/-s option found") |
343 | self.assertIn("No ENSEMBLE_AGENT_SOCKET/-s option found", |
344 | err.getvalue()) |
345 | |
346 | |
347 | === modified file 'ensemble/hooks/tests/test_cli.py' |
348 | --- ensemble/hooks/tests/test_cli.py 2011-06-29 09:02:33 +0000 |
349 | +++ ensemble/hooks/tests/test_cli.py 2011-08-26 13:09:07 +0000 |
350 | @@ -335,12 +335,15 @@ |
351 | # and check an error condition |
352 | options = ["content=@missing"] |
353 | error = self.assertRaises(EnsembleError, parse_keyvalue_pairs, options) |
354 | - self.assertIn("No such file", str(error)) |
355 | + self.assertEquals( |
356 | + str(error), |
357 | + "No such file or directory: missing (argument:content)") |
358 | |
359 | # and check when fed non-kvpairs the error makes sense |
360 | options = ["foobar"] |
361 | error = self.assertRaises(EnsembleError, parse_keyvalue_pairs, options) |
362 | - self.assertIn("Expected `option=value`. Found `foobar`", str(error)) |
363 | + self.assertEquals( |
364 | + str(error), "Expected `option=value`. Found `foobar`") |
365 | |
366 | def test_parse_port_protocol(self): |
367 | self.assertEqual(parse_port_protocol("80"), (80, "tcp")) |
368 | |
369 | === modified file 'ensemble/hooks/tests/test_executor.py' |
370 | --- ensemble/hooks/tests/test_executor.py 2011-07-06 18:00:07 +0000 |
371 | +++ ensemble/hooks/tests/test_executor.py 2011-08-26 13:09:07 +0000 |
372 | @@ -326,7 +326,7 @@ |
373 | error = yield self.assertFailure( |
374 | self._executor.run_priority_hook(invoke, "foobar"), |
375 | AssertionError) |
376 | - self.assertIn("Executor must not be running", str(error)) |
377 | + self.assertEquals(str(error), "Executor must not be running") |
378 | |
379 | @inlineCallbacks |
380 | def test_prioritize_with_queued(self): |
381 | |
382 | === modified file 'ensemble/hooks/tests/test_invoker.py' |
383 | --- ensemble/hooks/tests/test_invoker.py 2011-08-16 17:26:09 +0000 |
384 | +++ ensemble/hooks/tests/test_invoker.py 2011-08-26 13:09:07 +0000 |
385 | @@ -480,7 +480,7 @@ |
386 | error = self.failUnlessRaises(errors.FormulaError, exe, hook) |
387 | |
388 | self.assertEqual(error.path, hook) |
389 | - self.assertIn(error.message, "hook is not executable") |
390 | + self.assertEqual(error.message, "hook is not executable") |
391 | |
392 | @defer.inlineCallbacks |
393 | def test_spawn_success(self): |
394 | |
395 | === modified file 'ensemble/lib/tests/test_schema.py' |
396 | --- ensemble/lib/tests/test_schema.py 2011-07-27 08:54:46 +0000 |
397 | +++ ensemble/lib/tests/test_schema.py 2011-08-26 13:09:07 +0000 |
398 | @@ -183,7 +183,7 @@ |
399 | exp = "([a-" |
400 | error = self.assertRaises( |
401 | SchemaError, Regex().coerce, exp, PATH) |
402 | - self.assertIn("expected regex", str(error)) |
403 | + self.assertEquals(str(error), "<path>: expected regex, got '([a-'") |
404 | |
405 | |
406 | class ListTest(TestCase): |
407 | |
408 | === modified file 'ensemble/machine/tests/test_unit_deployment.py' |
409 | --- ensemble/machine/tests/test_unit_deployment.py 2011-02-15 15:15:16 +0000 |
410 | +++ ensemble/machine/tests/test_unit_deployment.py 2011-08-26 13:09:07 +0000 |
411 | @@ -115,8 +115,8 @@ |
412 | |
413 | self.failUnlessFailure(d, UnitDeploymentError) |
414 | |
415 | - def validate_result(result): |
416 | - self.assertIn("No module named magichat", str(result)) |
417 | + def validate_result(error): |
418 | + self.assertIn("No module named magichat", str(error)) |
419 | |
420 | d.addCallback(validate_result) |
421 | return d |
422 | @@ -170,7 +170,7 @@ |
423 | error = self.assertRaises( |
424 | UnitDeploymentError, |
425 | self.deployment.start, "0", get_test_zookeeper_address()) |
426 | - self.assertIn("Formula must be unpacked first.", str(error)) |
427 | + self.assertEquals(str(error), "Formula must be unpacked first.") |
428 | |
429 | @inlineCallbacks |
430 | def test_service_unit_destroy(self): |
431 | @@ -269,7 +269,9 @@ |
432 | error = self.assertRaises( |
433 | UnitDeploymentError, |
434 | self.deployment.unpack_formula, self.formula) |
435 | - self.assertIn("Invalid formula for deployment:", str(error)) |
436 | + self.assertEquals( |
437 | + str(error), |
438 | + "Invalid formula for deployment: %s" % self.formula.path) |
439 | |
440 | def test_is_running_no_pid_file(self): |
441 | """ |
442 | |
443 | === modified file 'ensemble/machine/unit.py' |
444 | --- ensemble/machine/unit.py 2011-02-15 15:15:16 +0000 |
445 | +++ ensemble/machine/unit.py 2011-08-26 13:09:07 +0000 |
446 | @@ -123,6 +123,6 @@ |
447 | """Unpack a formula to the service units directory.""" |
448 | if not isinstance(formula, FormulaBundle): |
449 | raise UnitDeploymentError( |
450 | - "Invalid formula for deployment: %r" % formula) |
451 | + "Invalid formula for deployment: %s" % formula.path) |
452 | |
453 | formula.extract_to(os.path.join(self.directory, "formula")) |
454 | |
455 | === modified file 'ensemble/providers/common/connect.py' |
456 | --- ensemble/providers/common/connect.py 2011-08-23 08:42:30 +0000 |
457 | +++ ensemble/providers/common/connect.py 2011-08-26 13:09:07 +0000 |
458 | @@ -36,7 +36,7 @@ |
459 | for machine in machines: |
460 | if machine.dns_name: |
461 | return machine |
462 | - raise EnvironmentPending("no machines have addresses assigned yet") |
463 | + raise EnvironmentPending("No machines have addresses assigned yet") |
464 | |
465 | def _connect_to_machine(self, machine, share): |
466 | log.info("Connecting to environment.") |
467 | @@ -48,7 +48,7 @@ |
468 | def _cannot_connect(self, failure, machine): |
469 | failure.trap(NoConnection, ConnectionTimeoutException) |
470 | raise EnvironmentPending( |
471 | - "machine %s may not be ready (%s)" |
472 | + "Cannot connect to machine %s (perhaps still initializing): %s" |
473 | % (machine.instance_id, str(failure.value))) |
474 | |
475 | @inlineCallbacks |
476 | |
477 | === modified file 'ensemble/providers/common/findzookeepers.py' |
478 | --- ensemble/providers/common/findzookeepers.py 2011-08-16 13:24:41 +0000 |
479 | +++ ensemble/providers/common/findzookeepers.py 2011-08-26 13:09:07 +0000 |
480 | @@ -26,5 +26,5 @@ |
481 | |
482 | if machines: |
483 | returnValue(machines) |
484 | - raise EnvironmentNotFound("machines are not running (%s)." |
485 | + raise EnvironmentNotFound("machines are not running (%s)" |
486 | % ", ".join(missing_instance_ids)) |
487 | |
488 | === modified file 'ensemble/providers/common/tests/test_connect.py' |
489 | --- ensemble/providers/common/tests/test_connect.py 2011-08-19 16:12:22 +0000 |
490 | +++ ensemble/providers/common/tests/test_connect.py 2011-08-26 13:09:07 +0000 |
491 | @@ -59,9 +59,7 @@ |
492 | |
493 | def check(error): |
494 | self.assertEquals( |
495 | - str(error), |
496 | - "Ensemble environment is not accessible: no machines have " |
497 | - "addresses assigned yet") |
498 | + str(error), "No machines have addresses assigned yet") |
499 | d.addCallback(check) |
500 | return d |
501 | |
502 | @@ -85,8 +83,8 @@ |
503 | """`NoConnection` errors should become `EnvironmentPending`s""" |
504 | return self.assert_connect_error( |
505 | NoConnection("KABOOM!"), EnvironmentPending, |
506 | - "Ensemble environment is not accessible: machine i-exist may " |
507 | - "not be ready (KABOOM!)") |
508 | + "Cannot connect to machine i-exist (perhaps still initializing): " |
509 | + "KABOOM!") |
510 | |
511 | def test_txzookeeper_error(self): |
512 | """ |
513 | @@ -94,8 +92,8 @@ |
514 | """ |
515 | return self.assert_connect_error( |
516 | ConnectionTimeoutException("SPLAT!"), EnvironmentPending, |
517 | - "Ensemble environment is not accessible: machine i-exist may " |
518 | - "not be ready (SPLAT!)") |
519 | + "Cannot connect to machine i-exist (perhaps still initializing): " |
520 | + "SPLAT!") |
521 | |
522 | def test_other_error(self): |
523 | """Other errors should propagate""" |
524 | |
525 | === modified file 'ensemble/providers/common/tests/test_findzookeepers.py' |
526 | --- ensemble/providers/common/tests/test_findzookeepers.py 2011-08-16 13:44:01 +0000 |
527 | +++ ensemble/providers/common/tests/test_findzookeepers.py 2011-08-26 13:09:07 +0000 |
528 | @@ -37,12 +37,27 @@ |
529 | provider = self.get_provider(False) |
530 | d = provider.get_zookeeper_machines() |
531 | self.assertFailure(d, EnvironmentNotFound) |
532 | + |
533 | + def check(error): |
534 | + self.assertEquals( |
535 | + str(error), |
536 | + "Ensemble environment not found: is the environment " |
537 | + "bootstrapped?") |
538 | + d.addCallback(check) |
539 | return d |
540 | |
541 | def test_empty_state(self): |
542 | provider = self.get_provider({}) |
543 | d = provider.get_zookeeper_machines() |
544 | self.assertFailure(d, EnvironmentNotFound) |
545 | + |
546 | + def check(error): |
547 | + self.assertEquals( |
548 | + str(error), |
549 | + "Ensemble environment not found: is the environment " |
550 | + "bootstrapped?") |
551 | + d.addCallback(check) |
552 | + return d |
553 | return d |
554 | |
555 | def test_get_machine_error_aborts(self): |
556 | @@ -56,14 +71,24 @@ |
557 | self.assertFailure(d, SomeError) |
558 | return d |
559 | |
560 | - def test_bad_machine(self): |
561 | - provider = self.get_provider({"zookeeper-instances": ["porter"]}) |
562 | + def test_bad_machines(self): |
563 | + provider = self.get_provider({ |
564 | + "zookeeper-instances": ["porter", "carter"]}) |
565 | provider.get_machines(["porter"]) |
566 | self.mocker.result(fail(MachinesNotFound(["porter"]))) |
567 | + provider.get_machines(["carter"]) |
568 | + self.mocker.result(fail(MachinesNotFound(["carter"]))) |
569 | self.mocker.replay() |
570 | |
571 | d = provider.get_zookeeper_machines() |
572 | self.assertFailure(d, EnvironmentNotFound) |
573 | + |
574 | + def check(error): |
575 | + self.assertEquals( |
576 | + str(error), |
577 | + "Ensemble environment not found: machines are not running " |
578 | + "(porter, carter)") |
579 | + d.addCallback(check) |
580 | return d |
581 | |
582 | def test_good_machine(self): |
583 | |
584 | === modified file 'ensemble/providers/common/tests/test_utils.py' |
585 | --- ensemble/providers/common/tests/test_utils.py 2011-08-11 22:04:38 +0000 |
586 | +++ ensemble/providers/common/tests/test_utils.py 2011-08-26 13:09:07 +0000 |
587 | @@ -63,9 +63,7 @@ |
588 | OSError("Bad")) |
589 | self.assertInstance(error, EnsembleError) |
590 | self.assertEqual( |
591 | - str(error), |
592 | - "ProviderError: Interaction with machine provider failed: " |
593 | - "OSError('Bad',)") |
594 | + str(error), "Unexpected OSError interacting with provider: Bad") |
595 | |
596 | def test_convert_unknown_error_ignores_ensemble_error(self): |
597 | error = self.assertRaises( |
598 | @@ -87,8 +85,7 @@ |
599 | self.assertInstance(failure.value, ProviderInteractionError) |
600 | self.assertEqual( |
601 | str(failure.value), |
602 | - "ProviderError: Interaction with machine provider failed: " |
603 | - "OSError('Bad',)") |
604 | + "Unexpected OSError interacting with provider: Bad") |
605 | |
606 | |
607 | class FormatCloudInitTest(TestCase): |
608 | |
609 | === modified file 'ensemble/providers/common/utils.py' |
610 | --- ensemble/providers/common/utils.py 2011-08-03 15:53:38 +0000 |
611 | +++ ensemble/providers/common/utils.py 2011-08-26 13:09:07 +0000 |
612 | @@ -31,7 +31,9 @@ |
613 | error = failure |
614 | |
615 | if not isinstance(error, EnsembleError): |
616 | - error = ProviderInteractionError(error) |
617 | + message = ("Unexpected %s interacting with provider: %s" |
618 | + % (type(error).__name__, str(error))) |
619 | + error = ProviderInteractionError(message) |
620 | |
621 | if isinstance(failure, Failure): |
622 | return Failure(error) |
623 | |
624 | === modified file 'ensemble/providers/ec2/__init__.py' |
625 | --- ensemble/providers/ec2/__init__.py 2011-08-25 16:41:03 +0000 |
626 | +++ ensemble/providers/ec2/__init__.py 2011-08-26 13:09:07 +0000 |
627 | @@ -77,13 +77,14 @@ |
628 | instances = yield self.ec2.describe_instances(*instance_ids) |
629 | except EC2Error as error: |
630 | code = error.get_error_codes() |
631 | + message = error.get_error_messages() |
632 | if code == "InvalidInstanceID.NotFound": |
633 | message = error.get_error_messages() |
634 | raise MachinesNotFound( |
635 | re.findall(r"\bi-[0-9a-f]{3,15}\b", message)) |
636 | raise ProviderInteractionError( |
637 | - "EC2 error when looking up instances %s: %s" |
638 | - % (", ".join(instance_ids), error.get_error_messages())) |
639 | + "Unexpected EC2Error getting machines %s: %s" |
640 | + % (", ".join(instance_ids), message)) |
641 | |
642 | machines = [] |
643 | for instance in instances: |
644 | |
645 | === modified file 'ensemble/providers/ec2/launch.py' |
646 | --- ensemble/providers/ec2/launch.py 2011-08-10 17:23:06 +0000 |
647 | +++ ensemble/providers/ec2/launch.py 2011-08-26 13:09:07 +0000 |
648 | @@ -123,9 +123,8 @@ |
649 | log.debug("Cannot delete security group %s: %s", |
650 | ensemble_machine_group, e) |
651 | raise ProviderInteractionError( |
652 | - "EC2 error when attempting to delete " |
653 | - "security group %s: %s" % ( |
654 | - ensemble_machine_group, e)) |
655 | + "Unexpected EC2Error deleting security group %s: %s" |
656 | + % (ensemble_machine_group, e.get_error_messages())) |
657 | log.debug("Creating ensemble machine security group %s", |
658 | ensemble_machine_group) |
659 | yield self._provider.ec2.create_security_group( |
660 | |
661 | === modified file 'ensemble/providers/ec2/securitygroup.py' |
662 | --- ensemble/providers/ec2/securitygroup.py 2011-08-10 17:23:06 +0000 |
663 | +++ ensemble/providers/ec2/securitygroup.py 2011-08-26 13:09:07 +0000 |
664 | @@ -38,8 +38,8 @@ |
665 | port, protocol, machine.instance_id) |
666 | except EC2Error, e: |
667 | raise ProviderInteractionError( |
668 | - "EC2 error when attempting to open %s/%s on machine %s: %s" % ( |
669 | - port, protocol, machine.instance_id, e)) |
670 | + "Unexpected EC2Error opening %s/%s on machine %s: %s" |
671 | + % (port, protocol, machine.instance_id, e.get_error_messages())) |
672 | |
673 | |
674 | @inlineCallbacks |
675 | @@ -55,8 +55,8 @@ |
676 | port, protocol, machine.instance_id) |
677 | except EC2Error, e: |
678 | raise ProviderInteractionError( |
679 | - "EC2 error when attempting to close %s/%s on machine %s: %s" % ( |
680 | - port, protocol, machine.instance_id, e)) |
681 | + "Unexpected EC2Error closing %s/%s on machine %s: %s" |
682 | + % (port, protocol, machine.instance_id, e.get_error_messages())) |
683 | |
684 | |
685 | @inlineCallbacks |
686 | @@ -72,9 +72,8 @@ |
687 | _get_machine_group_name(provider, machine_id)) |
688 | except EC2Error, e: |
689 | raise ProviderInteractionError( |
690 | - "EC2 error when attempting to get opened ports " |
691 | - "on machine %s: %s" % ( |
692 | - machine.instance_id, e)) |
693 | + "Unexpected EC2Error getting open ports on machine %s: %s" |
694 | + % (machine.instance_id, e.get_error_messages())) |
695 | |
696 | opened_ports = set() # made up of (port, protocol) pairs |
697 | for ip_permission in security_groups[0].allowed_ips: |
698 | |
699 | === modified file 'ensemble/providers/ec2/tests/common.py' |
700 | --- ensemble/providers/ec2/tests/common.py 2011-08-22 23:03:03 +0000 |
701 | +++ ensemble/providers/ec2/tests/common.py 2011-08-26 13:09:07 +0000 |
702 | @@ -59,16 +59,6 @@ |
703 | "<error><Code>1</Code><Message>%s</Message></error>" % message, |
704 | code) |
705 | |
706 | - def get_wrapped_ec2_error_text(self, entity_id, reason, |
707 | - format="The instance ID %r does not exist"): |
708 | - """By convention, `EC2Error` is wrapped as a `ProviderError`""" |
709 | - message = format % entity_id |
710 | - return ( |
711 | - "ProviderError: Interaction with machine provider failed: " |
712 | - "\"EC2 error when attempting to %s %s: " |
713 | - "Error Message: %s\"" % ( |
714 | - reason, entity_id, message)) |
715 | - |
716 | def setUp(self): |
717 | # mock out the aws services |
718 | service_factory = self.mocker.replace( |
719 | |
720 | === modified file 'ensemble/providers/ec2/tests/test_connect.py' |
721 | --- ensemble/providers/ec2/tests/test_connect.py 2011-08-19 16:12:22 +0000 |
722 | +++ ensemble/providers/ec2/tests/test_connect.py 2011-08-26 13:09:07 +0000 |
723 | @@ -59,9 +59,7 @@ |
724 | |
725 | def check_error(error): |
726 | self.assertEqual( |
727 | - str(error), |
728 | - "Ensemble environment is not accessible: no machines have " |
729 | - "addresses assigned yet") |
730 | + str(error), "No machines have addresses assigned yet") |
731 | |
732 | self.assertFailure(d, NoConnection) |
733 | d.addCallback(check_error) |
734 | @@ -89,8 +87,8 @@ |
735 | """`NoConnection` errors should become `EnvironmentPending`s""" |
736 | return self.assert_connect_error( |
737 | NoConnection("KABOOM!"), EnvironmentPending, |
738 | - "Ensemble environment is not accessible: machine i-foobar may " |
739 | - "not be ready (KABOOM!)") |
740 | + "Cannot connect to machine i-foobar (perhaps still initializing): " |
741 | + "KABOOM!") |
742 | |
743 | def test_txzookeeper_error(self): |
744 | """ |
745 | @@ -98,8 +96,8 @@ |
746 | """ |
747 | return self.assert_connect_error( |
748 | ConnectionTimeoutException("SPLAT!"), EnvironmentPending, |
749 | - "Ensemble environment is not accessible: machine i-foobar may " |
750 | - "not be ready (SPLAT!)") |
751 | + "Cannot connect to machine i-foobar (perhaps still initializing): " |
752 | + "SPLAT!") |
753 | |
754 | def test_other_error(self): |
755 | """Other errors should propagate""" |
756 | |
757 | === modified file 'ensemble/providers/ec2/tests/test_getmachines.py' |
758 | --- ensemble/providers/ec2/tests/test_getmachines.py 2011-08-17 08:58:51 +0000 |
759 | +++ ensemble/providers/ec2/tests/test_getmachines.py 2011-08-26 13:09:07 +0000 |
760 | @@ -132,9 +132,8 @@ |
761 | def verify(error): |
762 | self.assertEquals( |
763 | str(error), |
764 | - "ProviderError: Interaction with machine provider failed: " |
765 | - "\"EC2 error when looking up instances i-brokeit, i-msorry: " |
766 | - "unhelpful noises ('splat! kerpow!')\"") |
767 | + "Unexpected EC2Error getting machines i-brokeit, i-msorry: " |
768 | + "unhelpful noises ('splat! kerpow!')") |
769 | d.addCallback(verify) |
770 | return d |
771 | |
772 | |
773 | === modified file 'ensemble/providers/ec2/tests/test_launch.py' |
774 | --- ensemble/providers/ec2/tests/test_launch.py 2011-08-16 13:55:11 +0000 |
775 | +++ ensemble/providers/ec2/tests/test_launch.py 2011-08-26 13:09:07 +0000 |
776 | @@ -141,10 +141,9 @@ |
777 | ProviderInteractionError) |
778 | self.assertEqual( |
779 | str(ex), |
780 | - self.get_wrapped_ec2_error_text( |
781 | - "ensemble-moon-machine-1", "delete security group", |
782 | - "There are active instances using security group %r" |
783 | - )) |
784 | + "Unexpected EC2Error deleting security group " |
785 | + "ensemble-moon-machine-1: There are active instances using " |
786 | + "security group 'ensemble-moon-machine-1'") |
787 | |
788 | def test_provider_type_machine_variable(self): |
789 | """The provider type is available via cloud-init.""" |
790 | |
791 | === modified file 'ensemble/providers/ec2/tests/test_provider.py' |
792 | --- ensemble/providers/ec2/tests/test_provider.py 2011-08-11 19:49:32 +0000 |
793 | +++ ensemble/providers/ec2/tests/test_provider.py 2011-08-26 13:09:07 +0000 |
794 | @@ -127,4 +127,7 @@ |
795 | config["authorized-keys-path"] = "File path" |
796 | error = self.assertRaises(EnvironmentsConfigError, |
797 | MachineProvider, "some-env-name", config) |
798 | - self.assertIn("authorized-keys", str(error)) |
799 | + self.assertEquals( |
800 | + str(error), |
801 | + "Environment config cannot define both authorized-keys and " |
802 | + "authorized-keys-path. Pick one!") |
803 | |
804 | === modified file 'ensemble/providers/ec2/tests/test_securitygroup.py' |
805 | --- ensemble/providers/ec2/tests/test_securitygroup.py 2011-08-10 17:19:09 +0000 |
806 | +++ ensemble/providers/ec2/tests/test_securitygroup.py 2011-08-26 13:09:07 +0000 |
807 | @@ -89,8 +89,8 @@ |
808 | ProviderInteractionError) |
809 | self.assertEqual( |
810 | str(ex), |
811 | - self.get_wrapped_ec2_error_text( |
812 | - "i-foobar", "open 80/tcp on machine")) |
813 | + "Unexpected EC2Error opening 80/tcp on machine i-foobar: " |
814 | + "The instance ID 'i-foobar' does not exist") |
815 | |
816 | @inlineCallbacks |
817 | def test_close_provider_port_unknown_instance(self): |
818 | @@ -108,8 +108,8 @@ |
819 | ProviderInteractionError) |
820 | self.assertEqual( |
821 | str(ex), |
822 | - self.get_wrapped_ec2_error_text( |
823 | - "i-foobar", "close 80/tcp on machine")) |
824 | + "Unexpected EC2Error closing 80/tcp on machine i-foobar: " |
825 | + "The instance ID 'i-foobar' does not exist") |
826 | |
827 | @inlineCallbacks |
828 | def test_get_provider_opened_ports_unknown_instance(self): |
829 | @@ -125,5 +125,5 @@ |
830 | ProviderInteractionError) |
831 | self.assertEqual( |
832 | str(ex), |
833 | - self.get_wrapped_ec2_error_text( |
834 | - "i-foobar", "get opened ports on machine")) |
835 | + "Unexpected EC2Error getting open ports on machine i-foobar: " |
836 | + "The instance ID 'i-foobar' does not exist") |
837 | |
838 | === modified file 'ensemble/providers/orchestra/cobbler.py' |
839 | --- ensemble/providers/orchestra/cobbler.py 2011-08-22 13:01:02 +0000 |
840 | +++ ensemble/providers/orchestra/cobbler.py 2011-08-26 13:09:07 +0000 |
841 | @@ -3,8 +3,7 @@ |
842 | from twisted.internet.defer import inlineCallbacks, returnValue |
843 | from twisted.web.xmlrpc import Proxy |
844 | |
845 | -from ensemble.errors import ( |
846 | - MachinesNotFound, ProviderError, ProviderInteractionError) |
847 | +from ensemble.errors import MachinesNotFound, ProviderError |
848 | |
849 | |
850 | class CobblerCaller(object): |
851 | @@ -62,7 +61,7 @@ |
852 | def check(actual): |
853 | if actual != expect: |
854 | raise ProviderError( |
855 | - "Bad result from call to %s with %s (got %r, expected %r)" |
856 | + "Bad result from call to %s with %s: got %r, expected %r" |
857 | % (name, args, actual, expect)) |
858 | return actual |
859 | |
860 | @@ -84,8 +83,9 @@ |
861 | |
862 | def extract_name(systems): |
863 | if len(systems) > 1: |
864 | - raise ProviderInteractionError( |
865 | - "Got multiple instances with id %s" % instance_id) |
866 | + raise ProviderError( |
867 | + "Got multiple names for machine %s: %s" |
868 | + % (instance_id, ", ".join(systems))) |
869 | if not systems: |
870 | raise MachinesNotFound([instance_id]) |
871 | return systems[0] |
872 | |
873 | === modified file 'ensemble/providers/orchestra/tests/test_cobbler.py' |
874 | --- ensemble/providers/orchestra/tests/test_cobbler.py 2011-08-12 21:35:34 +0000 |
875 | +++ ensemble/providers/orchestra/tests/test_cobbler.py 2011-08-26 13:09:07 +0000 |
876 | @@ -3,8 +3,7 @@ |
877 | from twisted.internet.defer import fail, succeed |
878 | from twisted.web.xmlrpc import Proxy |
879 | |
880 | -from ensemble.errors import ( |
881 | - MachinesNotFound, ProviderError, ProviderInteractionError) |
882 | +from ensemble.errors import MachinesNotFound, ProviderError |
883 | from ensemble.lib.testing import TestCase |
884 | from ensemble.providers.orchestra.cobbler import CobblerCaller, CobblerClient |
885 | |
886 | @@ -243,11 +242,14 @@ |
887 | d.addCallback(verify) |
888 | return d |
889 | |
890 | - def check_bad_call(self, d, method): |
891 | + def check_bad_call(self, d, method, args): |
892 | self.assertFailure(d, ProviderError) |
893 | |
894 | def verify(error): |
895 | - self.assertIn("Bad result from call to %s" % method, str(error)) |
896 | + self.assertEquals( |
897 | + str(error), |
898 | + "Bad result from call to %s with %s: got False, expected True" |
899 | + % (method, args)) |
900 | d.addCallback(verify) |
901 | return d |
902 | |
903 | @@ -261,11 +263,13 @@ |
904 | self.mock_get_name(succeed(["some-name", "another-name"])) |
905 | self.mocker.replay() |
906 | d = self.call_cobbler_method(method_name, *args) |
907 | - self.assertFailure(d, ProviderInteractionError) |
908 | + self.assertFailure(d, ProviderError) |
909 | |
910 | def check_error(error): |
911 | - self.assertIn("Got multiple instances with id some-uid", |
912 | - str(error)) |
913 | + self.assertEquals( |
914 | + str(error), |
915 | + "Got multiple names for machine some-uid: some-name, " |
916 | + "another-name") |
917 | d.addCallback(check_error) |
918 | return d |
919 | |
920 | @@ -301,7 +305,8 @@ |
921 | self.mock_modify_system(succeed(False), key, value) |
922 | self.mocker.replay() |
923 | d = self.call_cobbler_method(method_name, *args) |
924 | - return self.check_bad_call(d, "modify_system") |
925 | + return self.check_bad_call( |
926 | + d, "modify_system", ("some-handle", key, value)) |
927 | |
928 | def check_modify_system_error(self, key, value, method_name, *args): |
929 | self.mock_modify_system(fail(SomeError()), key, value) |
930 | @@ -314,7 +319,7 @@ |
931 | self.mock_save_system(succeed(False)) |
932 | self.mocker.replay() |
933 | d = self.call_cobbler_method(method_name, *args) |
934 | - return self.check_bad_call(d, "save_system") |
935 | + return self.check_bad_call(d, "save_system", ("some-handle",)) |
936 | |
937 | def check_save_system_error(self, method_name, *args): |
938 | self.mock_save_system(fail(SomeError())) |
939 | |
940 | === modified file 'ensemble/providers/orchestra/tests/test_connect.py' |
941 | --- ensemble/providers/orchestra/tests/test_connect.py 2011-08-19 16:12:22 +0000 |
942 | +++ ensemble/providers/orchestra/tests/test_connect.py 2011-08-26 13:09:07 +0000 |
943 | @@ -62,8 +62,8 @@ |
944 | """`NoConnection` errors should become `EnvironmentPending`s""" |
945 | return self.assert_connect_error( |
946 | NoConnection("KABOOM!"), EnvironmentPending, |
947 | - "Ensemble environment is not accessible: machine foo may not be " |
948 | - "ready (KABOOM!)") |
949 | + "Cannot connect to machine foo (perhaps still initializing): " |
950 | + "KABOOM!") |
951 | |
952 | def test_txzookeeper_error(self): |
953 | """ |
954 | @@ -71,8 +71,8 @@ |
955 | """ |
956 | return self.assert_connect_error( |
957 | ConnectionTimeoutException("SPLAT!"), EnvironmentPending, |
958 | - "Ensemble environment is not accessible: machine foo may not be " |
959 | - "ready (SPLAT!)") |
960 | + "Cannot connect to machine foo (perhaps still initializing): " |
961 | + "SPLAT!") |
962 | |
963 | def test_other_error(self): |
964 | """Other errors should propagate""" |
965 | |
966 | === modified file 'ensemble/providers/orchestra/tests/test_launch.py' |
967 | --- ensemble/providers/orchestra/tests/test_launch.py 2011-08-11 23:27:45 +0000 |
968 | +++ ensemble/providers/orchestra/tests/test_launch.py 2011-08-26 13:09:07 +0000 |
969 | @@ -30,9 +30,13 @@ |
970 | |
971 | d = self.get_provider().start_machine({"machine-id": "32767"}) |
972 | self.assertFailure(d, ProviderError) |
973 | - self.assertIn("Could not find any Cobbler systems marked as available " |
974 | - "and configured for network boot.", |
975 | - str(d)) |
976 | + |
977 | + def verify(error): |
978 | + self.assertEquals( |
979 | + str(error), |
980 | + "Could not find any Cobbler systems marked as available and " |
981 | + "configured for network boot.") |
982 | + d.addCallback(verify) |
983 | return d |
984 | |
985 | def test_no_acceptable_machines(self): |
986 | @@ -43,9 +47,13 @@ |
987 | |
988 | d = self.get_provider().start_machine({"machine-id": "32767"}) |
989 | self.assertFailure(d, ProviderError) |
990 | - self.assertIn("All available Cobbler systems were also marked as " |
991 | - "acquired (instances: bad-system-uid).", |
992 | - str(d)) |
993 | + |
994 | + def verify(error): |
995 | + self.assertEquals( |
996 | + str(error), |
997 | + "All available Cobbler systems were also marked as acquired " |
998 | + "(instances: bad-system-uid).") |
999 | + d.addCallback(verify) |
1000 | return d |
1001 | |
1002 | def test_actually_launch(self): |
1003 | |
1004 | === modified file 'ensemble/providers/orchestra/tests/test_provider.py' |
1005 | --- ensemble/providers/orchestra/tests/test_provider.py 2011-08-23 16:07:09 +0000 |
1006 | +++ ensemble/providers/orchestra/tests/test_provider.py 2011-08-26 13:09:07 +0000 |
1007 | @@ -53,7 +53,10 @@ |
1008 | config["authorized-keys-path"] = "File path" |
1009 | error = self.assertRaises(EnvironmentsConfigError, |
1010 | MachineProvider, "some-env-name", config) |
1011 | - self.assertIn("authorized-keys", str(error)) |
1012 | + self.assertEquals( |
1013 | + str(error), |
1014 | + "Environment config cannot define both authorized-keys and " |
1015 | + "authorized-keys-path. Pick one!") |
1016 | |
1017 | @inlineCallbacks |
1018 | def test_open_port(self): |
1019 | @@ -75,4 +78,3 @@ |
1020 | yield MachineProvider("blah", CONFIG).get_opened_ports(None, None) |
1021 | self.assertIn( |
1022 | "Firewalling is not yet implemented in Orchestra", log.getvalue()) |
1023 | - |
1024 | |
1025 | === modified file 'ensemble/state/service.py' |
1026 | --- ensemble/state/service.py 2011-08-08 03:22:15 +0000 |
1027 | +++ ensemble/state/service.py 2011-08-26 13:09:07 +0000 |
1028 | @@ -855,7 +855,8 @@ |
1029 | cleared. |
1030 | """ |
1031 | if not isinstance(hook_names, (list, tuple)): |
1032 | - raise AssertionError("hook names must be a list %r" % hook_names) |
1033 | + raise AssertionError("Hook names must be a list: got %r" |
1034 | + % hook_names) |
1035 | |
1036 | if "*" in hook_names and len(hook_names) > 1: |
1037 | msg = "Ambigious to debug all hooks and named hooks %r" % ( |
1038 | |
1039 | === modified file 'ensemble/state/tests/test_security.py' |
1040 | --- ensemble/state/tests/test_security.py 2011-08-05 22:55:52 +0000 |
1041 | +++ ensemble/state/tests/test_security.py 2011-08-26 13:09:07 +0000 |
1042 | @@ -316,7 +316,7 @@ |
1043 | principal = Principal("zebra", "zoo") |
1044 | error = yield self.assertFailure(self.db.get(principal.name), |
1045 | PrincipalNotFound) |
1046 | - self.assertIn("zebra", str(error)) |
1047 | + self.assertEquals(str(error), "Principal 'zebra' not found") |
1048 | |
1049 | |
1050 | class PolicyTest(TestCase): |
1051 | |
1052 | === modified file 'ensemble/state/tests/test_service.py' |
1053 | --- ensemble/state/tests/test_service.py 2011-08-22 23:02:38 +0000 |
1054 | +++ ensemble/state/tests/test_service.py 2011-08-26 13:09:07 +0000 |
1055 | @@ -1111,8 +1111,10 @@ |
1056 | error = yield self.assertFailure( |
1057 | unit_state.enable_hook_debug(["*", "db-relation-changed"]), |
1058 | ValueError) |
1059 | - self.assertIn("Ambigious to debug all hooks and named hooks", |
1060 | - str(error)) |
1061 | + self.assertEquals( |
1062 | + str(error), |
1063 | + "Ambigious to debug all hooks and named hooks " |
1064 | + "['*', 'db-relation-changed']") |
1065 | |
1066 | @inlineCallbacks |
1067 | def test_enable_debug_requires_sequence(self): |
1068 | @@ -1123,7 +1125,7 @@ |
1069 | error = yield self.assertFailure( |
1070 | unit_state.enable_hook_debug(None), |
1071 | AssertionError) |
1072 | - self.assertIn("hook names must be a list", str(error)) |
1073 | + self.assertEquals(str(error), "Hook names must be a list: got None") |
1074 | |
1075 | @inlineCallbacks |
1076 | def test_enable_named_debug_hook(self): |
1077 | @@ -1144,8 +1146,10 @@ |
1078 | """ |
1079 | unit_state = yield self.get_unit_state() |
1080 | yield unit_state.enable_hook_debug(["*"]) |
1081 | - yield self.assertFailure(unit_state.enable_hook_debug(["*"]), |
1082 | - ServiceUnitDebugAlreadyEnabled) |
1083 | + error = yield self.assertFailure(unit_state.enable_hook_debug(["*"]), |
1084 | + ServiceUnitDebugAlreadyEnabled) |
1085 | + self.assertEquals( |
1086 | + str(error), "Service unit 'wordpress/0' is already in debug mode.") |
1087 | |
1088 | @inlineCallbacks |
1089 | def test_enable_debug_hook_lifetime(self): |
1090 | |
1091 | === modified file 'ensemble/tests/test_errors.py' |
1092 | --- ensemble/tests/test_errors.py 2011-08-17 17:26:19 +0000 |
1093 | +++ ensemble/tests/test_errors.py 2011-08-26 13:09:07 +0000 |
1094 | @@ -23,20 +23,20 @@ |
1095 | |
1096 | def test_FileNotFound(self): |
1097 | error = FileNotFound("/path") |
1098 | - self.assertEquals(str(error), "File was not found: /path") |
1099 | + self.assertEquals(str(error), "File was not found: '/path'") |
1100 | self.assertIsEnsembleError(error) |
1101 | |
1102 | def test_FileAlreadyExists(self): |
1103 | error = FileAlreadyExists("/path") |
1104 | self.assertEquals(str(error), |
1105 | - "File already exists, won't overwrite: /path") |
1106 | + "File already exists, won't overwrite: '/path'") |
1107 | self.assertIsEnsembleError(error) |
1108 | |
1109 | def test_InvalidEnsembleHeaderValue(self): |
1110 | error = InvalidEnsembleHeaderValue("/path", "xpctd", "fnd") |
1111 | self.assertEquals(str(error), |
1112 | "Expected a YAML file with an 'ensemble: xpctd' " |
1113 | - "header, found 'ensemble: fnd': /path") |
1114 | + "header, found 'ensemble: fnd': '/path'") |
1115 | self.assertIsEnsembleError(error) |
1116 | |
1117 | def test_NoConnection(self): |
1118 | @@ -62,12 +62,9 @@ |
1119 | self.assertIsEnsembleError(error) |
1120 | |
1121 | def test_ProviderInteractionError(self): |
1122 | - error = ProviderInteractionError(OSError("Bad Stuff")) |
1123 | + error = ProviderInteractionError("Bad Stuff") |
1124 | self.assertIsEnsembleError(error) |
1125 | - self.assertEquals( |
1126 | - str(error), |
1127 | - "ProviderError: Interaction with machine provider failed: " |
1128 | - "OSError('Bad Stuff',)") |
1129 | + self.assertEquals(str(error), "Bad Stuff") |
1130 | |
1131 | def test_CannotTerminateMachine(self): |
1132 | error = CannotTerminateMachine(0, "environment would be destroyed") |
1133 | @@ -101,11 +98,4 @@ |
1134 | |
1135 | def testEnvironmentPendingWithInfo(self): |
1136 | error = EnvironmentPending("problem") |
1137 | - self.assertEquals(str(error), |
1138 | - "Ensemble environment is not accessible: problem") |
1139 | - |
1140 | - def testEnvironmentPendingNoInfo(self): |
1141 | - error = EnvironmentPending() |
1142 | - self.assertEquals(str(error), |
1143 | - "Ensemble environment is not accessible: no " |
1144 | - "details available") |
1145 | + self.assertEquals(str(error), "problem") |
1146 | |
1147 | === modified file 'ensemble/unit/tests/test_formula.py' |
1148 | --- ensemble/unit/tests/test_formula.py 2011-08-18 21:33:55 +0000 |
1149 | +++ ensemble/unit/tests/test_formula.py 2011-08-26 13:09:07 +0000 |
1150 | @@ -191,4 +191,5 @@ |
1151 | self.client, "local:mickey-21", formula_directory), |
1152 | FormulaStateNotFound) |
1153 | |
1154 | - self.assertIn("Formula 'local:mickey-21' was not found", str(error)) |
1155 | + self.assertEquals(str(error), |
1156 | + "Formula 'local:mickey-21' was not found") |
Looks generally good, with a few exceptions:
[1]
+ self.name = type(error) .__name_ _ if error else "error"
+ self.action = action or "interacting with provider"
+ self.message = message or str(error)
def __str__(self):
- return "ProviderError: Interaction with machine provider failed: %r" \
- % self.error
+ return "Unexpected %s %s: %s" % (self.name, self.action, self.message)
This sounds somewhat confusing, and quite likely to turn out
in bad error messages. There's no way to make sense, or to
introduce new messages, without looking at the implementation.
Improving on the original error message is good, but please
use something more straightforward here and in the call sites.
[2]
- raise FormulaError(path, "Must be a ZIP-file (%s)." % str(exc))
+ raise FormulaError(path, "must be a zip file (%s)" % exc)
Our error messages start with a capitalized word.
[3]
raise FormulaError(path, yaml\". ") yaml\"" )
- "Archive does not contain required file: "
- "\"metadata.
+ "archive does not contain required file "
+ "\"metadata.
Same thing.
[4]
- @param master: if True, machine will initialize the ensemble admin
+ `master` if True, machine will initialize the ensemble admin
and run a provisioning agent.
I suspect this is the first time I see this notation, and our overall comment
style is starting to feel like a soup. We have at least three different kinds
of comments, besides the broken ones. Please revert the changes to parameter
names and raising/returning specifically so that this branch can be merged
and let's talk about styling in a different thread.