[1290] address review comments

src/bin/bindctl/bindcmd.py

@@ -123,7 +123,7 @@ class BindCmdInterpreter(Cmd):
         '''Parse commands from user and send them to cmdctl. '''
         try:
             if not self.login_to_cmdctl():
-                return
+                return 1
 
             self.cmdloop()
             print('\nExit from bindctl')

tests/lettuce/README

@@ -112,6 +112,11 @@ Some very general steps are defined in terrain/steps.py.
 Initialization code, cleanup code, and helper classes are defined in
 terrain/terrain.py.
 
+To find the right steps, case insensitive matching is used. Parameters taken
+from the steps are case-sensitive though. So a step defined as
+'do foo with value (bar)' will be matched when using
+'Do Foo with value xyz', but xyz will be taken as given.
+
 If you need to add steps that are very particular to one test, create a new 
 file with a name relevant for that test in terrain. We may want to consider 
 creating a specific subdirectory for these, but at this moment it is unclear 

tests/lettuce/README.tutorial

@@ -114,17 +114,27 @@ Feature: showing off BIND 10
 
 So take a look at one of those steps, let's pick the first one.
 
-A step is defined through a python decorator, which in essence is a
+A step is defined through a python decorator, which in essence is a regular
-regular expression; each captured group will be passed as an argument
+expression; lettuce searches through all defined steps to find one that
-to the function we define. For bind10, i defined a configuration file,
+matches. These are 'partial' matches (unless specified otherwise in the
-a cmdctl port, and a process name. The first two should be
+regular expression itself), so if the step is defined with "do foo bar", the
-self-evident, and the process name is an optional name we give it,
+scenario can add words for readability "When I do foo bar".
-should we want to address it in the rest of the tests. This is most
+
-useful if we want to start multiple instances. In the next step (the
+Each captured group will be passed as an argument to the function we define.
-wait for auth to start), I added a 'of <instance>'. So if we define
+For bind10, i defined a configuration file, a cmdctl port, and a process
-the bind10 'as my_bind10', we can specify that one here as 'of my
+name. The first two should be self-evident, and the process name is an
+optional name we give it, should we want to address it in the rest of the
+tests. This is most useful if we want to start multiple instances. In the
+next step (the wait for auth to start), I added a 'of <instance>'. So if we 
+define the bind10 'as my_bind10', we can specify that one here as 'of my
 bind10'.
 
+--
+        When I start bind10 with configuration second.config
+        with cmdctl port 12345 as b10_second_instance
+--
+(line wrapped for readability)
+
 But notice how we needed two steps, which we probably always need (but
 not entirely always)? We can also combine steps; for instance:
 

tests/lettuce/configurations/example.org.config.orig

@@ -1 +1,17 @@
-{"version": 2, "Logging": {"loggers": [{"debuglevel": 99, "severity": "DEBUG", "name": "auth"}]}, "Auth": {"database_file": "data/example.org.sqlite3", "listen_on": [{"port": 47806, "address": "127.0.0.1"}]}}
+{
+    "version": 2,
+    "Logging": {
+        "loggers": [ {
+            "debuglevel": 99,
+            "severity": "DEBUG",
+            "name": "auth"
+        } ]
+    },
+    "Auth": {
+        "database_file": "data/example.org.sqlite3",
+        "listen_on": [ {
+            "port": 47806,
+            "address": "127.0.0.1"
+        } ]
+    }
+}

tests/lettuce/configurations/example2.org.config

@@ -1 +1,18 @@
-{"version": 2, "Logging": {"loggers": [{"severity": "DEBUG", "name": "auth", "debuglevel": 99}]}, "Auth": {"database_file": "data/example.org.sqlite3", "listen_on": [{"port": 47807, "address": "127.0.0.1"}]}}
+{
+    "version": 2,
+    "Logging": {
+        "loggers": [ {
+            "severity": "DEBUG",
+            "name": "auth",
+            "debuglevel": 99
+        }
+        ]
+    },
+    "Auth": {
+        "database_file": "data/example.org.sqlite3",
+        "listen_on": [ {
+            "port": 47807,
+            "address": "127.0.0.1"
+        } ]
+    }
+}

tests/lettuce/configurations/no_db_file.config

