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

[Javier Collado]

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.

[Joe Talbott (joetalbott)]

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

[lp:~javier.collado/utah/static_analysis_7]

714. By Javier Collado on 2012-10-12

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

715. By Javier Collado on 2012-10-12

Fixed use hardcoded message by default as suggested by Joe

[Javier Collado (javier.collado)]

@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

[Joe Talbott (joetalbott)]

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

review: Approve

[Max Brustkern (nuclearbob)]

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

[Max Brustkern (nuclearbob)]

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

[Joe Talbott (joetalbott)]

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

[Javier Collado (javier.collado)]

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

[Max Brustkern (nuclearbob)]

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

=== modified file 'utah/client/common.py'
--- utah/client/common.py	2012-10-11 20:42:24 +0000
+++ utah/client/common.py	2012-10-12 09:39:20 +0000
@@ -52,7 +52,7 @@
 CMD_TS_CLEANUP = 'testsuite_cleanup'
 
 
-def do_nothing(obj=None):
+def do_nothing(_obj=None):
     """
     Do nothing method used a placeholder for save_state_callbacks.
     """
@@ -78,7 +78,7 @@
     class TimeoutAlarm(Exception):
         pass
 
-    def alarm_handler(signum, frame):
+    def alarm_handler(_signum, _frame):
         raise TimeoutAlarm
 
     start_time = datetime.datetime.now()
@@ -154,7 +154,7 @@
 def get_process_children(pid):
     p = subprocess.Popen('ps --no-headers -o pid --ppid %d' % pid, shell=True,
                          stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-    stdout, stderr = p.communicate()
+    stdout, _stderr = p.communicate()
 
     return [int(pid) for pid in stdout.split()]
 
@@ -258,10 +258,15 @@
     return orig_privs
 
 
-def gather_artifacts(extras=[], excludes=[]):
+def gather_artifacts(extras=None, excludes=None):
     """
     Include a set of useful artifacts (i.e. files) for the logs.
     """
+    if extras is None:
+        extras = []
+    if excludes is None:
+        excludes = []
+
     artifacts = ['/var/log/syslog', '/proc/cpuinfo']
 
     artifacts += extras
 
=== modified file 'utah/client/phoenix.py'
--- utah/client/phoenix.py	2012-10-11 20:43:24 +0000
+++ utah/client/phoenix.py	2012-10-12 09:39:20 +0000
@@ -13,9 +13,11 @@
 
 class Phoenix(object):
 
-    def __init__(self, testsuite, directory='.', testcases=['testcase1']):
+    def __init__(self, testsuite, directory='.', testcases=None):
         self.testsuite = testsuite
         self.directory = directory
+        if testcases is None:
+            testcases = ['testcase1']
         self.testcases = testcases
 
         self.tslist_content = ""
 
=== modified file 'utah/client/result.py'
--- utah/client/result.py	2012-10-02 15:59:47 +0000
+++ utah/client/result.py	2012-10-12 09:39:20 +0000
@@ -8,14 +8,14 @@
         )
 
 
-def get_smoke_data(result):
+def get_smoke_data(_result):
     data = {}
     data['build_no'] = get_build_number()
 
     return data
 
 
-def get_kernel_sru_data(result):
+def get_kernel_sru_data(_result):
     data = {}
 
     return data
@@ -60,7 +60,7 @@
         self.failures = 0
         self.passes = 0
 
