Merge lp:~jaypipes/nova/pylint into lp:~hudson-openstack/nova/trunk

[Jay Pipes]

pylint fixes for /nova/test.py

pylint fixes for /nova/test.py

[Eric Day (eday)]

Looks good! Just one thing... the disables at the top of the file. I would rather see them localized for a specific line (ie, one method name that can't change due to base names or something), or a global pylintrc rule. Can you give more info on the two disable msgs at the top?

review: Needs Fixing

[Jay Pipes (jaypipes)]

> Looks good! Just one thing... the disables at the top of the file. I would
> rather see them localized for a specific line (ie, one method name that can't
> change due to base names or something), or a global pylintrc rule. Can you
> give more info on the two disable msgs at the top?

Sure. :)

The C0103 is for non-conventional variable names. python's (and twisted's Trials) unittest module uses methods names that don't jive with our standards. Namely, they use camelCasing instead of underscore_lowered names because of its genesis from JUnit.

The W0511 is generated every time a TODO(xxx) is placed in code comments. We have these all over the place, and I suggest putting it in the global pylintrc file.

Thoughts?

-jay

[Eric Day (eday)]

For the camelCase methods, I was just disabling them on the line they appear on to make sure other cases get caught/fixed:

    def setUp(self): # pylint: disable-msg=C0103

I was leaving all the TODO() messages in there for now since those are valid that pylint can report for us. We can always disable those globally in pylintrc as you mention if we decide pylint isn't the best place to report them.

[Jay Pipes (jaypipes)]

> For the camelCase methods, I was just disabling them on the line they appear
> on to make sure other cases get caught/fixed:
>
> def setUp(self): # pylint: disable-msg=C0103

Done

> I was leaving all the TODO() messages in there for now since those are valid
> that pylint can report for us. We can always disable those globally in
> pylintrc as you mention if we decide pylint isn't the best place to report
> them.

I'd prefer to disable them and then to run a report, just do --enable-msg=R0511 on the command line.

-jay

[Eric Day (eday)]

Looks good! I like the idea of running reports separately as needed for TODOs. I've been changing the one-char variable names to conform to be more verbose, but I guess there are cases where we want to allow this. It should be avoided though for clarity.

review: Approve

=== modified file 'nova/server.py'
--- nova/server.py	2010-07-15 23:13:48 +0000
+++ nova/server.py	2010-08-09 16:39:45 +0000
@@ -52,13 +52,8 @@
     """
     # Get the pid from the pidfile
     try:
-        pf = file(pidfile,'r')
-        pid = int(pf.read().strip())
-        pf.close()
+        pid = int(open(pidfile,'r').read().strip())
     except IOError:
-        pid = None
-
-    if not pid:
         message = "pidfile %s does not exist. Daemon not running?\n"
         sys.stderr.write(message % pidfile)
         return # not an error in a restart
@@ -79,14 +74,15 @@
 
 
 def serve(name, main):
+    """Controller for server"""
     argv = FLAGS(sys.argv)
 
     if not FLAGS.pidfile:
         FLAGS.pidfile = '%s.pid' % name
 
-    logging.debug("Full set of FLAGS: \n\n\n" )
+    logging.debug("Full set of FLAGS: \n\n\n")
     for flag in FLAGS:
-        logging.debug("%s : %s" % (flag, FLAGS.get(flag, None) ))
+        logging.debug("%s : %s", flag, FLAGS.get(flag, None))
 
     action = 'start'
     if len(argv) > 1:
@@ -102,7 +98,11 @@
     else:
         print 'usage: %s [options] [start|stop|restart]' % argv[0]
         sys.exit(1)
-
+    daemonize(argv, name, main)
+
+
+def daemonize(args, name, main):
+    """Does the work of daemonizing the process"""
     logging.getLogger('amqplib').setLevel(logging.WARN)
     if FLAGS.daemonize:
         logger = logging.getLogger()
@@ -115,7 +115,7 @@
         else:
             if not FLAGS.logfile:
                 FLAGS.logfile = '%s.log' % name
-            logfile = logging.handlers.FileHandler(FLAGS.logfile)
+            logfile = logging.FileHandler(FLAGS.logfile)
             logfile.setFormatter(formatter)
             logger.addHandler(logfile)
         stdin, stdout, stderr = None, None, None
@@ -137,4 +137,4 @@
             stdout=stdout,
             stderr=stderr
             ):
-        main(argv)
+        main(args)

=== modified file 'nova/test.py'
--- nova/test.py	2010-07-23 22:27:18 +0000
+++ nova/test.py	2010-08-09 16:39:45 +0000
@@ -22,15 +22,14 @@
 and some black magic for inline callbacks.
 """
 
