Merge lp:~javier.collado/utah/static_analysis_7 into lp:utah

Proposed by Javier Collado
Status: Merged
Approved by: Javier Collado
Approved revision: 715
Merged at revision: 712
Proposed branch: lp:~javier.collado/utah/static_analysis_7
Merge into: lp:utah
Diff against target: 465 lines (+80/-54) (has conflicts)
15 files modified
utah/client/common.py (+9/-4)
utah/client/phoenix.py (+3/-1)
utah/client/result.py (+7/-5)
utah/client/runner.py (+9/-1)
utah/client/state_agent.py (+1/-1)
utah/client/testcase.py (+15/-5)
utah/client/tests/manual_privileges.py (+5/-5)
utah/client/testsuite.py (+1/-3)
utah/config.py (+7/-17)
utah/preseed.py (+2/-2)
utah/provisioning/provisioning.py (+4/-4)
utah/provisioning/vm/libvirtvm.py (+4/-2)
utah/run.py (+9/-0)
utah/timeout.py (+3/-3)
utah/url.py (+1/-1)
Text conflict in utah/client/runner.py
Text conflict in utah/run.py
To merge this branch: bzr merge lp:~javier.collado/utah/static_analysis_7
Reviewer Review Type Date Requested Status
Max Brustkern (community) Needs Information
Joe Talbott (community) Approve
Javier Collado (community) Needs Resubmitting
Review via email: mp+129238@code.launchpad.net

Description of the change

Today I used pylint instead of flake8 and found a lot of issues that were not detected previously,
so I decided to fix some of them.

To post a comment you must log in.
Revision history for this message
Joe Talbott (joetalbott) wrote :

- There's a bug on line 213 that shouldn't be 'stdout' with quotes which means you shouldn't rename 'stdout' to '_stdout'.

- There's a bug on line 222, that should pass msg into the inner assertion if msg is not None.

- Regarding the relative imports: One of my goals was that the tests and the client be runnable from a local copy of the repository. Can you confirm that both of these cases work as expected with these changes. I am fine with the requirement that both the client and the tests be run from the top-level directory of the branch which means utah resolves to the relative path and doesn't have to be in the python path.

714. By Javier Collado

Fixed use variable, not string ('stdout' -> stdout) as suggested by Joe

715. By Javier Collado

Fixed use hardcoded message by default as suggested by Joe

Revision history for this message
Javier Collado (javier.collado) wrote :

@Joe

Thanks for finding those bugs.

I've run the test cases from the root directory of the branch and from utah/client directory
and they work fine (I believe that nose takes care of setting PYTHONPATH properly).

review: Needs Resubmitting
Revision history for this message
Joe Talbott (joetalbott) wrote :

Looks fine to me. I'll let Max comment on the changes in the server code.

review: Approve
Revision history for this message
Max Brustkern (nuclearbob) wrote :

I've got questions! They may reveal gaps in my understanding of proper python programming.

It seems like in some cases, like config.py and client/testsuite.py, docstrings are being changed from
"""
Do a thing.
"""
to
# Do a thing.
But not everywhere. Is there a reason only some of them are coming up?

What's the deal with cls instead of self? Is that something we should be using elsewhere, or is there some reason it only gets changed in the one instance in preseed.py?

How come in provisioning.py we have _stdout and stderr, but in timeout.py we have stdout and _stderr?

Why do a bunch of parameters change to have underscores before them, but not all of them?

Also, lines 120-132 look like a merge conflict.

review: Needs Information
Revision history for this message
Max Brustkern (nuclearbob) wrote :

Okay, I read up on the cls vs. self thing, and I get that now, so nevermind on that one.

Revision history for this message
Joe Talbott (joetalbott) wrote :