-    def add_result(self, result, extra_info={}):
+    def add_result(self, result, extra_info=None):
         """
         Add a result to the object.
 
@@ -78,6 +78,9 @@
         if result is None:
             return
 
+        if extra_info is None:
+            extra_info = {}
+
         # Add items from extra_info into the result
         if len(extra_info) > 0:
             result['extra_info'] = extra_info
@@ -192,8 +195,7 @@
 
 
 def literal_block(dumper, data):
-        return dumper.represent_scalar('tag:yaml.org,2002:str',
-                                       data, style='|')
+    return dumper.represent_scalar('tag:yaml.org,2002:str', data, style='|')
 yaml.add_representer(LiteralString, literal_block)
 
 
 
=== modified file 'utah/client/runner.py'
--- utah/client/runner.py	2012-10-11 20:42:24 +0000
+++ utah/client/runner.py	2012-10-12 09:39:20 +0000
@@ -1,8 +1,16 @@
+<<<<<<< TREE
 from common import DEFAULT_TSLIST
 from result import Result
 from testsuite import TestSuite
 from state_agent import StateAgentYAML
 import exceptions
+=======
+from utah.client.common import DEFAULT_TSLIST
+from utah.client.result import Result
+from utah.client.testsuite import TestSuite
+from utah.client.state_agent import StateAgentYAML
+from utah.client import exceptions
+>>>>>>> MERGE-SOURCE
 
 import os
 import shutil
@@ -10,7 +18,7 @@
 import urllib
 import jsonschema
 
-from common import (
+from utah.client.common import (
         MASTER_RUNLIST,
         UTAH_DIR,
         DEFAULT_STATE_FILE,
 
=== modified file 'utah/client/state_agent.py'
--- utah/client/state_agent.py	2012-08-02 18:36:14 +0000
+++ utah/client/state_agent.py	2012-10-12 09:39:20 +0000
@@ -1,7 +1,7 @@
 import os
 import yaml
 
-from common import DEFAULT_STATE_FILE, parse_yaml_file
+from utah.client.common import DEFAULT_STATE_FILE, parse_yaml_file
 
 
 class StateAgent(object):
 
=== modified file 'utah/client/testcase.py'
--- utah/client/testcase.py	2012-10-04 15:39:50 +0000
+++ utah/client/testcase.py	2012-10-12 09:39:20 +0000
@@ -3,10 +3,20 @@
 """
 import jsonschema
 
-from common import run_cmd, parse_control_file
-from common import do_nothing
-from common import CMD_TC_BUILD, CMD_TC_SETUP, CMD_TC_TEST, CMD_TC_CLEANUP
-from exceptions import MissingFile, ValidationError, MissingData
+from utah.client.common import (
+        run_cmd,
+        parse_control_file,
+        do_nothing,
+        CMD_TC_BUILD,
+        CMD_TC_SETUP,
+        CMD_TC_TEST,
+        CMD_TC_CLEANUP,
+        )
+from utah.client.exceptions import (
+        MissingFile,
+        ValidationError,
+        MissingData,
+        )
 
 
 class TestCase(object):
@@ -175,7 +185,7 @@
                 or self.dependencies is None
                 or self.action is None
                 or self.expected_results is None):
-                raise MissingData
+            raise MissingData
 
         # Return value to indicate whether processing of a TestSuite should
         # continue.  This is to avoid a shutdown race on reboot cases.
 
=== modified file 'utah/client/tests/manual_privileges.py'
--- utah/client/tests/manual_privileges.py	2012-08-08 12:58:52 +0000
+++ utah/client/tests/manual_privileges.py	2012-10-12 09:39:20 +0000
@@ -64,7 +64,7 @@
         stderr=subprocess.PIPE
     )
 
-    (stdout, stderr) = proc.communicate()
+    (stdout, _stderr) = proc.communicate()
 
     if not quiet:
         print("{}:\n{}".format(cmd, stdout))
@@ -74,8 +74,8 @@
     proc = subprocess.Popen(['ls', '-al', filename], stdout=subprocess.PIPE,
             stderr=subprocess.PIPE)
 
-    (stdout, stderr) = proc.communicate()
-    print("{}: {}".format(filename, 'stdout'))
+    stdout, _stderr = proc.communicate()
+    print("{}: {}".format(filename, stdout))
 
 
 def get_privs():
@@ -91,8 +91,8 @@
 
 class baseTestPriv(unittest.TestCase):
 
-    def assertRoot(self, msg=None):
-        self.assertEqual(os.geteuid(), 0, msg="Must be root")
+    def assertRoot(self, msg="Must be root"):
+        self.assertEqual(os.geteuid(), 0, msg=msg)
 
     def assertPrivs(self, privs, msg=None):
         # Only check privileges passed in
 
=== modified file 'utah/client/testsuite.py'
--- utah/client/testsuite.py	2012-08-28 11:22:41 +0000
+++ utah/client/testsuite.py	2012-10-12 09:39:20 +0000
@@ -86,9 +86,7 @@
             os.mkdir(self.path)
 
         os.chdir(self.path)