-import logging
 import mox
 import stubout
+import sys
 import time
-import unittest
+
 from tornado import ioloop
 from twisted.internet import defer
-from twisted.python import failure
-from twisted.trial import unittest as trial_unittest
+from twisted.trial import unittest
 
 from nova import fakerabbit
 from nova import flags
@@ -41,20 +40,21 @@
                   'should we use everything for testing')
 
 
-def skip_if_fake(f):
+def skip_if_fake(func):
+    """Decorator that skips a test if running in fake mode"""
     def _skipper(*args, **kw):
+        """Wrapped skipper function"""
         if FLAGS.fake_tests:
-            raise trial_unittest.SkipTest('Test cannot be run in fake mode')
+            raise unittest.SkipTest('Test cannot be run in fake mode')
         else:
-            return f(*args, **kw)
-
-    _skipper.func_name = f.func_name
+            return func(*args, **kw)
     return _skipper
 
 
-class TrialTestCase(trial_unittest.TestCase):
-
-    def setUp(self):
+class TrialTestCase(unittest.TestCase):
+    """Test case base class for all unit tests"""
+    def setUp(self): # pylint: disable-msg=C0103
+        """Run before each test method to initialize test environment"""
         super(TrialTestCase, self).setUp()
 
         # emulate some of the mox stuff, we can't use the metaclass
@@ -63,7 +63,8 @@
         self.stubs = stubout.StubOutForTesting()
         self.flag_overrides = {}
 
-    def tearDown(self):
+    def tearDown(self): # pylint: disable-msg=C0103
+        """Runs after each test method to finalize/tear down test environment"""
         super(TrialTestCase, self).tearDown()
         self.reset_flags()
         self.mox.UnsetStubs()
@@ -75,6 +76,7 @@
             fakerabbit.reset_all()
 
     def flags(self, **kw):
+        """Override flag variables for a test"""
         for k, v in kw.iteritems():
             if k in self.flag_overrides:
                 self.reset_flags()
@@ -84,13 +86,17 @@
             setattr(FLAGS, k, v)
 
     def reset_flags(self):
+        """Resets all flag variables for the test.  Runs after each test"""
         for k, v in self.flag_overrides.iteritems():
             setattr(FLAGS, k, v)
 
 
 
 class BaseTestCase(TrialTestCase):
-    def setUp(self):
+    # TODO(jaypipes): Can this be moved into the TrialTestCase class?
+    """Base test case class for all unit tests."""
+    def setUp(self): # pylint: disable-msg=C0103
+        """Run before each test method to initialize test environment"""
         super(BaseTestCase, self).setUp()
         # TODO(termie): we could possibly keep a more global registry of
         #               the injected listeners... this is fine for now though
@@ -98,33 +104,27 @@
         self.ioloop = ioloop.IOLoop.instance()
 
         self._waiting = None
-        self._doneWaiting = False
-        self._timedOut = False
-        self.set_up()
-
-    def set_up(self):
-        pass
-
-    def tear_down(self):
-        pass
-
-    def tearDown(self):
+        self._done_waiting = False
+        self._timed_out = False
+
+    def tearDown(self):# pylint: disable-msg=C0103
+        """Runs after each test method to finalize/tear down test environment"""
         super(BaseTestCase, self).tearDown()
         for x in self.injected:
             x.stop()
         if FLAGS.fake_rabbit:
             fakerabbit.reset_all()
-        self.tear_down()
 
-    def _waitForTest(self, timeout=60):
+    def _wait_for_test(self, timeout=60):
         """ Push the ioloop along to wait for our test to complete. """
         self._waiting = self.ioloop.add_timeout(time.time() + timeout,
                                                 self._timeout)
         def _wait():
-            if self._timedOut:
+            """Wrapped wait function. Called on timeout."""
+            if self._timed_out:
                 self.fail('test timed out')
                 self._done()