On Mon, Oct 15, 2012 at 02:12:20PM -0000, Max Brustkern wrote:
> Review: Needs Information
>
> I've got questions! They may reveal gaps in my understanding of proper python programming.
>
> It seems like in some cases, like config.py and client/testsuite.py, docstrings are being changed from
> """
> Do a thing.
> """
> to
> # Do a thing.
> But not everywhere. Is there a reason only some of them are coming up?

I have a bad habit of commenting out large blocks of code by turning
them into strings with """ or '''. Because we're doing in-source
documentation and python convention uses these strings (depending on
where they are located) as documentation. So, in short, comments
should use the comment character '#' and documentation should be in
apparently. I'll leave it to others to argue if comments are
documentation or not. The way I look at it is if your comment is
related to a few lines of code it won't make sense in the on-line
documents.
>
> What's the deal with cls instead of self? Is that something we should be using elsewhere, or is there some reason it only gets changed in the one instance in preseed.py?
>
> How come in provisioning.py we have _stdout and stderr, but in timeout.py we have stdout and _stderr?
>
> Why do a bunch of parameters change to have underscores before them, but not all of them?

I'm used to the convention that a leading underscore means internal use.
Apparently pylint's authors think that prefixing an unused variable with
an underscore is a way to indicate to pylint that we meant to include a
variable that we aren't yet using.

I don't really agree with this and think it makes the code more
confusing.

Joe

Revision history for this message
Javier Collado (javier.collado) wrote :

- Documentation strings

Note that not all strings are documentation strings, in particular (pep257: http://www.python.org/dev/peps/pep-0257/):
---
A docstring is a string literal that occurs as the first statement in a module, function, class, or method definition. Such a docstring becomes the __doc__ special attribute of that object.
---

This means that strings literals in other locations are string objects that are constructed and then
thrown away. Those strings are the ones that should be marked as comments.

- Leading underscore in variable names

The general convention in python is to use a leading underscore to set private methods and
instance variables, that is, methods that are an implementation detail and shouldn't be accessed
from the outside (even if the language lets you do that).

Another general convention is to use a single underscore as a name for local variables
you don't care about and that can be safely discarded. An extension of that convention
(used by pylint, but maybe not widely adopted by the whole community) is to use variable names
with a leading underscore to state the same while providing a longer name that can be used
for documentation (when you see _stderr, it's clear that stderr is being discarded, when you see
just _, you have have to know the API or look at the documentation).

I don't find the two uses for the underscore confusing for variables because the scopes don't
mix together (instance variables vs. local variables).

Revision history for this message
Max Brustkern (nuclearbob) wrote :

All right. I like everything except the merge conflict, then.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'utah/client/common.py'
2--- utah/client/common.py 2012-10-11 20:42:24 +0000
3+++ utah/client/common.py 2012-10-12 09:39:20 +0000
4@@ -52,7 +52,7 @@
5 CMD_TS_CLEANUP = 'testsuite_cleanup'
6
7
8-def do_nothing(obj=None):
9+def do_nothing(_obj=None):
10 """
11 Do nothing method used a placeholder for save_state_callbacks.
12 """
13@@ -78,7 +78,7 @@
14 class TimeoutAlarm(Exception):
15 pass
16
17- def alarm_handler(signum, frame):
18+ def alarm_handler(_signum, _frame):
19 raise TimeoutAlarm
20
21 start_time = datetime.datetime.now()
22@@ -154,7 +154,7 @@
23 def get_process_children(pid):
24 p = subprocess.Popen('ps --no-headers -o pid --ppid %d' % pid, shell=True,
25 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
26- stdout, stderr = p.communicate()
27+ stdout, _stderr = p.communicate()
28
29 return [int(pid) for pid in stdout.split()]
30
31@@ -258,10 +258,15 @@
32 return orig_privs
33
34
35-def gather_artifacts(extras=[], excludes=[]):
36+def gather_artifacts(extras=None, excludes=None):
37 """
38 Include a set of useful artifacts (i.e. files) for the logs.
39 """
40+ if extras is None:
41+ extras = []
42+ if excludes is None:
43+ excludes = []
44+
45 artifacts = ['/var/log/syslog', '/proc/cpuinfo']
46
47 artifacts += extras
48
49=== modified file 'utah/client/phoenix.py'
50--- utah/client/phoenix.py 2012-10-11 20:43:24 +0000
51+++ utah/client/phoenix.py 2012-10-12 09:39:20 +0000
52@@ -13,9 +13,11 @@
53
54 class Phoenix(object):
55
56- def __init__(self, testsuite, directory='.', testcases=['testcase1']):
57+ def __init__(self, testsuite, directory='.', testcases=None):
58 self.testsuite = testsuite
59 self.directory = directory
60+ if testcases is None:
61+ testcases = ['testcase1']
62 self.testcases = testcases
63
64 self.tslist_content = ""
65
66=== modified file 'utah/client/result.py'
67--- utah/client/result.py 2012-10-02 15:59:47 +0000
68+++ utah/client/result.py 2012-10-12 09:39:20 +0000
69@@ -8,14 +8,14 @@
70 )
71
72
73-def get_smoke_data(result):
74+def get_smoke_data(_result):
75 data = {}
76 data['build_no'] = get_build_number()
77
78 return data
79
80
81-def get_kernel_sru_data(result):
82+def get_kernel_sru_data(_result):
83 data = {}
84
85 return data
86@@ -60,7 +60,7 @@
87 self.failures = 0
88 self.passes = 0
89
90- def add_result(self, result, extra_info={}):
91+ def add_result(self, result, extra_info=None):
92 """
93 Add a result to the object.
94
95@@ -78,6 +78,9 @@
96 if result is None:
97 return
98
99+ if extra_info is None:
100+ extra_info = {}
101+
102 # Add items from extra_info into the result
103 if len(extra_info) > 0:
104 result['extra_info'] = extra_info
105@@ -192,8 +195,7 @@
106
107
108 def literal_block(dumper, data):
109- return dumper.represent_scalar('tag:yaml.org,2002:str',
110- data, style='|')
111+ return dumper.represent_scalar('tag:yaml.org,2002:str', data, style='|')
112 yaml.add_representer(LiteralString, literal_block)
113
114
115
116=== modified file 'utah/client/runner.py'
117--- utah/client/runner.py 2012-10-11 20:42:24 +0000
118+++ utah/client/runner.py 2012-10-12 09:39:20 +0000
119@@ -1,8 +1,16 @@
120+<<<<<<< TREE
121 from common import DEFAULT_TSLIST
122 from result import Result
123 from testsuite import TestSuite
124 from state_agent import StateAgentYAML
125 import exceptions
126+=======
127+from utah.client.common import DEFAULT_TSLIST
128+from utah.client.result import Result
129+from utah.client.testsuite import TestSuite
130+from utah.client.state_agent import StateAgentYAML
131+from utah.client import exceptions
132+>>>>>>> MERGE-SOURCE
133
134 import os
135 import shutil
136@@ -10,7 +18,7 @@
137 import urllib
138 import jsonschema
139
140-from common import (
141+from utah.client.common import (
142 MASTER_RUNLIST,
143 UTAH_DIR,
144 DEFAULT_STATE_FILE,
145
146=== modified file 'utah/client/state_agent.py'
147--- utah/client/state_agent.py 2012-08-02 18:36:14 +0000
148+++ utah/client/state_agent.py 2012-10-12 09:39:20 +0000
149@@ -1,7 +1,7 @@
150 import os
151 import yaml
152
153-from common import DEFAULT_STATE_FILE, parse_yaml_file
154+from utah.client.common import DEFAULT_STATE_FILE, parse_yaml_file
155
156
157 class StateAgent(object):
158
159=== modified file 'utah/client/testcase.py'
160--- utah/client/testcase.py 2012-10-04 15:39:50 +0000
161+++ utah/client/testcase.py 2012-10-12 09:39:20 +0000
162@@ -3,10 +3,20 @@
163 """
164 import jsonschema
165
166-from common import run_cmd, parse_control_file
167-from common import do_nothing
168-from common import CMD_TC_BUILD, CMD_TC_SETUP, CMD_TC_TEST, CMD_TC_CLEANUP
169-from exceptions import MissingFile, ValidationError, MissingData
170+from utah.client.common import (
171+ run_cmd,
172+ parse_control_file,
173+ do_nothing,
174+ CMD_TC_BUILD,
175+ CMD_TC_SETUP,
176+ CMD_TC_TEST,
177+ CMD_TC_CLEANUP,
178+ )
179+from utah.client.exceptions import (
180+ MissingFile,
181+ ValidationError,
182+ MissingData,
183+ )
184
185
186 class TestCase(object):
187@@ -175,7 +185,7 @@
188 or self.dependencies is None
189 or self.action is None
190 or self.expected_results is None):
191- raise MissingData
192+ raise MissingData
193
194 # Return value to indicate whether processing of a TestSuite should
195 # continue. This is to avoid a shutdown race on reboot cases.
196
197=== modified file 'utah/client/tests/manual_privileges.py'
198--- utah/client/tests/manual_privileges.py 2012-08-08 12:58:52 +0000
199+++ utah/client/tests/manual_privileges.py 2012-10-12 09:39:20 +0000
200@@ -64,7 +64,7 @@
201 stderr=subprocess.PIPE
202 )
203
204- (stdout, stderr) = proc.communicate()
205+ (stdout, _stderr) = proc.communicate()
206
207 if not quiet:
208 print("{}:\n{}".format(cmd, stdout))
209@@ -74,8 +74,8 @@
210 proc = subprocess.Popen(['ls', '-al', filename], stdout=subprocess.PIPE,
211 stderr=subprocess.PIPE)
212
213- (stdout, stderr) = proc.communicate()
214- print("{}: {}".format(filename, 'stdout'))
215+ stdout, _stderr = proc.communicate()
216+ print("{}: {}".format(filename, stdout))
217
218
219 def get_privs():
220@@ -91,8 +91,8 @@
221
222 class baseTestPriv(unittest.TestCase):
223
224- def assertRoot(self, msg=None):
225- self.assertEqual(os.geteuid(), 0, msg="Must be root")
226+ def assertRoot(self, msg="Must be root"):
227+ self.assertEqual(os.geteuid(), 0, msg=msg)
228
229 def assertPrivs(self, privs, msg=None):
230 # Only check privileges passed in
231
232=== modified file 'utah/client/testsuite.py'
233--- utah/client/testsuite.py 2012-08-28 11:22:41 +0000
234+++ utah/client/testsuite.py 2012-10-12 09:39:20 +0000
235@@ -86,9 +86,7 @@
236 os.mkdir(self.path)
237
238 os.chdir(self.path)
239- """
240- Build a TestSuite from a control file's data.
241- """
242+ # Build a TestSuite from a control file's data.
243 control_file = os.path.join(name, control_file)
244
245 if os.path.exists(control_file):
246
247=== modified file 'utah/config.py'
248--- utah/config.py 2012-10-11 10:59:45 +0000
249+++ utah/config.py 2012-10-12 09:39:20 +0000
250@@ -54,10 +54,8 @@
251 template=None,
252 )
253
254-"""
255-These depend on the local user/path, and need to be filtered out of the config
256-file created at package build time.
257-"""
258+# These depend on the local user/path, and need to be filtered out
259+# of the config file created at package build time.
260 LOCALDEFAULTS = dict(
261 bridge=getbridge(),
262 hostname=socket.gethostname(),
263@@ -68,9 +66,7 @@
264 vmpath=os.path.join(os.path.expanduser('~'), 'vm'),
265 )
266
267-"""
268-These depend on other config options, so they're added last.
269-"""
270+# These depend on other config options, so they're added last.
271 LOCALDEFAULTS['logfile'] = os.path.join(DEFAULTS['logpath'],
272 LOCALDEFAULTS['hostname'] + '.log')
273 LOCALDEFAULTS['debuglog'] = os.path.join(DEFAULTS['logpath'],
274@@ -81,9 +77,7 @@
275 CONFIG = {}
276 CONFIG.update(DEFAULTS)
277
278-"""
279-Process config files.
280-"""
281+# Process config files.
282 files = ['/etc/utah/config', '~/.utah/config', os.getenv('UTAH_CONFIG_FILE')]
283 for conffile in files:
284 if conffile is not None:
285@@ -96,10 +90,8 @@
286
287 def dumpdefaults(build=False):
288 if build:
289- """
290- Remove user and path dependent items so they won't get set on the
291- machine that builds the package.
292- """
293+ # Remove user and path dependent items so they won't get set on the
294+ # machine that builds the package.
295 for item in LOCALDEFAULTS:
296 DEFAULTS.pop(item)
297 return json.dumps(DEFAULTS, sort_keys=True, indent=4)
298@@ -110,7 +102,5 @@
299
300
301 if __name__ == '__main__':
302- """
303- If we're not told otherwise, assume this is the package build.
304- """
305+ # If we're not told otherwise, assume this is the package build.
306 print(dumpdefaults(build=True))
307
308=== modified file 'utah/preseed.py'
309--- utah/preseed.py 2012-10-08 13:25:41 +0000
310+++ utah/preseed.py 2012-10-12 09:39:20 +0000
311@@ -153,7 +153,7 @@
312 return '<{}: {!r}>'.format(self.__class__.__name__, str(self))
313
314 @classmethod
315- def new(self, parent, lines):
316+ def new(cls, parent, lines):
317 """
318 Create new section subclass based on the lines in the preseed
319 """
320@@ -222,7 +222,7 @@
321 self.name = name
322 self.obj_name = '_{}'.format(name)
323
324- def __get__(self, obj, type=None):
325+ def __get__(self, obj, owner=None):
326 if not hasattr(obj, self.obj_name):
327 self.__set__(obj, '')
328 value = getattr(obj, self.obj_name)
329
330=== modified file 'utah/provisioning/provisioning.py'
331--- utah/provisioning/provisioning.py 2012-10-11 10:59:45 +0000
332+++ utah/provisioning/provisioning.py 2012-10-12 09:39:20 +0000
333@@ -324,7 +324,7 @@
334 'done ; '
335 'gdebi -n -q {}'
336 .format(remote_deb))
337- returncode, stdout, stderr = self.run(install_command, root=True)
338+ returncode, _stdout, stderr = self.run(install_command, root=True)
339 if (returncode != 0 or
340 re.search(r'script returned error exit status \d+',
341 stderr)):
342@@ -382,7 +382,7 @@
343 """
344 self._unimplemented(sys._getframe().f_code.co_name)
345
346- def stop(self, force=False):
347+ def stop(self, _force=False):
348 """
349 Stop the machine, and set active to False.
350 Graceful shutdown should be attempted if supported and force is False.
351@@ -391,7 +391,7 @@
352 """
353 self._unimplemented(sys._getframe().f_code.co_name)
354
355- def uploadfiles(self, files, target=os.path.normpath('/tmp/')):
356+ def uploadfiles(self, _files, _target=os.path.normpath('/tmp/')):
357 """
358 Upload a list of local files to a target on the machine and return
359 True if all uploads succeed.
360@@ -489,7 +489,7 @@
361 ssh_client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
362 self.ssh_client = ssh_client
363
364- def run(self, command, quiet=None, root=False, timeout=None):
365+ def run(self, command, _quiet=None, root=False, timeout=None):
366 """
367 Run a command using ssh.
368 """
369
370=== modified file 'utah/provisioning/vm/libvirtvm.py'
371--- utah/provisioning/vm/libvirtvm.py 2012-10-10 08:49:03 +0000
372+++ utah/provisioning/vm/libvirtvm.py 2012-10-12 09:39:20 +0000
373@@ -121,7 +121,7 @@
374 self.vm.shutdown()
375 self.active = False
376
377- def libvirterrorhandler(self, context, err):
378+ def libvirterrorhandler(self, _context, err):
379 """
380 Log libvirt errors instead of sending them directly to the console.
381 """
382@@ -348,7 +348,7 @@
383 """
384 def __init__(self, arch=None, boot=None, diskbus=None, disksizes=None,
385 emulator=None, image=None, initrd=None, installtype=None,
386- kernel=None, machineid=None, macs=[], name=None,
387+ kernel=None, machineid=None, macs=None, name=None,
388 prefix='utah', preseed=None, series=None, xml=None,
389 *args, **kw):
390 # Make sure that no other virtualization solutions are running
391@@ -413,6 +413,8 @@
392 if self.domaintype == 'qemu':
393 self.logger.debug('Raising boot timeout for qemu domain')
394 self.boottimeout *= 4
395+ if macs is None:
396+ macs = []
397 self.macs = macs
398 self.xml = xml
399 self.dircheck()
400
401=== modified file 'utah/run.py'
402--- utah/run.py 2012-10-10 09:46:47 +0000
403+++ utah/run.py 2012-10-12 09:39:20 +0000
404@@ -40,9 +40,18 @@
405 options = ' -r /tmp/' + os.path.basename(locallist)
406 utah_command = 'utah' + extraopts + options
407 try:
408+<<<<<<< TREE
409 returncode, stdout, stderr = machine.run(utah_command)
410 # TODO: Decide which returncode means utah client failure
411 # and which one means test case failure
412+=======
413+ returncode, stdout, _stderr = machine.run(utah_command)
414+ if returncode != 0:
415+ machine.logger.error('utah failed with return code: {}\n'
416+ .format(returncode))
417+ exitstatus = 1
418+ break
419+>>>>>>> MERGE-SOURCE
420 except UTAHException as error:
421 machine.logger.error('Failed to run test: ' + str(error))
422 exitstatus = 1
423
424=== modified file 'utah/timeout.py'
425--- utah/timeout.py 2012-08-30 09:18:10 +0000
426+++ utah/timeout.py 2012-10-12 09:39:20 +0000
427@@ -27,7 +27,7 @@
428 if command is None:
429 return
430
431- def alarm_handler(signum, frame):
432+ def alarm_handler(_signum, _frame):
433 raise UTAHTimeout
434
435 if timeout != 0:
436@@ -66,7 +66,7 @@
437 class TimeoutAlarm(Exception):
438 pass
439
440- def alarm_handler(signum, frame):
441+ def alarm_handler(_signum, _frame):
442 raise TimeoutAlarm
443
444 if timeout != 0:
445@@ -106,6 +106,6 @@
446 """
447 p = subprocess.Popen('ps --no-headers -o pid --ppid %d' % pid, shell=True,
448 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
449- stdout, stderr = p.communicate()
450+ stdout, _stderr = p.communicate()
451
452 return [int(pid) for pid in stdout.split()]
453
454=== modified file 'utah/url.py'
455--- utah/url.py 2012-08-08 15:11:52 +0000
456+++ utah/url.py 2012-10-12 09:39:20 +0000
457@@ -56,7 +56,7 @@
458 Check if local file exists
459 """
460 # Based on urllib.URLopener.open_local_file implementation
461- host, filename = urllib.splithost(url)
462+ _host, filename = urllib.splithost(url)
463 path = os.path.abspath(urllib.url2pathname(filename))
464
465 if not os.path.exists(path):

Subscribers

People subscribed via source and target branches