On Wed, Mar 02, 2011 at 09:16:14AM +0100, Wolfgang Schnerring wrote: > Hello Brian, > > it's taken a while, but I finally had a chance to review your branch(es).
Great! > * Brian Sutherland <br...@vanguardistas.net> [2011-02-12 18:57]: > >> > On Tue, Feb 01, 2011 at 09:32:11AM +0100, Wolfgang Schnerring wrote: > >> >> I'd prefer if we treated this as two separate steps, then: > >> >> a) improve the testbrwoser+wsgi story by replacing wsgi_intercept with > >> >> WebTest > > > > I pulled this out of my original branch and put it here: > > > > svn+ssh://svn.zope.org/repos/main/zope.testbrowser/jinty-webtest3-minimal > > Thanks, that helped me understand what's going on much better. > > I do have a few questions about this part: > > - Does the (webtest-based) wsgi.Browser behave similarly to the > Publisher-Browser? > > When we ported the wsgi_intercept variant from zope.app.wsgi, we found > that there were some idiosyncracies woven in that people may rely on. > This kind of stuff of course isn't documented, and we didn't do much > research, but rather took care to port what we found in zope.app.wsgi, > namely filtering some HTTP headers from the response (meh, doesn't feel > important), and handling Basic Auth headers (that feels important to > preserve). I feel strongly that these idiosyncrasies shouldn't get built into the new WSGI testbrowser and perpetuated. You had some WSGI middleware which implemented these. I think we can do it in the same way and anyone migrating from Publisher-Browser based code could use it. Perhaps that needs to be inside zope.testbrowser itself, or could be done in zope.app.wsgi. Depends on the details I guess. Unfortunately, as you say, they are undocumented and seem to be untested. I would like to make a deeper investigation of those issues and write some tests. Is there any test suite (grok perhaps?) that I can run using the new WSGI based browser that'll highlight these issues? > - What should happen in zope.app.wsgi? > > We moved the wsgi-based Testbrowser from there and left BBB imports to > zope.testbrowser.wsgi, taking care to be API compatible. I guess that > won't be possible with the WebTest-based Testbrowser (or would it?), > so we probably have to break that. (Hmm, would that imply a major version > bump there, too?) > > Since the Grok people are the ones who probably use the zope.app.wsgi > Testbrowser the most, they should probably take the WebTest-Testbrowser > for a test drive and see whether it works for them, otherwise we're > going to make them quite unhappy if we just break zope.app.wsgi. > (Could you ask on grok-dev about that? I guess it's too buried here to > be noticed.) I'll try to co-ordinate with the grok/zope.app.wsgi developers on this. I was unaware that they had BBB imports from zope.testbrowser. > - What's connection.py? > > I don't really understand where that came from or what its purpose is, > especially since wsgi.py seems to be the only one that uses it? > > Ah. I take that last bit back, the extracted Publisher-Browser in > zope.app.testing uses it, so I guess this is a split-off artifact of the > refactoring/extraction. > > But I still don't really understand what it is all about. %-) It's about minimizing the duplicated code in zope.app.testing.testbrowser and zope.testbrowser.wsgi. It's kind of "reusable pieces to build zope.testbrowser connections". That said, I'm uncomfortable with it. connection.py is probably not really very reusable. And anyone changing it would have to be aware that it was reused in zope.app.testing. Another way would be to fold connection.py into zope.testbrowser.wsgi and copy the necessary code to zope.app.testing. It's not obvious to me which way is better. You could easily persuade me another way is better. > - The layer looks good. (OK, so that's not a question ;) > > I'm glad you found a way that the browser can get the application "out > of the air". I've tried the explicit passing way on a project of mine, > and it's a huge hassle (I've ended up stuffing a preconfigured browser > instance into the doctest globs, ugh.) yeah, in the end wsgi_intercept uses global variables. So it's the same approach. > I think it would be good if the layer stored the application of > self.app, we did that in the wsgi_intercept variant and were glad of it > a few times already. Sure, I've added that. > >> >> b) extract the testbrowser part that talks to the Publisher > > > > This is here and by necessity includes the changes for step (a): > > svn+ssh://svn.zope.org/repos/main/zope.testbrowser/jinty-webtest3 > > > > svn+ssh://svn.zope.org/repos/main/zope.app.testing/branches/jinty-testbrowser > > The extraction of the Publisher-Browser makes a lot of sense, and looks > clean to me, as does the rewrite of the tests to use a plain WSGI app > instead of a Zope-based app. Great! > > I would much prefer to merge both steps together. > > Yes, I guess that makes sense because only step (b) includes proper > tests for everything. Perfect. > Wolfgang > > _______________________________________________ > Zope-Dev maillist - Zope-Dev@zope.org > https://mail.zope.org/mailman/listinfo/zope-dev > ** No cross posts or HTML encoding! ** > (Related lists - > https://mail.zope.org/mailman/listinfo/zope-announce > https://mail.zope.org/mailman/listinfo/zope ) -- Brian Sutherland _______________________________________________ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )