Merge lp:~marcoceppi/charm-tools/substrate-cfg into lp:charm-tools/1.2
- substrate-cfg
- Merge into 1.2
Proposed by
Marco Ceppi
Status: | Merged |
---|---|
Merged at revision: | 287 |
Proposed branch: | lp:~marcoceppi/charm-tools/substrate-cfg |
Merge into: | lp:charm-tools/1.2 |
Diff against target: |
438 lines (+187/-37) 7 files modified
.bzrignore (+2/-0) charmtools/bundles.py (+0/-1) charmtools/generate.py (+0/-1) charmtools/test.py (+109/-26) charmtools/update.py (+2/-0) tests/test_juju_test.py (+12/-9) tests/test_substrates.py (+62/-0) |
To merge this branch: | bzr merge lp:~marcoceppi/charm-tools/substrate-cfg |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
charmers | Pending | ||
Review via email: mp+201094@code.launchpad.net |
Commit message
Description of the change
Add substrate configuration to juju-test
To post a comment you must log in.
Revision history for this message
Marco Ceppi (marcoceppi) wrote : | # |
Revision history for this message
Benjamin Saller (bcsaller) wrote : | # |
LGTM
Thanks for including my suggestions (and making the actual integration
work). The changes to using a key whitelist in TestCfg are also good.
Revision history for this message
Marco Ceppi (marcoceppi) wrote : | # |
*** Submitted:
Add substrate configuration to juju-test
R=benjamin.saller
CC=
https:/
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file '.bzrignore' |
2 | --- .bzrignore 2013-12-16 16:12:29 +0000 |
3 | +++ .bzrignore 2014-01-09 20:17:53 +0000 |
4 | @@ -9,3 +9,5 @@ |
5 | lib |
6 | local |
7 | man |
8 | +tests/.ropeproject/ |
9 | +charmtools/.ropeproject/ |
10 | |
11 | === modified file 'charmtools/bundles.py' |
12 | --- charmtools/bundles.py 2013-12-20 14:06:43 +0000 |
13 | +++ charmtools/bundles.py 2014-01-09 20:17:53 +0000 |
14 | @@ -2,7 +2,6 @@ |
15 | import yaml |
16 | import glob |
17 | import json |
18 | -import requests |
19 | |
20 | from linter import Linter |
21 | from charmworldlib import bundle as cw_bundle |
22 | |
23 | === modified file 'charmtools/generate.py' |
24 | --- charmtools/generate.py 2013-12-12 17:20:39 +0000 |
25 | +++ charmtools/generate.py 2014-01-09 20:17:53 +0000 |
26 | @@ -16,7 +16,6 @@ |
27 | # along with this program. If not, see <http://www.gnu.org/licenses/>. |
28 | |
29 | import os |
30 | -import sys |
31 | import shutil |
32 | import argparse |
33 | |
34 | |
35 | === modified file 'charmtools/test.py' |
36 | --- charmtools/test.py 2013-12-21 12:59:08 +0000 |
37 | +++ charmtools/test.py 2014-01-09 20:17:53 +0000 |
38 | @@ -1,18 +1,16 @@ |
39 | #!/usr/bin/python |
40 | # coding=latin-1 |
41 | |
42 | +import argparse |
43 | +import glob |
44 | +import logging |
45 | import os |
46 | +import re |
47 | +import signal |
48 | +import subprocess |
49 | import sys |
50 | -import uuid |
51 | +import time |
52 | import yaml |
53 | -import time |
54 | -import glob |
55 | -import signal |
56 | -import logging |
57 | -import argparse |
58 | -import subprocess |
59 | - |
60 | -from tempfile import mkdtemp |
61 | from datetime import timedelta |
62 | from contextlib import contextmanager |
63 | |
64 | @@ -44,6 +42,10 @@ |
65 | pass |
66 | |
67 | |
68 | +class SubstrateMismatch(Exception): |
69 | + pass |
70 | + |
71 | + |
72 | class TimeoutError(Exception): |
73 | def __init__(self, value="Timed Out"): |
74 | self.value = value |
75 | @@ -114,7 +116,7 @@ |
76 | self.destroy(self.juju_env) |
77 | except DestroyUnreliable: |
78 | self.log.warn('Unable to destroy bootstrap, trying again') |
79 | - sleep(2) |
80 | + time.sleep(2) |
81 | try: |
82 | self.destroy(self.juju_env) |
83 | except: |
84 | @@ -258,6 +260,7 @@ |
85 | try: |
86 | with timeout(self.conductor.args.timeout): |
87 | output = subprocess.check_output(self.test, env=self.env) |
88 | + self.log.debug(output) |
89 | except TimeoutError, e: |
90 | self.log.debug('Killed by timeout after %s seconds', |
91 | self.conductor.args.timeout) |
92 | @@ -279,7 +282,7 @@ |
93 | try: |
94 | self.archive_logs() |
95 | except OrchestraError, e: |
96 | - juju.log.error(e) |
97 | + self.log.error(e) |
98 | |
99 | if error: |
100 | raise error |
101 | @@ -293,7 +296,7 @@ |
102 | raise OrchestraError('Unable to query juju status') |
103 | |
104 | services = status['services'] |
105 | - machines = status['machines'] |
106 | + # machines = status['machines'] |
107 | |
108 | if self.conductor.juju_version.major == 0: |
109 | logs.append('/var/lib/juju/units/./*/charm.log') |
110 | @@ -395,16 +398,16 @@ |
111 | class TestCfg(object): |
112 | _keys = ['timeout', 'set-e', 'on-timeout', 'fail-on-skip', 'tests'] |
113 | |
114 | - def __init__(self, config_file): |
115 | - if not os.path.exists(config_file): |
116 | - raise OSError("%s not found" % config_file) |
117 | - |
118 | - with open(config_file) as f: |
119 | - cfg = yaml.safe_load(f.read()) |
120 | - |
121 | - for key, val in cfg['options'].iteritems(): |
122 | - if key in self._keys: |
123 | - setattr(self, key, val) |
124 | + def __init__(self, cfg): |
125 | + if isinstance(cfg, basestring): |
126 | + cfg = yaml.safe_load(cfg) |
127 | + |
128 | + if 'options' in cfg: |
129 | + for key, val in cfg['options'].iteritems(): |
130 | + if key in self._keys: |
131 | + setattr(self, key, val) |
132 | + if 'substrates' in cfg: |
133 | + self.substrates = cfg.substrates |
134 | |
135 | |
136 | def get_juju_version(): |
137 | @@ -455,6 +458,71 @@ |
138 | return logger |
139 | |
140 | |
141 | +class SubstrateFilter(object): |
142 | + def __init__(self, spec): |
143 | + self.order = spec.get('order', ['include', 'skip']) |
144 | + self.include = spec.get('include', ['*']) |
145 | + self.skip = spec.get('skip', []) |
146 | + |
147 | + if isinstance(self.order, str): |
148 | + self.order = [s.strip() for s in self.order.split(',')] |
149 | + if self.order != ['include', 'skip'] and \ |
150 | + self.order != ['skip', 'include']: |
151 | + raise ValueError( |
152 | + 'order should be defined using only include and skip') |
153 | + |
154 | + if isinstance(self.include, str): |
155 | + self.include = [self.include] |
156 | + self.include = set(self.include) |
157 | + |
158 | + if isinstance(self.skip, str): |
159 | + self.skip = [self.skip] |
160 | + self.skip = set(self.skip) |
161 | + |
162 | + def filter(self, substrates): |
163 | + """ |
164 | + Filter a list of substrates relative to the rules generated on class |
165 | + creation. |
166 | + """ |
167 | + if isinstance(substrates, str): |
168 | + substrates = [s.strip() for s in re.split('[,\s]', substrates)] |
169 | + |
170 | + # Apply the rules to the list of substrates returning anything that |
171 | + # matches |
172 | + if self.order == ['include', 'skip']: |
173 | + result = self._filter_includes(substrates, True) |
174 | + result = self._filter_skips(result) |
175 | + else: |
176 | + result = self._filter_skips(substrates, True) |
177 | + result = self._filter_includes(result) |
178 | + return result |
179 | + |
180 | + def _filter_includes(self, inputList, priority=False): |
181 | + if priority and '*' in self.include: |
182 | + return inputList |
183 | + return sorted(list(set(inputList).intersection(self.include))) |
184 | + |
185 | + def _filter_skips(self, inputList, priority=False): |
186 | + if priority and '*' in self.skip: |
187 | + return list(self.include.intersection(inputList)) |
188 | + return sorted(list(set(inputList).difference(self.skip))) |
189 | + |
190 | + |
191 | +def parse_substrates(spec): |
192 | + if isinstance(spec, basestring): |
193 | + spec = yaml.safe_load(spec) |
194 | + if not spec or 'substrates' not in spec: |
195 | + raise ValueError( |
196 | + "Invalid data passed to parse_substrates: {}".format(spec)) |
197 | + |
198 | + specRules = SubstrateFilter(spec['substrates']) |
199 | + return specRules |
200 | + |
201 | + |
202 | +def allowed_substrates(spec, possible_substrates): |
203 | + return parse_substrates(spec).filter(possible_substrates) |
204 | + |
205 | + |
206 | def setup_parser(): |
207 | parser = argparse.ArgumentParser( |
208 | prog='juju test', |
209 | @@ -564,10 +632,14 @@ |
210 | logger.info('Starting test run on %s using Juju %s' |
211 | % (args.juju_env, get_juju_version())) |
212 | logger.debug('Loading configuration options from testplan YAML') |
213 | - test_plans = glob.glob(os.path.join(os.getcwd(), 'tests', 'testplan.y*ml')) |
214 | - test_plan = test_plan[0] if test_plans else None |
215 | - if test_plan: |
216 | - cfg = TestCfg(test_plan) |
217 | + test_plans = glob.glob(os.path.join(os.getcwd(), 'tests', |
218 | + 'test_config.y*ml')) |
219 | + if test_plans: |
220 | + with open(test_plans[0]) as f: |
221 | + test_cfg = f.read() |
222 | + |
223 | + if test_cfg: |
224 | + cfg = TestCfg(test_cfg) |
225 | for key, val in args.iteritems(): |
226 | logger.debug('Overwriting %s to %s from cmd' % (key, val)) |
227 | setattr(cfg, key, val) |
228 | @@ -577,10 +649,21 @@ |
229 | logger.debug('Creating a new Conductor') |
230 | try: |
231 | tester = Conductor(args) |
232 | + env_yaml = tester.get_environment(cfg.juju_env) |
233 | + if 'substrates' in cfg: |
234 | + rules = parse_substrates(cfg) |
235 | + allowed = rules.filter(env_yaml['type']) |
236 | + if env_yaml['type'] not in allowed: |
237 | + raise Exception('%s is not an allowed substrate: %s' % |
238 | + (env_yaml['type'], |
239 | + allowed.join(', '))) |
240 | errors, failures, passes = tester.run() |
241 | except NoTests: |
242 | logger.critical('No tests were found') |
243 | sys.exit(1) |
244 | + except Exception as e: |
245 | + logger.critical(str(e)) |
246 | + sys.exit(1) |
247 | except: |
248 | raise |
249 | |
250 | |
251 | === modified file 'charmtools/update.py' |
252 | --- charmtools/update.py 2013-12-16 16:12:29 +0000 |
253 | +++ charmtools/update.py 2014-01-09 20:17:53 +0000 |
254 | @@ -34,6 +34,7 @@ |
255 | |
256 | return parser |
257 | |
258 | + |
259 | def update(charm_dir, fix=False): |
260 | mr = Mr(charm_dir) |
261 | for charm in charms.remote(): |
262 | @@ -48,6 +49,7 @@ |
263 | except Exception as e: |
264 | raise Exception(".mrconfig not saved: %s" % e.strerror) |
265 | |
266 | + |
267 | def main(): |
268 | parser = setup_parser() |
269 | args = parser.parse_args() |
270 | |
271 | === modified file 'tests/test_juju_test.py' |
272 | --- tests/test_juju_test.py 2013-12-20 12:33:21 +0000 |
273 | +++ tests/test_juju_test.py 2014-01-09 20:17:53 +0000 |
274 | @@ -30,13 +30,16 @@ |
275 | |
276 | PARSED_ENVIRONMENTS_YAML = yaml.safe_load(RAW_ENVIRONMENTS_YAML) |
277 | |
278 | + |
279 | class Arguments(object): |
280 | def __init__(self, **kwargs): |
281 | for key, value in kwargs.items(): |
282 | setattr(self, key, value) |
283 | + |
284 | def __getattr__(self, name): |
285 | return None |
286 | |
287 | + |
288 | class JujuTestPluginTest(unittest.TestCase): |
289 | @patch('subprocess.check_output') |
290 | def test_get_gojuju_version(self, mock_check_output): |
291 | @@ -77,7 +80,7 @@ |
292 | self.assertEqual(100, juju_test.convert_to_timedelta('100s')) |
293 | self.assertEqual(100, juju_test.convert_to_timedelta(100)) |
294 | self.assertEqual(60, juju_test.convert_to_timedelta('1m')) |
295 | - self.assertEqual(60*60, juju_test.convert_to_timedelta('1h')) |
296 | + self.assertEqual(60 * 60, juju_test.convert_to_timedelta('1h')) |
297 | |
298 | @patch('glob.glob') |
299 | @patch('os.path.isfile') |
300 | @@ -89,7 +92,7 @@ |
301 | mock_isfile.side_effect = files_exist |
302 | mock_glob.return_value = tests_directory |
303 | |
304 | - args = Arguments(tests = 'dummy') |
305 | + args = Arguments(tests='dummy') |
306 | c = juju_test.Conductor(args) |
307 | results = c.find_tests() |
308 | |
309 | @@ -100,7 +103,7 @@ |
310 | def test_conductor_find_tests_fails(self, mock_glob): |
311 | mock_glob.return_value = [] |
312 | |
313 | - args = Arguments(tests = 'dummy') |
314 | + args = Arguments(tests='dummy') |
315 | c = juju_test.Conductor(args) |
316 | results = c.find_tests() |
317 | |
318 | @@ -126,7 +129,7 @@ |
319 | |
320 | mcheck_output.side_effect = [yml_output, Exception('not bootstrapped')] |
321 | juju_env = 'test' |
322 | - args = Arguments(tests = 'dummy') |
323 | + args = Arguments(tests='dummy') |
324 | c = juju_test.Conductor(args) |
325 | results = c.status(juju_env) |
326 | |
327 | @@ -193,7 +196,7 @@ |
328 | |
329 | @patch('subprocess.check_call') |
330 | def test_conductor_destroy(self, mock_check_call): |
331 | - args = Arguments(tests = 'dummy') |
332 | + args = Arguments(tests='dummy') |
333 | c = juju_test.Conductor(args) |
334 | c.juju_version = juju_test.JujuVersion(major=1, minor=1, patch=1) |
335 | good_env = 'valid' |
336 | @@ -216,7 +219,7 @@ |
337 | |
338 | bad_env = 'invalid' |
339 | |
340 | - args = Arguments(tests = 'dummy') |
341 | + args = Arguments(tests='dummy') |
342 | c = juju_test.Conductor(args) |
343 | c.juju_version = juju_test.JujuVersion(major=1, minor=8, patch=0) |
344 | self.assertRaises(juju_test.DestroyUnreliable, c.destroy, bad_env) |
345 | @@ -448,7 +451,7 @@ |
346 | o.log.status.assert_called_with('%s (%s)' % (juju_test.TEST_FAIL, |
347 | juju_test.TEST_TIMEOUT)) |
348 | |
349 | - o.conductor.args.on_timeout='skip' |
350 | + o.conductor.args.on_timeout = 'skip' |
351 | o.log.status.reset_mock() |
352 | o.print_status(124) |
353 | o.log.status.assert_called_with('%s (%s)' % (juju_test.TEST_FAIL, |
354 | @@ -583,7 +586,7 @@ |
355 | goyml_parsed = yaml.safe_load(goyml_output) |
356 | mstatus.return_value = goyml_parsed |
357 | |
358 | - juju_env='testing' |
359 | + juju_env = 'testing' |
360 | args = Arguments(tests='dummy', juju_env=juju_env, logdir='/tmp') |
361 | c = juju_test.Conductor(args) |
362 | c.juju_version = juju_test.JujuVersion(major=1, minor=10, patch=0) |
363 | @@ -623,7 +626,7 @@ |
364 | pyyml_parsed = yaml.safe_load(pyyml_output) |
365 | mstatus.return_value = pyyml_parsed |
366 | |
367 | - juju_env='testing' |
368 | + juju_env = 'testing' |
369 | args = Arguments(tests='dummy', juju_env=juju_env, logdir='/tmp') |
370 | c = juju_test.Conductor(args) |
371 | c.juju_version = juju_test.JujuVersion(major=0, minor=7, patch=0) |
372 | |
373 | === added file 'tests/test_substrates.py' |
374 | --- tests/test_substrates.py 1970-01-01 00:00:00 +0000 |
375 | +++ tests/test_substrates.py 2014-01-09 20:17:53 +0000 |
376 | @@ -0,0 +1,62 @@ |
377 | +"""Unit test for juju tests substrate handling""" |
378 | + |
379 | +import unittest |
380 | +import yaml |
381 | +from charmtools.test import parse_substrates, allowed_substrates |
382 | + |
383 | + |
384 | +class JujuTestSubstrateTests(unittest.TestCase): |
385 | + INCLUDE_SKIP = ''' |
386 | +substrates: |
387 | + order: include, skip |
388 | + skip: amazon |
389 | + include: "*" |
390 | + ''' |
391 | + |
392 | + SKIP_INCLUDE = ''' |
393 | +substrates: |
394 | + order: skip, include |
395 | + skip: "*" |
396 | + include: amazon |
397 | + ''' |
398 | + |
399 | + def test_parse_substrates_text(self): |
400 | + self.assertRaises(ValueError, parse_substrates, '') |
401 | + self.assertIsNotNone(parse_substrates(self.INCLUDE_SKIP)) |
402 | + |
403 | + def test_parse_substrate_object(self): |
404 | + data = yaml.load(self.INCLUDE_SKIP) |
405 | + self.assertIsNotNone(parse_substrates(data)) |
406 | + |
407 | + def test_parse_substrates_order(self): |
408 | + result = parse_substrates(self.INCLUDE_SKIP) |
409 | + self.assertEqual(result.order, ['include', 'skip']) |
410 | + |
411 | + def test_parse_substrates_order_invalid(self): |
412 | + data = self.INCLUDE_SKIP.replace('skip', 'foobar') |
413 | + self.assertRaises(ValueError, parse_substrates, data) |
414 | + |
415 | + def test_parse_substrates_include(self): |
416 | + result = parse_substrates(self.INCLUDE_SKIP) |
417 | + self.assertEqual(result.include, set(['*'])) |
418 | + |
419 | + def test_parse_substrates_skip(self): |
420 | + result = parse_substrates(self.INCLUDE_SKIP) |
421 | + self.assertEqual(result.skip, set(['amazon'])) |
422 | + |
423 | + def test_filter_include_skip(self): |
424 | + rules = parse_substrates(self.INCLUDE_SKIP) |
425 | + self.assertEqual(rules.filter('amazon,hp,azure'), ['azure', 'hp']) |
426 | + |
427 | + def test_filter_skip_include(self): |
428 | + rules = parse_substrates(self.SKIP_INCLUDE) |
429 | + self.assertEqual(rules.filter('amazon,hp,azure'), ['amazon']) |
430 | + |
431 | + def test_allowed_substrates(self): |
432 | + self.assertEqual(allowed_substrates(self.INCLUDE_SKIP, |
433 | + ['foo', 'bar', 'amazon']), |
434 | + ['bar', 'foo']) |
435 | + |
436 | + |
437 | +if __name__ == '__main__': |
438 | + unittest.main() |
Reviewers: mp+201094_ code.launchpad. net,
Message:
Please take a look.
Description:
Updates to bsaller's fixes to substrate parsing
https:/ /code.launchpad .net/~marcocepp i/charm- tools/substrate -cfg/+merge/ 201094
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/49940044/
Affected files (+189, -37 lines): bundles. py generate. py update. py juju_test. py substrates. py
M .bzrignore
A [revision details]
M charmtools/
M charmtools/
M charmtools/test.py
M charmtools/
M tests/test_
A tests/test_