web: Return server.NOT_DONE_YET from change_hooks.

twisted.web.resource predates deferreds, and so has idiosyncratic handling of
asynchronous results. Fix /change_hook handling to return proper results and
add support from properly testing this in FakeRequest.test_render.

Fixes #2206.

master/buildbot/status/web/change_hook.py

@@ -19,7 +19,7 @@
 # but "the rest" is pretty minimal
 
 import re
-from twisted.web import resource
+from twisted.web import resource, server
 from twisted.python.reflect import namedModule
 from twisted.python import log
 from twisted.internet import defer
@@ -62,16 +62,22 @@ def render_POST(self, request):
             changes, src = self.getChanges( request )
         except ValueError, err:
             request.setResponseCode(400, err.args[0])
-            return defer.succeed(err.args[0])
+            return err.args[0]
 
         log.msg("Payload: " + str(request.args))
 
         if not changes:
             log.msg("No changes found")
-            return defer.succeed("no changes found")
+            return "no changes found"
         d = self.submitChanges( changes, request, src )
-        d.addCallback(lambda _ : "OK")
-        return d
+        def ok(_):
+            request.setResponseCode(202)
+            request.finish()
+        def err(_):
+            requeset.setResponseCode(500)
+            request.finish()
+        d.addCallbacks(ok, err)
+        return server.NOT_DONE_YET
 
 
     def getChanges(self, request):

master/buildbot/test/fake/web.py

@@ -16,13 +16,20 @@
 from mock import Mock
 
 from twisted.internet import defer
+from twisted.web import server
 
-class MockRequest(Mock):
+class FakeRequest(Mock):
     """
     A fake Twisted Web Request object, including some pointers to the
     buildmaster and an addChange method on that master which will append its
     arguments to self.addedChanges.
     """
+
+    written = ''
+    finished = False
+    redirected_to = None
+    failure = None
+
     def __init__(self, args={}):
         self.args = args
         self.site = Mock()
@@ -35,4 +42,35 @@ def addChange(**kwargs):
             return defer.succeed(Mock())
         master.addChange = addChange
 
+        self.deferred = defer.Deferred()
+
         Mock.__init__(self)
+
+    def write(self, data):
+        self.written = self.written + data
+
+    def redirect(self, url):
+        self.redirected_to = url
+
+    def finish(self):
+        self.finished = True
+        self.deferred.callback(None)
+
+    def processingFailed(self, f):
+        self.deferred.errback(f)
+
+    # work around http://code.google.com/p/mock/issues/detail?id=105
+    def _get_child_mock(self, **kw):
+        return Mock(**kw)
+
+    # cribed from twisted.web.test._util._render
+    def test_render(self, resource):
+          result = resource.render(self)
+          if isinstance(result, str):
+              self.write(result)
+              self.finish()
+              return self.deferred
+          elif result is server.NOT_DONE_YET:
+              return self.deferred
+          else:
+              raise ValueError("Unexpected return value: %r" % (result,))

master/buildbot/test/unit/test_status_web_base.py

@@ -18,28 +18,7 @@
 from twisted.internet import defer
 from twisted.trial import unittest
 
-class FakeRequest(mock.Mock):
-    written = ''
-    finished = False
-    redirected_to = None
-    failure = None
-
-    def __init__(self):
-        mock.Mock.__init__(self)
-        self.deferred = defer.Deferred()
-
-    def write(self, data):
-        self.written = self.written + data
-
-    def redirect(self, url):
-        self.redirected_to = url
-
-    def finish(self):
-        self.finished = True
-        self.deferred.callback(None)
-
-    def processingFailed(self, f):
-        self.deferred.errback(f)
+from buildbot.test.fake.web import FakeRequest
 
 class ActionResource(unittest.TestCase):
 

master/buildbot/test/unit/test_status_web_change_hook.py

@@ -15,58 +15,60 @@
 
 from buildbot.status.web import change_hook
 from buildbot.util import json
-from buildbot.test.fake.web import MockRequest
-from mock import Mock
+from buildbot.test.fake.web import FakeRequest
 
 from twisted.trial import unittest
-from twisted.internet import defer
 
 class TestChangeHookUnconfigured(unittest.TestCase):
     def setUp(self):
-        self.request = Mock()
+        self.request = FakeRequest()
         self.changeHook = change_hook.ChangeHookResource()
 
     # A bad URI should cause an exception inside check_hook.
     # After writing the test, it became apparent this can't happen.
     # I'll leave the test anyway
     def testDialectReMatchFail(self):
         self.request.uri = "/garbage/garbage"
-        d = self.changeHook.render_GET(self.request)
+        self.request.method = "GET"
+        d = self.request.test_render(self.changeHook)
         def check(ret):
             expected = "URI doesn't match change_hook regex: /garbage/garbage"
-            self.assertEquals(ret, expected)
+            self.request.mockCheckCall(self, 0, "write", expected )
             self.request.mockCheckCall(self, 0, "setResponseCode", 400, expected)
         d.addCallback(check)
         return d
 
     def testUnkownDialect(self):
         self.request.uri = "/change_hook/garbage"