@@ -1 +1,10 @@
-{"version": 2, "Auth": {"database_file": "data/test_nonexistent_db.sqlite3", "listen_on": [{"port": 47806, "address": "127.0.0.1"}]}}
+{
+    "version": 2,
+    "Auth": {
+        "database_file": "data/test_nonexistent_db.sqlite3",
+        "listen_on": [ {
+            "port": 47806,
+            "address": "127.0.0.1"
+        } ]
+    }
+}

tests/lettuce/features/example.feature

@@ -18,7 +18,7 @@ Feature: Example feature
         # that we are sure this file does not exist, see
         # features/terrain/terrain.py
         
-        # Standard check to test (non-)existance of a file
+        # Standard check to test (non-)existence of a file
         # This file is actually automatically
         The file data/test_nonexistent_db.sqlite3 should not exist
 
@@ -85,6 +85,8 @@ Feature: Example feature
         The last query response should have ancount 0
         The last query response should have nscount 1
         The last query response should have adcount 0
+        # When checking flags, we must pass them exactly as they appear in
+        # the output of dig.
         The last query response should have flags qr aa rd
 
         A query for www.example.org type TXT should have rcode NOERROR

tests/lettuce/features/terrain/bind10_control.py

@@ -1,15 +1,43 @@
+# Copyright (C) 2011  Internet Systems Consortium.
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND INTERNET SYSTEMS CONSORTIUM
+# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
+# INTERNET SYSTEMS CONSORTIUM BE LIABLE FOR ANY SPECIAL, DIRECT,
+# INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
+# FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+# WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
 from lettuce import *
 import subprocess
 import re
 
-def check_lines(output, lines):
-    for line in lines:
-        if output.find(line) != -1:
-            return line
-
 @step('start bind10(?: with configuration (\S+))?' +\
       '(?: with cmdctl port (\d+))?(?: as (\S+))?')
 def start_bind10(step, config_file, cmdctl_port, process_name):
+    """
+    Start BIND 10 with the given optional config file, cmdctl port, and
+    store the running process in world with the given process name.
+    Parameters:
+    config_file ('with configuration <file>', optional): this configuration
+                will be used. The path is relative to the base lettuce
+                directory.
+    cmdctl_port ('with cmdctl port <portnr>', optional): The port on which
+                b10-cmdctl listens for bindctl commands. Defaults to 47805.
+    process_name ('as <name>', optional). This is the name that can be used
+                 in the following steps of the scenario to refer to this
+                 BIND 10 instance. Defaults to 'bind10'.
+    This call will block until BIND10_STARTUP_COMPLETE or BIND10_STARTUP_ERROR
+    is logged. In the case of the latter, or if it times out, the step (and
+    scenario) will fail.
+    It will also fail if there is a running process with the given process_name
+    already.
+    """
     args = [ 'bind10', '-v' ]
     if config_file is not None:
         args.append('-p')
@@ -36,6 +64,12 @@ def start_bind10(step, config_file, cmdctl_port, process_name):
 
 @step('wait for bind10 auth (?:of (\w+) )?to start')
 def wait_for_auth(step, process_name):