-        """
-        Build a TestSuite from a control file's data.
-        """
+        # Build a TestSuite from a control file's data.
         control_file = os.path.join(name, control_file)
 
         if os.path.exists(control_file):
 
=== modified file 'utah/config.py'
--- utah/config.py	2012-10-11 10:59:45 +0000
+++ utah/config.py	2012-10-12 09:39:20 +0000
@@ -54,10 +54,8 @@
     template=None,
 )
 
-"""
-These depend on the local user/path, and need to be filtered out of the config
-file created at package build time.
-"""
+# These depend on the local user/path, and need to be filtered out
+# of the config file created at package build time.
 LOCALDEFAULTS = dict(
     bridge=getbridge(),
     hostname=socket.gethostname(),
@@ -68,9 +66,7 @@
     vmpath=os.path.join(os.path.expanduser('~'), 'vm'),
 )
 
-"""
-These depend on other config options, so they're added last.
-"""
+# These depend on other config options, so they're added last.
 LOCALDEFAULTS['logfile'] = os.path.join(DEFAULTS['logpath'],
                                         LOCALDEFAULTS['hostname'] + '.log')
 LOCALDEFAULTS['debuglog'] = os.path.join(DEFAULTS['logpath'],
@@ -81,9 +77,7 @@
 CONFIG = {}
 CONFIG.update(DEFAULTS)
 
-"""
-Process config files.
-"""
+# Process config files.
 files = ['/etc/utah/config', '~/.utah/config', os.getenv('UTAH_CONFIG_FILE')]
 for conffile in files:
     if conffile is not None:
@@ -96,10 +90,8 @@
 
 def dumpdefaults(build=False):
     if build:
-        """
-        Remove user and path dependent items so they won't get set on the
-        machine that builds the package.
-        """
+        # Remove user and path dependent items so they won't get set on the
+        # machine that builds the package.
         for item in LOCALDEFAULTS:
             DEFAULTS.pop(item)
     return json.dumps(DEFAULTS, sort_keys=True, indent=4)
@@ -110,7 +102,5 @@
 
 
 if __name__ == '__main__':
-    """
-    If we're not told otherwise, assume this is the package build.
-    """
+    # If we're not told otherwise, assume this is the package build.
     print(dumpdefaults(build=True))
 
=== modified file 'utah/preseed.py'
--- utah/preseed.py	2012-10-08 13:25:41 +0000
+++ utah/preseed.py	2012-10-12 09:39:20 +0000
@@ -153,7 +153,7 @@
         return '<{}: {!r}>'.format(self.__class__.__name__, str(self))
 
     @classmethod
-    def new(self, parent, lines):
+    def new(cls, parent, lines):
         """
         Create new section subclass based on the lines in the preseed
         """
@@ -222,7 +222,7 @@
         self.name = name
         self.obj_name = '_{}'.format(name)
 
-    def __get__(self, obj, type=None):
+    def __get__(self, obj, owner=None):
         if not hasattr(obj, self.obj_name):
             self.__set__(obj, '')
         value = getattr(obj, self.obj_name)
 
=== modified file 'utah/provisioning/provisioning.py'
--- utah/provisioning/provisioning.py	2012-10-11 10:59:45 +0000
+++ utah/provisioning/provisioning.py	2012-10-12 09:39:20 +0000
@@ -324,7 +324,7 @@
                     'done ; '
                     'gdebi -n -q {}'
                     .format(remote_deb))
-            returncode, stdout, stderr = self.run(install_command, root=True)
+            returncode, _stdout, stderr = self.run(install_command, root=True)
             if (returncode != 0 or
                     re.search(r'script returned error exit status \d+',
                               stderr)):
@@ -382,7 +382,7 @@
         """
         self._unimplemented(sys._getframe().f_code.co_name)
 
-    def stop(self, force=False):
+    def stop(self, _force=False):
         """
         Stop the machine, and set active to False.
         Graceful shutdown should be attempted if supported and force is False.
