Merge lp:~nuclearbob/utah/docstring-cleanup into lp:utah

[Max Brustkern]

This branch cleans up docstrings to be PEP257 compliant, updates string formatting, and addresses a number of minor issues raised during the code review while attempting to minimize potential functionality changes.

[Max Brustkern (nuclearbob)]

I'm still testing provisioning and some client self tests are failing, but I'd still like to get some other eyes on this since it's a big one.

[Max Brustkern (nuclearbob)]

All the provisioning bugs are fixed now, so if anyone wants to look things over or help with the client self-test issues, that would be rad.

[Andy Doan (doanac)]

Looks good. How bout we add a unit test to cover the get_children_process change. here's something i hacked together:

 http://paste.ubuntu.com/5680415/

[lp:~nuclearbob/utah/docstring-cleanup]

961. By Max Brustkern on 2013-04-05

Accidentally removed a function when converting to private, reinstating

962. By Max Brustkern on 2013-04-05

Converting pid to string before passing it to subprocess

963. By Max Brustkern on 2013-04-05

I misunderstood the difference between revision and get_revision, and get_revision was only used by tests, so I'm using the correct revision as revision now

[Max Brustkern (nuclearbob)]

I've fixed all but one of the client test issues. I'm still not raising MissingData, because we decided in the code review to take out the block of code that raises that. I think some other exception should be raised, however, and that doesn't appear to be happening. I think the lack of another exception is not a problem with this merge, but we should either fix that problem, or reinstate the MissingData block before moving forward. I'm in favor of fixing the problem, so I'm going to try for that.

[lp:~nuclearbob/utah/docstring-cleanup]

964. By Max Brustkern on 2013-04-05

utah/client/tests/test_testcase.py

[Max Brustkern (nuclearbob)]

I've removed that test. The DefaultValidator isn't working, and I think we should have some different tests for it. I've opened a bug:
https://bugs.launchpad.net/utah/+bug/1165175
and I'll work on that separately. I think we should have probably at least three tests for the DefaultValidator:
test that it validates valid data
test that it fails to validate bad data
test that it populates a default if needed
and maybe
test that not supplying a default doesn't cause validation to fail?

Additionally, in test_testcase, I think we should test for:
missing field
extra field
bad reboot
bad timeout
bad type

I'll add that info to the bug as well.

[lp:~nuclearbob/utah/docstring-cleanup]

965. By Max Brustkern on 2013-04-05

Removed unneeded exception and backed out my testing changes to DefaultValidator

966. By Max Brustkern on 2013-04-08

Fixed static analysis errors, added test suggested by Andy, removed correct exception

[Max Brustkern (nuclearbob)]

> Looks good. How bout we add a unit test to cover the get_children_process
> change. here's something i hacked together:
>
> http://paste.ubuntu.com/5680415/

Added that. Good idea.

[Andy Doan (doanac)]

common.py:validate_properties has some extra "print" statements that seem to be left over from debugging.

[Andy Doan (doanac)]

You removed the MissingData check:

        # Do not proceed if we are missing data
        if (self.description is None
                or self.dependencies is None
                or self.action is None
                or self.expected_results is None):
            raise MissingData

However, I discovered a bug the other data in this same place. Our DefaultValidator is broken and doesn't raise errors when this data is missing. So this can't be removed until we fix that code. I glanced at fixing it, but haven't gotten around to making it work. NOTE: we did our own DefaultValidator so that we could set default attributes on our object.

[Andy Doan (doanac)]

rsyslogd.py
            # TODO: Find out if this can/should be None
            self._port = 0

the TODO line that was added isn't needed. This line is required and safe.

[Andy Doan (doanac)]

should _UTAHISOException really be listed as private? We don't seem to catch this in our code, so I *think* its a user visible exception?

[Max Brustkern (nuclearbob)]

I'll fix the print statements.

I removed the MissingData check because we said in the code review that we wanted to remove that since we're using the validator now. The validator code that's in dev right now (and I think production) is already broken, and I don't think removing that check will make things worse. We can leave it in until that's fixed if you want, but either way, we should fix it soon, since we're currently behaving in confusing and bad ways when you try to run a bad tc_control file.

That TODO was for me, and I forget to get to it before I finished the merge. Whoops. I did want to ask you about that, when we're using a file, do we want port to be 0 or would it be better as None? Does the code to send the rsyslog arguments to the kernel command line still check if that value is None? I don't think we need those parameters if we're using a file, so I wanted to check if we were still setting them.

You make a good point about the exception. I think I went a little overboard making things private. I'll roll that back.

[Andy Doan (doanac)]

On 04/08/2013 11:03 AM, Max Brustkern wrote:
> I removed the MissingData check because we said in the code review
> that we wanted to remove that since we're using the validator now.
> The validator code that's in dev right now (and I think production)
> is already broken, and I don't think removing that check will make
> things worse.

If you remove this, then we need to assign those attributes default
values in the schema so that they get set. Otherwise, I think a "bad"
control file could cause us to try and reference variables that haven't
been initialized.

> We can leave it in until that's fixed if you want, but
> either way, we should fix it soon, since we're currently behaving in
> confusing and bad ways when you try to run a bad tc_control file.

I would like to fix it soon. Just not sure we want to fit it in this.

[Andy Doan (doanac)]

On 04/08/2013 11:03 AM, Max Brustkern wrote:
> That TODO was for me, and I forget to get to it before I finished the
> merge. Whoops. I did want to ask you about that, when we're using a
> file, do we want port to be 0 or would it be better as None? Does
> the code to send the rsyslog arguments to the kernel command line
> still check if that value is None? I don't think we need those
> parameters if we're using a file, so I wanted to check if we were
> still setting them.