+    """Wait for b10-auth to run. This is done by blocking until the message
+       AUTH_SERVER_STARTED is logged.
+       Parameters:
+       process_name ('of <name', optional): The name of the BIND 10 instance
+                    to wait for. Defaults to 'bind10'.
+    """
     if process_name is None:
         process_name = "bind10"
     world.processes.wait_for_stderr_str(process_name, ['AUTH_SERVER_STARTED'],
@@ -43,12 +77,28 @@ def wait_for_auth(step, process_name):
 
 @step('have bind10 running(?: with configuration ([\w.]+))?')
 def have_bind10_running(step, config_file):
+    """
+    Compound convenience step for running bind10, which consists of
+    start_bind10 and wait_for_auth.
+    Currently only supports the 'with configuration' option.
+    """
     step.given('start bind10 with configuration ' + config_file)
     step.given('wait for bind10 auth to start')
 
-@step('set bind10 configuration (\S+) to (.*)')
+@step('set bind10 configuration (\S+) to (.*)(?: with cmdctl port (\d+))?')
-def set_config_command(step, name, value):
+def set_config_command(step, name, value, cmdctl_port):
-    args = ['bindctl', '-p', '47805']
+    """
+    Run bindctl, set the given configuration to the given value, and commit it.
+    Parameters:
+    name ('configuration <name>'): Identifier of the configuration to set
+    value ('to <value>'): value to set it to.
+    cmdctl_port ('with cmdctl port <portnr>', optional): cmdctl port to send
+                the command to. Defaults to 47805.
+    Fails if cmdctl does not exit with status code 0.
+    """
+    if cmdctl_port is None:
+        cmdctl_port = '47805'
+    args = ['bindctl', '-p', cmdctl_port]
     bindctl = subprocess.Popen(args, 1, None, subprocess.PIPE,
                                subprocess.PIPE, None)
     bindctl.stdin.write("config set " + name + " " + value + "\n")
@@ -56,5 +106,3 @@ def set_config_command(step, name, value):
     bindctl.stdin.write("quit\n")
     result = bindctl.wait()
     assert result == 0, "bindctl exit code: " + str(result)
-
-

tests/lettuce/features/terrain/querying.py

@@ -1,6 +1,17 @@
-from lettuce import *
+# Copyright (C) 2011  Internet Systems Consortium.
-import subprocess
+#
-import re
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND INTERNET SYSTEMS CONSORTIUM
+# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
+# INTERNET SYSTEMS CONSORTIUM BE LIABLE FOR ANY SPECIAL, DIRECT,
+# INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
+# FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+# WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 # This script provides querying functionality
 # The most important step is
@@ -16,6 +27,10 @@ import re
 #
 # Also see example.feature for some examples
 
+from lettuce import *
+import subprocess
+import re
+
 #
 # define a class to easily access different parts
 # We may consider using our full library for this, but for now
@@ -27,7 +42,8 @@ import re
 # The following attributes are 'parsed' from the response, all as strings,
 # and end up as direct attributes of the QueryResult object:
 # opcode, rcode, id, flags, qdcount, ancount, nscount, adcount
-# (flags is one string with all flags)
+# (flags is one string with all flags, in the order they appear in the
+# response packet.)
 #
 # this will set 'rcode' as the result code, we 'define' one additional
 # rcode, "NO_ANSWER", if the dig process returned an error code itself
@@ -43,7 +59,19 @@ class QueryResult(object):
                           "([0-9]+), AUTHORITY: ([0-9]+), ADDITIONAL: ([0-9]+)")
 
     def __init__(self, name, qtype, qclass, address, port):
-        args = [ 'dig', '+tries=1', '@' + address, '-p', str(port) ]
+        """
+        Constructor. This fires of a query using dig.
+        Parameters:
+        name: The domain name to query
+        qtype: The RR type to query. Defaults to A if it is None.
+        qclass: The RR class to query. Defaults to IN if it is None.
+        address: The IP adress to send the query to.
+        port: The port number to send the query to.
+        All parameters must be either strings or have the correct string
+        representation.
+        Only one query attempt will be made.
+        """
+        args = [ 'dig', '+tries=1', '@' + str(address), '-p', str(port) ]
         if qtype is not None:
             args.append('-t')
             args.append(str(qtype))
@@ -68,8 +96,9 @@ class QueryResult(object):
                 self.line_handler(out)
 
     def _check_next_header(self, line):