@@ -391,7 +391,7 @@
         """
         self._unimplemented(sys._getframe().f_code.co_name)
 
-    def uploadfiles(self, files, target=os.path.normpath('/tmp/')):
+    def uploadfiles(self, _files, _target=os.path.normpath('/tmp/')):
         """
         Upload a list of local files to a target on the machine and return
         True if all uploads succeed.
@@ -489,7 +489,7 @@
         ssh_client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
         self.ssh_client = ssh_client
 
-    def run(self, command, quiet=None, root=False, timeout=None):
+    def run(self, command, _quiet=None, root=False, timeout=None):
         """
         Run a command using ssh.
         """
 
=== modified file 'utah/provisioning/vm/libvirtvm.py'
--- utah/provisioning/vm/libvirtvm.py	2012-10-10 08:49:03 +0000
+++ utah/provisioning/vm/libvirtvm.py	2012-10-12 09:39:20 +0000
@@ -121,7 +121,7 @@
                 self.vm.shutdown()
         self.active = False
 
-    def libvirterrorhandler(self, context, err):
+    def libvirterrorhandler(self, _context, err):
         """
         Log libvirt errors instead of sending them directly to the console.
         """
@@ -348,7 +348,7 @@
     """
     def __init__(self, arch=None, boot=None, diskbus=None, disksizes=None,
                  emulator=None, image=None, initrd=None, installtype=None,
-                 kernel=None, machineid=None, macs=[], name=None,
+                 kernel=None, machineid=None, macs=None, name=None,
                  prefix='utah', preseed=None, series=None, xml=None,
                  *args, **kw):
         # Make sure that no other virtualization solutions are running
@@ -413,6 +413,8 @@
         if self.domaintype == 'qemu':
             self.logger.debug('Raising boot timeout for qemu domain')
             self.boottimeout *= 4
+        if macs is None:
+            macs = []
         self.macs = macs
         self.xml = xml
         self.dircheck()
 
=== modified file 'utah/run.py'
--- utah/run.py	2012-10-10 09:46:47 +0000
+++ utah/run.py	2012-10-12 09:39:20 +0000
@@ -40,9 +40,18 @@
         options = ' -r /tmp/' + os.path.basename(locallist)
         utah_command = 'utah' + extraopts + options
         try:
+<<<<<<< TREE
             returncode, stdout, stderr = machine.run(utah_command)
             # TODO: Decide which returncode means utah client failure
             # and which one means test case failure
+=======
+            returncode, stdout, _stderr = machine.run(utah_command)
+            if returncode != 0:
+                machine.logger.error('utah failed with return code: {}\n'
+                                     .format(returncode))
+                exitstatus = 1
+                break
+>>>>>>> MERGE-SOURCE
         except UTAHException as error:
             machine.logger.error('Failed to run test: ' + str(error))
             exitstatus = 1
 
=== modified file 'utah/timeout.py'
--- utah/timeout.py	2012-08-30 09:18:10 +0000
+++ utah/timeout.py	2012-10-12 09:39:20 +0000
@@ -27,7 +27,7 @@
     if command is None:
         return
 
-    def alarm_handler(signum, frame):
+    def alarm_handler(_signum, _frame):
         raise UTAHTimeout
 
     if timeout != 0:
@@ -66,7 +66,7 @@
     class TimeoutAlarm(Exception):
         pass
 
-    def alarm_handler(signum, frame):
+    def alarm_handler(_signum, _frame):
         raise TimeoutAlarm
 
     if timeout != 0:
@@ -106,6 +106,6 @@
     """
     p = subprocess.Popen('ps --no-headers -o pid --ppid %d' % pid, shell=True,
                          stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-    stdout, stderr = p.communicate()
+    stdout, _stderr = p.communicate()
 
     return [int(pid) for pid in stdout.split()]
 
=== modified file 'utah/url.py'
--- utah/url.py	2012-08-08 15:11:52 +0000
+++ utah/url.py	2012-10-12 09:39:20 +0000
@@ -56,7 +56,7 @@
         Check if local file exists
         """
         # Based on urllib.URLopener.open_local_file implementation
-        host, filename = urllib.splithost(url)
+        _host, filename = urllib.splithost(url)
         path = os.path.abspath(urllib.url2pathname(filename))
 
         if not os.path.exists(path):