-        d = self.changeHook.render_GET(self.request)
+        self.request.method = "GET"
+        d = self.request.test_render(self.changeHook)
         def check(ret):
             expected = "The dialect specified, 'garbage', wasn't whitelisted in change_hook"
-            self.assertEquals(ret, expected )
+            self.request.mockCheckCall(self, 0, "write", expected )
             self.request.mockCheckCall(self, 0, "setResponseCode", 400, expected)
         d.addCallback(check)
         return d
 
     def testDefaultDialect(self):
         self.request.uri = "/change_hook/"
-        d = self.changeHook.render_GET(self.request)
+        self.request.method = "GET"
+        d = self.request.test_render(self.changeHook)
         def check(ret):
             expected = "The dialect specified, 'base', wasn't whitelisted in change_hook"
-            self.assertEquals(ret, expected)
+            self.request.mockCheckCall(self, 0, "write", expected )
             self.request.mockCheckCall(self, 0, "setResponseCode", 400, expected)
         d.addCallback(check)
         return d
 
 class TestChangeHookConfigured(unittest.TestCase):
     def setUp(self):
-        self.request = MockRequest()
+        self.request = FakeRequest()
         self.changeHook = change_hook.ChangeHookResource(dialects={'base' : True})
 
     def testDefaultDialectGetNullChange(self):
         self.request.uri = "/change_hook/"
-        d = defer.maybeDeferred(lambda : self.changeHook.render_GET(self.request))
+        self.request.method = "GET"
+        d = self.request.test_render(self.changeHook)
         def check_changes(r):
             self.assertEquals(len(self.request.addedChanges), 1)
             change = self.request.addedChanges[0]
@@ -89,6 +91,7 @@ def check_changes(r):
     # a Change object as a dictionary. All values show be set.
     def testDefaultDialectWithChange(self):
         self.request.uri = "/change_hook/"
+        self.request.method = "GET"
         self.request.args = { "category" : ["mycat"],
                        "files" : [json.dumps(['file1', 'file2'])],
                        "repository" : ["myrepo"],
@@ -102,7 +105,7 @@ def testDefaultDialectWithChange(self):
                        "revlink" : ["a revlink"],
                        "properties" : [json.dumps( { "prop1" : "val1", "prop2" : "val2" })],
                        "revision" : [99] }
-        d = defer.maybeDeferred(lambda : self.changeHook.render_GET(self.request))
+        d = self.request.test_render(self.changeHook)
         def check_changes(r):
             self.assertEquals(len(self.request.addedChanges), 1)
             change = self.request.addedChanges[0]

master/buildbot/test/unit/test_status_web_change_hooks_github.py

@@ -14,7 +14,7 @@
 # Copyright Buildbot Team Members
 
 import buildbot.status.web.change_hook as change_hook
-from buildbot.test.fake.web import MockRequest
+from buildbot.test.fake.web import FakeRequest
 
 from twisted.trial import unittest
 from twisted.internet import defer
@@ -69,14 +69,15 @@
 class TestChangeHookConfiguredWithGitChange(unittest.TestCase):
     def setUp(self):
         changeDict={"payload" : [gitJsonPayload]}
-        self.request = MockRequest(changeDict)
+        self.request = FakeRequest(changeDict)
         self.changeHook = change_hook.ChangeHookResource(dialects={'github' : True})
 
     # Test 'base' hook with attributes. We should get a json string representing
     # a Change object as a dictionary. All values show be set.
     def testGitWithChange(self):
         self.request.uri = "/change_hook/github"
-        d = defer.maybeDeferred(lambda : self.changeHook.render_GET(self.request))
+        self.request.method = "GET"
+        d = self.request.test_render(self.changeHook)
         def check_changes(r):
             self.assertEquals(len(self.request.addedChanges), 2)
             change = self.request.addedChanges[0]

master/buildbot/test/unit/test_status_web_change_hooks_googlecode.py

@@ -18,7 +18,7 @@
 import StringIO
 
 import buildbot.status.web.change_hook as change_hook
-from buildbot.test.fake.web import MockRequest
+from buildbot.test.fake.web import FakeRequest
 
 from twisted.trial import unittest
 from twisted.internet import defer
@@ -47,7 +47,7 @@
 
 class TestChangeHookConfiguredWithGoogleCodeChange(unittest.TestCase):
     def setUp(self):
-        self.request = MockRequest()
+        self.request = FakeRequest()
         # Google Code simply transmit the payload as an UTF-8 JSON body
         self.request.content = StringIO.StringIO(googleCodeJsonBody)
         self.request.received_headers = {
@@ -70,7 +70,8 @@ def setUp(self):
     # a Change object as a dictionary. All values show be set.
     def testGoogleCodeWithHgChange(self):
         self.request.uri = "/change_hook/googlecode"
-        d = defer.maybeDeferred(lambda : self.changeHook.render_GET(self.request))
+        self.request.method = "GET"
+        d = self.request.test_render(self.changeHook)
         def check_changes(r):
             # Only one changeset has been submitted.
             self.assertEquals(len(self.request.addedChanges), 1)