-            if self._doneWaiting:
+            if self._done_waiting:
                 self.ioloop.stop()
                 return
             # we can use add_callback here but this uses less cpu when testing
@@ -134,15 +134,18 @@
         self.ioloop.start()
 
     def _done(self):
+        """Callback used for cleaning up deferred test methods."""
         if self._waiting:
             try:
                 self.ioloop.remove_timeout(self._waiting)
-            except Exception:
+            except Exception: # pylint: disable-msg=W0703
+                # TODO(jaypipes): This produces a pylint warning.  Should
+                # we really be catching Exception and then passing here?
                 pass
             self._waiting = None
-        self._doneWaiting = True
+        self._done_waiting = True
 
-    def _maybeInlineCallbacks(self, f):
+    def _maybe_inline_callbacks(self, func):
         """ If we're doing async calls in our tests, wait on them.
 
         This is probably the most complicated hunk of code we have so far.
@@ -165,7 +168,7 @@
         d.addCallback(_describe)
         d.addCallback(_checkDescribe)
         d.addCallback(lambda x: self._done())
-        self._waitForTest()
+        self._wait_for_test()
 
         Example (inline callbacks! yay!):
 
@@ -179,16 +182,17 @@
         # TODO(termie): this can be a wrapper function instead and
         #               and we can make a metaclass so that we don't
         #               have to copy all that "run" code below.
-        g = f()
+        g = func()
         if not hasattr(g, 'send'):
             self._done()
             return defer.succeed(g)
 
-        inlined = defer.inlineCallbacks(f)
+        inlined = defer.inlineCallbacks(func)
         d = inlined()
         return d
 
-    def _catchExceptions(self, result, failure):
+    def _catch_exceptions(self, result, failure):
+        """Catches all exceptions and handles keyboard interrupts."""
         exc = (failure.type, failure.value, failure.getTracebackObject())
         if isinstance(failure.value, self.failureException):
             result.addFailure(self, exc)
@@ -200,44 +204,46 @@
         self._done()
 
     def _timeout(self):
+        """Helper method which trips the timeouts"""
         self._waiting = False
-        self._timedOut = True
+        self._timed_out = True
 
     def run(self, result=None):
-        if result is None: result = self.defaultTestResult()
+        """Runs the test case"""
 
         result.startTest(self)
-        testMethod = getattr(self, self._testMethodName)
+        test_method = getattr(self, self._testMethodName)
         try:
             try:
                 self.setUp()
             except KeyboardInterrupt:
                 raise
             except:
-                result.addError(self, self._exc_info())
+                result.addError(self, sys.exc_info())
                 return
 
             ok = False
             try:
-                d = self._maybeInlineCallbacks(testMethod)
-                d.addErrback(lambda x: self._catchExceptions(result, x))
+                d = self._maybe_inline_callbacks(test_method)
+                d.addErrback(lambda x: self._catch_exceptions(result, x))
                 d.addBoth(lambda x: self._done() and x)
-                self._waitForTest()
+                self._wait_for_test()
                 ok = True
             except self.failureException:
-                result.addFailure(self, self._exc_info())
+                result.addFailure(self, sys.exc_info())
             except KeyboardInterrupt:
                 raise
             except:
-                result.addError(self, self._exc_info())
+                result.addError(self, sys.exc_info())
 
             try:
                 self.tearDown()
             except KeyboardInterrupt:
                 raise
             except:
-                result.addError(self, self._exc_info())
+                result.addError(self, sys.exc_info())
                 ok = False
-            if ok: result.addSuccess(self)
+            if ok:
+                result.addSuccess(self)
         finally:
             result.stopTest(self)

=== modified file 'pylintrc'
--- pylintrc	2010-08-08 02:51:17 +0000
+++ pylintrc	2010-08-09 16:39:45 +0000
@@ -2,8 +2,18 @@
 disable-msg=C0103
 
 [Basic]
+# Variables can be 1 to 31 characters long, with
+# lowercase and underscores
+variable-rgx=[a-z_][a-z0-9_]{0,30}$
+
+# Method names should be at least 3 characters long
+# and be lowecased with underscores
 method-rgx=[a-z_][a-z0-9_]{2,50}$
 
+[MESSAGES CONTROL]
+# TODOs in code comments are fine...
+disable-msg=W0511
+
 [Design]
 max-public-methods=100
 min-public-methods=0