Merge pull request #2016 from anarkiwi/master

Add lint checking of test directory.

Author: anarkiwi

clib/clib_mininet_test_unit.py

@@ -9,8 +9,6 @@
 from mininet.net import Mininet
 
 import mininet_test_base
-import mininet_test_util
-import mininet_test_topo
 
 from tcpdump_helper import TcpdumpHelper
 from docker_host import MakeDockerHost

clib/docker_host.py

@@ -13,25 +13,6 @@
 
 from mininet_test_util import DEVNULL
 
-def MakeDockerHost(image, prefix='mininet', startup_timeout_ms=None):
-
-    class ImageHost(DockerHost):
-
-        def __init__(self, *args, **kwargs):
-            host_name = args[0]
-            kwargs['image'] = image
-            assert kwargs['tmpdir'], 'tmpdir required for docker host'
-            kwargs['tmpdir'] = os.path.join(kwargs['tmpdir'], host_name)
-            kwargs['prefix'] = prefix
-            if startup_timeout_ms:
-                kwargs['startup_timeout_ms'] = startup_timeout_ms
-            elif 'DOCKER_STARTUP_TIMEOUT_MS' in os.environ:
-                env_val = os.environ['DOCKER_STARTUP_TIMEOUT_MS']
-                if env_val:
-                    kwargs['startup_timeout_ms'] = int(env_val)
-            DockerHost.__init__(self, *args, **kwargs)
-    return ImageHost
-
 
 class DockerHost(Host):
 
@@ -63,12 +44,16 @@ class DockerHost(Host):
     pollIn = None
     active_log = None
 
-    def __init__(self, name, image=None, tmpdir=None, prefix=None, env_vars=[],
-                 vol_maps=[], startup_timeout_ms=STARTUP_TIMEOUT_MS, **kwargs):
+    def __init__(self, name, image=None, tmpdir=None, prefix=None, env_vars=None,
+                 vol_maps=None, startup_timeout_ms=STARTUP_TIMEOUT_MS, **kwargs):
         self.image = image
         self.tmpdir = tmpdir
         self.prefix = prefix
+        if env_vars is None:
+            env_vars = []
         self.env_vars = env_vars
+        if vol_maps is None:
+            vol_maps = []
         self.vol_maps = vol_maps
         self.startup_timeout_ms = startup_timeout_ms
         Host.__init__(self, name, **kwargs)
@@ -131,6 +116,7 @@ def startShell(self):
         self.cmd('unset HISTFILE; stty -echo; set +m') # pylint: disable=no-member
 
     def kill(self, purge=False):
+        """Kill a container."""
         debug('killing container %s.' % self.container)
         if purge:
             kill_cmd = ["docker", "rm", "-f", self.container]
@@ -146,6 +132,7 @@ def kill(self, purge=False):
             raise
 
     def inspect_pid(self):
+        """Return container PID."""
         try:
             pid_cmd = ["docker", "inspect", "--format={{ .State.Pid }}", self.container]
             pid_pipe = self._popen(pid_cmd, stdin=DEVNULL, stdout=PIPE, stderr=STDOUT)
@@ -158,9 +145,11 @@ def inspect_pid(self):
             raise
 
     def open_log(self, log_name='activate.log'):
+        """Open a log file for writing and return it."""
         return open(os.path.join(self.tmpdir, log_name), 'w')
 
     def activate(self, log_name='activate.log'):
+        """Active a container and return STDOUT to it."""
         assert not self.active_pipe, 'container %s already activated' % self.container
         debug('activating container %s.' % self.container)
         inspect_cmd = ["docker", "inspect", "--format={{json .Config}}", self.image]
@@ -194,6 +183,7 @@ def activate(self, log_name='activate.log'):
         return self.active_pipe
 
     def wait(self):
+        """Wait for an activated container to terminate."""
         try:
             if self.active_pipe_returncode != None:
                 return self.active_pipe_returncode
@@ -209,6 +199,7 @@ def wait(self):
             raise
 
     def read(self, maxbytes=1024):
+        """Read from an activated container."""
         poll_results = self.pollIn.poll(self.startup_timeout_ms)
         data_ready = poll_results and (poll_results[0][1] & select.POLLIN)
         assert data_ready, (
@@ -272,3 +263,23 @@ def _popen(self, cmd, **params):
             out_fd = pipe.stdout.fileno() if stdout else None
             debug('docker pid %d: %s, fd %s' % (pipe.pid, cmd, out_fd))
         return pipe
+
+
+def MakeDockerHost(image, prefix='mininet', startup_timeout_ms=None):
+
+    class ImageHost(DockerHost):
+
+        def __init__(self, *args, **kwargs):
+            host_name = args[0]
+            kwargs['image'] = image
+            assert kwargs['tmpdir'], 'tmpdir required for docker host'
+            kwargs['tmpdir'] = os.path.join(kwargs['tmpdir'], host_name)
+            kwargs['prefix'] = prefix
+            if startup_timeout_ms:
+                kwargs['startup_timeout_ms'] = startup_timeout_ms
+            elif 'DOCKER_STARTUP_TIMEOUT_MS' in os.environ:
+                env_val = os.environ['DOCKER_STARTUP_TIMEOUT_MS']
+                if env_val:
+                    kwargs['startup_timeout_ms'] = int(env_val)
+            DockerHost.__init__(self, *args, **kwargs)
+    return ImageHost

clib/tcpdump_helper.py

@@ -10,7 +10,8 @@
 from mininet.log import error, debug
 
 
-class TcpdumpHelper():
+class TcpdumpHelper(object):
+    """Run tcpdump on interface, then a list of functions, and return tcpdump's parsed output."""
 
     pipe = None
     started = False
@@ -30,8 +31,11 @@ def __init__(self, tcpdump_host, tcpdump_filter, funcs=None,
         tcpdump_flags = vflags
         tcpdump_flags += ' -c %u' % packets if packets else ''
         tcpdump_flags += ' -w %s' % pcap_out if pcap_out else ''
-        tcpdump_cmd = 'tcpdump -i %s %s -e -n -U %s' % (self.intf_name, tcpdump_flags, tcpdump_filter)
-        pipe_cmd = mininet_test_util.timeout_soft_cmd(tcpdump_cmd, timeout) if timeout else tcpdump_cmd
+        tcpdump_cmd = 'tcpdump -i %s %s -e -n -U %s' % (
+            self.intf_name, tcpdump_flags, tcpdump_filter)
+        pipe_cmd = tcpdump_cmd
+        if timeout:
+            pipe_cmd = mininet_test_util.timeout_soft_cmd(tcpdump_cmd, timeout)
 
         debug(pipe_cmd)
         self.pipe = tcpdump_host.popen(
@@ -47,21 +51,25 @@ def __init__(self, tcpdump_host, tcpdump_filter, funcs=None,
         self.set_blocking(blocking)
 
     def stream(self):
-        return self.pipe.stdout if self.pipe else None
+        """Return pipe's STDOUT, or None."""
+        if self.pipe:
+            return self.pipe.stdout
+        return None
 
     def set_blocking(self, blocking=True):
-        fd = self.pipe.stdout.fileno()
-        flags = fcntl.fcntl(fd, fcntl.F_GETFL)
+        """Set blocking on pipe's STDOUT."""
+        stdout_fd = self.pipe.stdout.fileno()
+        flags = fcntl.fcntl(stdout_fd, fcntl.F_GETFL)
         self.blocking = blocking
         if blocking:
             flags = flags & ~os.O_NONBLOCK
         else:
             flags = flags | os.O_NONBLOCK
-        fcntl.fcntl(fd, fcntl.F_SETFL, flags)
+        fcntl.fcntl(stdout_fd, fcntl.F_SETFL, flags)
 
     def execute(self):
+        """Run the helper and accumulate tcpdump output."""
         tcpdump_txt = ''
-        # Initialize with meaningless truthy value.
         line = ' '
         while line:
             tcpdump_txt += line.strip()
@@ -70,6 +78,7 @@ def execute(self):
         return tcpdump_txt
 
     def terminate(self):
+        """Terminate the helper."""
         if not self.pipe:
             return -1
 
@@ -109,6 +118,7 @@ def readline(self):
         return line
 
     def next_line(self):
+        """Retrieve next line from helper."""
         while True:
             try:
                 line = self.readline()

git-hook/pre-commit

@@ -14,7 +14,7 @@ BASE=`dirname $0`/../..
 cd $BASE || exit 1
 BASE=`pwd`
 for mypy_file in faucet/faucet.py faucet/gauge.py ; do
-    PYTHONPATH=$BASE mypy --ignore-missing-imports $mypy_file || exit 1
+    PYTHONPATH=$BASE:$BASE/clib mypy --ignore-missing-imports $mypy_file || exit 1
 done
 cd tests
 ./test_coverage.sh || exit 1

tests/test_config_gauge.py

@@ -46,10 +46,10 @@ def tearDown(self):
         shutil.rmtree(self.tmpdir)
 
     def conf_file_name(self, faucet=False):
+        """Return path for configuration file."""
         if faucet:
             return os.path.join(self.tmpdir, 'faucet.yaml')
-        else:
-            return os.path.join(self.tmpdir, 'gauge.yaml')
+        return os.path.join(self.tmpdir, 'gauge.yaml')
 
     def create_config_files(self, config, faucet_config=None):
         """Returns file path to file containing the config parameter."""
@@ -65,10 +65,11 @@ def create_config_files(self, config, faucet_config=None):
         return (gauge_file_name, faucet_file_name)
 
     def get_config(self, conf_suffix):
+        """Return config file together with header template."""
         return self.GAUGE_CONFIG_HEADER + conf_suffix
 
-
     def test_all_dps(self):
+        """Test config applies for all DPs."""
         GAUGE_CONF = """
 watchers:
     port_stats_poller:
@@ -81,7 +82,7 @@ def test_all_dps(self):
         type: 'prometheus'
 """
         conf = self.get_config(GAUGE_CONF)
-        gauge_file, faucet_file = self.create_config_files(conf)
+        gauge_file, _ = self.create_config_files(conf)
         watcher_confs = cp.watcher_parser(gauge_file, 'gauge_config_test', None)
         self.assertEqual(len(watcher_confs), 2, 'failed to create config for each dp')
         for watcher_conf in watcher_confs:
@@ -91,6 +92,7 @@ def test_all_dps(self):
             self.assertEqual(watcher_conf.db_type, 'prometheus', msg)
 
     def test_no_faucet_config_file(self):
+        """Test missing FAUCET config."""
         GAUGE_CONF = """
 faucet:
     dps:
@@ -111,7 +113,7 @@ def test_no_faucet_config_file(self):
     prometheus:
         type: 'prometheus'
 """
-        gauge_file, faucet_file = self.create_config_files(GAUGE_CONF, '')
+        gauge_file, _ = self.create_config_files(GAUGE_CONF, '')
         watcher_conf = cp.watcher_parser(
             gauge_file, 'gauge_config_test', None)[0]
         msg = 'failed to create watcher correctly when dps configured in gauge.yaml'

tests/test_main.py

@@ -34,6 +34,7 @@ def test_parse_args(self):
         self.assertTrue(parse_args(['--verbose']).verbose)
 
     def test_build_ryu_args(self):
+        """Test build_ryu_args()."""
         self.assertTrue(build_ryu_args(['faucet', '--use-stderr', '--use-syslog', '--verbose']))
         self.assertTrue(build_ryu_args(['gauge', '--use-stderr', '--use-syslog', '--verbose']))
         self.assertFalse(build_ryu_args(['faucet', '--version']))

tests/test_min_pylint.sh

@@ -1,16 +1,20 @@
 #!/bin/bash
 
 MINRATING="9.2"
-SRC_FILES=`dirname $0`"/../faucet/[a-z]*.py"
+FAUCETHOME=`dirname $0`"/.."
+PYTHONPATH=$FAUCETHOME:$FAUCETHOME/clib
 
+for src_dir in clib faucet tests ; do
+    src_files=$FAUCETHOME/$src_dir/[a-z]*.py
 
-for f in $SRC_FILES ; do
-  RATING=`pylint $f | grep -ohE "rated at [0-9\.]+" | sed "s/rated at //g"`
-  echo pylint $f: $RATING
-  if [ $(bc <<< "$RATING < $MINRATING") -eq 1 ] ; then
-      echo "$RATING below min ($MINRATING)"
-      exit 1
-  fi
+    for f in $src_files ; do
+        rating=`PYTHONPATH=$PYTHONPATH pylint --rcfile=/dev/null $f | grep -ohE "rated at [0-9\.]+" | sed "s/rated at //g"`
+        echo pylint $f: $rating
+        if [ $(bc <<< "$rating < $MINRATING") -eq 1 ] ; then
+            echo "$rating below min ($MINRATING)"
+            exit 1
+        fi
+   done
 done
 
 exit 0

tests/test_valve.py

@@ -304,6 +304,7 @@ def serialize(layers):
 
 
 class ValveTestBases:
+    """Insulate test base classes from unittest so we can reuse base clases."""
 
 
     class ValveTestSmall(unittest.TestCase):
@@ -480,6 +481,7 @@ def learn_hosts(self):
                     'vid': 0x200})
 
         def verify_expiry(self):
+            """Verify FIB resolution attempts expire."""
             now = time.time()
             for _ in range(self.valve.dp.max_host_fib_retry_count + 1):
                 now += (self.valve.dp.timeout * 2)
@@ -532,8 +534,9 @@ def _verify_flood_to_port(match, port, valve_vlan, port_number=None):
                         if port == in_port:
                             self.assertNotEqual(
                                 combinatorial_port_flood, output,
-                                msg='flooding to in_port (%s) not compatible with flood mode (%s)' % (
-                                    output, combinatorial_port_flood))
+                                msg=('flooding to in_port (%s) not '
+                                     'compatible with flood mode (%s)') % (
+                                         output, combinatorial_port_flood))
                             continue
                         self.assertTrue(
                             output,
@@ -579,8 +582,11 @@ def rcv_packet(self, port, vid, match):
                 'of_packet_ins')
             rcv_packet_ofmsgs = self.last_flows_to_dp[self.DP_ID]
             self.table.apply_ofmsgs(rcv_packet_ofmsgs)
-            for valve_service in ('resolve_gateways', 'advertise', 'send_lldp_beacons', 'state_expire'):
-                ofmsgs = self.valves_manager.valve_flow_services(now, valve_service)
+            for valve_service in (
+                    'resolve_gateways', 'advertise',
+                    'send_lldp_beacons', 'state_expire'):
+                ofmsgs = self.valves_manager.valve_flow_services(
+                    now, valve_service)
                 if ofmsgs:
                     self.table.apply_ofmsgs(ofmsgs)
             self.valves_manager.update_metrics(now)
@@ -624,9 +630,13 @@ def test_oferror(self):
 
         def test_switch_features(self):
             """Test switch features handler."""
-            self.assertTrue(isinstance(self.valve, TfmValve), msg=type(self.valve))
+            self.assertTrue(
+                isinstance(self.valve, TfmValve),
+                msg=type(self.valve))
             features_flows = self.valve.switch_features(None)
-            tfm_flows = [flow for flow in features_flows if isinstance(flow, valve_of.parser.OFPTableFeaturesStatsRequest)]
+            tfm_flows = [
+                flow for flow in features_flows if isinstance(
+                    flow, valve_of.parser.OFPTableFeaturesStatsRequest)]
             # TODO: verify TFM content.
             self.assertTrue(tfm_flows)
 
@@ -1255,6 +1265,7 @@ def test_ofdescstats_handler(self):
 
 
 class ValveTestCase(ValveTestBases.ValveTestBig):
+    """Run complete set of basic tests."""
 
     pass
 
@@ -1391,6 +1402,7 @@ def setUp(self):
         self.setup_valve(CONFIG)
 
     def test_stack_learn(self):
+        """Test host learning on stack root."""
         self.rcv_packet(1, 0x300, {
             'eth_src': self.P1_V300_MAC,
             'eth_dst': self.UNKNOWN_MAC,
@@ -1424,6 +1436,7 @@ def setUp(self):
         self.setup_valve(CONFIG)
 
     def test_stack_learn(self):
+        """Test host learning on non-root switch."""
         self.rcv_packet(1, 0x300, {
             'eth_src': self.P1_V300_MAC,
             'eth_dst': self.UNKNOWN_MAC,
@@ -1665,6 +1678,7 @@ def setUp(self):
         self.setup_valve(self.CONFIG)
 
     def test_lacp(self):
+        """Test LACP comes up."""
         # TODO: verify LACP state
         self.rcv_packet(1, 0, {
             'actor_system': '0e:00:00:00:00:02',
@@ -1768,7 +1782,8 @@ def setUp(self):
 class RyuAppSmokeTest(unittest.TestCase):
     """Test bare instantiation of controller classes."""
 
-    def _fake_dp(self):
+    @staticmethod
+    def _fake_dp():
         datapath = namedtuple('datapath', 'id')
         datapath.id = 0
         datapath.close = lambda: None