I think it can be None or 0. If you look at provisioning.py we have a check:

  if self.rsyslog.port:
    ... (setup rsylog'ing)
  else:
    ... (set up serial console logging)

Maybe a smarter thing would be to make this more explicit like:

  if self.rsyslog.local_file()

not sure.

[lp:~nuclearbob/utah/docstring-cleanup]

967. By Max Brustkern on 2013-04-08

Implemented some fixes suggested by Andy

[Max Brustkern (nuclearbob)]

> On 04/08/2013 11:03 AM, Max Brustkern wrote:
> > That TODO was for me, and I forget to get to it before I finished the
> > merge. Whoops. I did want to ask you about that, when we're using a
> > file, do we want port to be 0 or would it be better as None? Does
> > the code to send the rsyslog arguments to the kernel command line
> > still check if that value is None? I don't think we need those
> > parameters if we're using a file, so I wanted to check if we were
> > still setting them.
>
> I think it can be None or 0. If you look at provisioning.py we have a check:
>
> if self.rsyslog.port:
> ... (setup rsylog'ing)
> else:
> ... (set up serial console logging)
>
> Maybe a smarter thing would be to make this more explicit like:
>
> if self.rsyslog.local_file()
>
> not sure.

I think I originally wrote things as 'if config.rsyslogport is not None' and I forgot that you had improved on that by testing the value directly, which will return False on a 0. Good stuff. I implemented the three things you said other than the MissingData, which I'll comment on separately.

[Max Brustkern (nuclearbob)]

> On 04/08/2013 11:03 AM, Max Brustkern wrote:
> > I removed the MissingData check because we said in the code review
> > that we wanted to remove that since we're using the validator now.
> > The validator code that's in dev right now (and I think production)
> > is already broken, and I don't think removing that check will make
> > things worse.
>
> If you remove this, then we need to assign those attributes default
> values in the schema so that they get set. Otherwise, I think a "bad"
> control file could cause us to try and reference variables that haven't
> been initialized.
>
> > We can leave it in until that's fixed if you want, but
> > either way, we should fix it soon, since we're currently behaving in
> > confusing and bad ways when you try to run a bad tc_control file.
>
> I would like to fix it soon. Just not sure we want to fit it in this.

My feeling is, this is already broken. I don't see a MissingData exception get raised when I feed in a file with bad data, even without that change in place, so I don't think that code is doing anything useful. Maybe that means we should leave it in until the branch that fixes it (which I'm hoping to propose today or tomorrow.) As I'm typing this, that actually makes more sense. If nobody disagrees, I can put that block and the test back in. That will mean the test is failing, but the correction for that is something we're going to want as a bugfix anyway, so landing it separately makes sense.

[Andy Doan (doanac)]

On 04/08/2013 12:33 PM, Max Brustkern wrote:
> My feeling is, this is already broken. I don't see a MissingData
> exception get raised when I feed in a file with bad data, even
> without that change in place, so I don't think that code is doing
> anything useful.

hmm - I've seen it. it was what prompted me to start on:

  https://code.launchpad.net/~doanac/utah/schema-bug/+merge/156215

> Maybe that means we should leave it in until the
> branch that fixes it (which I'm hoping to propose today or tomorrow.)
> As I'm typing this, that actually makes more sense. If nobody
> disagrees, I can put that block and the test back in. That will mean
> the test is failing, but the correction for that is something we're
> going to want as a bugfix anyway, so landing it separately makes
> sense.

okay

[Andy Doan (doanac)]

+1

[lp:~nuclearbob/utah/docstring-cleanup]

968. By Max Brustkern on 2013-04-08

Put MissingData bits back in. We'll resolve that in a separate branch.

969. By Max Brustkern on 2013-04-08

Static analysis fixed

[Max Brustkern (nuclearbob)]

Okay, all of the issues you raised should be fixed now. We can handle the MissingData thing with a new validator in a separate branch.

review: Needs Resubmitting

[Andy Doan (doanac)]

+1 for me.

review: Approve

[Javier Collado (javier.collado)]

Thanks for all the effort on this branch.

The changes look good and I've been able to successfully execute the pass
runlist. However, there are still a few issues reported by the pep257 tool:
------------
provisioning.py:76:8: PEP257 Return value type should be mentioned.
runner.py:415:8: PEP257 Return value type should be mentioned.
setup.py:1:0: PEP257 Modules should have docstrings.
------------

I don't think they are really important, but if you could fix them before
merging to the development branch, that would be great.

[Max Brustkern (nuclearbob)]

I didn't see this before I merged, but the first two are false positives. Those functions both define functions internally that have return statements, but those functions themselves do not have return statements. I think we should separate the internal functions to get rid of those warnings.

As to setup.py, I was mostly ignoring that, along with distribute_setup.py, but I forgot we actually do maintain setup.py. I'll address that issue before release, and we can take care of the other ones later since they involve restructuing.

[Javier Collado (javier.collado)]

That's interesting, I didn't know why the error was triggered just for some
methods.

Looking at them I see that:
- one of them could be a function at the module level because is defined there
  just for the convenience of having it close to where it's used, but it's not
  really part of the method.
- the other one uses a variable from the outer scope. Hence the signature must
  be change if it's moved as an instance method in the class itself.

I don't like much the second bullet (changing the signature), so I've made a
couple of tests to verify that adding 'return' to the documentation is enough
to get rid of the error message. Consequently, if we stick to use sphinx
markup, then we could just use `:returns: None` after all the `:param:` and
`:type:` tags and the error message would be gone. What do you think about this
option?

By the way, some other detail is that the private methods are not added by
default to the documentation, which I think is fine, but that means that the
documentation for `__init__` sometimes has to be moved to the class docstring
to make it available in the documentation. I've done that for a couple of
classes in the code and I think it applies to the `Machine` class as well.

[Max Brustkern (nuclearbob)]

I don't like that second bullet either. `:returns: None` sounds like a good solution.

That makes me wonder what, if any, other private methods we do want documented. We can make another merge proposal to move __init__ documentation up to the class level for things that need it.

=== modified file 'client.py'
--- client.py	2013-04-02 13:56:33 +0000
+++ client.py	2013-04-08 19:17:22 +0000
@@ -15,6 +15,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"UTAH client used to run tests."""
+
+
 import logging
 import platform
 import sys
@@ -33,9 +36,7 @@
 
 
 def get_parser():
-    """
-    Process the command line arguments.
-    """
+    """Process the command line arguments."""
     # requires Python2.7+
     import argparse
 
@@ -48,8 +49,8 @@
     parser.add_argument('--resume', action='store_true',
                         help='Continue a previous run.  Used after a reboot')
     parser.add_argument('-s', '--state-file',
-                        help=('File to use for storing state (default "%s")'
-                              % DEFAULT_STATE_FILE))
+                        help=('File to use for storing state (default "{}")'
+                              .format(DEFAULT_STATE_FILE)))
     parser.add_argument('-f', '--format',
                         choices=['text', 'yaml', 'json'],
                         default='yaml',
@@ -87,9 +88,7 @@
 
 
 def main():
-    """
-    The main driver for the 'utah' application.
-    """
+    """Interpret command line arguments and run tests."""
     # TODO: write <2.7 optparse version and set based on version of python
     #       being used.
     parser = get_parser()

=== modified file 'debian/changelog'
--- debian/changelog	2013-04-04 14:58:12 +0000
+++ debian/changelog	2013-04-08 19:17:22 +0000
@@ -1,5 +1,6 @@
 utah (0.10ubuntu1) UNRELEASED; urgency=low
 
+  [ Javier Collado ]
   * Return error code on unhandled error (LP: #1160857)
   * Remove temporary files downloaded based on URL (LP: #1101186)
   * Stop server on installation failure (LP: #1161855)
@@ -9,7 +10,23 @@
   * Use sys.exit when SIGTERM is received (LP: #1160247)
   * Write syslog pattern matched and timeouts timestamps (LP: #1162862)
 
- -- Javier Collado <[e-mail]>  Wed, 27 Mar 2013 13:00:47 +0100
+  [ Max Brustkern ]
+  * Cleaned up docstrings to be PEP257 compliant
+  * Improved string formatting
+  * Harmonized Machine provision method signatures and returns
+  * Separated VCS handling into its own file and simplified revision
+    handling
+  * Privatized public functions
+  * Removed publishing code
+  * Cleaned up import ordering
+  * Refactored kernel command line processing
+  * Added local copy of specified kernel and initrd
+  * Removed useless comments
+  * Improved exception handling
+  * Improved logging
+  * Cleaned up issues raised during code review
+
+ -- Max Brustkern <[e-mail]>  Thu, 04 Apr 2013 12:29:54 -0400
 
 utah (0.9.2ubuntu1) quantal; urgency=low
 

=== modified file 'docs/source/conf.py'
--- docs/source/conf.py	2012-12-06 16:08:13 +0000
+++ docs/source/conf.py	2013-04-08 19:17:22 +0000
@@ -28,10 +28,13 @@
 # All configuration values have a default; values that are commented out
 # serve to show the default.
 
-import sys
+"""Provide documentation configuration."""
+
+
+import itertools
 import os
-import itertools
 import re
+import sys
 
 # If extensions (or modules to document with autodoc) are in another directory,
 # add these directories to sys.path here. If the directory is relative to the
@@ -41,10 +44,9 @@
 
 
 class ModuleMock(object):
-    """
-    Mock class to avoid import errors when building the documentation in
-    readthedocs.org
-    """
+
+    """Mock class to avoid import errors."""
+
     def __init__(self, name):
         self.__name__ = name
 
@@ -69,10 +71,9 @@
 
 
 class ClassMock(object):
-    """
-    Mock class to avoid module member access errors when building the
-    documentation in readthedocs.org
-    """
+
+    """Mock class to avoid import errors."""
+
     def __init__(self, *args, **kwargs):
         pass
 
@@ -84,8 +85,13 @@
     sys.modules[mod_name] = ModuleMock(mod_name)
 
 
-# Autogenerate help contents for manual pages when building manual pages
 def get_parser_strings(script_name, parser):
+    """Autogenerate help contents for manual pages when building them.
+
+    :returns: description, options, and epilog
+    :rtype: tuple
+
+    """
     description = parser.description
 
     help_lines = parser.format_help().splitlines()
@@ -98,9 +104,11 @@
     options_lines.next()  # Drop first line
 
     def fix_line(line):
-        """
-        Remove indentation and replace curly braces with angle brackets
-        to match the expected format
+        """Format argparse output.
+
+        Remove indentation and replace curly braces with angle brackets to
+        match the expected format.
+
         """
         line = line[2:]
 

=== modified file 'examples/run_install_test.py'
--- examples/run_install_test.py	2013-04-03 14:25:21 +0000
+++ examples/run_install_test.py	2013-04-08 19:17:22 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
 
 # Ubuntu Testing Automation Harness
 # Copyright 2012 Canonical Ltd.
@@ -15,6 +15,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Create a VM and run a test."""
+
+
 import argparse
 import logging
 import sys

=== modified file 'examples/run_test_bamboo_feeder.py'
--- examples/run_test_bamboo_feeder.py	2013-04-03 14:25:21 +0000
+++ examples/run_test_bamboo_feeder.py	2013-04-08 19:17:22 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
 
 # Ubuntu Testing Automation Harness
 # Copyright 2012 Canonical Ltd.
@@ -15,6 +15,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Provision a panda board in a bamboo-feeder setup and run a test."""
+
+
 import argparse
 import os
 import sys

=== modified file 'examples/run_test_cobbler.py'
--- examples/run_test_cobbler.py	2013-04-03 14:25:21 +0000
+++ examples/run_test_cobbler.py	2013-04-08 19:17:22 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
 
 # Ubuntu Testing Automation Harness
 # Copyright 2012 Canonical Ltd.
@@ -15,6 +15,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Provision a machine with cobbler and run a test."""
+
+
 import argparse
 import sys
 import logging

=== modified file 'examples/run_test_vm.py'
--- examples/run_test_vm.py	2013-04-03 14:25:21 +0000
+++ examples/run_test_vm.py	2013-04-08 19:17:22 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
 
 # Ubuntu Testing Automation Harness
 # Copyright 2012 Canonical Ltd.
@@ -15,6 +15,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Create a VM and run a test."""
+
+
 import argparse
 import logging
 import sys

=== modified file 'examples/run_utah_tests.py'
--- examples/run_utah_tests.py	2013-04-03 14:25:21 +0000
+++ examples/run_utah_tests.py	2013-04-08 19:17:22 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
 
 # Ubuntu Testing Automation Harness
 # Copyright 2012 Canonical Ltd.
@@ -15,6 +15,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Provision a machine a run a test."""
+
+
 import argparse
 import logging
 import sys

=== modified file 'tests/test_rsyslog.py'
--- tests/test_rsyslog.py	2013-03-29 16:22:16 +0000
+++ tests/test_rsyslog.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test syslog processing functionality."""
+
+
 import logging
 import socket
 import time
@@ -25,13 +28,17 @@
 
 
 class TestRsyslog(unittest.TestCase):
+
+    """Test syslog processing code."""
+
     logger = logging.getLogger()
     logger.setLevel(logging.DEBUG)
     logging.getLogger('rsyslog').addHandler(logging.StreamHandler())
 
     @staticmethod
     def producer(port, messages):
-        print 'sending fake messages to port: %d' % port
+        """Send messages to a port for testing."""
+        print('sending fake messages to port: {}'.format(port))
         s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
         s.connect(('localhost', port))
         for m in messages:
@@ -40,6 +47,7 @@
 
     @staticmethod
     def file_producer(f, messages, truncate=False):
+        """Produce a file for testing."""
         for m in messages:
             f.write(m)
             f.write('\n')
@@ -52,9 +60,7 @@
                 truncate = False
 
     def test_easy(self):
-        """
-        minimal test to make sure we can match a typical first message
-        """
+        """Make sure we can match a typical first message."""
         steps = [
             {
                 "message": "test_easy",
@@ -94,10 +100,7 @@
         self.assertRaises(UTAHException, r.wait_for_install, steps)
 
     def test_future(self):
-        """
-        test to make sure we can handle missing a message and understanding
-        where things are in the future
-        """
+        """Test handling a missing message."""
         steps = [
             {
                 "message": "test_future",
@@ -134,9 +137,7 @@
         self.assertTrue(self.test_future_booted, 'booted callback not made')
 
     def test_callbacks(self):
-        """
-        minimal test to make sure wait_for_steps works
-        """
+        """Make sure wait_for_steps works."""
         steps = [
             {
                 "message": "test callbacks",
@@ -182,9 +183,7 @@
         self.assertTrue(self.test_callbacks_blah, 'blah callback not made')
 
     def test_booted(self):
-        """
-        minimal test to make sure wait_for_booted works
-        """
+        """Make sure wait_for_booted works."""
         steps = [
             {
                 "message": "test booted",
@@ -203,9 +202,7 @@
         r.wait_for_booted(steps)
 
     def usefile(self, truncate):
-        """
-        minimal test to make sure wait_for_booted works when tailing a file
-        """
+        """Make sure wait_for_booted works when tailing a file."""
         steps = [
             {
                 "message": "truncate test",
@@ -229,7 +226,9 @@
             r.wait_for_booted(steps)
 
     def test_usefile(self):
+        """Test tailing a file."""
         self.usefile(False)
 
     def test_usefile_truncation(self):
+        """Test tailing a file with truncation."""
         self.usefile(True)

=== modified file 'utah-done.py'
--- utah-done.py	2013-03-15 13:13:28 +0000
+++ utah-done.py	2013-04-08 19:17:22 +0000
@@ -10,6 +10,7 @@
 - Fetch command failure
 - Setup command failure
 UNKNOWN: Unable to retrieve state file to check if client finished properly.
+
 """
 
 

=== modified file 'utah/__init__.py'
--- utah/__init__.py	2012-12-03 14:14:15 +0000
+++ utah/__init__.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,4 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-utah package
-"""
+"""utah"""

=== modified file 'utah/cleanup.py'
--- utah/cleanup.py	2013-04-03 14:25:21 +0000
+++ utah/cleanup.py	2013-04-08 19:17:22 +0000
@@ -15,6 +15,7 @@
 
 """Generic functionality to execute callbacks on exit."""
 
+
 import atexit
 import logging
 import os
@@ -67,7 +68,7 @@
 
         """
         timeout, command, args, kw = function
-        self.logger.debug('Running: ' +
+        self.logger.debug('Running: %s',
                           commandstr(command, *args, **kw))
         try:
             utah.timeout.timeout(timeout, command, *args, **kw)
@@ -102,7 +103,7 @@
 
         """
         if os.path.islink(path):
-            self.logger.debug('Removing link ' + path)
+            self.logger.debug('Removing link %s', path)
             os.unlink(path)
         elif os.path.isfile(path):
             self._clean_file(path)
@@ -121,14 +122,14 @@
         :type path: str
 
         """
-        self.logger.debug('Changing permissions of ' + path)
+        self.logger.debug('Changing permissions of %s', path)
         try:
             os.chmod(path, 0664)
         except OSError as err:
             self.logger.warning(
                 'OSError when changing file permissions: {}'
                 .format(str(err)))
-        self.logger.debug('Removing file ' + path)
+        self.logger.debug('Removing file %s', path)
         try:
             os.unlink(path)
         except OSError as err:

=== modified file 'utah/client/__init__.py'
--- utah/client/__init__.py	2012-12-03 14:31:27 +0000
+++ utah/client/__init__.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,4 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-utah.client
-"""
+"""utah.client"""

=== modified file 'utah/client/battery.py'
--- utah/client/battery.py	2013-03-13 12:13:12 +0000
+++ utah/client/battery.py	2013-04-08 19:17:22 +0000
@@ -1,11 +1,30 @@
+# Ubuntu Testing Automation Harness
+# Copyright 2013 Canonical Ltd.
+
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+
 """Helpers for battery measurements."""
+
+
 import os
 import re
 import logging
 
 
 class _Battery(object):
+
     """Battery information gathering."""
+
     POWER_SUPPLY_DIR = '/sys/class/power_supply'
     BATTERY_DIR_REGEX = re.compile(r'bat.*', re.IGNORECASE)
     BATTERY_FILE_REGEX = re.compile(r'(.*_now|capacity|status)')
@@ -14,13 +33,14 @@
         self.filenames = None
 
     def get_data(self):
-        """Get data from the battery information files
+        """Get data from the battery information files.
 
         Every file is read and the contents of the file is written to a
         dictionary using as key the basename of the file.
 
         :returns: Data as read from the battery information files
         :rtype: dict
+
         """
         def read_file(filename):
             """Read contents of a file and try to convert data to integer

=== modified file 'utah/client/common.py'
--- utah/client/common.py	2013-04-02 13:56:33 +0000
+++ utah/client/common.py	2013-04-08 19:17:22 +0000
@@ -15,25 +15,27 @@
 
 """UTAH client common classes and functions."""
 
+
 import datetime
-import re
-import grp
 import getpass
-import json
 import os
 import platform
 import pwd
+import re
 import signal
 import subprocess
+import time
+
+import jsonschema
 import yaml
-import jsonschema
+
+from utah.client.battery import battery
 
 from utah.client.exceptions import (
     YAMLParsingError,
     YAMLEmptyFile,
 )
 
-from utah.client.battery import battery
 
 CONFIG = {
     'DEBUG': False,
@@ -52,19 +54,29 @@
 CLIENT_CONFIG = os.path.join(UTAH_DIR, 'config', 'client.json')
 DEFAULT_STATE_FILE = os.path.join(UTAH_DIR, "state.yaml")
 
-
-# UTAH client return codes
-# PASS: All test cases were executed and passed
-# FAIL: All test cases were executed, but at least one of them failed
-# ERROR: At least one error was detected that prevented a test case from being
-# executed. Examples of situations that are considered an error are:
-# - Fetch command failure
-# - Setup command failure
-# REBOOT: The system under test rebooted as required by a test case. To get
-# final result the state file has to be checked.
-# UNKNOWN: Unable to retrieve state file to check if client finished properly.
-# INVALID_GROUP: The client was launched with a user other than root.
+MEDIA_INFO = '/var/log/installer/media-info'
+PRODUCT_UUID = '/sys/class/dmi/id/product_uuid'
+
+
 class ReturnCodes:
+
+    """Provide consistent return codes for UTAH client.
+
+    PASS: All test cases were executed and passed
+    FAIL: All test cases were executed, but at least one of them failed
+    ERROR: At least one error was detected that prevented a test case from
+    being executed. Examples of situations that are considered an error are:
+    - Fetch command failure
+    - Setup command failure
+    REBOOT: The system under test rebooted as required by a test case. To get
+    final result the state file has to be checked.
+    UNKNOWN: Unable to retrieve state file to check if client finished
+    properly.
+    INVALID_USER: The client was launched with a user other than root.
+    EXCEPTION_ERROR: An exception error was encountered.
+
+    """
+
     PASS = 0
     FAIL = 1
     ERROR = 2
@@ -73,6 +85,7 @@
     INVALID_USER = 5
     EXCEPTION_ERROR = 6
 
+
 CMD_TC_BUILD = 'testcase_build'
 CMD_TC_SETUP = 'testcase_setup'
 CMD_TC_TEST = 'testcase_test'
@@ -164,20 +177,22 @@
 
     try:
         stdout, stderr = p.communicate()
-        stdout = normalize_encoding(stdout)
-        stderr = normalize_encoding(stderr)
+        stdout = _normalize_encoding(stdout)
+        stderr = _normalize_encoding(stderr)
 
         if timeout != 0:
             signal.alarm(0)
     except TimeoutAlarm:
         pids = [p.pid]
         # Kill p's children too.
-        pids.extend(get_process_children(p.pid))
+        pids.extend(_get_process_children(p.pid))
 
         for pid in pids:
             # process might have died before getting to this line
             # so wrap to avoid OSError: no such process
             try:
+                os.kill(pid, signal.SIGTERM)
+                time.sleep(5)
                 os.kill(pid, signal.SIGKILL)
             except OSError:
                 pass
@@ -212,7 +227,7 @@
     return make_result(**kwargs)
 
 
-def normalize_encoding(value, encoding='utf-8'):
+def _normalize_encoding(value, encoding='utf-8'):
     """Normalize string encoding.
 
     Make sure that byte strings are used only for ascii data and unicode
@@ -226,7 +241,10 @@
     :rtype: str
 
     """
-    unicode_value = value.decode(encoding)
+    try:
+        unicode_value = value.decode(encoding)
+    except UnicodeDecodeError:
+        return value
     try:
         output = unicode_value.encode('ascii')
     except UnicodeEncodeError:
@@ -291,7 +309,7 @@
     return res
 
 
-def get_process_children(pid):
+def _get_process_children(pid):
     """Get the children processes of a given one.
 
     :param pid: Process ID of the parent process
@@ -300,11 +318,12 @@
     :rtype: list(int)
 
     """
-    p = subprocess.Popen('ps --no-headers -o pid --ppid %d' % pid, shell=True,
-                         stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-    stdout, _stderr = p.communicate()
-
-    return [int(pid) for pid in stdout.split()]
+    try:
+        pids = subprocess.check_output(['ps', '--no-headers', '-o', 'pid',
+                                        '--ppid', str(pid)]).split()
+        return [int(pid) for pid in pids]
+    except subprocess.CalledProcessError:
+        return []
 
 
 def parse_yaml_file(filename):
@@ -409,102 +428,6 @@
     return debug
 
 
-def raise_privileges(orig_privs):
-    """Raise privileges to original effective privileges.
-
-    :param orig_privs: Original privileges
-    :type orig_privs: dict
-
-    """
-    os.seteuid(orig_privs['euid'])
-    os.setegid(orig_privs['egid'])
-    os.setgroups(orig_privs['groups'])
-
-
-def drop_privileges(user='nobody', group='nogroup', full=False):
-    """Change user and/or group to decrease privileges.
-
-    Currently only drops effective privileges.
-
-    :param user: User name to change to.
-    :type user: str
-    :param group: Group name to change to.
-    :type group: str
-
-    :returns: Original prrivileges
-    :rtype: dict
-
-    """
-
-    orig_privs = {}
-
-    orig_privs['uid'] = os.getuid()
-    orig_privs['euid'] = os.geteuid()
-    orig_privs['gid'] = os.getgid()
-    orig_privs['egid'] = os.getegid()
-    orig_privs['groups'] = os.getgroups()
-
-    # If root, drop privileges
-    # inspired by: http://stackoverflow.com/a/2699996
-    if os.getuid() == 0 or os.geteuid() == 0:
-
-        running_uid = pwd.getpwnam(user).pw_uid
-        running_gid = grp.getgrnam(group).gr_gid
-        os.setgroups([])
-
-        if full:
-            os.setgid(running_gid)
-        else:
-            os.setegid(running_gid)
-
-        if full:
-            os.setuid(running_uid)
-        else:
-            os.seteuid(running_uid)
-
-        os.umask(077)
-
-    return orig_privs
-
-
-def gather_artifacts(extras=None, excludes=None):
-    """Print files contents, so that it's included in the logs.
-
-    :param extras: Path to files to be included in the artifacts
-    :type extras: list(str)
-    :param excludes: Path to files to be exluded from the artifacts
-    :type excludes: list(str)
-
-    """
-    if extras is None:
-        extras = []
-    if excludes is None:
-        excludes = []
-
-    artifacts = ['/var/log/syslog', '/proc/cpuinfo']
-
-    artifacts += extras
-
-    # if excludes is a single item make it a list
-    if not isinstance(excludes, list):
-        excludes = [excludes]
-
-    for exc in excludes:
-        try:
-            artifacts.remove(exc)
-        except ValueError:
-            print("{} is not already an artifact".format(exc))
-            pass
-
-    for artifact in artifacts:
-        print("### Start {} ###".format(artifact))
-        with open(artifact, 'r') as fp:
-            for line in fp:
-                print(line.strip())
-        fp.close()
-        print("### End {} ###".format(artifact))
-
-
 def get_media_info():
     """Get the contents of the media-info file if available.
 
@@ -516,13 +439,14 @@
 
     """
 
-    filename = '/var/log/installer/media-info'
-
     media_info = 'unknown'
 
-    if os.path.exists(filename) and os.access(filename, os.R_OK):
-        with open(filename, 'r') as f:
+    try:
+        with open(MEDIA_INFO, 'r') as f:
             media_info = f.read()
+    except IOError:
+        # If this fails, return the default
+        pass
 
     return media_info
 
@@ -537,13 +461,14 @@
 
     """
 
-    filename = '/sys/class/dmi/id/product_uuid'
-
     product_uuid = None
 
-    if os.path.exists(filename) and os.access(filename, os.R_OK):
-        with open(filename) as f:
+    try:
+        with open(PRODUCT_UUID) as f:
             product_uuid = f.read().strip()
+    except IOError:
+        # If this fails, return the default
+        pass
 
     return product_uuid
 
@@ -581,7 +506,8 @@
         'i686': 'i386',
     }
 
-    return arches.get(platform.machine(), 'unknown')
+    arch = platform.machine()
+    return arches.get(arch, arch)
 
 
 def get_release():
@@ -594,22 +520,6 @@
     return platform.linux_distribution()[2]
 
 
-def get_api_config():
-    """Parse the client configuration file for API configuration data.
-
-    :returns: The value for the ``API`` field in the client configuration file.
-
-    """
-
-    data = {}
-    data['API'] = {}
-
-    with open(CLIENT_CONFIG, 'r') as fp:
-        data = json.load(fp)
-
-    return data['API']
-
-
 def get_build_number():
     """Get build number.
 
@@ -624,152 +534,3 @@
     match = pattern.match(host_info['media-info'])
 
     return match.group(1) if match else '?'
-
-# VCS Support
-# XXX: This maybe ought to be moved to another file
-REPO_DEFAULT_DIR = "."
-
-
-class VCSHandler(object):
-
-    """Base class for Version Control System support.
-
-    :param repo: Repository location
-    :type repo: str
-    :param destination:
-        Local directory where the repository should be made available
-    :type destination: str
-    :param battery_measurements:
-        Whether battery information should be gathered when running any of the
-        VCS commands
-    :type battery_measurements: bool
-
-    """
-
-    def __init__(self, repo, destination='', battery_measurements=False):
-        self.repo = repo
-        self.destination = destination
-        self.battery_measurements = battery_measurements
-
-    def get(self, directory=REPO_DEFAULT_DIR):
-        """Execute VCS get command.
-
-        The get command will make a copy of a given repository to the directory
-        specified as destination.
-
-        :param directory: Working directory
-        :type directory: str
-        :returns: Get command execution result
-        :rtype: dict
-
-        .. seealso:: :func:`run_cmd`
-
-        """
-        return run_cmd(self.get_command,
-                       cwd=directory,
-                       cmd_type=CMD_TS_FETCH,
-                       battery_measurements=self.battery_measurements)
-
-    def revision(self, directory=REPO_DEFAULT_DIR):
-        """Execute revision command.
-
-        :param directory: Working directory
-        :type directory: str
-        :returns: Revision command execution result.
-        :rtype: dict
-
-        .. seealso:: :func:`run_cmd`, :meth:`get_revision`
-
-        """
-        return run_cmd(self.rev_command,
-                       cwd=directory,
-                       cmd_type=CMD_TS_FETCH,
-                       battery_measurements=self.battery_measurements)
-
-    def get_revision(self, directory=REPO_DEFAULT_DIR):
-        """Print revision information.
-
-        This is a wrapper around :meth:`revision` to make sure that the command
-        returned a success code.
-
-        :param directory: Working directory
-        :type directory: str
-        :returns: Revision command returncode
-        :rtype: int
-
-        .. seealso:: :meth:`revision`
-
-        """
-        retval = None
-        result = self.revision(directory=directory)
-
-        if result['returncode'] == 0:
-            retval = result['stdout'].strip()
-        else:
-            print(result)
-
-        return retval
-
-
-class BzrHandler(VCSHandler):
-
-    """Bazaar VCS handler."""
-
-    def __init__(self, repo, branch=True, options="", destination="",
-                 **kwargs):
-        super(BzrHandler, self).__init__(repo, destination, **kwargs)
-        self.options = options
-
-        if branch:
-            self.get_command = "bzr branch {} {} {}".format(
-                options,
-                self.repo,
-                self.destination,
-            )
-
-            self.rev_command = "bzr revno"
-        else:
-            self.get_command = "bzr export {} {} {}".format(
-                options,
-                self.destination,
-                self.repo,
-            )
-
-            self.rev_command = "bzr revno {}".format(self.repo)
-
-
-class GitHandler(VCSHandler):
-
-    """Git VCS handler."""
-
-    def __init__(self, repo, options="", destination="", **kwargs):
-        super(GitHandler, self).__init__(repo, destination, **kwargs)
-        self.options = options
-
-        self.get_command = "git clone {} {} {}".format(
-            options,
-            self.repo,
-            self.destination,
-        )
-
-        self.rev_command = "git rev-parse HEAD"
-
-
-class DevHandler(VCSHandler):
-
-    """Development VCS handler.
-
-    This isn't a handler for any VCS, but a helper to ease development and let
-    runlists point to a path that can be copied locally.
-
-    """
-
-    def __init__(self, repo, destination=".", **kwargs):
-        super(DevHandler, self).__init__(repo, destination, **kwargs)
-
-        self.get_command = "cp -r {} {}".format(
-            self.repo,
-            self.destination,
-        )
-
-        self.rev_command = "echo 'DEVELOPMENT'"

=== modified file 'utah/client/examples/examples/test_one/test_one.py'
--- utah/client/examples/examples/test_one/test_one.py	2012-03-27 21:22:53 +0000
+++ utah/client/examples/examples/test_one/test_one.py	2013-04-08 19:17:22 +0000
@@ -1,5 +1,8 @@
 #!/usr/bin/env python
 
+"""Basic example test script."""
+
+
 import time
 
 

=== modified file 'utah/client/examples/examples/test_two/test_two.py'
--- utah/client/examples/examples/test_two/test_two.py	2012-03-27 15:58:56 +0000
+++ utah/client/examples/examples/test_two/test_two.py	2013-04-08 19:17:22 +0000
@@ -1,3 +1,6 @@
 #!/usr/bin/env python
 
+"""An even more basic example test script."""
+
+
 print "test_two"

=== modified file 'utah/client/examples/utah_tests/test_one/test_one.py'
--- utah/client/examples/utah_tests/test_one/test_one.py	2012-04-10 20:23:50 +0000
+++ utah/client/examples/utah_tests/test_one/test_one.py	2013-04-08 19:17:22 +0000
@@ -1,5 +1,8 @@
 #!/usr/bin/env python
 
+"""Basic example test script."""
+
+
 import time
 
 

=== modified file 'utah/client/examples/utah_tests/test_two/test_two.py'
--- utah/client/examples/utah_tests/test_two/test_two.py	2012-04-10 20:23:50 +0000
+++ utah/client/examples/utah_tests/test_two/test_two.py	2013-04-08 19:17:22 +0000
@@ -1,3 +1,6 @@
 #!/usr/bin/env python
 
+"""An even more basic example test script."""
+
+
 print "test_two"

=== modified file 'utah/client/examples/utah_tests_sample/sample_one/sample.py'
--- utah/client/examples/utah_tests_sample/sample_one/sample.py	2012-04-10 20:23:50 +0000
+++ utah/client/examples/utah_tests_sample/sample_one/sample.py	2013-04-08 19:17:22 +0000
@@ -1,3 +1,6 @@
 #!/usr/bin/env python
 
+"""A very basic example test script."""
+
+
 print("sample_one test output goes here")

=== modified file 'utah/client/exceptions.py'
--- utah/client/exceptions.py	2013-03-14 20:23:09 +0000
+++ utah/client/exceptions.py	2013-04-08 19:17:22 +0000
@@ -13,59 +13,70 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-UTAH client exceptions
-"""
+"""Provide UTAH client exceptions."""
 
 
 class UTAHClientError(Exception):
-    """
-    Base class of all exceptions in the client
-    """
+
+    """Base class of all exceptions in the client."""
+
+    pass
 
 
 class BadDir(UTAHClientError):
-    """
+
+    """Directory error.
+
     Raised when some test directory isn't found
     or an error happens when trying to change to it
+
     """
 
+    pass
+
 
 class MissingFile(UTAHClientError):
-    """
-    Raised when yaml file with metadata about tests
-    cannot be found
-    """
+
+    """Raised when yaml file with metadata about tests cannot be found."""
+
+    pass
 
 
 class BadMasterRunlist(UTAHClientError):
-    """
-    Raised when master runlist isn't in the expected format
-    """
+
+    """Raised when master runlist isn't in the expected format."""
+
+    pass
 
 
 class YAMLParsingError(UTAHClientError):
-    """
+
+    """Provide more detailed yaml.load exception handling.
+
     Used to provide the filename and the location
     in which the parsing error happened when calling yaml.load
+
     """
 
+    pass
+
 
 class YAMLEmptyFile(UTAHClientError):
-    """
-    Used to signal that a file that was supposed to contain yaml data
-    is actually empty
-    """
+
+    """Raised when a file that was supposed to contain yaml data is empty."""
+
+    pass
 
 
 class ValidationError(UTAHClientError):
-    """
-    Used to provide additional information when schema validation fails
-    """
+
+    """Used to provide additional information when schema validation fails."""
+
+    pass
 
 
 class MissingData(UTAHClientError):
-    """
-    Raised when there is missing data that is required for an object
-    to proceed.
-    """
+
+    """Raised when there is missing data required for an object to proceed."""
+
+    pass

=== modified file 'utah/client/phoenix.py'
--- utah/client/phoenix.py	2012-12-11 15:18:01 +0000
+++ utah/client/phoenix.py	2013-04-08 19:17:22 +0000
@@ -15,7 +15,8 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-# phoenix.py - A bootstrap script for testsuite/testcase authors
+"""phoenix.py - A bootstrap script for testsuite/testcase authors"""
+
 
 import argparse
 import os

=== modified file 'utah/client/result.py'
--- utah/client/result.py	2013-03-01 13:04:22 +0000
+++ utah/client/result.py	2013-04-08 19:17:22 +0000
@@ -13,11 +13,14 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Provide functionality for test result handling."""
+
+
 import json
 import sys
 import yaml
 
-from .common import (
+from utah.client.common import (
     get_host_info,
     get_build_number,
     get_release,
@@ -25,41 +28,10 @@
 )
 
 
-def get_smoke_data(_result):
-    data = {}
-    data['build_no'] = get_build_number()
-
-    return data
-
-
-def get_kernel_sru_data(_result):
-    data = {}
-
-    return data
-
-
-# XXX: this logic should probably be moved into the __init__ method
-# of the result object since there will be one result object per
-# master.run
-def create_payload(result):
-    publish_type = result.publish_type
-
-    if publish_type is None:
-        return None
-
-    type_map = {
-        'smoke': get_smoke_data,
-        'kernel-sru': get_kernel_sru_data,
-    }
-
-    if publish_type and publish_type in type_map.iterkeys():
-        return type_map[publish_type](result)
-
-
 class Result(object):
-    """
-    Result collection class.
-    """
+
+    """Result collection class."""
+
     def __init__(self, name=None, testsuite=None, testcase=None,
                  runlist=None, publish_type=None, install_type=None):
         self.results = []
@@ -85,14 +57,13 @@
         :param old_results: Old results as loaded from the report file
         :type old_results: str
 
-        .. seealso:: :meth:`payload`
+        .. seealso:: :meth:`_payload`
 
         """
         raise NotImplementedError
 
     def add_result(self, result, extra_info=None):
-        """
-        Add a result to the object.
+        """Add a result to the object.
 
         Note: 'result' is expected to be a dictionary like this::
 
@@ -104,6 +75,7 @@
                 'start_time': '',
                 'time_delta': '',
             }
+
         """
         if result is None:
             return
@@ -127,8 +99,13 @@
             self.status = 'FAIL'
 
     def result(self, verbose=False):
-        """
-        Output a text based result.
+        """Output a text based result.
+
+        :param verbose: Enable verbose mode
+        :type verbose: bool
+        :returns: Status 'PASS' 'FAIL' or 'ERROR'
+        :rtype: str
+
         """
         status = self.status
         sep = '-' * 70
@@ -140,19 +117,19 @@
             print sep
 
         for result in self.results:
-            print "command: %s" % result['command']
-            print "returned: %d" % result['returncode']
-            print "started: %s" % result['start_time']
-            print "runtime: %s" % result['time_delta']
+            print 'command: {}'.format(result['command'])
+            print 'returned: {}'.format(str(result['returncode']))
+            print 'started: {}'.format(result['start_time'])
+            print 'runtime: {}'.format(result['time_delta'])
 
             if self.testsuite is not None:
-                print "testsuite: %s" % self.testsuite
+                print "testsuite: {}".format(self.testsuite)
 
             if self.testcase is not None:
-                print "testcase: %s" % self.testcase
+                print "testcase: {}".format(self.testcase)
 
             if self.runlist is not None:
-                print "runlist: %s" % self.runlist
+                print "runlist: {}".format(self.runlist)
 
             if self.status != 'PASS' or verbose and result['stdout'] != '':
                 print "stdout: "
@@ -164,37 +141,33 @@
 
         print
 
-        data = self.payload()
+        data = self._payload()
 
         for key, value in data.iteritems():
             print '{}: {}'.format(key, value)
 
-        self.clear_results()
+        self._clear_results()
 
         return status
 
-    def clear_results(self):
-        # reset the list of results so this object can be reused.
+    def _clear_results(self):
+        """Reset the list of results so this object can be reused."""
         self.results = []
 
         # reset the status, this should be safe since the only places that
-        # should be calling clear_results() is testcase.run(), testsuite.run(),
-        # and runner after attempting to fetch the testsuite.
+        # should be calling _clear_results() is testcase.run(),
+        # testsuite.run(), and runner after attempting to fetch the testsuite.
         self.status = 'PASS'
 
-    def count_results(self):
+    def _count_results(self):
         return len(self.results)
 
-    def publish_results(self):
-        # TBD: Use url and token
-        # url = self.publish_url
-        # token = self.publish_token
-
-        create_payload(self)
-
-    def payload(self):
-        """
-        Construct the result payload.
+    def _payload(self):
+        """Construct the result payload.
+
+        :returns: Test result
+        :rtype: dict
+
         """
         host_info = get_host_info()
         data = {
@@ -229,27 +202,35 @@
 
 # Trick to get strings printed as literal blocks
 # inspired by: http://stackoverflow.com/a/7445560/183066
-class LiteralString(object):
+class _LiteralString(object):
     def __init__(self, str_data):
         self.str_data = str_data
 
 
-def literal_block(dumper, data):
+def _literal_block(dumper, data):
     return dumper.represent_scalar('tag:yaml.org,2002:str',
                                    data.str_data, style='|')
-yaml.add_representer(LiteralString, literal_block)
+yaml.add_representer(_LiteralString, _literal_block)
 
 
 class ResultYAML(Result):
+
+    """Return results in a YAML format."""
+
     def _literalize(self, data):
-        """
-        Transform long strings into literal blocks
+        """Transform long strings into literal blocks.
+
+        :param data: Data to literalize
+        :type data: string, dict, or list
+        :returns: literalized form of data
+        :rtype: str, dict, or list
+
         """
         if isinstance(data, basestring) and '\n' in data:
             # Remove trailing whitespace to serialize in yaml
             # as a literal string
             lines = [line.rstrip() for line in data.splitlines()]
-            return LiteralString('\n'.join(lines))
+            return _LiteralString('\n'.join(lines))
 
         if isinstance(data, dict):
             for key, value in data.iteritems():
@@ -259,16 +240,22 @@
                     for element in data]
         return data
 
-    def result(self, verbose=False):
+    def result(self, _verbose=False):
+        """Output a YAML result.
+
+        :returns: Status 'PASS' 'FAIL' or 'ERROR'
+        :rtype: str
+
+        """
         if self.results:
-            data = self._literalize(self.payload())
+            data = self._literalize(self._payload())
             yaml.dump(data, sys.stdout,
                       explicit_start='---',
                       default_flow_style=False,
                       allow_unicode=True)
 
         status = self.status
-        self.clear_results()
+        self._clear_results()
 
         return status
 
@@ -278,7 +265,7 @@
         :param old_results: Old results as loaded from the report file
         :type old_results: str
 
-        .. seealso:: :meth:`payload`
+        .. seealso:: :meth:`_payload`
 
         """
         data = yaml.load(old_results)
@@ -293,13 +280,21 @@
 
 
 class ResultJSON(Result):
-    def result(self, verbose=False):
-
+
+    """Return results in a JSON format."""
+
+    def result(self, _verbose=False):
+        """Output a YAML result.
+
+        :returns: Status 'PASS' 'FAIL' or 'ERROR'
+        :rtype: str
+
+        """
         if self.results:
-            data = self.payload()
+            data = self._payload()
             json.dump(data, sys.stdout, indent=4)
 
         status = self.status
-        self.clear_results()
+        self._clear_results()
 
         return status

=== modified file 'utah/client/runner.py'
--- utah/client/runner.py	2013-03-21 15:43:29 +0000
+++ utah/client/runner.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,8 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Provide code to actually run the tests."""
+
 
 import datetime
 import jsonschema
@@ -25,14 +27,11 @@
 from utah.client import exceptions
 from utah.client.battery import battery
 from utah.client.common import (
-    BzrHandler,
     CMD_TC_REBOOT,
     DATE_FORMAT,
     DEFAULT_STATE_FILE,
     DEFAULT_TSLIST,
     DefaultValidator,
-    DevHandler,
-    GitHandler,
     make_result,
     MASTER_RUNLIST,
     parse_yaml_file,
@@ -42,12 +41,17 @@
 from utah.client.result import Result
 from utah.client.state_agent import StateAgentYAML
 from utah.client.testsuite import TestSuite
+from utah.client.vcs import (
+    BzrHandler,
+    DevHandler,
+    GitHandler,
+)
 from utah.exceptions import UTAHException
 from utah.retry import retry
 from utah.timeout import timeout
 
 RC_LOCAL = '/etc/rc.local'
-RC_LOCAL_BACKUP = '%s-utah.bak' % RC_LOCAL
+RC_LOCAL_BACKUP = '{}-utah.bak'.format(RC_LOCAL)
 
 # XXX: need to output to the same file that was supplied on the original
 # run.
@@ -58,10 +62,11 @@
 
 
 class Runner(object):
-    """
-    The main runner.
+
+    """Provide The main runner class.
 
     Parses a master runlist, builds a list of TestSuites, and runs them.
+
     """
 
     status = "NOTRUN"
@@ -216,10 +221,7 @@
         self.result.name = self.name
 
     def backup_rc_local(self):
-        """
-        Backup /etc/rc.local if it exists.
-        """
-
+        """Backup /etc/rc.local if it exists."""
         # Ignore permission denied errors since we only care if a reboot is
         # pending whether or not we can write to RC_LOCAL(_BACKUP).
         try:
@@ -230,11 +232,7 @@
                     'Failed to backup rc.local: {}'.format(e))
 
     def reset_rc_local(self):
-        """
-        Restore original /etc/rc.local if there is a backup
-        Otherwise remove it.
-        """
-
+        """Restore /etc/rc.local if there is a backup, remove it otherwise."""
         # Ignore permission denied errors since we only care if a reboot is
         # pending whether or not we can write to RC_LOCAL(_BACKUP).
         try:
@@ -248,9 +246,7 @@
                     'Failed to backup rc.local: {}'.format(e))
 
     def setup_rc_local(self, rc_local=RC_LOCAL, runlist=None):
-        """
-        Setup /etc/rc.local to kick-off a --resume on successful boot.
-        """
+        """Setup /etc/rc.local to kick-off a --resume on successful boot."""
         runlist = runlist or self.master_runlist or MASTER_RUNLIST
 
         self.rc_local_content = rc_local_content
@@ -265,6 +261,12 @@
                 'Error setting up rc.local: {}'.format(err))
 
     def process_results(self):
+        """Add stats to results and process them.
+
+        :returns: A return code based on the test status.
+        :rtype: int
+
+        """
         # Add stats to the results
         self.result.failures = self.failures
         self.result.passes = self.passes
@@ -276,8 +278,13 @@
         return self.returncode()
 
     def run(self):
-        """
-        Run the test suites we've parsed.
+        """Run the test suites we've parsed.
+
+        :returns: The result of process_results, which is a return code.
+        :rtype: int
+
+        .. seealso:: :meth:`process_results`
+
         """
         if self.battery_measurements:
             self.result.start_battery = battery.get_data()
@@ -315,12 +322,11 @@
         return self.process_results()
 
     def add_suite(self, suite):
+        """Add a test suite to run."""
         self.suites.append(suite)
 
     def get_fetched_suites(self):
-        """
-        Return a list of fetched suites from the state_agent.
-        """
+        """Return a list of fetched suites from the state_agent."""
         state = self.state_agent.load_state()
 
         fetched_suites = []
@@ -331,6 +337,7 @@
         return fetched_suites
 
     def load_state(self):
+        """Load the state saved by a previous partial run (i.e., a reboot)."""
         state = self.state_agent.load_state()
 
         self.master_runlist = state['master_runlist']
@@ -351,10 +358,12 @@
         self.fetched_suites = state['fetched_suites']
 
     def save_state(self):
-        """
-        Save the list of tests we are to run and whether we've run them.
-        """
-
+        """Save the list of tests we are to run and whether we've run them.
+
+        :returns: state of currently run tests
+        :rtype: dict
+
+        """
         self.backup_runlist = os.path.join(self.testdir, 'master.run-reboot')
 
         if (os.path.exists(
@@ -390,9 +399,11 @@
         return state
 
     def count_suites(self):
+        """Return the number of test suites in the runner."""
         return len(self.suites)
 
     def count_tests(self):
+        """Return the number of test cases in the runner."""
         tests = 0
 
         for suite in self.suites:
@@ -401,10 +412,7 @@
         return tests
 
     def process_master_runlist(self, runlist=None, resume=False):
-        """
-        Parse a master runlist building a list of suites from the parsed data.
-
-        """
+        """Parse a master runlist and build a list of suites from the data."""
         runlist = runlist or self.master_runlist
 
         # Download runlist using the URL passed through the commmand line to
@@ -511,6 +519,7 @@
                 except OSError as err:
                     raise exceptions.BadDir(err)
 
+            # TODO: move this somewhere else
             def vcs_get_retriable():
                 result = vcs_handler.get(directory=name)
                 if (isinstance(vcs_handler, BzrHandler) and
@@ -555,12 +564,11 @@
                 self.add_suite(s)
 
     def get_next_suite(self):
-        """
-        Return the next suite to be run.
+        """Return the next suite to be run.
 
         Mainly used for debugging.
+
         """
-
         suite = None
 
         for s in self.suites:
@@ -571,18 +579,18 @@
         return suite
 
     def get_next_test(self):
-        """
-        Return the next test to be run.
+        """Return the next test to be run.
 
         Mainly used for debugging.
+
         """
         return self.get_next_suite().get_next_test()
 
     def reboot(self):
-        """
-        Reboot the machine.
+        """Reboot the machine.
 
         Save state, setup /etc/rc.local, and shutdown.
+
         """
         # Create fake result for logging purposes
         command = 'shutdown -r now'
@@ -613,25 +621,10 @@
         # End of execution
 
     def returncode(self):
+        """Provide return code based on test status."""
         if self.errors > 0 or self.fetch_errors > 0:
             return ReturnCodes.ERROR
         elif self.failures > 0:
             return ReturnCodes.FAIL
         else:
             return ReturnCodes.PASS
-
-    def report(self):
-        tests_run = self.passes + self.errors + self.failures
-
-        output = ("total: %d, passes: %d, failure: %d, error: %d"
-                  % (tests_run, self.passes, self.failures,
-                     self.errors + self.fetch_errors))
-
-        if self.errors > 0 or self.fetch_errors > 0:
-            result = "ERROR"
-        elif self.failures > 0:
-            result = "FAIL"
-        else:
-            result = "PASS"
-
-        return "%s - %s" % (result, output)

=== modified file 'utah/client/state_agent.py'
--- utah/client/state_agent.py	2012-12-03 14:02:18 +0000
+++ utah/client/state_agent.py	2013-04-08 19:17:22 +0000
@@ -13,17 +13,22 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Provide functionality for saving and restoring run state."""
+
+
 import os
+
 import yaml
 
 from utah.client.common import DEFAULT_STATE_FILE, parse_yaml_file
 
 
 class StateAgent(object):
-    """
-    State saving base class.
+
+    """State saving base class.
 
     Accepts a dictionary of state info and prints it to STDOUT.
+
     """
 
     state_file = DEFAULT_STATE_FILE
@@ -33,9 +38,7 @@
             self.state_file = state_file
 
     def clean(self):
-        """
-        Clean up the state file if it exists.
-        """
+        """Clean up the state file if it exists."""
         # Don't fail if the state_file doesn't exists.
         try:
             os.remove(self.state_file)
@@ -44,17 +47,17 @@
                 pass
 
     def save_state(self, state):
-        """
-        Save state to the state_file.
-        """
-
+        """Save state to the state_file."""
         fp = open(self.state_file, 'w')
         fp.write(str(state))
         fp.close()
 
     def load_state(self):
-        """
-        Load state from the state_file.
+        """Load state from the state_file.
+
+        :returns: state information from file.
+        :rtype: dict
+
         """
         state = {}
 
@@ -67,22 +70,21 @@
 
 
 class StateAgentYAML(StateAgent):
-    """
-    YAML based state saver.
-    """
+
+    """YAML based state saver."""
 
     def save_state(self, state):
-        """
-        Output the state as YAML.
-        """
+        """Output the state as YAML."""
         yaml_state = yaml.dump(state, default_flow_style=False)
         super(StateAgentYAML, self).save_state(yaml_state)
 
     def load_state(self):
-        """
-        Load state from YAML state_file.
-        """
-
+        """Load state from YAML state_file.
+
+        :returns: state information from YAML file
+        :rtype: dict
+
+        """
         state = {}
 
         if os.path.exists(self.state_file):

=== modified file 'utah/client/testcase.py'
--- utah/client/testcase.py	2013-03-14 20:53:20 +0000
+++ utah/client/testcase.py	2013-04-08 19:17:22 +0000
@@ -13,35 +13,35 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Testcase specific code.
-"""
-
+"""Testcase specific code."""
+
+
+import os
 
 import jsonschema
-import os
 
 from utah.client.common import (
-    run_cmd,
-    parse_control_file,
-    do_nothing,
     CMD_TC_BUILD,
+    CMD_TC_CLEANUP,
     CMD_TC_SETUP,
     CMD_TC_TEST,
-    CMD_TC_CLEANUP,
+    do_nothing,
+    parse_control_file,
+    run_cmd,
 )
 from utah.client.exceptions import (
+    MissingData,
     MissingFile,
     ValidationError,
-    MissingData,
 )
 
 
 class TestCase(object):
-    """
-    The TestCase class.
+
+    """Base class describing a test case.
 
     status is one of 'NOTRUN', 'BUILD', 'SETUP', 'RUN', 'CLEANUP', or 'DONE'
+
     """
 
     status = 'NOTRUN'
@@ -100,15 +100,15 @@
                  battery_measurements=False,
                  _control_data=None, _save_state_callback=None,
                  _reboot_callback=None):
-        """
-        Build a TestCase from a control file's data.
+        """Build a TestCase from a control file's data.
 
         'name' is a directory where the test case resides.
+
         """
         self.name = name
         self.path = path
         self.working_dir = path
-        self.filename = "%s/tc_control" % path
+        self.filename = "{}/tc_control".format(path)
         self.results = []
         self.result = result
         self.timeout = timeout
@@ -158,20 +158,16 @@
             self.command = command
 
     def __str__(self):
-        return ("%s: %s, %s, %s"
-                % (self.name, self.description, self.command, self.timeout))
+        return ("{}: {}, {}, {}".format(
+                self.name, self.description, self.command, self.timeout))
 
     def set_status(self, status):
-        """
-        Set the status for the test case and call the save state callback.
-        """
+        """Set the status and call the save state callback."""
         self.status = status
         self.save_state_callback()
 
     def build(self, result):
-        """
-        Run build, but only if we haven't started a run yet.
-        """
+        """Run build, but only if we haven't started a run yet."""
         if self.status == 'NOTRUN':
             self.set_status('BUILD')
             cmd_result = run_cmd(
@@ -182,9 +178,7 @@
             result.add_result(cmd_result)
 
     def setup(self, result):
-        """
-        Run tc_setup, but only if build() has just passed.
-        """
+        """Run tc_setup, but only if build() has just passed."""
         if self.status == 'BUILD' and result.status == 'PASS':
             self.set_status('SETUP')
             cmd_result = run_cmd(
@@ -195,9 +189,7 @@
             result.add_result(cmd_result)
 
     def cleanup(self, result):
-        """
-        Run tc_cleanup after a run.
-        """
+        """Run tc_cleanup after a run."""
         if self.status == 'RUN':
             self.set_status('CLEANUP')
             cmd_result = run_cmd(
@@ -208,10 +200,14 @@
             result.add_result(cmd_result)
 
     def run(self):
-        """
-        Run the test case; including any build, setup, and cleanup commands.
-        """
-
+        """Run the complete test case.
+
+        This includes any build, setup, and cleanup commands.
+
+        :returns: Whether to keep running tests (True) or reboot (False)
+        :rtype: bool
+
+        """
         if self.is_done():
             return 'PASS'
 
@@ -280,9 +276,8 @@
 
         need_reboot = False
         if (
-                self.reboot == 'always'
-                or self.reboot == 'pass'
-                and result.status == 'PASS'
+                self.reboot == 'always' or
+                (self.reboot == 'pass' and result.status == 'PASS')
         ):
             need_reboot = True
 
@@ -299,25 +294,21 @@
         return keep_going
 
     def process_overrides(self, overrides):
-        """
-        Sets override values from a TestSuite runlist for this test case.
-        """
+        """Set override values from a TestSuite runlist for this test case."""
         for key, value in overrides.iteritems():
             setattr(self, key, value)
 
     def load_state(self, state):
-        """
-        Restore state from the supplied dictionary.
+        """Restore state from the supplied dictionary.
 
         Requires that 'state' has the same fieldnames as the TestCase class.
+
         """
         for key, value in state.iteritems():
             setattr(self, key, value)
 
     def save_state(self):
-        """
-        Returns a dictionary representing the test's state.
-        """
+        """Return a dictionary representing the test's state."""
         state = {
             'name': self.name,
             'path': self.path,
@@ -338,9 +329,13 @@
         return state
 
     def is_done(self):
-        """
-        Determine if the case is done.  This might mean that something has
-        failed.  Used by suite to determine if the suite needs to be re-run
-        on resume.
+        """Determine if the case is done.
+
+        This might mean that something has failed.
+        Used by suite to determine if the suite needs to be re-run on resume.
+
+        :returns: Whether the test case is finished (done or cleaned up)
+        :rtype: bool
+
         """
         return self.status == 'DONE' or self.status == 'CLEANUP'

=== modified file 'utah/client/tests/__init__.py'
--- utah/client/tests/__init__.py	2012-12-08 02:10:12 +0000
+++ utah/client/tests/__init__.py	2013-04-08 19:17:22 +0000
@@ -12,3 +12,5 @@
 
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""utah.client.tests"""

=== modified file 'utah/client/tests/common.py'
--- utah/client/tests/common.py	2013-03-21 20:46:21 +0000
+++ utah/client/tests/common.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Provide common functions and content for self tests."""
+
+
 import os
 import shutil
 
@@ -20,8 +23,7 @@
 
 
 def get_module_path():
-    """
-    Return the path for the 'utah' directory.
+    """Return the path for the 'utah' directory.
 
     This is used in tests to refer to the location where the branch resides.
     It is used in conjunction with running the self tests via:
@@ -29,6 +31,7 @@
     sudo nosetests utah/client/tests
 
     from the top-level directory in the repo.
+
     """
     import utah
 
@@ -38,6 +41,7 @@
 
     return module_path
 
+
 master_runlist_content = """# utah/self_test.py master runlist
 # needed for utah/self_test.py runs
 ---
@@ -64,11 +68,12 @@
 
 
 def setUp():
-    """
-    Set up a master.run file that will copy our 'examples' directory to the
-    testing directory (usually /var/lib/utah).
-    """
-
+    """Create a sample master.run file.
+
+    This file will copy our 'examples' directory to the testing directory
+    (usually /var/lib/utah).
+
+    """
     # If we're not root don't bother
     if os.geteuid() != 0 and os.getuid() != 0:
         raise RuntimeError('These tests must be run as root')
@@ -92,10 +97,7 @@
 
 
 def tearDown():
-    """
-    Cleanup after ourselves.
-    """
-
+    """Clean up after ourselves."""
     # restore the copy of the master runlist
     if os.path.exists(master_runlist_bak):
         os.rename(master_runlist_bak, master_runlist)
@@ -107,10 +109,12 @@
 
 
 def _get_partial_state_file(filename):
-    """
-    Read a partial state file from disk.
-    """
-
+    """Read a partial state file from disk.
+
+    :returns: A pre-defined partially run test suite state.
+    :rtype: dict
+
+    """
     state = None
 
     with open(filename, 'r') as fp:

=== removed file 'utah/client/tests/manual_privileges.py'
--- utah/client/tests/manual_privileges.py	2012-12-08 02:10:12 +0000
+++ utah/client/tests/manual_privileges.py	1970-01-01 00:00:00 +0000
@@ -1,188 +0,0 @@
-# Ubuntu Testing Automation Harness
-# Copyright 2012 Canonical Ltd.
-
-# This program is free software: you can redistribute it and/or modify it
-# under the terms of the GNU General Public License version 3, as published
-# by the Free Software Foundation.
-
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranties of
-# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
-# PURPOSE.  See the GNU General Public License for more details.
-
-# You should have received a copy of the GNU General Public License along
-# with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-######################################################################
-### Privileges Tests
-######################################################################
-
-import os
-import pwd
-import grp
-import shlex
-import subprocess
-import unittest
-
-from utah.client.common import drop_privileges, raise_privileges
-
-NOBODY_UID = pwd.getpwnam('nobody').pw_uid
-NOGROUP_GID = grp.getgrnam('nogroup').gr_gid
-
-TMP_CONTENT = """Some data"""
-TMP_FILENAME = '/tmp/priv_test.tmp'
-
-
-def print_priv(msg=None):
-    """
-    Print out the current privileges.
-    """
-    if msg is not None:
-        print(msg)
-
-    print("uid: {}, euid: {}".format(os.getuid(), os.geteuid()))
-    print("gid: {}, egid: {}".format(os.getgid(), os.getegid()))
-
-
-def write_temp_file(filename=TMP_FILENAME):
-    """
-    Write the contents to a temp file.
-    """
-    with open(filename, 'w') as fp:
-        fp.write(TMP_CONTENT)
-    fp.close()
-
-
-def read_temp_file(filename=TMP_FILENAME):
-    """
-    Print the contents of a temp file.
-    """
-    with open(filename, 'r') as fp:
-        for line in fp:
-            print(line.strip())
-    fp.close()
-
-
-def _do_cmd(cmd, quiet=False, shell=False):
-    """
-    Run a command.
-    """
-    cmd_args = cmd
-
-    if not shell:
-        cmd_args = shlex.split(cmd)
-
-    proc = subprocess.Popen(
-        cmd_args,
-        shell=shell,
-        stdout=subprocess.PIPE,
-        stderr=subprocess.PIPE
-    )
-
-    (stdout, _stderr) = proc.communicate()
-
-    if not quiet:
-        print("{}:\n{}".format(cmd, stdout))
-
-
-def stat_file(filename=TMP_FILENAME):
-    proc = subprocess.Popen(['ls', '-al', filename], stdout=subprocess.PIPE,
-                            stderr=subprocess.PIPE)
-
-    stdout, _stderr = proc.communicate()
-    print("{}: {}".format(filename, stdout))
-
-
-def get_privs():
-    privs = {}
-
-    privs['uid'] = os.getuid()
-    privs['euid'] = os.geteuid()
-    privs['gid'] = os.getgid()
-    privs['egid'] = os.getegid()
-
-    return privs
-
-
-class baseTestPriv(unittest.TestCase):
-
-    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
-        if 'uid' in privs:
-            self.assertEqual(os.getuid(), privs['uid'], msg)
-
-        if 'euid' in privs:
-            self.assertEqual(os.geteuid(), privs['euid'], msg)
-
-        if 'gid' in privs:
-            self.assertEqual(os.getuid(), privs['uid'], msg)
-
-        if 'egid' in privs:
-            self.assertEqual(os.geteuid(), privs['euid'], msg)
-
-    def setUp(self):
-        print_priv("before")
-        _do_cmd("id")
-
-    def tearDown(self):
-        print_priv("after")
-        _do_cmd("id")
-
-
-class TestPriv(baseTestPriv):
-    def test_drop_priv(self):
-        """
-        Test that we drop effective privileges appropriately.
-        """
-        self.assertRoot()
-        old_privs = drop_privileges()
-
-        self.assertPrivs({'euid': NOBODY_UID, 'egid': NOGROUP_GID})
-
-        print_priv("middle")
-        _do_cmd("id")
-
-        write_temp_file()
-        read_temp_file()
-        stat_file()
-
-        raise_privileges(old_privs)
-
-    def test_restore_priv(self):
-        """
-        Test that we restore effective privileges appropriately.
-        """
-        self.assertRoot()
-        old_privs = drop_privileges()
-
-        self.assertPrivs({'euid': NOBODY_UID, 'egid': NOGROUP_GID})
-
-        print_priv("middle")
-
-        raise_privileges(old_privs)
-
-        self.assertPrivs(old_privs)
-
-
-class TestXXXLast(baseTestPriv):
-    """
-    Test's to be run last.
-    """
-    def test_xxx_drop_priv_full(self):
-        """
-        Test dropping all privileges.
-        """
-
-        self.assertRoot()
-        drop_privileges(full=True)
-        self.assertPrivs({
-            'uid': NOBODY_UID,
-            'euid': NOBODY_UID,
-            'gid': NOGROUP_GID,
-            'egid': NOGROUP_GID,
-        })
-
-        _do_cmd("id")

=== modified file 'utah/client/tests/test_common.py'
--- utah/client/tests/test_common.py	2013-01-29 14:56:33 +0000
+++ utah/client/tests/test_common.py	2013-04-08 19:17:22 +0000
@@ -13,11 +13,15 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test utah client common functions."""
+
 import os
+import signal
 import unittest
 
 # REQUIRES that the top level utah package be in the Python path.
 from utah.client.common import (
+    _get_process_children,
     get_media_info,
     get_product_uuid,
     get_host_info,
@@ -28,12 +32,11 @@
 
 
 class TestCommon(unittest.TestCase):
-    """
-    Tests for utah.client.common.
-    """
+
+    """Tests for utah.client.common."""
 
     def test_get_product_uuid(self):
-        """ Test that product_uuid is returned when possible. """
+        """Test that product_uuid is returned when possible."""
 
         product_uuid = get_product_uuid()
 
@@ -41,9 +44,7 @@
             self.assertIsNotNone(product_uuid)
 
     def test_get_media_info(self):
-        """
-        Test that if there is a media-info file that it is parsed.
-        """
+        """Test that if there is a media-info file that it is parsed."""
         media_info = get_media_info()
 
         print("media_info: {}".format(media_info))
@@ -54,10 +55,7 @@
             self.assertTrue(media_info == 'unknown')
 
     def test_get_host_info(self):
-        """
-        Test that get_host_info returns results.
-        """
-
+        """Test that get_host_info returns results."""
         host_info = get_host_info()
 
         print("host_info: {}".format(host_info))
@@ -65,14 +63,17 @@
         self.assertTrue(host_info is not None)
 
     def test_run_cmd(self):
+        """Test running a command."""
         result = run_cmd('find . -name "*.py"')
 
         self.assertEqual(result['returncode'], 0)
 
     def test_debug_print(self):
-        """
-        Test that debug_print doesn't print when DEBUG is False unless
-        'debug' is passed in and is True.
+        """Test that debug_print prints at the appropriate times.
+
+        It shouldn't print when DEBUG is False unless 'debug' is passed in and
+        is True.
+
         """
         old_debug = CONFIG['DEBUG']
 
@@ -94,9 +95,7 @@
         CONFIG['DEBUG'] = old_debug
 
     def test_run_as(self):
-        """
-        Test that run_as functions correctly.
-        """
+        """Test that run_as functions correctly."""
         user = "nobody"
         result = run_cmd("touch /tmp/run_as_test.tmp", run_as=user)
         print("result: {}".format(result))
@@ -111,10 +110,7 @@
         self.assertEqual(result['user'], user)
 
     def test_run_as_default(self):
-        """
-        Test that the default user is correctly identified.
-        """
-
+        """Test that the default user is correctly identified."""
         result = run_cmd("touch /tmp/run_as_test.tmp")
         print("result: {}".format(result))
 
@@ -126,3 +122,22 @@
 
         self.assertEqual(result['returncode'], 0)
         self.assertNotEqual(result['user'], "unknown")
+
+    def test_get_process_children(self):
+        """Test that the get_process_children function works."""
+        children = []
+        for _ in xrange(3):
+            pid = os.fork()
+            if pid == 0:
+                # child to wait
+                os.execv('/bin/sleep', ['/bin/sleep', '5'])
+            children.append(pid)
+
+        ret = _get_process_children(str(os.getpid()))
+        # the last PID from childre will be the "ps" process we ran, so
+        # we can ignore that one
+        ret.pop()
+        for pid in children:
+            os.kill(pid, signal.SIGTERM)
+            os.waitpid(pid, 0)
+        self.assertEqual(children, ret)

=== modified file 'utah/client/tests/test_jsonschema.py'
--- utah/client/tests/test_jsonschema.py	2012-12-11 09:30:00 +0000
+++ utah/client/tests/test_jsonschema.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test schema parsing functionality."""
+
+
 import jsonschema
 import os
 import unittest
@@ -80,25 +83,22 @@
 
 class TestJSONSchema(unittest.TestCase):
 
+    """Test schema validation."""
+
     def test_orig_schema(self):
-        """
-        Test that the original master.run is correctly validated by the schema.
-        """
-
+        """Test that the original master.run validates correctly."""
         data = yaml.load(yaml_content)
         print("data: {}".format(data))
         jsonschema.validate(data, Runner.MASTER_RUNLIST_SCHEMA)
 
     def test_multi_schema(self):
-        """
-        Test that the new master.run is correctly validated by the schema.
-        """
-
+        """Test that the new master.run validates correctly."""
         data_new = yaml.load(yaml_content_new)
         print("data_new: {}".format(data_new))
         jsonschema.validate(data_new, Runner.MASTER_RUNLIST_SCHEMA)
 
     def test_example_runlist(self):
+        """Test that the example runlist validates correctly."""
         example_runlist = os.path.join(os.path.dirname(__file__),
                                        "../examples/master.run")
 
@@ -111,19 +111,13 @@
             fp.close()
 
     def test_include_schema(self):
-        """
-        Test that include works correctly.
-        """
-
+        """Test that include works correctly."""
         data = yaml.load(yaml_content_include)
         print("data: {}".format(data))
         jsonschema.validate(data, Runner.MASTER_RUNLIST_SCHEMA)
 
     def test_bad_schemas(self):
-        """
-        Test that required fields are enforced by the schema.
-        """
-
+        """Test that required fields are enforced by the schema."""
         for content in yaml_content_bad:
             data = yaml.load(content)
             print("data: {}".format(data))

=== modified file 'utah/client/tests/test_misc.py'
--- utah/client/tests/test_misc.py	2012-12-03 14:02:18 +0000
+++ utah/client/tests/test_misc.py	2013-04-08 19:17:22 +0000
@@ -13,15 +13,21 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test miscellaneous functionality."""
+
+
 import os
 import unittest
 
-from .common import get_module_path
+from utah.client.tests.common import get_module_path
 
 
 class TestMisc(unittest.TestCase):
+
+    """Test miscellaneous functionality."""
+
     def test_get_module_path(self):
-
+        """Test getting the module path."""
         print("__file__: {}".format(__file__))
 
         # assumes this file is in 'utah/client/tests'

=== modified file 'utah/client/tests/test_phoenix.py'
--- utah/client/tests/test_phoenix.py	2013-03-21 15:51:53 +0000
+++ utah/client/tests/test_phoenix.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test phoenix metadata generation tool."""
+
+
 import os
 import shutil
 import unittest
@@ -23,21 +26,27 @@
 
 
 def setUp():
+    """Set up test prerequisite."""
     if os.path.exists(directory_name):
         raise Exception("directory '{}' already exists".format(directory_name))
 
 
 def tearDown():
+    """Remove test resources."""
     if os.path.exists(directory_name):
         shutil.rmtree(directory_name)
 
 
 class TestPhoenix(unittest.TestCase):
+
+    """Test phoenix test suite creation utility."""
+
     testsuite_name = "testsuite_name"
     testcases = ['test_one', 'test_two']
     directory_name = directory_name
 
     def setUp(self):
+        """Set up test suite creation information."""
         self.phoenix = Phoenix(
             self.testsuite_name,
             directory=self.directory_name,
@@ -45,61 +54,43 @@
         )
 
     def test_missing_required_args(self):
-        """
-        Test that __init__ fails without the required arguments
-        """
+        """Test that __init__ fails without the required arguments."""
         self.assertRaises(
             TypeError,
             Phoenix,
         )
 
     def test_providing_required_args(self):
-        """
-        Test that __init__ succeeds with the required arguments.
-        """
-
+        """Test that __init__ succeeds with the required arguments."""
         self.assertTrue(self.phoenix is not None)
 
     def test_suite_name(self):
-        """
-        Test that the testsuite name is correctly set.
-        """
-
+        """Test that the testsuite name is correctly set."""
         self.assertEqual(self.phoenix.testsuite, self.testsuite_name)
 
     def test_directory_name(self):
-        """
-        Test that the directory name is correctly set.
-        """
-
+        """Test that the directory name is correctly set."""
         self.assertEqual(self.phoenix.directory, self.directory_name)
 
     def test_default_directory_name(self):
-        """
-        Test that the default directory name is correctly set.
-        """
-
+        """Test that the default directory name is correctly set."""
         phoenix = Phoenix(self.testsuite_name)
 
         self.assertEqual(phoenix.directory, '.')
 
     def test_testcases(self):
-        """
-        Test that the testcases are correctly set.
-        """
-
+        """Test that the testcases are correctly set."""
         self.assertListEqual(self.phoenix.testcases, self.testcases)
 
     def test_testcases_default(self):
-        """
-        Test that the default testcase is correctly set.
-        """
+        """Test that the default testcase is correctly set."""
 
         phoenix = Phoenix(self.testsuite_name)
         # there should be only one default testcase.
         self.assertEqual(len(phoenix.testcases), 1)
 
     def test_build_suite(self):
+        """Test building a test suite."""
         self.phoenix.build_suite()
 
         dir_path = self.directory_name
@@ -118,9 +109,7 @@
                             "{} doesn't exist".format(path))
 
     def test_create_file(self):
-        """
-        Test that Phoenix create file works correctly.
-        """
+        """Test that Phoenix create file works correctly."""
         fn = '/tmp/phoenix_create_file_test.file'
         fc = 'a test file'
 

=== modified file 'utah/client/tests/test_result.py'
--- utah/client/tests/test_result.py	2013-03-20 19:20:13 +0000
+++ utah/client/tests/test_result.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test our ability to report results."""
+
+
 import unittest
 import yaml
 
@@ -23,7 +26,11 @@
 
 
 class TestResult(unittest.TestCase):
+
+    """Test result processing and display functionality."""
+
     def setUp(self):
+        """Create needed data structures."""
         self.result_yaml = ResultYAML("result_yaml")
         self.result_text = Result("result")
 
@@ -44,58 +51,49 @@
         }
 
     def test_add_result(self):
+        """Test adding test results."""
         self.result_yaml.add_result(self.result)
 
-        self.assertEqual(self.result_yaml.count_results(), 1)
+        self.assertEqual(self.result_yaml._count_results(), 1)
 
         self.result_yaml.result()
 
     def test_result(self):
+        """Test generating results."""
         self.result_yaml.add_result(self.result)
 
         # check that the result was added.
-        self.assertEqual(self.result_yaml.count_results(), 1)
+        self.assertEqual(self.result_yaml._count_results(), 1)
 
         self.result_yaml.result()
 
         # check that the result was processed and removed.
-        self.assertEqual(self.result_yaml.count_results(), 0)
+        self.assertEqual(self.result_yaml._count_results(), 0)
 
     def test_result_pass(self):
-        """
-        Test that adding a pass result sets the Result object's status
-        to 'PASS'.
-        """
+        """Test that adding a pass result sets the status to 'PASS'."""
         self.result_yaml.add_result(self.result)
 
         self.assertEqual(self.result_yaml.status, 'PASS')
 
     def test_result_failure(self):
-        """
-        Test that adding a failure result sets the Result object's status
-        to 'ERROR'.
-        """
+        """Test that adding a failure result sets the status to 'FAIL'."""
         self.result_yaml.add_result(self.result_fail)
 
         self.assertEqual(self.result_yaml.status, 'FAIL')
 
     def test_result_clear_result(self):
-        """
-        Test that the result is actually cleared.
-        """
+        """Test that the result is actually cleared."""
         self.result_yaml.add_result(self.result_fail)
         self.assertEqual(self.result_yaml.status, 'FAIL')
 
-        self.result_yaml.clear_results()
+        self.result_yaml._clear_results()
 
         self.assertEqual(self.result_yaml.status, 'PASS')
-        self.assertEqual(self.result_yaml.count_results(), 0)
+        self.assertEqual(self.result_yaml._count_results(), 0)
 
     def test_result_result(self):
-        """
-        Test that ERROR status is retained after adding pass
-        results.
-        """
+        """Test that ERROR status is retained after adding pass results."""
         self.result_yaml.add_result(self.result_fail)
 
         self.assertEqual(self.result_yaml.status, 'FAIL')
@@ -105,6 +103,7 @@
         self.assertEqual(self.result_yaml.status, 'FAIL')
 
     def test_result_yaml(self):
+        """Test generating YAML results."""
         self.result_yaml.add_result(self.result)
 
         import sys
@@ -122,7 +121,7 @@
 
         result_str = str_output.getvalue()
 
-        print("result_str: %s" % result_str)
+        print('result_str: {}'.format(result_str))
 
         data = yaml.load(result_str)
 
@@ -130,7 +129,7 @@
 
         result = data['commands'][0]
 
-        print("data: %s" % data)
+        print('data: {}'.format(data))
 
         self.assertEqual(result['command'], self.result['command'])
         self.assertEqual(result['returncode'], self.result['returncode'])

=== modified file 'utah/client/tests/test_runner.py'
--- utah/client/tests/test_runner.py	2013-03-21 21:04:14 +0000
+++ utah/client/tests/test_runner.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Who tests the test runners?"""
+
+
 import os
 import shutil
 import unittest
@@ -24,7 +27,7 @@
 from utah.client.common import ReturnCodes
 from utah.client import exceptions
 
-from .common import (  # NOQA
+from utah.client.tests.common import (  # NOQA
     setUp,  # Used by nosetests
     tearDown,  # Used by nosetests
     master_runlist_content,
@@ -33,7 +36,11 @@
 
 
 class TestRunner(unittest.TestCase):
+
+    """Test the TestRunner class."""
+
     def setUp(self):
+        """Set up test prerequisites."""
         self.result_class = ResultYAML
         self.state_file = '/tmp/state.yaml'
         self.state_agent = StateAgentYAML(state_file=self.state_file)
@@ -46,7 +53,7 @@
                          "{} shouldn't exist".format(self.bad_dir))
 
     def tearDown(self):
-
+        """Clean up test resources."""
         # Remove self.bad_dir if it got created.
         try:
             shutil.rmtree(self.bad_dir)
@@ -58,6 +65,7 @@
                 raise
 
     def test_fetch_failed(self):
+        """Test handling of failed fetch commands."""
         bad_runlist_content = """---
 testsuites:
   - name: fake_tests
@@ -75,17 +83,18 @@
         os.remove(runlist)
 
         runner.run()
-        print runner.report()
 
         self.assertEqual(runner.fetch_errors, 1)
 
     def test_rc_local(self):
+        """Test rc.local functionality for reboots."""
         tmp_rc_local = '/tmp/rc.local'
         self.runner.setup_rc_local(tmp_rc_local)
 
         self.assertTrue(os.path.exists(tmp_rc_local))
 
     def test_run(self):
+        """Test running all test suites."""
         retcode = self.runner.run()
 
         for suite in self.runner.suites:
@@ -94,9 +103,7 @@
         self.assertEqual(retcode, ReturnCodes.FAIL)
 
     def test_missing_testdir(self):
-        """
-        Test that the runner fails gracefully on missing testdir.
-        """
+        """Test that the runner fails gracefully on missing testdir."""
         self.assertRaises(exceptions.BadDir,
                           Runner,
                           install_type="desktop",
@@ -104,10 +111,7 @@
                           testdir=self.bad_dir)
 
     def test_missing_master_runlist(self):
-        """
-        Test that the runner fails gracefully on missing 'master.run'
-        """
-
+        """Test that the runner fails gracefully on missing 'master.run'."""
         tmp_master_runlist = os.path.join(self.bad_dir, 'master.run')
 
         if not os.path.exists(self.bad_dir):
@@ -125,9 +129,7 @@
                           testdir=self.bad_dir)
 
     def test_runlist(self):
-        """
-        Test that passing a runlist to the Runner works properly.
-        """
+        """Test that passing a runlist to the Runner works properly."""
         runlist = '/tmp/master_runlist_test'
 
         with open(runlist, 'w') as fp:
@@ -145,17 +147,14 @@
         os.remove(runlist)
 
     def test_testdir(self):
-        """
-        Test that the default test directory is created.
-        """
-
+        """Test that the default test directory is created."""
         testdir = self.runner.testdir
 
         self.assertTrue(os.path.exists(testdir))
 
     @patch('utah.client.runner.TestSuite')
     def test_include_exclude(self, mock_class):
-        """Test that includes/excludes are passed to the test suite"""
+        """Test that includes/excludes are passed to the test suite."""
         include_runlist_content = """---
 testsuites:
   - name: utah_tests

=== modified file 'utah/client/tests/test_state_agent.py'
--- utah/client/tests/test_state_agent.py	2013-03-21 21:22:10 +0000
+++ utah/client/tests/test_state_agent.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test our ability to save and restore state information."""
+
+
 import os
 import unittest
 import yaml
@@ -21,7 +24,7 @@
 from utah.client.runner import Runner
 from utah.client.result import ResultYAML
 
-from utah.client.tests.common import (
+from utah.client.tests.common import (  # NOQA
     setUp,  # Used by nosetests
     tearDown,  # Used by nosetests
     partial_state_file_content,
@@ -31,10 +34,11 @@
 
 
 class TestStateAgentYAML(unittest.TestCase):
+
+    """Test the YAML state agent."""
+
     def setUp(self):
-        """
-        Create a state agent for testing.
-        """
+        """Create a state agent for testing."""
         self.state_file = '/tmp/state.yaml'
 
         if os.path.exists(self.state_file):
@@ -46,19 +50,18 @@
             state_agent=self.state_agent)
 
     def tearDown(self):
-        """
-        Clean up the test state file.
-        """
-
+        """Clean up the test state file."""
         if os.path.exists(self.state_file):
             os.remove(self.state_file)
 
     def test_creation(self):
+        """Test that state file is correctly created."""
         self.runner.run()
 
         self.assertTrue(os.path.exists(self.state_file))
 
     def test_cleanup(self):
+        """Test that cleanup removes state file."""
         self.runner.save_state()
 
         self.assertTrue(os.path.exists(self.state_file),
@@ -67,12 +70,11 @@
         self.state_agent.clean()
 
         self.assertFalse(os.path.exists(self.state_file),
-                         "Failed to remove state file (%s)" % self.state_file)
+                         'Failed to remove state file ({})'
+                         .format(self.state_file))
 
     def test_status(self):
-        """
-        Test that a runner properly saves its state during test runs.
-        """
+        """Test that a runner properly saves its state during test runs."""
         suite_count = self.runner.count_suites()
         test_count = self.runner.count_tests()
 
@@ -121,21 +123,15 @@
         self.assertEqual(runner.passes + runner.failures + runner.errors, t)
 
     def test_load_state_partial(self):
-        """
-        Test that a partially run state file can be resumed.
-        """
+        """Test that a partially run state file can be resumed."""
         self._test_partial(partial_state_file_content, 'test_two', 4, 1, 0)
 
     def test_load_state_partial_run_all(self):
-        """
-        Test partially run state file - run all jobs.
-        """
+        """Test partially run state file - run all jobs."""
         self._test_partial(
             partial_state_file_content_run_all, 'test_one', 3, 2, 0)
 
     def test_load_state_partial_done_all_failed(self):
-        """
-        Test partially run state file - all jobs failed.
-        """
+        """Test partially run state file - all jobs failed."""
         self._test_partial(
             partial_state_file_content_done_all_failed, None, 0, 5, 0)

=== modified file 'utah/client/tests/test_testcase.py'
--- utah/client/tests/test_testcase.py	2013-02-11 19:41:45 +0000
+++ utah/client/tests/test_testcase.py	2013-04-08 19:17:22 +0000
@@ -13,27 +13,30 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test handling of test cases."""
+
+
 import os
 import unittest
 
+from utah.client import testcase
+from utah.client.common import (
+    UTAH_DIR,
+)
 from utah.client.exceptions import MissingData
 from utah.client.result import ResultYAML
-from utah.client.common import (
-    UTAH_DIR,
-)
-from utah.client import testcase
-
-from .common import (  # NOQA
+from utah.client.tests.common import (  # NOQA
     setUp,  # Used by nosetests
     tearDown,  # Used by nosetests
 )
 
 
 class TestTestCase(unittest.TestCase):
-    """
-    Tests for the TestCase class.
-    """
+
+    """Tests for the TestCase class."""
+
     def setUp(self):
+        """Initialize basic test case data."""
         self.timeout = 10
         self.command = 'echo "command"'
         self.result = ResultYAML()
@@ -53,9 +56,7 @@
         )
 
     def test_run(self):
-        """
-        Test that a TestCase can be run.
-        """
+        """Test that a TestCase can be run."""
         self.case.run()
 
         # make sure after a testcase run that the cwd hasn't changed.
@@ -63,12 +64,15 @@
         self.assertTrue(self.case.is_done())
 
     def test_timeout(self):
+        """Test that timeouts are parsed correctly."""
         self.assertEqual(self.case.timeout, self.timeout)
 
     def test_command(self):
+        """Test that commands are parsed correctly."""
         self.assertEqual(self.case.command, self.command)
 
     def test_save_state(self):
+        """Test that state can be saved."""
         state = self.case.save_state()
 
         self.assertEqual(state['status'], 'NOTRUN')
@@ -76,6 +80,7 @@
         self.assertEqual(state['timeout'], self.timeout)
 
     def test_save_and_load_state(self):
+        """Test that state can be saved and loaded."""
         state = self.case.save_state()
 
         case = testcase.TestCase(
@@ -97,6 +102,7 @@
         self.assertTrue(case.is_done())
 
     def test_timeout_override(self):
+        """Test that overriding timeouts works."""
         timeout = 99
         case = testcase.TestCase(
             name=self.name,
@@ -108,10 +114,7 @@
         self.assertEqual(timeout, case.timeout)
 
     def test_control_data(self):
-        """
-        Test that a correctly formed control_data that's passed in
-        doesn't raise an exception.
-        """
+        """Test that valid control_data doesn't raise an exception."""
         control_data = {
             'description': 'a test case',
             'command': '/bin/true',
@@ -137,10 +140,7 @@
         self.assertTrue(case.is_done())
 
     def test_bad_control_data(self):
-        """
-        Test that an incorrectly formed control_data that's passed in
-        does raise an exception.
-        """
+        """Test that bad control_data does raise an exception."""
         control_data = {
             'description': 'a test case',
             'command': '/bin/true',

=== modified file 'utah/client/tests/test_testsuite.py'
--- utah/client/tests/test_testsuite.py	2012-12-11 09:47:51 +0000
+++ utah/client/tests/test_testsuite.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test handling of test suites."""
+
+
 import os
 import unittest
 
@@ -22,24 +25,23 @@
 from utah.client.result import ResultYAML
 from utah.client import testsuite
 
-from .common import (  # NOQA
+from utah.client.tests.common import (  # NOQA
     setUp,  # Used by nosetests
     tearDown,  # Used by nosetests
 )
 
 
 def state_saver():
+    """Provide state saving functionality for testing."""
     debug_print("Saving state", force=True)
 
 
 class TestTestSuite(unittest.TestCase):
-    """
-    Tests for the TestSuite class.
-    """
+
+    """Tests for the TestSuite class."""
+
     def setUp(self):
-        """
-        Use the 'examples' test suite that's part of the UTAH package.
-        """
+        """Use the 'examples' test suite that's part of the UTAH package."""
         self.name = 'examples'
         self.path = '/var/lib/utah/testsuites'
         self.assertTrue(os.path.exists(os.path.join(self.path, self.name)))
@@ -49,23 +51,28 @@
                                          _save_state_callback=state_saver)
 
     def test_run(self):
-        print "found %d tests" % self.suite.count_tests()
+        """Test running a test suite."""
+        print 'found {} tests'.format(str(self.suite.count_tests()))
         self.suite.run()
         self.assertEqual(self.suite.result.status, 'PASS')
 
     def test_count_tests(self):
+        """Test ability of test suite to count tests."""
         self.assertEquals(self.suite.count_tests(), 2)
 
     def test_next_test(self):
+        """Test ability of test suite to indicate next test to run."""
         next_test = self.suite.get_next_test()
 
         self.assertTrue(hasattr(next_test, 'name'))
         self.assertEquals(self.suite.get_next_test().name, 'test_one')
 
     def test_is_done(self):
+        """Test test suite completion reporting."""
         self.assertFalse(self.suite.is_done())
 
     def test_save_state(self):
+        """Test saving test suite state."""
         state = self.suite.save_state()
 
         print state
@@ -79,6 +86,7 @@
         self.assertEqual(tests, self.suite.count_tests())
 
     def test_timeout_override(self):
+        """Test overriding a timeout value."""
         timeout = 101
         suite = testsuite.TestSuite(
             name=self.name,
@@ -94,7 +102,7 @@
             self.assertEqual(timeout, test.timeout)
 
     def test_include(self):
-        """Included tests are executed"""
+        """Verify that included tests are executed."""
         included_test_name = 'test_one'
 
         suite = testsuite.TestSuite(
@@ -112,7 +120,7 @@
         self.assertEqual(case.name, included_test_name)
 
     def test_exclude(self):
-        """Excluded tests are *not* executed"""
+        """Verify that excluded tests are not executed."""
         excluded_test_name = 'test_two'
 
         suite = testsuite.TestSuite(
@@ -130,7 +138,7 @@
         self.assertNotEqual(case.name, excluded_test_name)
 
     def test_include_exclude(self):
-        """Include/exclude combination works fine"""
+        """Verify that include/exclude combination works fine."""
         included_test_name = 'test_one'
         excluded_test_name = 'test_two'
 

=== modified file 'utah/client/tests/test_vcs_bzr.py'
--- utah/client/tests/test_vcs_bzr.py	2012-12-12 11:22:58 +0000
+++ utah/client/tests/test_vcs_bzr.py	2013-04-08 19:17:22 +0000
@@ -13,20 +13,22 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test bzr interface."""
+
+
 import os
 import shutil
 import unittest
 
-from utah.client.common import BzrHandler, run_cmd
+from utah.client.common import run_cmd
+from utah.client.vcs import BzrHandler
+
 
 BZR_TEST_REPO = "/tmp/utahbzrtest"
 
 
 def setUp():
-    """
-    Create a local bzr repository to test with.
-    """
-
+    """Create a local bzr repository for testing."""
     # should fail if the repo already exists.
     os.mkdir(BZR_TEST_REPO)
 
@@ -37,24 +39,17 @@
 
 
 def tearDown():
-    """
-    Remove the test repo.
-    """
-
+    """Remove the test repo."""
     # We should only ever get here if the repo was created in setUp().
     shutil.rmtree(BZR_TEST_REPO)
 
 
 class TestBzrHandler(unittest.TestCase):
-    """
-    Test the bazaar VCS handler.
-    """
+
+    """Test the bazaar VCS handler."""
 
     def test_init(self):
-        """
-        Test the initial setup.
-        """
-
+        """Test the initial setup."""
         repo = BZR_TEST_REPO
         bzr = BzrHandler(repo=repo)
 
@@ -64,6 +59,7 @@
         self.assertEqual(bzr.rev_command, "bzr revno")
 
     def test_bzr_get(self):
+        """Test retrieving a bzr repo."""
         repo = BZR_TEST_REPO
         dest = "utah-dev"
         bzr = BzrHandler(repo=repo, destination=dest)
@@ -79,6 +75,7 @@
         shutil.rmtree(path)
 
     def test_bzr_revision(self):
+        """Test getting revision information from a bzr repo."""
         repo = BZR_TEST_REPO
         dest = "utah-dev"
         bzr = BzrHandler(repo=repo, destination=dest)
@@ -92,7 +89,7 @@
 
         self.assertTrue(os.path.exists(path), path)
 
-        result = bzr.get_revision(directory=path)
+        result = bzr.revision(directory=path)
 
         shutil.rmtree(path)
-        self.assertIsInstance(int(result), int)
+        self.assertEqual(result['returncode'], 0)

=== modified file 'utah/client/tests/test_vcs_dev.py'
--- utah/client/tests/test_vcs_dev.py	2012-12-12 11:22:58 +0000
+++ utah/client/tests/test_vcs_dev.py	2013-04-08 19:17:22 +0000
@@ -13,20 +13,22 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test the dev pseudo-VCS interface."""
+
+
 import os
 import shutil
 import unittest
 
-from utah.client.common import DevHandler, run_cmd
+from utah.client.common import run_cmd
+from utah.client.vcs import DevHandler
+
 
 DEV_TEST_REPO = "/tmp/utahdevtest"
 
 
 def setUp():
-    """
-    Create a local directory to test on.
-    """
-
+    """Create a local directory for testing."""
     # should fail if the repo already exists.
     os.mkdir(DEV_TEST_REPO)
 
@@ -34,24 +36,17 @@
 
 
 def tearDown():
-    """
-    Remove the test repo.
-    """
-
+    """Remove the test repo."""
     # We should only ever get here if the repo was created in setUp().
     shutil.rmtree(DEV_TEST_REPO)
 
 
 class TestDevHandler(unittest.TestCase):
-    """
-    Test the bazaar VCS handler.
-    """
+
+    """Test the dev pseudo-VCS handler."""
 
     def test_init(self):
-        """
-        Test the initial setup.
-        """
-
+        """Test the initial setup."""
         repo = DEV_TEST_REPO
         dev = DevHandler(repo=repo)
 
@@ -61,6 +56,7 @@
         self.assertEqual(dev.rev_command, "echo 'DEVELOPMENT'")
 
     def test_dev_get(self):
+        """Test copying files."""
         repo = DEV_TEST_REPO
         dest = "utah-dev"
         dev = DevHandler(repo=repo, destination=dest)
@@ -76,6 +72,7 @@
         shutil.rmtree(path)
 
     def test_dev_revision(self):
+        """Test fake revision gathering support."""
         repo = DEV_TEST_REPO
         dest = "utah-dev"
         dev = DevHandler(repo=repo, destination=dest)
@@ -87,7 +84,7 @@
         self.assertEqual(result['returncode'], 0)
         self.assertTrue(os.path.exists(path))
 
-        result = dev.get_revision(directory=path)
+        result = dev.revision(directory=path)
 
         shutil.rmtree(path)
-        self.assertIsInstance(result, str)
+        self.assertEqual(result['returncode'], 0)

=== modified file 'utah/client/tests/test_vcs_git.py'
--- utah/client/tests/test_vcs_git.py	2012-12-12 11:22:58 +0000
+++ utah/client/tests/test_vcs_git.py	2013-04-08 19:17:22 +0000
@@ -13,20 +13,22 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test the git interface."""
+
+
 import os
 import shutil
 import unittest
 
-from utah.client.common import GitHandler, run_cmd
+from utah.client.common import run_cmd
+from utah.client.vcs import GitHandler
+
 
 GIT_TEST_REPO = "/tmp/utahgittest"
 
 
 def setUp():
-    """
-    Create a local git repository to test on.
-    """
-
+    """Create a local git repository for testing."""
     # should fail if the repo already exists.
     os.mkdir(GIT_TEST_REPO)
 
@@ -37,24 +39,17 @@
 
 
 def tearDown():
-    """
-    Remove the test repo.
-    """
-
+    """Remove the test repo."""
     # We should only ever get here if the repo was created in setUp().
     shutil.rmtree(GIT_TEST_REPO)
 
 
 class TestGitHandler(unittest.TestCase):
-    """
-    Test the bazaar VCS handler.
-    """
+
+    """Test the bazaar VCS handler."""
 
     def test_init(self):
-        """
-        Test the initial setup.
-        """
-
+        """Test the initial setup."""
         repo = GIT_TEST_REPO
         git = GitHandler(repo=repo)
 
@@ -64,6 +59,7 @@
         self.assertEqual(git.rev_command, "git rev-parse HEAD")
 
     def test_git_get(self):
+        """Test retrieving a git repo."""
         repo = GIT_TEST_REPO
         dest = "utah-dev"
         git = GitHandler(repo=repo, destination=dest)
@@ -79,6 +75,7 @@
         shutil.rmtree(path)
 
     def test_git_revision(self):
+        """Test getting revision information from a git repo."""
         repo = GIT_TEST_REPO
         dest = "utah-dev"
         git = GitHandler(repo=repo, destination=dest)
@@ -90,7 +87,7 @@
         self.assertEqual(result['returncode'], 0)
         self.assertTrue(os.path.exists(path))
 
-        result = git.get_revision(directory=path)
+        result = git.revision(directory=path)
 
         shutil.rmtree(path)
-        self.assertIsInstance(result, str)
+        self.assertEqual(result['returncode'], 0)

=== modified file 'utah/client/tests/test_yaml.py'
--- utah/client/tests/test_yaml.py	2012-12-08 02:10:12 +0000
+++ utah/client/tests/test_yaml.py	2013-04-08 19:17:22 +0000
@@ -13,12 +13,19 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+"""Test YAML functionality."""
+
+
 import unittest
 import yaml
 
 
 class TestYAML(unittest.TestCase):
+
+    """Test YAML processing."""
+
     def test_parts(self):
+        """Test initial YAML processing."""
         content = {}
         content['metadata'] = {
             'runlist': '/tmp/master.run',
@@ -41,6 +48,7 @@
         print(dec_data)
 
     def test_parts_2(self):
+        """Test additional YAML processing."""
         content = {
             'file': '/tmp/file'
         }
@@ -76,6 +84,7 @@
         print(dec_data)
 
     def test_parts_3(self):
+        """Test final YAML processing."""
         extra_args = {}
         content = [
             {

=== modified file 'utah/client/testsuite.py'
--- utah/client/testsuite.py	2013-03-14 20:36:41 +0000
+++ utah/client/testsuite.py	2013-04-08 19:17:22 +0000
@@ -13,9 +13,8 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Testsuite specific code.
-"""
+"""Testsuite specific code."""
+
 
 import jsonschema
 import os
@@ -36,8 +35,11 @@
 
 
 def parse_runlist_file(runlist_file):
-    """
-    Parse a tslist.run runlist file and check agains schema
+    """Parse a tslist.run runlist file and check against schema.
+
+    :returns: Parsed data from the runlist
+    :rtype: dict
+
     """
     with open(runlist_file, 'r') as fp:
         data = yaml.load(fp)
@@ -48,9 +50,8 @@
 
 
 class TestSuite(object):
-    """
-    The TestSuite class.
-    """
+
+    """Base class describing a test suite."""
 
     build_cmd = None
     timeout = None
@@ -138,7 +139,7 @@
                 control_data = None
             except jsonschema.ValidationError as exception:
                 raise exceptions.ValidationError(
-                    '{!r} test suite control file invalid: {!r}\n1'
+                    '{!r} test suite control file invalid: {!r}\n'
                     'Detailed information: {}'
                     .format(self.name, self.control_file, exception))
 
@@ -192,19 +193,15 @@
             self.tests.append(tc)
 
     def __str__(self):
-        return "%s: %s" % (self.control_file, self.runlist_file)
+        return "{}: {}".format(self.control_file, self.runlist_file)
 
     def set_status(self, status):
-        """
-        Set the status for the test suite and call the save state callback.
-        """
+        """Set the status and call the save state callback."""
         self.status = status
         self.save_state_callback()
 
     def build(self, result):
-        """
-        Run build, but only if we haven't started a run yet.
-        """
+        """Run build, but only if we haven't started a run yet."""
         if self.status == 'NOTRUN':
             self.set_status('BUILD')
             cmd_result = run_cmd(
@@ -215,9 +212,7 @@
             result.add_result(cmd_result)
 
     def setup(self, result):
-        """
-        Run ts_setup, but only if build() has just passed.
-        """
+        """Run ts_setup, but only if build() has just passed."""
         if self.status == 'BUILD' and result.status == 'PASS':
             self.set_status('SETUP')
             cmd_result = run_cmd(
@@ -228,9 +223,7 @@
             result.add_result(cmd_result)
 
     def cleanup(self, result):
-        """
-        Run ts_cleanup after a run.
-        """
+        """Run ts_cleanup after a run."""
         if self.status == 'INPROGRESS':
             self.set_status('CLEANUP')
             cmd_result = run_cmd(
@@ -241,8 +234,14 @@
             result.add_result(cmd_result)
 
     def run(self):
-        """
-        Run the test cases; including any build, setup, and cleanup commands.
+        """Run the complete test suite.
+
+        This includes any build, setup, and cleanup commands, as well as all
+        test cases (including build, setup, and cleanup.)
+
+        :returns: Whether to keep running tests (True) or reboot (False)
+        :rtype: bool
+
         """
 
         # we gather this info from the testcases
@@ -291,9 +290,7 @@
         return keep_going
 
     def add_test(self, tests):
-        """
-        Add a single test or list of tests to this suite.
-        """
+        """Add a single test or list of tests to this suite."""
         if isinstance(tests, TestCase):
             self.tests.append(tests)
         else:
@@ -304,16 +301,17 @@
                 pass
 
     def count_tests(self):
+        """Return the number of test cases in the suite."""
         return len(self.tests)
 
     def load_state(self, state):
-        """
-        Restore our state from the supplied dictionary.
+        """Restore our state from the supplied dictionary.
 
         Requires that the fieldnames in the dictionary match the class
         properties.
+
         """
-        # XXX: Should this be done explicitly?
+        # TODO: do this explicitly
         self.__dict__.update(state)
 
         self.tests = []
@@ -326,6 +324,7 @@
             self.tests.append(test)
 
     def save_state(self):
+        """Return a dictionary representing the suite's state."""
         state = {
             'name': self.name,
             'status': self.status,
@@ -345,20 +344,23 @@
         return state
 
     def is_done(self):
-        """
-        Determine if the suite is done.  This might mean that something has
-        failed.  Used by Runner to determine if the suite needs to be re-run
-        on resume.
+        """Determine if the suite is done.
+
+        This might mean that something has failed.
+        Used by Runner to determine if the suite needs to be re-run on resume.
+
+        :returns: Whether the test suite is finished (done or cleaned up)
+        :rtype: bool
+
         """
         return self.status == 'DONE' or self.status == 'CLEANUP'
 
     def get_next_test(self):
-        """
-        Return the next test to be run.
+        """Return the next test to be run.
 
         Mainly used for debugging.
+
         """
-
         test = None
 
         for t in self.tests:

=== added file 'utah/client/vcs.py'
--- utah/client/vcs.py	1970-01-01 00:00:00 +0000
+++ utah/client/vcs.py	2013-04-08 19:17:22 +0000
@@ -0,0 +1,144 @@
+# Ubuntu Testing Automation Harness
+# Copyright 2012 Canonical Ltd.
+
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""UTAH client VCS support."""
+
+
+from utah.client.common import (
+    CMD_TS_FETCH,
+    run_cmd,
+)
+
+
+REPO_DEFAULT_DIR = "."
+
+
+class VCSHandler(object):
+
+    """Base class for Version Control System support.
+
+    :param repo: Repository location
+    :type repo: str
+    :param destination:
+        Local directory where the repository should be made available
+    :type destination: str
+    :param battery_measurements:
+        Whether battery information should be gathered when running any of the
+        VCS commands
+    :type battery_measurements: bool
+
+    """
+
+    def __init__(self, repo, destination='', battery_measurements=False):
+        self.repo = repo
+        self.destination = destination
+        self.battery_measurements = battery_measurements
+
+    def get(self, directory=REPO_DEFAULT_DIR):
+        """Execute VCS get command.
+
+        The get command will make a copy of a given repository to the directory
+        specified as destination.
+
+        :param directory: Working directory
+        :type directory: str
+        :returns: Get command execution result
+        :rtype: dict
+
+        .. seealso:: :func:`run_cmd`
+
+        """
+        return run_cmd(self.get_command,
+                       cwd=directory,
+                       cmd_type=CMD_TS_FETCH,
+                       battery_measurements=self.battery_measurements)
+
+    def revision(self, directory=REPO_DEFAULT_DIR):
+        """Execute revision command.
+
+        :param directory: Working directory
+        :type directory: str
+        :returns: Revision command execution result.
+        :rtype: dict
+
+        """
+        return run_cmd(self.rev_command,
+                       cwd=directory,
+                       cmd_type=CMD_TS_FETCH,
+                       battery_measurements=self.battery_measurements)
+
+
+class BzrHandler(VCSHandler):
+
+    """Bazaar VCS handler."""
+
+    def __init__(self, repo, branch=True, options="", destination="",
+                 **kwargs):
+        super(BzrHandler, self).__init__(repo, destination, **kwargs)
+        self.options = options
+
+        if branch:
+            self.get_command = "bzr branch {} {} {}".format(
+                options,
+                self.repo,
+                self.destination,
+            )
+
+            self.rev_command = "bzr revno"
+        else:
+            self.get_command = "bzr export {} {} {}".format(
+                options,
+                self.destination,
+                self.repo,
+            )
+
+            self.rev_command = "bzr revno {}".format(self.repo)
+
+
+class GitHandler(VCSHandler):
+
+    """Git VCS handler."""
+
+    def __init__(self, repo, options="", destination="", **kwargs):
+        super(GitHandler, self).__init__(repo, destination, **kwargs)
+        self.options = options
+
+        self.get_command = "git clone {} {} {}".format(
+            options,
+            self.repo,
+            self.destination,
+        )
+
+        self.rev_command = "git rev-parse HEAD"
+
+
+class DevHandler(VCSHandler):
+
+    """Development VCS handler.
+
+    This isn't a handler for any VCS, but a helper to ease development and let
+    runlists point to a path that can be copied locally.
+
+    """
+
+    def __init__(self, repo, destination=".", **kwargs):
+        super(DevHandler, self).__init__(repo, destination, **kwargs)
+
+        self.get_command = "cp -r {} {}".format(
+            self.repo,
+            self.destination,
+        )
+
+        self.rev_command = "echo 'DEVELOPMENT'"

=== modified file 'utah/commandstr.py'
--- utah/commandstr.py	2012-12-03 14:02:18 +0000
+++ utah/commandstr.py	2013-04-08 19:17:22 +0000
@@ -13,19 +13,17 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Provide stringification for commands used by timeout and retry.
-"""
+"""Provide stringification for commands used by timeout and retry."""
 
 
 def commandstr(command, *args, **kw):
-    """
-    Convert a command and argument lists into a representation of that command
-    suitable for display to a user.
+    """Provide a user-oriented command display.
+
     i.e. commandstr(command, arg1, arg2, kw1=1, kw2=2)
     should return 'command(arg1, arg2, kw1=1, kw2=2)'
+
     """
-    msg = command.__name__ + '('
+    msg = '{}('.format(command.__name__)
     for arg in args:
         try:
             msg += arg.__name__

=== modified file 'utah/config.py'
--- utah/config.py	2013-03-29 12:26:45 +0000
+++ utah/config.py	2013-04-08 19:17:22 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
 
 # Ubuntu Testing Automation Harness
 # Copyright 2012 Canonical Ltd.
@@ -15,10 +15,11 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Provide config variables via utah.config.
+"""Provide config variables via utah.config.
+
 When run directly as part of the package build, output machine-independent
 defaults as json for writing a config file.
+
 """
 
 import getpass
@@ -36,9 +37,10 @@
 
 
 def getbridge():
-    """
-    Get virbr0 if it is set in the qemu config or the utah config.
+    """Get virbr0 if it is set in the qemu config or the utah config.
+
     Use this (or the default virbr0) as the bridge for bridged network VMs.
+
     """
     dns = os.path.join('/', 'etc', 'utah', 'dns')
     if os.path.isfile(dns):
@@ -235,13 +237,12 @@
 
 # These depend on other config options, so they're added last.
 # Default logfile is /var/log/utah/{hostname}.log
-LOCALDEFAULTS['logfile'] = os.path.join(DEFAULTS['logpath'],
-                                        LOCALDEFAULTS['hostname'] + '.log')
+LOCALDEFAULTS['logfile'] = os.path.join(
+    DEFAULTS['logpath'], '{}.log'.format(LOCALDEFAULTS['hostname']))
 # Default logfile is /var/log/utah/{hostname}-debug.log
 # Set to None to disable separate debug log
 LOCALDEFAULTS['debuglog'] = os.path.join(
-    DEFAULTS['logpath'],
-    LOCALDEFAULTS['hostname'] + '-debug.log')
+    DEFAULTS['logpath'], '{}-debug.log'.format(LOCALDEFAULTS['hostname']))
 
 # Log file used to log SSH related information
 LOCALDEFAULTS['ssh_logfile'] = \
@@ -314,7 +315,8 @@
             try:
                 CONFIG.update(json.load(fp))
             except ValueError:
-                sys.stderr.write(conffile + ' is not a valid JSON file')
+                sys.stderr.write('{} is not a valid JSON file'
+                                 .format(conffile))
 
 
 # Support ~-based paths on variables that use paths

=== modified file 'utah/exceptions.py'
--- utah/exceptions.py	2013-03-15 14:59:35 +0000
+++ utah/exceptions.py	2013-04-08 19:17:22 +0000
@@ -13,16 +13,17 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Provide exceptions for the rest of the tool.
-"""
+"""Provide exceptions for UTAH."""
 
 
 class UTAHException(Exception):
-    """
-    Provide a foundation class for UTAH exceptions.
-    Support retry argument, but default to False.
-    """
+
+    """Provide a foundation class for UTAH exceptions.
+
+    Support retry and external arguments, but default to False.
+
+    """
+
     def __init__(self, *args, **kw):
         self.retry = kw.pop('retry', False)
         self.external = kw.pop('external', False)

=== modified file 'utah/group.py'
--- utah/group.py	2012-12-03 14:02:18 +0000
+++ utah/group.py	2013-04-08 19:17:22 +0000
@@ -13,9 +13,9 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Unix group checking functionality
-"""
+"""Provide unix group checking functionality."""
+
+
 import grp
 import os
 import sys
@@ -25,9 +25,7 @@
 
 
 def check_user_group(group=config.group):
-    """
-    Return if the user is a member of the given group
-    """
+    """Return whether the user is a member of the given group."""
     user_gids = os.getgroups()
     gid = grp.getgrnam(group).gr_gid
 
@@ -35,16 +33,14 @@
 
 
 def print_group_error_message(script):
-    """
-    Print error message to stderr to be used by scripts
-    """
+    """Print error message to stderr to be used by scripts."""
     argv = list(sys.argv)
     argv[0] = os.path.abspath(script)
     message = ["Error: you are not in the utah group.",
                ("If you believe you have properly configured "
                 "your user account for UTAH use, try:"),
-               '    sudo usermod -a -G utah ' + getpass.getuser(),
+               '    sudo usermod -a -G utah {}'.format(getpass.getuser()),
                "Otherwise, please run this script as the utah user, i.e.:",
-               "    sudo su - utah -c '" + ' '.join(argv) + "'\n"]
+               "    sudo su - utah -c '{}'\n".format(' '.join(argv))]
     message = '\n'.join(message)
     sys.stderr.write(message)

=== modified file 'utah/iso.py'
--- utah/iso.py	2013-03-20 19:17:23 +0000
+++ utah/iso.py	2013-04-08 19:17:22 +0000
@@ -33,10 +33,13 @@
 
 
 class UTAHISOException(UTAHException):
+
+    """Provide an exception specific to ISO errors."""
+
     pass
 
 
-def get_resource(method, url, *args, **kw):
+def _get_resource(method, url, *args, **kw):
     try:
         resource = method(url, *args, **kw)
         return resource
@@ -51,9 +54,9 @@
 
 
 class ISO(object):
-    """
-    Provide a simplified method of interfacing with images.
-    """
+
+    """Provide a simplified method of interfacing with images."""
+
     def __init__(self, arch=None, dlpercentincrement=None, dlretries=None,
                  image=None, installtype=None, logger=None, series=None):
         if dlpercentincrement is None:
@@ -69,22 +72,21 @@
         else:
             self.logger = logger
         if image is None:
-            self.logger.debug('Need to retrieve image')
             path = self.downloadiso(arch=arch, installtype=installtype,
                                     series=series)
         else:
             if image.startswith('~'):
                 path = os.path.expanduser(image)
-                self.logger.debug('Rewriting ~-based path: ' + image +
-                                  ' to: ' + path)
+                self.logger.debug('Rewriting ~-based path: %s to: %s', image,
+                                  path)
             else:
                 path = image
 
         self.percent = 0
-        self.logger.info('Preparing image: ' + path)
-        self.image = get_resource(urllib.urlretrieve, path,
-                                  reporthook=self.dldisplay)[0]
-        self.logger.debug(path + ' is locally available as ' + self.image)
+        self.logger.info('Preparing image: %s', path)
+        self.image = _get_resource(urllib.urlretrieve, path,
+                                   reporthook=self.dldisplay)[0]
+        self.logger.info('%s is locally available as %s', path, self.image)
         self.installtype = self.getinstalltype()
         if self.installtype == 'mini':
             self.logger.debug('Using mini image')
@@ -104,9 +106,7 @@
         self.buildnumber = self.getbuildnumber()
 
     def _loggersetup(self):
-        """
-        Initialize the logging for the image.
-        """
+        """Initialize the logging for the image."""
         self.logger = logging.getLogger('utah.iso')
         self.logger.propagate = False
         self.logger.setLevel(logging.INFO)
@@ -119,8 +119,8 @@
         self.consolehandler.setLevel(config.consoleloglevel)
         self.logger.addHandler(self.consolehandler)
         self.filehandler = logging.handlers.WatchedFileHandler(config.logfile)
-        file_formatter = logging.Formatter('%(asctime)s iso '
-                                           + ' %(levelname)s: %(message)s')
+        file_formatter = logging.Formatter(
+            '%(asctime)s iso %(levelname)s: %(message)s')
         self.filehandler.setFormatter(file_formatter)
         self.filehandler.setLevel(config.fileloglevel)
         self.logger.addHandler(self.filehandler)
@@ -133,13 +133,16 @@
             self.logger.addHandler(self.debughandler)
 
     def getinstalltype(self):
-        """
-        Inspect the image's files to get the image type.
+        """Inspect the image's files to get the image type.
+
         If .disk/mini-info exists, it's mini.
         If the casper directory exists, it's desktop.
         If ubuntu-server.seed exists in the preseeds directory, it's server.
+        :returns: Image type
+        :rtype: str
+
         """
-        self.logger.info('Getting image type of ' + self.image)
+        self.logger.info('Getting image type of %s', self.image)
         files = set(self.listfiles(returnlist=True))
         installtype = 'alternate'
         if '.disk/mini-info' in files:
@@ -149,53 +152,76 @@
         elif ('preseed/ubuntu-server.seed' in files or
               './preseed/ubuntu-server.seed' in files):
             installtype = 'server'
-        self.logger.info('Image type is: ' + installtype)
+        self.logger.info('Image type is: %s', installtype)
         return installtype
 
     def getarch(self):
-        """
-        Unpack the image's info file to get the arch.
+        """Unpack the image's info file to get the arch.
+
+        :returns: Image architecture
+        :rtype: str
+
         """
         arch = self.media_info.split()[-2]
-        self.logger.info('Arch is: ' + arch)
+        self.logger.info('Arch is: %s', arch)
         return arch
 
     def getseries(self):
-        """
-        Unpack the image's info file to get the series.
+        """Unpack the image's info file to get the series.
+
+        :returns: Image series
+        :rtype: str
+
         """
         for word in self.media_info.split():
             if word.startswith('"'):
                 series = word.strip('"').lower()
                 break
-        self.logger.info('Series is ' + series)
+        self.logger.info('Series is %s', series)
         return series
 
     def getbuildnumber(self):
-        """Unpack the image's info file to get the build number."""
+        """Unpack the image's info file to get the build number.
+
+        :returns: Build number of the image.
+        :rtype: str
+
+        """
         match = re.search('.*\(([0-9.]+)\)$', self.media_info)
         build_number = match.group(1) if match else '?'
         return build_number
 
     def dldisplay(self, blocks, size, total):
-        """
-        Display download information.
-        Intended for use as a urllib callback.
+        """Log download information (i.e., as a urllib callback).
+
+        :param blocks: Number of blocks downloaded
+        :type blocks: int
+        :param size: Size of blocks downloaded
+        :type size: int
+        :param total: Total size of download
+        :type size: int
+
         """
         read = blocks * size
         percent = 100 * read / total
         if percent >= self.percent:
-            self.logger.info('File ' + str(percent) + '% downloaded')
+            self.logger.info('File %s%% downloaded', percent)
             self.percent += self.dlpercentincrement
-        self.logger.debug("%s read, %s%% of %s total" % (read, percent, total))
+        self.logger.debug('%s read, %s%% of %s total', read, percent, total)
 
     def listfiles(self, returnlist=False):
-        """
+        """Return the contents of the ISO.
+
         Return either a subprocess instance listing the contents of an ISO, or
         a list of files in the ISO if returnlist is True.
+        :returns: The contents of the ISO
+        :rtype: list or object
+
         """
+        # TODO: See if we still need this outside of validation
+        # move it to validation if it's only needed there
         cmd = ['bsdtar', '-t', '-f', self.image]
-        self.logger.debug('bsdtar list command: ' + ' '.join(cmd))
+        self.logger.debug('bsdtar list command: %s', ' '.join(cmd))
         if returnlist:
             try:
                 proc = subprocess.check_output(cmd).strip().split('\n')
@@ -223,7 +249,7 @@
 
         """
         cmd = ['bsdtar', '-t', '-v', '-f', self.image, filename]
-        self.logger.debug('bsdtar list command: ' + ' '.join(cmd))
+        self.logger.debug('bsdtar list command: %s', ' '.join(cmd))
         try:
             output = subprocess.check_output(cmd)
         except (OSError, subprocess.CalledProcessError) as err:
@@ -237,7 +263,7 @@
                                    'for {} in {}: {}'
                                    .format(filename, self.image, output))
         cmd = ['bsdtar', '-x', '-f', self.image, '-O', realfile]
-        self.logger.debug('bsdtar extract command: ' + ' '.join(cmd))
+        self.logger.debug('bsdtar extract command: %s', ' '.join(cmd))
         return cmd
 
     def extract(self, filename, outdir='', outfile=None, **kw):
@@ -293,25 +319,39 @@
 
         return stdout
 
-    def getmd5(self, path):
-        """
-        Return the md5 checksum of a file.
+    def getmd5(self, path=None):
+        """Return the MD5 checksum of a file.
+
         Default file is this image.
+        :param path: Path of file to checksum
+        :type path: str
+        :returns: MD5 checksum of file
+        :rtype: str
+
         """
         if path is None:
             path = self.image
-        self.logger.debug('Getting md5 of ' + path)
+        self.logger.debug('Getting md5 of %s', path)
         isohash = md5()
         with open(path) as myfile:
             for block in iter(lambda: myfile.read(128), ""):
                 isohash.update(block)
         filemd5 = isohash.hexdigest()
-        self.logger.debug('md5 of ' + path + ' is ' + filemd5)
+        self.logger.debug('md5 of %s is %s', path, filemd5)
         return filemd5
 
     def downloadiso(self, arch=None, installtype=None, series=None):
-        """
-        Download an ISO given series, type, and arch.
+        """Download an ISO given series, type, and arch.
+
+        :param arch: Architecture of image to download
+        :type arch: str
+        :param installtype: Install type of image to download
+        :type installtype: str
+        :param series: Series codename of image to download
+        :type series: str
+        :returns: Local path of downloaded image
+        :rtype: str
+
         """
         if arch is None:
             arch = config.arch
@@ -319,26 +359,26 @@
             installtype = config.installtype
         if series is None:
             series = config.series
-        filename = '-'.join([series, installtype, arch]) + '.iso'
-        self.logger.info('Attempting to retrieve ' + filename)
+        filename = '{}.iso'.format('-'.join([series, installtype, arch]))
+        self.logger.info('Attempting to retrieve %s', filename)
         with open(os.devnull, "w") as fnull:
             # If dlcommand (default dl-ubuntu-test-iso) is available, use it
             cmd = ['which', config.dlcommand]
             try:
                 if subprocess.call(cmd,
                                    stdout=fnull, stderr=fnull) == 0:
-                    self.logger.debug('Using ' + config.dlcommand)
+                    self.logger.debug('Using %s', config.dlcommand)
                     if installtype == 'server':
                         flavor = 'ubuntu-server'
                     else:
                         flavor = 'ubuntu'
                     cmd = [config.dlcommand,
                            '-q',
-                           '--flavor=' + flavor,
-                           '--release=' + series,
-                           '--variant=' + installtype,
-                           '--arch=' + arch,
-                           '--isoroot=' + config.isodir]
+                           '--flavor={}'.format(flavor),
+                           '--release={}'.format(series),
+                           '--variant={}'.format(installtype),
+                           '--arch={}'.format(arch),
+                           '--isoroot={}'.format(config.isodir)]
                     self.logger.info('Downloading ISO')
                     self.logger.debug(' '.join(cmd))
                     try:
@@ -361,7 +401,7 @@
         # First, we'll check the image.  If it matches, we return
         # If not, we'll download it, and start the loop over
         for attempt in range(config.dlretries):
-            self.logger.info('Download attempt ' + str(attempt))
+            self.logger.info('Download attempt %s', str(attempt))
             # Set the path to our files and the name of the iso we want
             if installtype == 'mini':
                 remotepath = ('http://archive.ubuntu.com/ubuntu/dists/'
@@ -371,39 +411,34 @@
             else:
                 remotepath = ('http://releases.ubuntu.com/'
                               '{series}/'.format(series=series))
-                isopattern = installtype + '-' + arch + '.iso'
+                isopattern = '{}-{}.iso'.format(installtype, arch)
 
-            # Scan the MD5SUMS file for our file
             servermd5 = None
             md5path = '{}/MD5SUMS'.format(remotepath)
-            md5list = get_resource(urllib.urlopen, md5path)
+            md5list = _get_resource(urllib.urlopen, md5path)
             for line in md5list:
                 if isopattern in line:
                     servermd5, isofile = line.split()
                     isofile = isofile.strip('*')
                     break
 
-            # If we can't find the ISO, raise an exception
             if servermd5 is None:
                 raise UTAHISOException(
                     'Specified ISO: {} not found on mirrors.'
                     .format(filename), external=True)
-            self.logger.debug('Server md5 is ' + servermd5)
+            self.logger.debug('Server md5 is %s', servermd5)
 
-            # If the ISO exists locally and the md5 matches, use it
             if os.path.isfile(path) and servermd5 == self.getmd5(path):
-                self.logger.info('Using ISO at ' + path)
+                self.logger.info('Using ISO at %s', path)
                 return path
 
-            # Try to download the ISO
             isopath = os.path.join(remotepath, isofile)
             self.percent = 0
-            self.logger.info('Attempting to download ' + isopath)
-            temppath = get_resource(urllib.urlretrieve, isopath,
-                                    reporthook=self.dldisplay)[0]
+            self.logger.info('Attempting to download %s', isopath)
+            temppath = _get_resource(urllib.urlretrieve, isopath,
+                                     reporthook=self.dldisplay)[0]
 
-            # Try to copy it into our cache
-            self.logger.debug('Copying ' + temppath + ' to ' + path)
+            self.logger.debug('Copying %s to %s', temppath, path)
             shutil.copyfile(temppath, path)
         else:
             if os.path.isfile(path) and servermd5 == self.getmd5(path):
@@ -414,6 +449,12 @@
                     .format(config.dlretries), external=True)
 
     def kernelpath(self):
+        """Return the path of the kernel inside the image.
+
+        :returns: Path to the kernel pulled from bootloader config
+        :rtype: str
+
+        """
         kernelpath = './install/vmlinuz'
         if self.installtype == 'mini':
             self.logger.debug('Getting kernel for mini image')
@@ -437,8 +478,8 @@
                                 self.logger.debug('Rejecting '
                                                   'memtest kernel')
                             else:
-                                self.logger.debug('Found kernel: '
-                                                  + newkernel)
+                                self.logger.debug(
+                                    'Found kernel: %s', newkernel)
                                 kernels[newkernel] += 1
             # Now we have a list of kernel paths and the number of times
             # each once occurs. We'll use the one that occurs most.

=== modified file 'utah/isotest/iso_static_validation.py'
--- utah/isotest/iso_static_validation.py	2013-01-03 11:51:00 +0000
+++ utah/isotest/iso_static_validation.py	2013-04-08 19:17:22 +0000
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python
 
 # Ubuntu Testing Automation Harness
 # Copyright 2012 Canonical Ltd.
@@ -35,14 +35,14 @@
 # <http://www.gnu.org/licenses/>.
 #
 
-# TODO: support for non-daily images
-
 """ISO static validation.
 
 The test cases in this scripts are used to make sure that an ISO image isn't
 broken and can be used for smoke testing.
 
 """
+# TODO: support for non-daily images
+
 
 import os
 import argparse
@@ -143,14 +143,16 @@
                              description=self.shortDescription() or ''))
         self.block_size = ONE_MB_BLOCK
         self.iso_location = iso_location
-        logging.debug('Using iso at: ' + self.iso_location)
+        logging.debug('Using iso at: %s', self.iso_location)
         self.iso = ISO(image=self.iso_location, logger=logging)
         self.st_release = self.iso.series
         self.st_variant = self.iso.installtype
         self.st_arch = self.iso.arch
-        self.iso_name = '-'.join([self.st_release, self.st_variant,
-                                  self.st_arch]) + '.iso'
-        logging.debug('Standard name for this iso is: ' + self.iso_name)
+        self.iso_name = ('{}.iso'
+                         .format('-'.join([self.st_release,
+                                           self.st_variant,
+                                           self.st_arch])))
+        logging.debug('Standard name for this iso is: %s', self.iso_name)
 
         if self.st_release != 'precise':
             self.url = DEFAULT_URL
@@ -212,15 +214,9 @@
 
     def test_files_list(self):
         """Test if the list repository file matches the content of the ISO."""
-        if self.st_variant == 'server':
-            list_url = os.path.join(self.url, 'current',
-                                    self.st_release + '-' + 'server' + '-' +
-                                    self.st_arch + '.list')
-        else:
-            list_url = os.path.join(self.url, 'current',
-                                    self.st_release + '-' +
-                                    self.st_variant + '-' +
-                                    self.st_arch + '.list')
+        list_url = os.path.join(
+            self.url, 'current', '{}.list'
+            .format('-'.join([self.st_release, st_variant, self.st_arch])))
 
         try:
             list_repository = urllib2.urlopen(list_url)
@@ -247,10 +243,10 @@
                          "manifest only for ubiquity based images")
     def test_manifest(self):
         """Test if the ISO manifest matches the one in the server."""
-        manifest_url = os.path.join(self.url, 'current',
-                                    self.st_release + '-' +
-                                    self.st_variant + '-' +
-                                    self.st_arch + '.manifest')
+        manifest_url = os.path.join(
+            self.url, 'current', '{}.manifest'
+            .format('-'.join([self.st_release, st_variant, self.st_arch])))
+
         try:
             manifest = urllib2.urlopen(manifest_url)
         except urllib2.HTTPError, e:
@@ -326,7 +322,7 @@
         for list_server in files_list:
             path = list_server.rstrip()
             if path in exclude_files:
-                logging.debug(path + ' excluded based on release')
+                logging.debug('%s excluded based on release', path)
             else:
                 self.assertIn(path, stdout)
 
@@ -370,7 +366,7 @@
             logging.debug('check if important d-i files are present in iso')
             path = list_server.rstrip()
             if path in exclude_files:
-                logging.debug(path + ' excluded based on release')
+                logging.debug('%s excluded based on release', path)
             else:
                 self.assertIn(path, stdout)
 

=== modified file 'utah/orderedcollections.py'
--- utah/orderedcollections.py	2012-12-08 02:10:12 +0000
+++ utah/orderedcollections.py	2013-04-08 19:17:22 +0000
@@ -13,9 +13,7 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Provide additional collections to support cleanup functions.
-"""
+"""Provide additional collections to support cleanup functions."""
 
 
 import collections
@@ -25,6 +23,8 @@
 
 class OrderedSet(collections.MutableSet):
 
+    """Provide a set with ordering."""
+
     def __init__(self, iterable=None):
         self.end = end = []
         end += [None, end, end]         # sentinel node for doubly linked list
@@ -39,12 +39,14 @@
         return key in self.map
 
     def add(self, key):
+        """add an item to the set."""
         if key not in self.map:
             end = self.end
             curr = end[PREV]
             curr[NEXT] = end[PREV] = self.map[key] = [key, curr, end]
 
     def discard(self, key):
+        """remove an item from the set."""
         if key in self.map:
             key, prev, next = self.map.pop(key)
             prev[NEXT] = next
@@ -65,6 +67,12 @@
             curr = curr[PREV]
 
     def pop(self, last=True):
+        """Remove an item from the set and return that item.
+
+        :param last: Remove the last added item (instead of the first)
+        :type last: bool
+
+        """
         if not self:
             raise KeyError('set is empty')
         key = next(reversed(self)) if last else next(iter(self))
@@ -86,5 +94,8 @@
 
 
 class HashableDict(dict):
+
+    """Provide a hashable dict."""
+
     def __hash__(self):
         return hash(tuple(sorted(self.items())))

=== modified file 'utah/parser.py'
--- utah/parser.py	2013-03-14 13:37:03 +0000
+++ utah/parser.py	2013-04-08 19:17:22 +0000
@@ -14,6 +14,8 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 """UTAH Client log parser."""
+
+
 import argparse
 import jsonschema
 import logging

=== modified file 'utah/preseed.py'
--- utah/preseed.py	2012-12-03 14:02:18 +0000
+++ utah/preseed.py	2013-04-08 19:17:22 +0000
@@ -63,6 +63,8 @@
 <BLANKLINE>
 
 """
+
+
 import string
 
 
@@ -885,6 +887,8 @@
 
     """
 
+    pass
+
 
 class DuplicatedQuestionName(Exception):
 
@@ -904,3 +908,5 @@
     DuplicatedQuestionName: passwd/username
 
     """
+
+    pass

=== modified file 'utah/process.py'
--- utah/process.py	2013-03-29 09:00:34 +0000
+++ utah/process.py	2013-04-08 19:17:22 +0000
@@ -14,6 +14,8 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 """Process related utilities."""
+
+
 import logging
 import subprocess
 

=== modified file 'utah/provisioning/__init__.py'
--- utah/provisioning/__init__.py	2012-12-03 14:02:18 +0000
+++ utah/provisioning/__init__.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,4 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-utah.provisioning
-"""
+"""utah.provisioning"""

=== modified file 'utah/provisioning/baremetal/__init__.py'
--- utah/provisioning/baremetal/__init__.py	2012-12-03 14:02:18 +0000
+++ utah/provisioning/baremetal/__init__.py	2013-04-08 19:17:22 +0000
@@ -13,6 +13,4 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-utah.provisioning.baremetal
-"""
+"""utah.provisioning.baremetal"""

=== modified file 'utah/provisioning/baremetal/bamboofeeder.py'
--- utah/provisioning/baremetal/bamboofeeder.py	2013-03-28 10:58:47 +0000
+++ utah/provisioning/baremetal/bamboofeeder.py	2013-04-08 19:17:22 +0000
@@ -13,9 +13,8 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Support bare-metal provisioning of bamboo-feeder-based systems.
-"""
+"""Support provisioning of bamboo-feeder-based systems."""
+
 
 import os
 import pipes
@@ -36,12 +35,13 @@
 
 
 class BambooFeederMachine(CustomInstallMixin, SSHMixin, PowerMixin, Machine):
-    """
-    Provide a class to provision an ARM board from a bamboo-feeder setup.
-    """
+
+    """Provision and manage an ARM board in a bamboo-feeder setup."""
+
+    # TODO: raise more exceptions if ProcessRunner fails
+    # maybe get some easy way to do that like failok
     def __init__(self, inventory=None, machineinfo=None, name=None,
                  preboot=None, *args, **kw):
-        # TODO: change the name of cargs here and elsewhere
         # TODO: respect rewrite setting
         if name is None:
             raise UTAHBMProvisioningException(
@@ -64,8 +64,8 @@
         self.logger.debug('Configuring for %s with IP %s',
                           config.wwwiface, self.ip)
         self._cmdlinesetup()
-        imageurl = 'http://' + self.ip + '/utah/' + self.name + '.img'
-        preenvurl = 'http://' + self.ip + '/utah/' + self.name + '.preEnv'
+        imageurl = 'http://{}/utah/{}.img'.format(self.ip, self.name)
+        preenvurl = 'http://{}/utah/{}.preEnv'.format(self.ip, self.name)
         if preboot is None:
             self.preboot = ('console=ttyO2,115200n8 imageurl={imageurl} '
                             'bootcfg={preenvurl}'.format(
@@ -78,20 +78,22 @@
         self.logger.debug('BambooFeederMachine init finished')
 
     def _prepareimage(self):
-        """
-        Make a copy of the image shared via wwa so we can edit it.
+        """Make a copy of the image and share it via www.
+
+        :returns: Path to www-available image
+        :rtype: str
+
         """
         self.logger.info('Making copy of install image')
-        self.wwwimage = os.path.join(config.wwwdir, self.name + '.img')
-        self.logger.debug('Copying ' + self.image + ' to ' + self.wwwimage)
+        self.wwwimage = os.path.join(config.wwwdir,
+                                     '{}.img'.format(self.name))
+        self.logger.debug('Copying %s to %s', self.image, self.wwwimage)
         self.cleanfile(self.wwwimage)
         shutil.copyfile(self.image, self.wwwimage)
         return self.wwwimage
 
     def _mountimage(self):
-        """
-        Mount an ARM boot image so we can edit it.
-        """
+        """Mount an ARM boot image."""
         self.logger.info('Mounting install image')
         self.imagedir = os.path.join(self.tmpdir, 'image.d')
         if not os.path.isdir(self.imagedir):
@@ -102,28 +104,24 @@
         except (OSError, subprocess.CalledProcessError) as err:
             raise UTAHBMProvisioningException('Failed to get image info: {}'
                                               .format(err))
-        self.logger.debug('Image start sector is ' + str(self.sector))
+        self.logger.debug('Image start sector is %s', str(self.sector))
         self.cleanfunction(self._umountimage)
-        self.logger.debug('Mounting ' + self.wwwimage + ' at ' +
-                          self.imagedir)
+        self.logger.debug('Mounting %s at %s', self.wwwimage, self.imagedir)
         ProcessRunner(['sudo', 'mount', self.wwwimage, self.imagedir,
-                       '-o', 'loop', '-o', 'offset=' + str(self.sector * 512),
-                       '-o', 'uid=' + config.user])
+                       '-o', 'loop',
+                       '-o', 'offset={}'.format(str(self.sector * 512)),
+                       '-o', 'uid={}'.format(config.user)])
 
     def _setupconsole(self):
-        """
-        Setup the install to use a serial console."
-        """
+        """Setup the install to use a serial console."""
         self.logger.info('Setting up the install to use the serial console')
         preenvfile = os.path.join(self.imagedir, 'preEnv.txt')
         serialpreenvfile = os.path.join(self.imagedir, 'preEnv.txt-serial')
-        self.logger.debug('Copying ' + serialpreenvfile + ' to ' + preenvfile)
+        self.logger.debug('Copying %s to %s', serialpreenvfile, preenvfile)
         shutil.copyfile(serialpreenvfile, preenvfile)
 
     def _unpackinitrd(self):
-        """
-        Unpack the uInitrd file into a directory so we can modify it.
-        """
+        """Unpack the uInitrd file into a directory."""
         self.logger.info('Unpacking uInitrd')
         if not os.path.isdir(os.path.join(self.tmpdir, 'initrd.d')):
             os.makedirs(os.path.join(self.tmpdir, 'initrd.d'))
@@ -139,10 +137,10 @@
             raise UTAHBMProvisioningException('Failed to get initrd info: {}'
                                               .format(err))
         headersize = filesize - datasize
-        self.logger.debug('uInitrd header size is ' + str(headersize))
+        self.logger.debug('uInitrd header size is %s', str(headersize))
         pipe = pipes.Template()
-        pipe.prepend('dd if=$IN bs=1 skip=' + str(headersize) +
-                     ' 2>/dev/null', 'f-')
+        pipe.prepend('dd if=$IN bs=1 skip={} 2>/dev/null'
+                     .format(str(headersize)), 'f-')
         pipe.append('gunzip', '--')
         pipe.append('cpio -ivd 2>/dev/null', '-.')
         exitstatus = pipe.copy(self.initrd, '/dev/null')
@@ -153,9 +151,7 @@
                 'Unpacking initrd exited with status {}'.format(exitstatus))
 
     def _repackinitrd(self):
-        """
-        Repack the uInitrd file from the initrd.d directory.
-        """
+        """Repack the uInitrd file from the initrd.d directory."""
         self.logger.info('Repacking uInitrd')
         os.chdir(os.path.join(self.tmpdir, 'initrd.d'))
         pipe = pipes.Template()
@@ -181,44 +177,40 @@
                        self.initrd])
 
     def _umountimage(self):
-        """
-        Unmount the image after we're done with it."
-        """
+        """Unmount the image after we're done with it."""
         self.logger.info('Unmounting image')
         ProcessRunner(['sudo', 'umount', self.imagedir])
 
     def _cmdlinesetup(self, boot=None):
-        """
-        Add the needed options to the command line for an automatic install.
-        """
+        """Add options to the command line for an automatic install."""
         if boot is None:
             boot = self.boot
         super(BambooFeederMachine, self)._cmdlinesetup(boot=boot)
         # TODO: minimize these
         for option in ('auto', 'ro', 'text'):
             if option not in self.cmdline:
-                self.logger.info('Adding boot option: ' + option)
-                self.cmdline += ' ' + option
-        for parameter in (('cdrom-detect/try_usb', 'true'),
-                          ('console', 'ttyO2,115200n8'),
-                          ('country', 'US'),
-                          ('hostname', self.name),
-                          ('language', 'en'),
-                          ('locale', 'en_US'),
-                          ('loghost', self.ip),
-                          ('log_port', '10514'),
-                          ('netcfg/choose_interface', 'auto'),
-                          ('url', 'http://' + self.ip + '/utah/' + self.name +
-                           '.cfg')):
+                self.logger.info('Adding boot option: %s', option)
+                self.cmdline += ' {}'.format(option)
+        for parameter in (
+            ('cdrom-detect/try_usb', 'true'),
+            ('console', 'ttyO2,115200n8'),
+            ('country', 'US'),
+            ('hostname', self.name),
+            ('language', 'en'),
+            ('locale', 'en_US'),
+            ('loghost', self.ip),
+            ('log_port', '10514'),
+            ('netcfg/choose_interface', 'auto'),
+            ('url', 'http://{}/utah/{}.cfg'.format(self.ip, self.name)),
+        ):
             if parameter[0] not in self.cmdline:
-                self.logger.info('Adding boot option: ' + '='.join(parameter))
-                self.cmdline += ' ' + '='.join(parameter)
+                self.logger.info('Adding boot option: %s',
+                                 '='.join(parameter))
+                self.cmdline += ' {}'.format('='.join(parameter))
         self.cmdline.strip()
 
     def _configurepxe(self):
-        """
-        Setup PXE configuration to boot remote image.
-        """
+        """Setup PXE configuration to boot remote image."""
         # TODO: Maybe move this into pxe.py
         # TODO: look into cutting out the middleman/
         # booting straight into the installer? (maybe nfs?)
@@ -234,37 +226,36 @@
 """.format(preboot=self.preboot)
         tmppxefile = os.path.join(self.tmpdir, 'pxe')
         open(tmppxefile, 'w').write(pxeconfig)
-        self.logger.debug('PXE info written to ' + tmppxefile)
-        pxefile = os.path.join(config.pxedir,
-                               '01-' + self.macaddress.replace(':', '-'))
+        self.logger.debug('PXE info written to %s', tmppxefile)
+        pxefile = os.path.join(
+            config.pxedir, '01-{}'.format(self.macaddress.replace(':', '-')))
         self.cleancommand(('sudo', 'rm', '-f', pxefile))
-        self.logger.debug('Copying ' + tmppxefile + ' to ' + pxefile)
+        self.logger.debug('Copying %s to %s', tmppxefile, pxefile)
         ProcessRunner(['sudo', 'cp', tmppxefile, pxefile])
-        preenvfile = os.path.join(config.wwwdir, self.name + '.preEnv')
+        preenvfile = os.path.join(config.wwwdir,
+                                  '{}.preEnv'.format(self.name))
         # TODO: sort this out with self.boot
         # figure out which one should be what and customizable
-        self.preenv = 'bootargs=' + self.cmdline
+        self.preenv = 'bootargs={}'.format(self.cmdline)
         self.logger.debug('Preenv setup:')
         self.logger.debug(self.preenv)
         self.cleanfile(preenvfile)
-        self.logger.debug('Writing preenv setup to ' + preenvfile)
+        self.logger.debug('Writing preenv setup to %s', preenvfile)
         open(preenvfile, 'w').write(self.preenv)
 
     def _create(self):
-        """
-        Install the OS on the system.
-        """
+        """Install the OS on the system."""
         # TODO: more checks and exceptions for failures
         self.logger.info('Preparing system install')
-        self.tmpdir = tempfile.mkdtemp(prefix='/tmp/' + self.name + '_')
+        self.tmpdir = tempfile.mkdtemp(prefix='/tmp/{}_'.format(self.name))
         self.cleanfile(self.tmpdir)
-        self.logger.debug('Using ' + self.tmpdir + ' as temp dir')
+        self.logger.debug('Using %s as temp dir', self.tmpdir)
         os.chdir(self.tmpdir)
         self._prepareimage()
         self._mountimage()
         self._setupconsole()
         self.initrd = os.path.join(self.imagedir, 'uInitrd')
-        self.logger.debug('uInitrd is ' + self.initrd)
+        self.logger.debug('uInitrd is %s', self.initrd)
         self._unpackinitrd()
         self._setuplatecommand()
         # TODO: check if this is still needed
@@ -278,7 +269,7 @@
             myfile.close()
         self._setuppreseed()
         self.logger.debug('Copying preseed to download location')
-        preseedfile = os.path.join(config.wwwdir, self.name + '.cfg')
+        preseedfile = os.path.join(config.wwwdir, '{}.cfg'.format(self.name))
         self.cleanfile(preseedfile)
         shutil.copyfile(os.path.join(self.tmpdir, 'initrd.d', 'preseed.cfg'),
                         preseedfile)
@@ -300,12 +291,9 @@
         self.cleanfunction(self.run, (
             'dd', 'bs=512k', 'count=10', 'if=/dev/zero', 'of=/dev/mmcblk0'),
             root=True)
-        return True
 
     def _depcheck(self):
-        """
-        Check for dependencies that are in Recommends or Suggests.
-        """
+        """Check for dependencies that are in Recommends or Suggests."""
         super(BambooFeederMachine, self)._depcheck()
         cmd = ['which', 'mkimage']
         if ProcessRunner(cmd).returncode != 0:

=== modified file 'utah/provisioning/baremetal/cobbler.py'
--- utah/provisioning/baremetal/cobbler.py	2013-03-29 15:55:46 +0000
+++ utah/provisioning/baremetal/cobbler.py	2013-04-08 19:17:22 +0000
@@ -13,9 +13,8 @@
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-Support bare metal provisioning through cobbler.
-"""
+"""Support bare metal provisioning through cobbler."""
+
 
 import os
 import pipes
@@ -35,9 +34,11 @@
 
 
 class CobblerMachine(CustomInstallMixin, SSHMixin, PowerMixin, Machine):
-    """
-    Provide a class to provision a machine via cobbler.
-    """
+
+    """Provision and manage a machine via cobbler."""
+
+    # TODO: raise more exceptions if ProcessRunner fails
+    # maybe get some easy way to do that like failok
     def __init__(self, inventory=None, machineinfo=None,
                  name=None, *args, **kw):
         # TODO: support for reusing existing machines
@@ -73,27 +74,22 @@
             self.carch = 'x86_64'
         else:
             self.carch = self.arch
-        self.cname = self.name + '-' + self.carch
-        self.logger.debug('Cobbler arch is ' + self.carch)
+        self.cname = '-'.join([self.name, self.carch])
+        self.logger.debug('Cobbler arch is %s', self.carch)
         self.logger.debug('Cobbler machine init finished')
 
     def _load(self):
-        """
-        Verify the machine is in cobbler.
-        """
-        # TODO: consider reworking _cobble to provide this
-        # (only if we'll be using cobbler for a while longer)
+        """Verify the machine is in cobbler."""
         machines = self._cobble(['system', 'find'])['output'].splitlines()
         if self.name not in machines:
             raise UTAHBMProvisioningException(
                 'No machine named {} exists in cobbler'.format(self.name))
+        else:
+            self.provisioned = True
 
-    def _create(self, checktimeout=config.checktimeout,
-                installtimeout=config.installtimeout):
-        """
-        Install a machine.
-        """
-        self.tmpdir = tempfile.mkdtemp(prefix='/tmp/' + self.name + '_')
+    def _create(self):
+        """Install the OS on the machine."""
+        self.tmpdir = tempfile.mkdtemp(prefix='/tmp/{}_'.format(self.name))
         self.cleanfile(self.tmpdir)
         os.chmod(self.tmpdir, 0775)
 
@@ -134,7 +130,7 @@
         if self.rewrite == 'all':
             self._setuplogging(tmpdir=self.tmpdir)
         else:
-            self.logger.debug('Skipping logging setup because rewrite is' +
+            self.logger.debug('Skipping logging setup because rewrite is %s',
                               self.rewrite)
 
         initrd = self._repackinitrd()
@@ -146,27 +142,34 @@
 
         if self.installtype in ['alternate', 'server']:
             self.logger.info('Importing image')
-            self._cobble(['import', '--name=' + self.cname,
-                          '--path=' + self.isodir, '--arch=' + self.carch])
-            self._cobble(['distro', 'edit', '--name=' + self.cname,
-                          '--kernel=' + kernel, '--initrd=' + initrd])
+            self._cobble(['import',
+                          '--name={}'.format(self.cname),
+                          '--path={}'.format(self.isodir),
+                          '--arch={}'.format(self.carch)])
+            self._cobble(['distro', 'edit',
+                          '--name={}'.format(self.cname),
+                          '--kernel={}'.format(kernel),
+                          '--initrd={}'.format(initrd)])
         elif self.installtype in ['mini', 'desktop']:
             self.logger.info('Creating distro')
-            self._cobble(['distro', 'add', '--name=' + self.cname,