Generally good, but a few notes:


=== modified file 'oops_wsgi/middleware.py'
--- oops_wsgi/middleware.py	2011-08-18 00:45:58 +0000
+++ oops_wsgi/middleware.py	2011-08-22 07:04:24 +0000
@@ -85,8 +85,23 @@
                 state['write'] = start_response(status, headers)
                 write = state['write']
             write(bytes)
-        def oops_start_response(status, headers):
-            state['response'] = (status, headers)
+        def oops_start_response(status, headers, exc_info=None):
+            if exc_info is not None:
+                # The app is explicitly signalling an error (rather than
+                # returning a page describing the error). Capture that and then
+                # forward to the containing element verbatim. In future we might
+                # choose to add the OOPS id to the headers (but we can't fiddle
+                # the body as it is being generated by the contained app.
+                report = config.create(
+                        dict(exc_info=exc_info, url=construct_url(environ)))

Careful!  You seem to be indenting by 8 instead of 4.

+                ids = config.publish(report)
+                try:
+                    state['write'] = start_response(status, headers, exc_info)
+                    return state['write']
+                finally:
+                    del exc_info

Out of interest, whence the del?


=== modified file 'oops_wsgi/tests/test_middleware.py'
--- oops_wsgi/tests/test_middleware.py	2011-08-18 22:44:58 +0000
+++ oops_wsgi/tests/test_middleware.py	2011-08-22 07:04:24 +0000

@@ -52,7 +53,21 @@
         environ['HTTP_HOST'] = 'example.com'
         environ['PATH_INFO'] = '/demo'
         environ['QUERY_STRING'] = 'param=value'
-        def start_response(status, headers):
+        def start_response(status, headers, exc_info=None):
+            if exc_info:
+                # If an exception is raised after we have written (via the app
+                # calling our oops_write) or after we have yielded data, we
+                # must supply exc_info in the call to start_response, per the
+                # wsgi spec. The easiest way to do that is to always supply
+                # exc_info when reporting an exception - that will DTRT if
+                # start_response upstream had already written data. Thus
+                # our test start_response which lives above the oops middleware
+                # captures the fact exc_info was called and all our tests that
+                # expect oopses raised expect the exc_info to be present.
+                self.calls.append(
+                        ('start error', status, headers, exc_info[0],
+                        exc_info[1].args))

Here too, indentation goes too far.  You're not getting tabs, are you?


@@ -191,7 +206,8 @@
              'url': 'http://example.com/demo?param=value',
              'value': u'boom, yo'},
             # Then the middleware responses
-            ('500 Internal Server Error', [('Content-Type', 'text/html')]),
+            ('start error', '500 Internal Server Error',
+             [('Content-Type', 'text/html')], ValueError, ('boom, yo',)),

Horrid layout.  Your branch didn't start it, so I won't insist that you fix it.  But take my review with a grain of salt since I can't really make sense of this in reasonable time.


 
@@ -250,3 +268,20 @@
         self.wrap_and_run(inner, config)
         self.assertEqual('http://example.com/demo?param=value',
                 self.calls[0]['url'])
+
+    def test_start_response_with_just_exc_info_generates_oops(self):
+        def inner(environ, start_response):
+            try:
+                raise ValueError('boom, yo')
+            except ValueError:
+                exc_info = sys.exc_info()
+            start_response('500 FAIL', [], exc_info)
+            yield 'body'
+        config = self.config_for_oopsing()
+        self.wrap_and_run(inner, config)
+        self.assertEqual([{'id': 1,
+            'type': 'ValueError',
+            'url': 'http://example.com/demo?param=value',
+            'value': u'boom, yo'},
+            ('start error', '500 FAIL', [], ValueError, ('boom, yo',))],
+            self.calls)

In this case though, please do fix the layout!  So far the best solution I've found is to move the expected dict into a variable.  Not great, but better than any of the alternatives I'm aware of.


Jeroen