-        """Returns true if we found a next header, and sets the internal
+        """
-           line handler to the appropriate value.
+        Returns true if we found a next header, and sets the internal
+        line handler to the appropriate value.
         """
         if line == ";; ANSWER SECTION:\n":
             self.line_handler = self.parse_answer
@@ -84,6 +113,11 @@ class QueryResult(object):
         return True
 
     def parse_header(self, line):
+        """
+        Parse the header lines of the query response.
+        Parameters:
+        line: The current line of the response.
+        """
         if not self._check_next_header(line):
             status_match = self.status_re.search(line)
             flags_match = self.flags_re.search(line)
@@ -98,31 +132,69 @@ class QueryResult(object):
                 self.adcount = flags_match.group(5)
 
     def parse_question(self, line):
+        """
+        Parse the question section lines of the query response.
+        Parameters:
+        line: The current line of the response.
+        """
         if not self._check_next_header(line):
             if line != "\n":
                 self.question_section.append(line.strip())
 
     def parse_answer(self, line):
+        """
+        Parse the answer section lines of the query response.
+        Parameters:
+        line: The current line of the response.
+        """
         if not self._check_next_header(line):
             if line != "\n":
                 self.answer_section.append(line.strip())
 
     def parse_authority(self, line):
+        """
+        Parse the authority section lines of the query response.
+        Parameters:
+        line: The current line of the response.
+        """
         if not self._check_next_header(line):
             if line != "\n":
                 self.authority_section.append(line.strip())
 
-    def parse_authority(self, line):
+    def parse_additional(self, line):
+        """
+        Parse the additional section lines of the query response.
+        Parameters:
+        line: The current line of the response.
+        """
         if not self._check_next_header(line):
             if line != "\n":
                 self.additional_section.append(line.strip())
 
     def parse_footer(self, line):
+        """
+        Parse the footer lines of the query response.
+        Parameters:
+        line: The current line of the response.
+        """
         pass
 
 @step('A query for ([\w.]+) (?:type ([A-Z]+) )?(?:class ([A-Z]+) )?' +
       '(?:to ([^:]+)(?::([0-9]+))? )?should have rcode ([\w.]+)')
 def query(step, query_name, qtype, qclass, addr, port, rcode):
+    """
+    Run a query, check the rcode of the response, and store the query
+    result in world.last_query_result.
+    Parameters:
+    query_name ('query for <name>'): The domain name to query.
+    qtype ('type <type>', optional): The RR type to query. Defaults to A.
+    qclass ('class <class>', optional): The RR class to query. Defaults to IN.
+    addr ('to <address>', optional): The IP address of the nameserver to query.
+                           Defaults to 127.0.0.1.
+    port (':<port>', optional): The port number of the nameserver to query.
+                      Defaults to 47806.
+    rcode ('should have rcode <rcode>'): The expected rcode of the answer.
+    """
     if qtype is None:
         qtype = "A"
     if qclass is None:
@@ -138,6 +210,15 @@ def query(step, query_name, qtype, qclass, addr, port, rcode):
 
 @step('The SOA serial for ([\w.]+) should be ([0-9]+)')
 def query_soa(step, query_name, serial):
+    """
+    Convenience function to check the SOA SERIAL value of the given zone at
+    the nameserver at the default address (127.0.0.1:47806).
+    Parameters:
+    query_name ('for <name>'): The zone to find the SOA record for.
+    serial ('should be <number>'): The expected value of the SOA SERIAL.
+    If the rcode is not NOERROR, or the answer section does not contain the
+    SOA record, this step fails.
+    """
     query_result = QueryResult(query_name, "SOA", "IN", "127.0.0.1", "47806")
     assert "NOERROR" == query_result.rcode,\
         "Got " + query_result.rcode + ", expected NOERROR"
@@ -149,6 +230,16 @@ def query_soa(step, query_name, serial):
 
 @step('last query response should have (\S+) (.+)')
 def check_last_query(step, item, value):
+    """
+    Check a specific value in the reponse from the last successful query sent.
+    Parameters:
+    item: The item to check the value of
+    value: The expected value.
+    This performs a very simple direct string comparison of the QueryResult
+    member with the given item name and the given value.
+    Fails if the item is unknown, or if its value does not match the expected
+    value.
+    """
     assert world.last_query_result is not None
     assert item in world.last_query_result.__dict__
     lq_val = world.last_query_result.__dict__[item]
@@ -157,6 +248,17 @@ def check_last_query(step, item, value):
 
 @step('([a-zA-Z]+) section of the last query response should be')
 def check_last_query_section(step, section):
+    """
+    Check the entire contents of the given section of the response of the last
+    query.
+    Parameters:
+    section ('<section> section'): The name of the section (QUESTION, ANSWER,
+                                   AUTHORITY or ADDITIONAL).
+    The expected response is taken from the multiline part of the step in the
+    scenario. Differing whitespace is ignored, but currently the order is
+    significant.
+    Fails if they do not match.
+    """
     response_string = None
     if section.lower() == 'question':
         response_string = "\n".join(world.last_query_result.question_section)

tests/lettuce/features/terrain/steps.py

@@ -1,3 +1,18 @@
+# Copyright (C) 2011  Internet Systems Consortium.
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND INTERNET SYSTEMS CONSORTIUM
+# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
+# INTERNET SYSTEMS CONSORTIUM BE LIABLE FOR ANY SPECIAL, DIRECT,
+# INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
+# FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+# WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
 #
 # This file contains a number of common steps that are general and may be used
 # By a lot of feature files.
@@ -8,18 +23,50 @@ import os
 
 @step('stop process (\w+)')
 def stop_a_named_process(step, process_name):
+    """
+    Stop the process with the given name.
+    Parameters:
+    process_name ('process <name>'): Name of the process to stop.
+    """
     world.processes.stop_process(process_name)
 
 @step('wait for (new )?(\w+) stderr message (\w+)')
 def wait_for_message(step, new, process_name, message):
+    """
+    Block until the given message is printed to the given process's stderr
+    output.
+    Parameter:
+    new: (' new', optional): Only check the output printed since last time
+                             this step was used for this process.
+    process_name ('<name> stderr'): Name of the process to check the output of.
+    message ('message <message>'): Output (part) to wait for.
+    Fails if the message is not found after 10 seconds.
+    """
     world.processes.wait_for_stderr_str(process_name, [message], new)
 
 @step('wait for (new )?(\w+) stdout message (\w+)')
 def wait_for_message(step, process_name, message):
+    """
+    Block until the given message is printed to the given process's stdout
+    output.
+    Parameter:
+    new: (' new', optional): Only check the output printed since last time
+                             this step was used for this process.
+    process_name ('<name> stderr'): Name of the process to check the output of.
+    message ('message <message>'): Output (part) to wait for.
+    Fails if the message is not found after 10 seconds.
+    """
     world.processes.wait_for_stdout_str(process_name, [message], new)
 
 @step('the file (\S+) should (not )?exist')
 def check_existence(step, file_name, should_not_exist):
+    """
+    Check the existence of the given file.
+    Parameters:
+    file_name ('file <name>'): File to check existence of.
+    should_not_exist ('not', optional): Whether it should or should not exist.
+    Fails if the file should exist and does not, or vice versa.
+    """
     if should_not_exist is None:
         assert os.path.exists(file_name), file_name + " does not exist"
     else:

tests/lettuce/features/terrain/terrain.py

@@ -1,3 +1,18 @@
+# Copyright (C) 2011  Internet Systems Consortium.
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND INTERNET SYSTEMS CONSORTIUM
+# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
+# INTERNET SYSTEMS CONSORTIUM BE LIABLE FOR ANY SPECIAL, DIRECT,
+# INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING
+# FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+# NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+# WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
 #
 # This is the 'terrain' in which the lettuce lives. By convention, this is
 # where global setup and teardown is defined.
@@ -7,6 +22,7 @@
 #
 # We also use it to provide scenario invariants, such as resetting data.
 #
+
 from lettuce import *
 import subprocess
 import os.path
@@ -46,6 +62,15 @@ OUTPUT_WAIT_MAX_INTERVALS = 20
 class RunningProcess:
     def __init__(self, step, process_name, args):
         # set it to none first so destructor won't error if initializer did
+        """
+        Initialize the long-running process structure, and start the process.
+        Parameters:
+        step: The scenario step it was called from. This is used for
+              determining the output files for redirection of stdout
+              and stderr.
+        process_name: The name to refer to this running process later.
+        args: Array of arguments to pass to Popen().
+        """
         self.process = None
         self.step = step
         self.process_name = process_name
@@ -55,6 +80,12 @@ class RunningProcess:
         self._start_process(args)
 
     def _start_process(self, args):
+        """
+        Start the process.
+        Parameters:
+        args:
+        Array of arguments to pass to Popen().
+        """
         stderr_write = open(self.stderr_filename, "w")
         stdout_write = open(self.stdout_filename, "w")
         self.process = subprocess.Popen(args, 1, None, subprocess.PIPE,
@@ -64,6 +95,16 @@ class RunningProcess:
         self.stdout = open(self.stdout_filename, "r")
 
     def mangle_filename(self, filebase, extension):
+        """
+        Remove whitespace and non-default characters from a base string,
+        and return the substituted value. Whitespace is replaced by an
+        underscore. Any other character that is not an ASCII letter, a
+        number, a dot, or a hyphen or underscore is removed.
+        Parameter:
+        filebase: The string to perform the substitution and removal on
+        extension: An extension to append to the result value
+        Returns the modified filebase with the given extension
+        """
         filebase = re.sub("\s+", "_", filebase)
         filebase = re.sub("[^a-zA-Z0-9.\-_]", "", filebase)
         return filebase + "." + extension
@@ -73,6 +114,12 @@ class RunningProcess:
         # through an environment variable. Since we currently expect
         # lettuce to be run from our lettuce dir, we shall just use
         # the relative path 'output/'
+        """
+        Make sure the output directory for stdout/stderr redirection
+        exists.
+        Fails if it exists but is not a directory, or if it does not
+        and we are unable to create it.
+        """
         self._output_dir = os.getcwd() + os.sep + "output"
         if not os.path.exists(self._output_dir):
             os.mkdir(self._output_dir)
@@ -80,6 +127,11 @@ class RunningProcess:
             self._output_dir + " is not a directory."
 
     def _create_filenames(self):
+        """
+        Derive the filenames for stdout/stderr redirection from the
+        feature, scenario, and process name. The base will be
+        "<Feature>-<Scenario>-<process name>.[stdout|stderr]"
+        """
         filebase = self.step.scenario.feature.name + "-" +\
                    self.step.scenario.name + "-" + self.process_name
         self.stderr_filename = self._output_dir + os.sep +\
@@ -88,6 +140,11 @@ class RunningProcess:
                                self.mangle_filename(filebase, "stdout")
 
     def stop_process(self):
+        """
+        Stop this process by calling terminate(). Blocks until process has
+        exited. If remove_files_on_exit is True, redirected output files
+        are removed.
+        """
         if self.process is not None:
             self.process.terminate()
             self.process.wait()
@@ -96,10 +153,30 @@ class RunningProcess:
             self._remove_files()
 
     def _remove_files(self):
+        """
+        Remove the files created for redirection of stdout/stderr output.
+        """
         os.remove(self.stderr_filename)
         os.remove(self.stdout_filename)
 
     def _wait_for_output_str(self, filename, running_file, strings, only_new):
+        """
+        Wait for a line of output in this process. This will (if only_new is
+        False) first check all previous output from the process, and if not
+        found, check all output since the last time this method was called.
+        For each line in the output, the given strings array is checked. If
+        any output lines checked contains one of the strings in the strings
+        array, that string (not the line!) is returned.
+        Parameters:
+        filename: The filename to read previous output from, if applicable.
+        running_file: The open file to read new output from.
+        strings: Array of strings to look for.
+        only_new: If true, only check output since last time this method was
+                  called. If false, first check earlier output.
+        Returns the matched string.
+        Fails if none of the strings was read after 10 seconds
+        (OUTPUT_WAIT_INTERVAL * OUTPUT_WAIT_MAX_INTERVALS).
+        """
         if not only_new:
             full_file = open(filename, "r")
             for line in full_file:
@@ -122,10 +199,30 @@ class RunningProcess:
         assert False, "Timeout waiting for process output: " + str(strings)
 
     def wait_for_stderr_str(self, strings, only_new = True):
+        """
+        Wait for one of the given strings in this processes stderr output.
+        Parameters:
+        strings: Array of strings to look for.
+        only_new: If true, only check output since last time this method was
+                  called. If false, first check earlier output.
+        Returns the matched string.
+        Fails if none of the strings was read after 10 seconds
+        (OUTPUT_WAIT_INTERVAL * OUTPUT_WAIT_MAX_INTERVALS).
+        """
         return self._wait_for_output_str(self.stderr_filename, self.stderr,
                                          strings, only_new)
 
     def wait_for_stdout_str(self, strings, only_new = True):
+        """
+        Wait for one of the given strings in this processes stdout output.
+        Parameters:
+        strings: Array of strings to look for.
+        only_new: If true, only check output since last time this method was
+                  called. If false, first check earlier output.
+        Returns the matched string.
+        Fails if none of the strings was read after 10 seconds
+        (OUTPUT_WAIT_INTERVAL * OUTPUT_WAIT_MAX_INTERVALS).
+        """
         return self._wait_for_output_str(self.stdout_filename, self.stdout,
                                          strings, only_new)
 
@@ -134,51 +231,96 @@ class RunningProcess:
 # one-shot programs like dig or bindctl are started and closed separately
 class RunningProcesses:
     def __init__(self):
+        """
+        Initialize with no running processes.
+        """
         self.processes = {}
     
     def add_process(self, step, process_name, args):
+        """
+        Start a process with the given arguments, and store it under the given
+        name.
+        Parameters:
+        step: The scenario step it was called from. This is used for
+              determining the output files for redirection of stdout
+              and stderr.
+        process_name: The name to refer to this running process later.
+        args: Array of arguments to pass to Popen().
+        Fails if a process with the given name is already running.
+        """
         assert process_name not in self.processes,\
             "Process " + name + " already running"
         self.processes[process_name] = RunningProcess(step, process_name, args)
 
     def get_process(self, process_name):
+        """
+        Return the Process with the given process name.
+        Parameters:
+        process_name: The name of the process to return.
+        Fails if the process is not running.
+        """
         assert process_name in self.processes,\
             "Process " + name + " unknown"
         return self.processes[process_name]
 
     def stop_process(self, process_name):
+        """
+        Stop the Process with the given process name.
+        Parameters:
+        process_name: The name of the process to return.
+        Fails if the process is not running.
+        """
         assert process_name in self.processes,\
             "Process " + name + " unknown"
         self.processes[process_name].stop_process()
         del self.processes[process_name]
         
     def stop_all_processes(self):
+        """
+        Stop all running processes.
+        """
         for process in self.processes.values():
             process.stop_process()
     
     def keep_files(self):
+        """
+        Keep the redirection files for stdout/stderr output of all processes
+        instead of removing them when they are stopped later.
+        """
         for process in self.processes.values():
             process.remove_files_on_exit = False
 
     def wait_for_stderr_str(self, process_name, strings, only_new = True):
-        """Wait for any of the given strings in the given processes stderr 
+        """
-        output. If only_new is True, it will only look at the lines that are 
+        Wait for one of the given strings in the given processes stderr output.
-        printed to stderr since the last time this method was called. If 
+        Parameters:
-        False, it will also look at the previously printed lines. This will 
+        process_name: The name of the process to check the stderr output of.
-        block until one of the strings is found. TODO: we may want to put in 
+        strings: Array of strings to look for.
-        a timeout for this... Returns the string that is found"""
+        only_new: If true, only check output since last time this method was
+                  called. If false, first check earlier output.
+        Returns the matched string.
+        Fails if none of the strings was read after 10 seconds
+        (OUTPUT_WAIT_INTERVAL * OUTPUT_WAIT_MAX_INTERVALS).
+        Fails if the process is unknown.
+        """
         assert process_name in self.processes,\
            "Process " + process_name + " unknown"
         return self.processes[process_name].wait_for_stderr_str(strings,
                                                                 only_new)
 
     def wait_for_stdout_str(self, process_name, strings, only_new = True):
-        """Wait for any of the given strings in the given processes stderr 
+        """
-        output. If only_new is True, it will only look at the lines that are 
+        Wait for one of the given strings in the given processes stdout output.
-        printed to stderr since the last time this method was called. If 
+        Parameters:
-        False, it will also look at the previously printed lines. This will 
+        process_name: The name of the process to check the stdout output of.
-        block until one of the strings is found. TODO: we may want to put in 
+        strings: Array of strings to look for.
-        a timeout for this... Returns the string that is found"""
+        only_new: If true, only check output since last time this method was
+                  called. If false, first check earlier output.
+        Returns the matched string.
+        Fails if none of the strings was read after 10 seconds
+        (OUTPUT_WAIT_INTERVAL * OUTPUT_WAIT_MAX_INTERVALS).
+        Fails if the process is unknown.
+        """
         assert process_name in self.processes,\
            "Process " + process_name + " unknown"
         return self.processes[process_name].wait_for_stdout_str(strings,
@@ -186,6 +328,9 @@ class RunningProcesses:
 
 @before.each_scenario
 def initialize(scenario):
+    """
+    Global initialization for each scenario.
+    """
     # Keep track of running processes
     world.processes = RunningProcesses()
 
@@ -204,6 +349,9 @@ def initialize(scenario):
 
 @after.each_scenario
 def cleanup(scenario):
+    """
+    Global cleanup for each scenario.
+    """
     # Keep output files if the scenario failed
     if not scenario.passed:
         world.processes.keep_files()