Merge lp:~nskaggs/juju-ci-tools/add-sane-args-defaults into lp:juju-ci-tools
- add-sane-args-defaults
- Merge into trunk
Status: | Superseded | ||||||||
---|---|---|---|---|---|---|---|---|---|
Proposed branch: | lp:~nskaggs/juju-ci-tools/add-sane-args-defaults | ||||||||
Merge into: | lp:juju-ci-tools | ||||||||
Diff against target: |
234 lines (+147/-19) 2 files modified
tests/test_utility.py (+65/-0) utility.py (+82/-19) |
||||||||
To merge this branch: | bzr merge lp:~nskaggs/juju-ci-tools/add-sane-args-defaults | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Christopher Lee (community) | Needs Fixing | ||
Martin Packman (community) | Needs Fixing | ||
Review via email: mp+296358@code.launchpad.net |
This proposal has been superseded by a proposal from 2016-07-08.
Commit message
Description of the change
Make juju_bin, temp_env_name and logs all as optional arguments with sane defaults
Nicholas Skaggs (nskaggs) wrote : | # |
Martin Packman (gz) wrote : | # |
The general idea of having sane defaults for the positional arguments I'm down with, I think we could even default 'env' to 'lxd' or something.
Given this changes add_basic_
$ rm -rf /tmp/logs
$ touch /tmp/logs
$ make test
...
FAILED (failures=2, errors=37)
Some other comments inline.
Aaron Bentley (abentley) wrote : | # |
It seems strange to provide a default for the juju under test. It is, after all, the thing we are testing, and if we don't want to specify it, we might as well not run the test at all.
Nicholas Skaggs (nskaggs) wrote : | # |
Replies.
Aaron Bentley (abentley) wrote : | # |
You don't seem to have replied to me.
Nicholas Skaggs (nskaggs) wrote : | # |
@abentley, I suppose that is the crux of the argument for /usr/bin/juju vs $GOPATH/bin/juju. Odds are you care enough to specify. However, in the case of someone wanting to run the tests locally, they might not particularly care beyond "use the juju I have".
However, I went full bore on Martin's idea of going ahead and removing env too and making a sane default for it. I think the ability to just be able to run our tests is useful. For those of us who will actually attempt to do CI, we'll always want to specify everything, but we're presumably not the primary audience here (other than our localized test writing).
Aaron Bentley (abentley) wrote : | # |
> However, in the case of
> someone wanting to run the tests locally, they might not particularly care
> beyond "use the juju I have".
Who is that person? Are they a juju developer?
If so, I think they are probably
1. writing a test for a feature they have written, or
2. running a test to make sure their changes don't break it
In both cases, they most likely have a custom binary that they want to test against.
So I still don't see who you're thinking would want to omit the path to the juju binary.
Nicholas Skaggs (nskaggs) wrote : | # |
> > However, in the case of
> > someone wanting to run the tests locally, they might not particularly care
> > beyond "use the juju I have".
>
> Who is that person? Are they a juju developer?
Anyone who wishes to run tests locally; aka the maas guys, charm guys, openstack team, docs guys, juju developers. Granted ATM, few if any outside of our team run these tests locally.
> If so, I think they are probably
> 1. writing a test for a feature they have written, or
> 2. running a test to make sure their changes don't break it
>
> In both cases, they most likely have a custom binary that they want to test
> against.
I will grant you that indeed that a developer will have a custom binary in either case. Hence I was trying to phrase it as 'use the juju I have'. I won't pretend to know how many binaries developers tend to keep on there system; thus it depends on the developer if we can pick a sane default for them or not. It's likely they'll specify things.
If a developer has the juju binary they are hacking on set in a single place, it would be nice to just use that to enable that workflow, if indeed it exists. That would be an argument for $GOPATH. For the others listed above, I think /usr/bin/juju would be there expectation.
However, even if we assume developers and ourselves will always specify (and I agree that is likely), I do see value in having things just work when executed beyond enabling others. Removing all required arguments does that, so I would argue for adding the default on that basis alone. If we are left with only one argument, and we can reasonably remove it, let's do so. Lowering the barrier to running tests is the goal; albeit without breaking ourselves.
Aaron Bentley (abentley) wrote : | # |
Additional temp_env_name comment.
Nicholas Skaggs (nskaggs) wrote : | # |
Addressed uniqueness issue with temp_env_name
Martin Packman (gz) wrote : | # |
Most changes seem fine, but we still have the test isolation issue from creating the logdir I pointed out before:
ubuntu@
modified:
tests/
utility.py
pending merge tips: (use -v to see all merge revisions)
Nicholas Skaggs 2016-06-07 make temp_env_name truly unique
ubuntu@
...
FAILED (failures=2, errors=39)
ubuntu@
modified:
tests/
utility.py
unknown:
baz/
d/
log/
log_dir/
test_
pending merge tips: (use -v to see all merge revisions)
Nicholas Skaggs 2016-06-07 make temp_env_name truly unique
Nicholas Skaggs (nskaggs) wrote : | # |
@mgz, I don't get any errors running this. I think you are pointing out I shouldn't be creating a folder during test runs; absolutely correct, so I've patched that out. I also will add some more tests to ensure __clean_dir() works as it should given I've modified it as well. Is there something else you are referring to?
Martin Packman (gz) wrote : | # |
See my first comment for instructions. Tests should not fail based on the contents on /tmp.
Nicholas Skaggs (nskaggs) wrote : | # |
I fixed everything in test_utility to use a temp dir instead of a hardcoded path. In a seperate MP I'll fix the rest of trunk with the same fix.
Nicholas Skaggs (nskaggs) wrote : | # |
Ok, I did this in a simpler way -- only throw an exception if the directory doesn't exist, and we fail to create it. We continue on with warnings otherwise. Tests pass, new tests confirm my expectations.
Nicholas Skaggs (nskaggs) wrote : | # |
This fixes the /tmp/logs problem by not raising the issue. The only remaining annoyance is -h or --help will create a directory and leave the warning (since the default functions still run). Also, running make test will spam blank folders in /tmp. Comments welcome.
Christopher Lee (veebers) wrote : | # |
Some comments inline.
Nicholas Skaggs (nskaggs) wrote : | # |
Replies inline.
Christopher Lee (veebers) wrote : | # |
Comment re: readability
Christopher Lee (veebers) wrote : | # |
Reply to comments
Christopher Lee (veebers) wrote : | # |
> LGTM
Actually I take that back. I have concerns on calling --help creating a directory.
- 1469. By Nicholas Skaggs
-
fix for bug #1601987
- 1470. By Nicholas Skaggs
-
fix help test
- 1471. By Nicholas Skaggs
-
re-merge trunk
- 1472. By Nicholas Skaggs
-
remove directory creation as part of arg parsing. By default if nothing is specified, we will us the current directory
- 1473. By Nicholas Skaggs
-
move directory creation to bootstrap manager
- 1474. By Nicholas Skaggs
-
flake8
- 1475. By Nicholas Skaggs
-
patch out directory creation during no_args test
- 1476. By Nicholas Skaggs
-
restore _generate_
default_ clean_dir
Unmerged revisions
Preview Diff
1 | === modified file 'tests/test_utility.py' |
2 | --- tests/test_utility.py 2016-05-26 16:28:21 +0000 |
3 | +++ tests/test_utility.py 2016-07-07 21:48:09 +0000 |
4 | @@ -11,6 +11,7 @@ |
5 | import logging |
6 | import os |
7 | import socket |
8 | +from tempfile import mkdtemp |
9 | from time import time |
10 | import warnings |
11 | |
12 | @@ -361,6 +362,38 @@ |
13 | |
14 | class TestAddBasicTestingArguments(TestCase): |
15 | |
16 | + def test_no_args(self): |
17 | + cmd_line = [] |
18 | + parser = add_basic_testing_arguments(ArgumentParser()) |
19 | + with patch('utility.os.makedirs'): |
20 | + with patch('utility.os.getenv', return_value=''): |
21 | + args = parser.parse_args(cmd_line) |
22 | + self.assertEqual(args.env, 'lxd') |
23 | + self.assertEqual(args.juju_bin, '/usr/bin/juju') |
24 | + |
25 | + logs_arg = args.logs.split("/") |
26 | + logs_ts = logs_arg[4] |
27 | + self.assertEqual(logs_arg[1:4], ['tmp', 'test_utility', 'logs']) |
28 | + self.assertTrue(logs_ts, datetime.strptime(logs_ts, "%Y%m%d%H%M%S")) |
29 | + |
30 | + temp_env_name_arg = args.temp_env_name.split("_") |
31 | + temp_env_name_ts = temp_env_name_arg[2] |
32 | + self.assertEqual(temp_env_name_arg[0:2], ['test', 'utility']) |
33 | + self.assertTrue(temp_env_name_ts, |
34 | + datetime.strptime(temp_env_name_ts, "%Y%m%d%H%M%S")) |
35 | + self.assertEqual(temp_env_name_arg[3:5], ['temp', 'env']) |
36 | + |
37 | + self.assertEqual(logs_ts, temp_env_name_ts) |
38 | + |
39 | + def test_default_binary(self): |
40 | + cmd_line = [] |
41 | + with patch('utility.os.makedirs'): |
42 | + with patch('utility.os.getenv', return_value='/tmp'): |
43 | + with patch('utility.os.path.isfile', return_value=True): |
44 | + parser = add_basic_testing_arguments(ArgumentParser()) |
45 | + args = parser.parse_args(cmd_line) |
46 | + self.assertEqual(args.juju_bin, '/tmp/bin/juju') |
47 | + |
48 | def test_positional_args(self): |
49 | cmd_line = ['local', '/foo/juju', '/tmp/logs', 'testtest'] |
50 | parser = add_basic_testing_arguments(ArgumentParser()) |
51 | @@ -408,6 +441,38 @@ |
52 | self.assertEqual(warned, []) |
53 | self.assertEqual("", self.log_stream.getvalue()) |
54 | |
55 | + def test_warn_on_nonexistent_directory_creation(self): |
56 | + with warnings.catch_warnings(record=True) as warned: |
57 | + log_dir = mkdtemp() |
58 | + os.rmdir(log_dir) |
59 | + cmd_line = ['local', '/foo/juju', log_dir, 'testtest'] |
60 | + parser = add_basic_testing_arguments(ArgumentParser()) |
61 | + parser.parse_args(cmd_line) |
62 | + self.assertEqual(len(warned), 1) |
63 | + self.assertRegexpMatches( |
64 | + str(warned[0].message), |
65 | + r"Not a directory " + log_dir) |
66 | + self.assertEqual("", self.log_stream.getvalue()) |
67 | + |
68 | + def test_warn_on_directory_creation_failure(self): |
69 | + with warnings.catch_warnings(record=True) as warned: |
70 | + with patch('utility.os.makedirs', side_effect=OSError): |
71 | + cmd_line = [] |
72 | + try: |
73 | + parser = add_basic_testing_arguments(ArgumentParser()) |
74 | + parser.parse_args(cmd_line) |
75 | + except OSError: |
76 | + # we catch our thrown OSError |
77 | + pass |
78 | + else: |
79 | + self.fail('No exception thrown after' + |
80 | + ' directory creation failure') |
81 | + self.assertEqual(len(warned), 1) |
82 | + self.assertRegexpMatches( |
83 | + str(warned[0].message), |
84 | + r"Failed to create logging directory: /tmp/test_utility/" + |
85 | + "logs/.*. Please specify empty folder or try again") |
86 | + |
87 | def test_debug(self): |
88 | cmd_line = ['local', '/foo/juju', '/tmp/logs', 'testtest', '--debug'] |
89 | parser = add_basic_testing_arguments(ArgumentParser()) |
90 | |
91 | === modified file 'utility.py' |
92 | --- utility.py 2016-06-27 11:41:04 +0000 |
93 | +++ utility.py 2016-07-07 21:48:09 +0000 |
94 | @@ -104,18 +104,20 @@ |
95 | try: |
96 | contents = os.listdir(maybe_dir) |
97 | except OSError as e: |
98 | - if e.errno != errno.ENOENT: |
99 | - raise |
100 | - # GZ 2016-03-01: We may want to raise or just create the dir here, but |
101 | - # that confuses expectations of all existing parse_args tests. |
102 | + if e.errno == errno.ENOENT: |
103 | + # we don't raise this error due to tests abusing /tmp/logs |
104 | + warnings.warn('Not a directory {}'.format(maybe_dir)) |
105 | + if e.errno == errno.EEXIST: |
106 | + warnings.warn('Directory {} already exists'.format(maybe_dir)) |
107 | else: |
108 | if contents and contents != ["empty"]: |
109 | - warnings.warn("Directory %r has existing contents." % (maybe_dir,)) |
110 | + warnings.warn( |
111 | + 'Directory {!r} has existing contents.'.format(maybe_dir)) |
112 | return maybe_dir |
113 | |
114 | |
115 | def pause(seconds): |
116 | - print_now('Sleeping for %d seconds.' % seconds) |
117 | + print_now('Sleeping for {:d} seconds.'.format(seconds)) |
118 | sleep(seconds) |
119 | |
120 | |
121 | @@ -194,7 +196,7 @@ |
122 | except socket.gaierror as e: |
123 | print_now(str(e)) |
124 | except Exception as e: |
125 | - print_now('Unexpected %r: %s' % (type(e), e)) |
126 | + print_now('Unexpected {!r}: {}'.format((type(e), e))) |
127 | raise |
128 | else: |
129 | conn.close() |
130 | @@ -290,14 +292,59 @@ |
131 | return subprocess.check_output(command) |
132 | |
133 | |
134 | +def _get_test_name_from_filename(): |
135 | + try: |
136 | + calling_file = sys._getframe(2).f_back.f_globals['__file__'] |
137 | + return os.path.splitext(os.path.basename(calling_file))[0] |
138 | + except: |
139 | + return 'unknown_test' |
140 | + |
141 | + |
142 | +def _generate_default_clean_dir(timestamp): |
143 | + """Creates a new unique directory for logging and returns the name""" |
144 | + log_dir = os.path.join('/tmp', _get_test_name_from_filename(), |
145 | + 'logs', timestamp) |
146 | + try: |
147 | + os.makedirs(log_dir) |
148 | + warnings.warn('"Created logging directory {}'.format(log_dir)) |
149 | + except OSError as e: |
150 | + if e.errno == errno.EEXIST: |
151 | + pass |
152 | + else: |
153 | + warnings.warn('Failed to create logging directory: ' + |
154 | + log_dir + |
155 | + ". Please specify empty folder or try again") |
156 | + raise |
157 | + return log_dir |
158 | + |
159 | + |
160 | +def _generate_default_temp_env_name(timestamp): |
161 | + """Creates a new unique name for environment and returns the name""" |
162 | + return '{}_{}_temp_env'.format(_get_test_name_from_filename(), timestamp) |
163 | + |
164 | + |
165 | +def _generate_default_binary(): |
166 | + """Returns GOPATH juju binary if it exists, otherwise /usr/bin/juju""" |
167 | + if os.getenv('GOPATH'): |
168 | + go_bin = os.getenv('GOPATH') + '/bin/juju' |
169 | + if os.path.isfile(go_bin): |
170 | + return go_bin |
171 | + |
172 | + return '/usr/bin/juju' |
173 | + |
174 | + |
175 | def add_basic_testing_arguments(parser, using_jes=False): |
176 | """Returns the parser loaded with basic testing arguments. |
177 | |
178 | The basic testing arguments, used in conjuction with boot_context ensures |
179 | a test can be run in any supported substrate in parallel. |
180 | |
181 | - This helper adds 4 positional arguments that define the minimum needed |
182 | - to run a test script: env, juju_bin, logs, and temp_env_name. |
183 | + This helper adds 4 positional arguments that defines the minimum needed |
184 | + to run a test script. |
185 | + |
186 | + These arguments (env, juju_bin, logs, temp_env_name) allow you to specify |
187 | + specifics for which env, juju binary, which folder for logging and an |
188 | + environment name for your test respectively. |
189 | |
190 | There are many optional args that either update the env's config or |
191 | manipulate the juju command line options to test in controlled situations |
192 | @@ -308,16 +355,32 @@ |
193 | :param parser: an ArgumentParser. |
194 | :param using_jes: whether args should be tailored for JES testing. |
195 | """ |
196 | - # Required positional arguments. |
197 | - parser.add_argument( |
198 | - 'env', |
199 | - help='The juju environment to base the temp test environment on.') |
200 | - parser.add_argument( |
201 | - 'juju_bin', help='Full path to the Juju binary.') |
202 | - parser.add_argument( |
203 | - 'logs', type=_clean_dir, help='A directory in which to store logs.') |
204 | - parser.add_argument( |
205 | - 'temp_env_name', help='A temporary test environment name.') |
206 | + |
207 | + # Optional postional arguments |
208 | + # generate timestamp for use with default args |
209 | + timestamp = datetime.now().strftime("%Y%m%d%H%M%S") |
210 | + parser.add_argument( |
211 | + 'env', nargs='?', |
212 | + help='The juju environment to base the temp test environment on.', |
213 | + default='lxd') |
214 | + parser.add_argument('juju_bin', nargs='?', |
215 | + help='Full path to the Juju binary. By default, this' |
216 | + ' will use $GOPATH/bin/juju or /usr/bin/juju in that' |
217 | + ' order.', |
218 | + default=_generate_default_binary()) |
219 | + parser.add_argument('logs', nargs='?', type=_clean_dir, |
220 | + help='A directory in which to store logs. By default,' |
221 | + ' this will generate a directory named after the test' |
222 | + ' and under /tmp, with a timestamp of the run. ' |
223 | + ' /tmp/test_name/logs/timestamp', |
224 | + default=_generate_default_clean_dir(timestamp)) |
225 | + parser.add_argument('temp_env_name', nargs='?', |
226 | + help='A temporary test environment name. By default, ' |
227 | + ' this will generate an enviroment name using the ' |
228 | + ' timestamp and testname. ' |
229 | + ' test_name_timestamp_temp_env', |
230 | + default=_generate_default_temp_env_name(timestamp)) |
231 | + |
232 | # Optional keyword arguments. |
233 | parser.add_argument('--debug', action='store_true', |
234 | help='Pass --debug to Juju.') |
Reviewers note; I'm tempted to add a timestamp to the environment name or otherwise make it more unique. Ideas welcome. Would you find that useful or annoying (especially since it may not be the exact same ts as the logging folder)?