p4poller: Clean up Deferred errbacks, log.err problems, fix tests to …

…ignore

logged errors when testing bad input.

buildbot/changes/p4poller.py

@@ -6,14 +6,18 @@
 import time
 import os
 
-from twisted.python import log, failure
+from twisted.python import log
 from twisted.internet import defer, reactor
 from twisted.internet.utils import getProcessOutput
 from twisted.internet.task import LoopingCall
 
 from buildbot import util
 from buildbot.changes import base, changes
 
+class P4PollerError(Exception):
+    """Something went wrong with the poll. This is used as a distinctive
+    exception type so that unit tests can detect and ignore it."""
+
 def get_simple_split(branchfile):
     """Splits the branchfile argument and assuming branch is 
        the first path component in branchfile, will return
@@ -116,9 +120,10 @@ def _finished_failure(self, res):
         assert self.working
         self.working = False
 
-        # Again, the return value is only for unit testing.
-        # If there's a failure, log it so it isn't lost.
-        log.msg('P4 poll failed: %s' % res)
+        # Again, the return value is only for unit testing. If there's a
+        # failure, log it so it isn't lost. Use log.err to make sure unit
+        # tests flunk if there was a problem.
+        log.err(res, 'P4 poll failed')
         return None
 
     def _get_process_output(self, args):
@@ -148,7 +153,8 @@ def _process_changes(self, result):
             line = line.strip()
             if not line: continue
             m = self.changes_line_re.match(line)
-            assert m, "Unexpected 'p4 changes' output: %r" % result
+            if not m:
+                raise P4PollerError("Unexpected 'p4 changes' output: %r" % result)
             num = int(m.group('num'))
             if last_change is None:
                 log.msg('P4Poller: starting at change %d' % num)
@@ -181,7 +187,8 @@ def _process_describe(self, result, num):
         # field. The rstrip() is intended to remove that.
         lines[0] = lines[0].rstrip()
         m = self.describe_header_re.match(lines[0])
-        assert m, "Unexpected 'p4 describe -s' result: %r" % result
+        if not m:
+            raise P4PollerError("Unexpected 'p4 describe -s' result: %r" % result)
         who = m.group('who')
         when = time.mktime(time.strptime(m.group('when'), self.datefmt))
         comments = ''
@@ -194,7 +201,8 @@ def _process_describe(self, result, num):
             line = lines.pop(0).strip()
             if not line: continue
             m = self.file_re.match(line)
-            assert m, "Invalid file line: %r" % line
+            if not m:
+                raise P4PollerError("Invalid file line: %r" % line)
             path = m.group('path')
             if path.startswith(self.p4base):
                 branch, file = self.split_file(path[len(self.p4base):])

buildbot/test/unit/test_p4poller.py

@@ -4,7 +4,7 @@
 from twisted.trial import unittest
 
 from buildbot.changes.changes import Change
-from buildbot.changes.p4poller import P4Source, get_simple_split
+from buildbot.changes.p4poller import P4Source, get_simple_split, P4PollerError
 
 first_p4changes = \
 """Change 1 on 2006/04/13 by slamb@testclient 'first rev'
@@ -96,14 +96,18 @@ def testCheck(self):
         # The first time, it just learns the change to start at.
         self.assert_(self.t.last_change is None)
         self.assert_(not self.t.working)
-        return self.t.checkp4().addCallback(self._testCheck2)
+        d = self.t.checkp4()
+        d.addCallback(self._testCheck2)
+        return d
 
     def _testCheck2(self, res):
         self.assertEquals(self.changes, [])
         self.assertEquals(self.t.last_change, 1)
 
         # Subsequent times, it returns Change objects for new changes.
-        return self.t.checkp4().addCallback(self._testCheck3)
+        d = self.t.checkp4()
+        d.addCallback(self._testCheck3)
+        return d
 
     def _testCheck3(self, res):
         self.assertEquals(len(self.changes), 3)
@@ -157,6 +161,7 @@ def testFailedChanges(self):
     def _testFailedChanges2(self, f):
         self.failUnlessEqual(f, None)
         self.assert_(not self.t.working)
+        self.flushLoggedErrors(P4PollerError)
 
     def testFailedDescribe(self):
         """'p4 describe' failure is properly ignored"""
@@ -171,12 +176,15 @@ def testFailedDescribe(self):
 
     def _testFailedDescribe2(self, res):
         # first time finds nothing; check again.
-        return self.t.checkp4().addCallback(self._testFailedDescribe3)
+        d = self.t.checkp4()
+        d.addCallback(self._testFailedDescribe3)
+        return d
 
     def _testFailedDescribe3(self, f):
         self.failUnlessEqual(f, None)
         self.assert_(not self.t.working)
         self.assertEquals(self.t.last_change, 2)
+        self.flushLoggedErrors(P4PollerError)
 
     def testAlreadyWorking(self):
         """don't launch a new poll while old is still going"""
@@ -185,6 +193,7 @@ def testAlreadyWorking(self):
         self.assert_(self.t.last_change is None)
         d = self.t.checkp4()
         d.addCallback(self._testAlreadyWorking2)
+        return d
 
     def _testAlreadyWorking2(self, res):
         self.assert_(self.t.last_change is None)
@@ -200,6 +209,7 @@ def testSplitFile(self):
         self.t.last_change = 50
         d = self.t.checkp4()
         d.addCallback(self._testSplitFile)
+        return d
 
     def _testSplitFile(self, res):
         self.assertEquals(len(self.changes), 2)