On Aug 27, 2009, at 12:19 AM, Edwin Grubbs wrote: > Review: Approve code > Hi Gary, > > This branch looks good. I only have suggestions for minor changes and > fixing the lint items that are possible. Thank you Edwin! I fixed all the lint issues except for pylint not findling lazr.restful. I glanced at that; looks like somebody has already tried to improve this, but its not quite there yet, and I could not make it better with a quick fly-by. I'm only responding to the questions in-line, and then putting the incremental diff at the end. >> def addHeadersTo(self, full_url, full_headers): >> if (self.consumer is not None and self.access_token is not >> None): >> @@ -123,6 +120,8 @@ >> request.sign_request(OAuthSignatureMethod_PLAINTEXT(), >> self.consumer, self.access_token) >> full_headers.update(request.to_header(OAUTH_REALM)) >> + if not self.handle_errors: >> + full_headers['X_Zope_handle_errors'] = 'False' > > > > Is this why the webservice object in tests no longer raises an > exception? No, that's because we were relying on jsonBody raising an exception, and it still does, but now it is a "this isn't json!" exception rather than a useful report of the response status and the server's traceback. > def extract_url_parameter(url, parameter): >> @@ -619,7 +618,7 @@ >> access_token = request_token.createAccessToken() >> logout() >> return LaunchpadWebServiceCaller( >> - consumer_key, access_token.key, port=9000) >> + consumer_key, access_token.key) # "port=9000" was ignored, >> now not sent > > > I don't think this comment is necessary. If it needs to stay, it's > too long, needs a period, and I've been dinged for using eol comments, > although I don't see anything in the style guide about that. :-) Cool. Removed. ... >> >> === modified file 'lib/canonical/testing/layers.py' >> --- lib/canonical/testing/layers.py 2009-08-21 19:38:21 +0000 >> +++ lib/canonical/testing/layers.py 2009-08-26 22:51:00 +0000 >> @@ -69,7 +69,10 @@ >> import psycopg2 >> from storm.zope.interfaces import IZStorm >> import transaction >> +import wsgi_intercept >> >> +from zope.app.publication.httpfactory import chooseClasses >> +from zope.app.publication.http import HTTPPublication >> import zope.app.testing.functional >> from zope.app.testing.functional import FunctionalTestSetup, >> ZopePublication >> from zope.component import getUtility, provideUtility >> @@ -752,6 +755,47 @@ >> "DELETE FROM SessionData") >> >> >> +def wsgi_application(environ, start_response): >> + """This is a wsgi application for Zope functional testing. >> + >> + We use it with wsgi_intercept, which is itself mostly >> interesting >> + for our webservice (lazr.restful) tests. >> + """ >> + # Committing work done up to now is a convenience that the Zope >> + # zope.app.testing.functional.HTTPCaller does. We're >> replacing that bit, >> + # so it is easiest to follow that lead, even if it feels a >> little loose. >> + transaction.commit() >> + # Let's support post-mortem debugging. >> + if environ.get('HTTP_X_ZOPE_HANDLE_ERRORS') == 'False': >> + environ['wsgi.handleErrors'] = False >> + if 'HTTP_X_ZOPE_HANDLE_ERRORS' in environ: >> + del environ['HTTP_X_ZOPE_HANDLE_ERRORS'] > > > environ.pop(key, default) would reduce the usage of the super long > string. Good call, done. > It would be nice if the string was defined as a constant somewhere. Yeah, good idea. I believe that would be in zope.app.testing if it were defined. I looked, and didn't find such a constant definition. Since I'm only using it once now, I didn't bother to define my own constant. >> + handle_errors = environ.get('wsgi.handleErrors', True) >> + # Now we do the proper dance to get the desired request. This >> is an >> + # almalgam of code from zope.app.testing.functional.HTTPCaller >> and >> + # zope.publisher.paste.Application. >> + request_cls, publication_cls = chooseClasses( >> + environ['REQUEST_METHOD'], environ) >> + publication = publication_cls(FunctionalTestSetup().db) >> + request = request_cls(environ['wsgi.input'], environ) >> + request.setPublication(publication) >> + # The rest of this function is an amalgam of >> + # zope.publisher.paste.Application.__call__ and >> van.testing.layers. > > > What's van.testing.layers? Is that a typo. No, van.testing is a package that lazr.restful uses. It is an indirect dependency; you'll find it in the download-cache and eggs directories. ... >> >> === modified file 'utilities/sourcedeps.conf' >> --- utilities/sourcedeps.conf 2009-08-14 17:26:21 +0000 >> +++ utilities/sourcedeps.conf 2009-08-26 22:51:00 +0000 >> @@ -3,13 +3,7 @@ >> bzr-svn=lp:~launchpad-pqm/bzr-svn/devel/ >> dulwich=lp:~launchpad-pqm/dulwich/devel/ >> launchpad-loggerhead=lp:~launchpad-pqm/launchpad-loggerhead/devel/ >> -lazr.batchnavigator=lp:~launchpad-pqm/lazr.batchnavigator/trunk/ >> -lazr.config=lp:~launchpad-pqm/lazr.config/devel/ >> -lazr.delegates=lp:~launchpad-pqm/lazr.delegates/devel/ >> -lazr.enum=lp:~launchpad-pqm/lazr.enum/trunk/ >> lazr-js=lp:~launchpad-pqm/lazr-js/toolchain/ >> -lazr.lifecycle=lp:~launchpad-pqm/lazr.lifecycle/trunk/ >> -lazr.restful=lp:~launchpad-pqm/lazr.restful/trunk/ >> loggerhead=lp:~launchpad-pqm/loggerhead/devel/ >> mailman=lp:~launchpad-pqm/mailman/2.1/ >> pygpgme=lp:~launchpad-pqm/pygpgme/devel/ >> >> === modified file 'versions.cfg' >> --- versions.cfg 2009-08-24 21:21:35 +0000 >> +++ versions.cfg 2009-08-26 22:51:00 +0000 >> @@ -11,14 +11,20 @@ >> ctypes = 1.0.2 >> docutils = 0.5 >> elementtree = 1.2.6-20050316 >> +epydoc = 3.0.1 >> feedvalidator = 0.0.0DEV-r1049 >> functest = 0.8.7 >> funkload = 1.10.0 >> httplib2 = 0.4.0 >> ipython = 0.9.1 >> -jquery.javascript = 1.0.0 >> -jquery.layer = 1.0.0 >> -launchpadlib = 1.0.3 > > > > Are these just not used anymore, or are they included in a different > package? The jquery bits are not used, as far as my investigations were able to determine. launchpadlib changed its version (immediately below this excerpt). So that's it for the answers. The incremental diff follows. Thank you again for the review! Gary === modified file 'lib/canonical/launchpad/pagetests/webservice/xx- private-membership.txt' --- lib/canonical/launchpad/pagetests/webservice/xx-private- membership.txt 2009-08-20 04:46:48 +0000 +++ lib/canonical/launchpad/pagetests/webservice/xx-private- membership.txt 2009-08-27 12:05:56 +0000 @@ -101,7 +101,8 @@ ... simplejson.dumps(representation), ... headers) - >>> print modify_team('/~my-new-team', {'visibility' : 'Private Membership'}, + >>> print modify_team( + ... '/~my-new-team', {'visibility' : 'Private Membership'}, ... 'PATCH', comm_webservice) HTTP/1.1 209 Content Returned ... @@ -116,7 +117,8 @@ As an admin, Salgado can also change the team's visibility. - >>> print modify_team('/~my-new-team', {'visibility' : 'Public'}, + >>> print modify_team( + ... '/~my-new-team', {'visibility' : 'Public'}, ... 'PATCH', webservice) HTTP/1.1 209 Content Returned ... === modified file 'lib/canonical/launchpad/testing/pages.py' --- lib/canonical/launchpad/testing/pages.py 2009-08-21 19:49:18 +0000 +++ lib/canonical/launchpad/testing/pages.py 2009-08-27 12:04:47 +0000 @@ -25,7 +25,7 @@ BeautifulSoup, Comment, Declaration, NavigableString, PageElement, ProcessingInstruction, SoupStrainer, Tag) from contrib.oauth import OAuthRequest, OAuthSignatureMethod_PLAINTEXT -from urlparse import urljoin, urlparse +from urlparse import urljoin from zope.app.testing.functional import HTTPCaller, SimpleCookie from zope.component import getUtility @@ -58,13 +58,13 @@ del kw['debug'] else: self._debug = False - super(UnstickyCookieHTTPCaller, self).__init__(*args, **kw) + HTTPCaller.__init__(self, *args, **kw) def __call__(self, *args, **kw): if self._debug: pdb.set_trace() try: - return super(UnstickyCookieHTTPCaller, self).__call__(*args, **kw) + return HTTPCaller.__call__(self, *args, **kw) finally: self.resetCookies() @@ -77,8 +77,7 @@ if 'PATH_INFO' not in environment: environment = dict(environment) environment['PATH_INFO'] = path - return super(UnstickyCookieHTTPCaller, self).chooseRequestClass( - method, path, environment) + return HTTPCaller.chooseRequestClass(self, method, path, environment) def resetCookies(self): self.cookies = SimpleCookie() @@ -617,8 +616,7 @@ request_token.review(person, permission, context) access_token = request_token.createAccessToken() logout() - return LaunchpadWebServiceCaller( - consumer_key, access_token.key) # "port=9000" was ignored, now not sent + return LaunchpadWebServiceCaller(consumer_key, access_token.key) def stop(): === modified file 'lib/canonical/testing/layers.py' --- lib/canonical/testing/layers.py 2009-08-25 19:09:14 +0000 +++ lib/canonical/testing/layers.py 2009-08-27 19:06:57 +0000 @@ -72,7 +72,6 @@ import wsgi_intercept from zope.app.publication.httpfactory import chooseClasses -from zope.app.publication.http import HTTPPublication import zope.app.testing.functional from zope.app.testing.functional import FunctionalTestSetup, ZopePublication from zope.component import getUtility, provideUtility @@ -766,10 +765,8 @@ # so it is easiest to follow that lead, even if it feels a little loose. transaction.commit() # Let's support post-mortem debugging. - if environ.get('HTTP_X_ZOPE_HANDLE_ERRORS') == 'False': + if environ.pop('HTTP_X_ZOPE_HANDLE_ERRORS', 'True') == 'False': environ['wsgi.handleErrors'] = False - if 'HTTP_X_ZOPE_HANDLE_ERRORS' in environ: - del environ['HTTP_X_ZOPE_HANDLE_ERRORS'] handle_errors = environ.get('wsgi.handleErrors', True) # Now we do the proper dance to get the desired request. This is an # almalgam of code from zope.app.testing.functional.HTTPCaller and @@ -1326,8 +1323,9 @@ access_logger.log(MockHTTPTask(response._response, first_line)) return response - # Setting STAGGER_RETRIES makes tests like notfound- traversals.txt go - # much, much faster by avoiding calls to time.sleep() + # Setting STAGGER_RETRIES to False makes tests like + # notfound-traversals.txt go much, much faster by avoiding calls to + # time.sleep() cls._original_stagger_retries = zope.publisher.http.STAGGER_RETRIES zope.publisher.http.STAGGER_RETRIES = False === modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt' --- lib/lp/soyuz/stories/webservice/xx-archive.txt 2009-08-21 19:49:18 +0000 +++ lib/lp/soyuz/stories/webservice/xx-archive.txt 2009-08-27 19:11:15 +0000 @@ -629,8 +629,8 @@ syncSources is invoked directly by the client, and any problems are the client's fault. Therefore, there's no need to record an OOPS. - >>> 'X-Lazr-Oopsid' in str(already_copied) - False + >>> print already_copied.getheader('X-Lazr-Oopsid') + None 'syncSources' behaves trasactionally, i.e. it will only synchronise all packages or none of them if there was a problem.