Re: [Checkins] SVN: Zope/trunk/ Fixed collector 2057: Testing.makequest broke getPhysicalPath()
For the record: I am still opposed to this change. It basically endows the request (as in self.REQUEST) with a getPhysicalPath method, and I have no idea what kind of side-effects this may have. AFAICS your test suite is the only suite around that wants to request- wrap non-root objects. There's nothing wrong with using your own makerequest for your own test suite, if this is what you want. But I don't think your use-case warrants changing Zope. Stefan On 5. Apr 2006, at 22:51, Paul Winkler wrote:
Log message for revision 66581: Fixed collector 2057: Testing.makequest broke getPhysicalPath() when used on anything other than an app object. Also added an 'environ' argument if you want to use something other than os.environ, and added a couple trivial tweaks from ZopeTestCase.makereqeust. Added unit tests for this stuff.
Changed: U Zope/trunk/doc/CHANGES.txt U Zope/trunk/lib/python/Testing/makerequest.py A Zope/trunk/lib/python/Testing/tests/ A Zope/trunk/lib/python/Testing/tests/__init__.py A Zope/trunk/lib/python/Testing/tests/test_makerequest.py
-=- Modified: Zope/trunk/doc/CHANGES.txt =================================================================== --- Zope/trunk/doc/CHANGES.txt 2006-04-05 20:04:59 UTC (rev 66580) +++ Zope/trunk/doc/CHANGES.txt 2006-04-05 20:51:21 UTC (rev 66581) @@ -46,6 +46,9 @@
Features added
+ - Testing.makerequest: Added an 'environ' argument so + clients can use mappings other than os.environ. + - Updated to Docutils 0.4.0
- reStructuredText: The default value for the 'stylesheet' @@ -202,6 +205,10 @@
Bugs Fixed
+ - Collector #2057: Allow Testing.makerequest to work with + any acquisition-supporting root object, not just Zope2.app. + Formerly, if you did that, getPhysicalPath() was broken. + - Collector #2051: Applied patch by Yoshinori Okuji to fix some XML export/import problems, including tests for that feature.
Modified: Zope/trunk/lib/python/Testing/makerequest.py =================================================================== --- Zope/trunk/lib/python/Testing/makerequest.py 2006-04-05 20:04:59 UTC (rev 66580) +++ Zope/trunk/lib/python/Testing/makerequest.py 2006-04-05 20:51:21 UTC (rev 66581) @@ -19,27 +19,50 @@ import makerequest app = makerequest.makerequest(Zope2.app())
+You can optionally pass stdout to be used by the response, +and an environ mapping to be used in the request. +Defaults are sys.stdout and os.environ. + +If you don't want to start a zope app in your test, you can wrap other +objects, but they must support acquisition and you should only wrap +your root object. + + $Id$
"""
import os -from os import environ from sys import stdin, stdout from ZPublisher.HTTPRequest import HTTPRequest from ZPublisher.HTTPResponse import HTTPResponse from ZPublisher.BaseRequest import RequestContainer
-def makerequest(app, stdout=stdout): +def makerequest(app, stdout=stdout, environ=None): resp = HTTPResponse(stdout=stdout) - environ['SERVER_NAME']='foo' - environ['SERVER_PORT']='80' - environ['REQUEST_METHOD'] = 'GET' + if environ is None: + environ = os.environ + environ.setdefault('SERVER_NAME', 'foo') + environ.setdefault('SERVER_PORT', '80') + environ.setdefault('REQUEST_METHOD', 'GET') req = HTTPRequest(stdin, environ, resp) - + req._steps = ['noobject'] # Fake a published object. + req['ACTUAL_URL'] = req.get('URL') # Zope 2.7.4 + # set Zope3-style default skin so that the request is usable for - # Zope3-style view look-ups + # Zope3-style view look-ups. from zope.app.publication.browser import setDefaultSkin setDefaultSkin(req)
- return app.__of__(RequestContainer(REQUEST = req)) + requestcontainer = RequestContainer(REQUEST = req) + # Workaround for collector 2057: ensure that we don't break + # getPhysicalPath if app has that method. + # We could instead fix Traversable.getPhysicalPath() to check for + # existence of p.getPhysicalPath before calling it; but it's such + # a commonly called method that I don't want to impact performance + # for something that AFAICT only affects makerequest() in + # practice. + if getattr(app, 'getPhysicalPath', None) is not None: + requestcontainer.getPhysicalPath = lambda: () + + return app.__of__(requestcontainer)
Added: Zope/trunk/lib/python/Testing/tests/__init__.py =================================================================== --- Zope/trunk/lib/python/Testing/tests/__init__.py 2006-04-05 20:04:59 UTC (rev 66580) +++ Zope/trunk/lib/python/Testing/tests/__init__.py 2006-04-05 20:51:21 UTC (rev 66581) @@ -0,0 +1 @@ +#
Property changes on: Zope/trunk/lib/python/Testing/tests/__init__.py ___________________________________________________________________ Name: svn:keywords + "Author Date Revision"
Added: Zope/trunk/lib/python/Testing/tests/test_makerequest.py =================================================================== --- Zope/trunk/lib/python/Testing/tests/test_makerequest.py 2006-04-05 20:04:59 UTC (rev 66580) +++ Zope/trunk/lib/python/Testing/tests/test_makerequest.py 2006-04-05 20:51:21 UTC (rev 66581) @@ -0,0 +1,65 @@ +##################################################################### ######### +# +# Copyright (c) 2006 Zope Corporation and Contributors. All Rights Reserved. +# +# This software is subject to the provisions of the Zope Public License, +# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution. +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS +# FOR A PARTICULAR PURPOSE. +# +##################################################################### ######### +"""Unit tests of makequest. + +$Id$ +""" + +import unittest + +from Acquisition import Implicit +from Testing.makerequest import makerequest +from OFS.SimpleItem import SimpleItem + +class MakerequestTests(unittest.TestCase): + + def test_makerequest(self): + # The argument must support acquisition. + self.assertRaises(AttributeError, makerequest, object()) + # After the call, it will have a REQUEST attribute. + item = Implicit() + self.failIf(hasattr(item, 'REQUEST')) + item = makerequest(item) + self.failUnless(hasattr(item, 'REQUEST')) + + def test_dont_break_getPhysicalPath(self): + # see http://www.zope.org/Collectors/Zope/2057 + item = SimpleItem() + self.assertEqual(item.getPhysicalPath(), ('',)) + self.assertEqual(item.getPhysicalPath(), + makerequest(item).getPhysicalPath()) + + def test_stdout(self): + # You can pass a stdout arg and it's used by the response. + import cStringIO + out = cStringIO.StringIO() + item = makerequest(SimpleItem(), stdout=out) + item.REQUEST.RESPONSE.write('aaa') + out.seek(0) + written = out.read() + self.failUnless(written.startswith('Status: 200 OK\n')) + self.failUnless(written.endswith('\naaa')) + + def test_environ(self): + # You can pass an environ argument to use in the request. + environ = {'foofoo': 'barbar'} + item = makerequest(SimpleItem(), environ=environ) + self.assertEqual(item.REQUEST.environ['foofoo'], 'barbar') + +def test_suite(): + suite = unittest.TestSuite() + suite.addTest(unittest.makeSuite(MakerequestTests)) + return suite + +if __name__=='__main__': + unittest.TextTestRunner().run(test_suite())
Property changes on: Zope/trunk/lib/python/Testing/tests/ test_makerequest.py ___________________________________________________________________ Name: svn:keywords + "Author Date Revision"
_______________________________________________ Checkins mailing list Checkins@zope.org http://mail.zope.org/mailman/listinfo/checkins
-- Anything that happens, happens. --Douglas Adams
On Thu, Apr 06, 2006 at 11:17:20AM +0200, Stefan H. Holek wrote:
For the record: I am still opposed to this change. It basically endows the request (as in self.REQUEST) with a getPhysicalPath method, and I have no idea what kind of side-effects this may have.
That's because there aren't any ;-) The only way you can ever hit REQUEST.getPhysicalPath is if you specifically ask for it, which is obviously stupid; or if the object you're wrapping doesn't terminate getPhysicalPath().
AFAICS your test suite is the only suite around that wants to request- wrap non-root objects.
I'd reprhase that as "... wants to request-wrap non-Zope2.app() objects." FWIW, I didn't write it, and at the time those tests were added, I believe they worked. This was using zope 2.8.something. I know who originally added those tests, and he's pretty high profile in the community, so I wouldn't be surprised if there are many such tests around. I was hired several months later and discovered some apparent bit-rot in our test suite, i.e. a number of failures and errors, and one of the principal points of failure was the issue we're discussing. I'm not sure, but I'm guessing that the calls to getPhysicalPath() in our app were added later, and whoever did that must've punted on figuring out the resulting test issues.
There's nothing wrong with using your own makerequest for your own test suite, if this is what you want. But I don't think your use-case warrants changing Zope.
Noted, and that's a reasonable position. Does nobody else care? Using an Item or Folder as your root object for tests works fine except for this one issue, so why not allow that? My feeling is that setting up an app is unnecessary work when you don't need one; for one thing, your test module needs to call Zope2.startup() first; for another, afaict creating a Zope2.app is a couple orders of magnitude slower than creating a SimpleItem. So maybe more people *should* use makerequest(NotAnApp) ;-) -- Paul Winkler http://www.slinkp.com
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Paul Winkler wrote:
On Thu, Apr 06, 2006 at 11:17:20AM +0200, Stefan H. Holek wrote:
For the record: I am still opposed to this change. It basically endows the request (as in self.REQUEST) with a getPhysicalPath method, and I have no idea what kind of side-effects this may have.
That's because there aren't any ;-)
The only way you can ever hit REQUEST.getPhysicalPath is if you specifically ask for it, which is obviously stupid; or if the object you're wrapping doesn't terminate getPhysicalPath().
AFAICS your test suite is the only suite around that wants to request- wrap non-root objects.
I'd reprhase that as "... wants to request-wrap non-Zope2.app() objects."
FWIW, I didn't write it, and at the time those tests were added, I believe they worked. This was using zope 2.8.something.
I know who originally added those tests,
That would be me. I don't see anything wrong with using a non-Zope2-app object for unit testing: in fact, I think it is *superior* practice. Most tests don't need to have the whole shooting match of the Zope startup dance done, and in fact will be better unit tests if they are tested *outside* of that environment.
I was hired several months later and discovered some apparent bit-rot in our test suite, i.e. a number of failures and errors, and one of the principal points of failure was the issue we're discussing. I'm not sure, but I'm guessing that the calls to getPhysicalPath() in our app were added later, and whoever did that must've punted on figuring out the resulting test issues.
I think you will see lots of usage of the pattern in CMF and related products: the RequestTest and SecurityRequestTest base classes from CMFCore are used *everywhere*. The only caveat is that, if your tests *needs* to call 'getPhysicalPath' and get a "realistic" value, you need to ensure that the object used as the root does the Right Thing (TM).
There's nothing wrong with using your own makerequest for your own test suite, if this is what you want. But I don't think your use-case warrants changing Zope.
Noted, and that's a reasonable position. Does nobody else care?
I don't beleive that "must be a Zope2.app()" is a real part of the contract of 'makerequest': "must be able to emulate one well enough for the rest of the test to succeed" is.
Using an Item or Folder as your root object for tests works fine except for this one issue, so why not allow that? My feeling is that setting up an app is unnecessary work when you don't need one; for one thing, your test module needs to call Zope2.startup() first; for another, afaict creating a Zope2.app is a couple orders of magnitude slower than creating a SimpleItem.
So maybe more people *should* use makerequest(NotAnApp) ;-)
+1. Tres. - -- =================================================================== Tres Seaver +1 202-558-7113 tseaver@palladion.com Palladion Software "Excellence by Design" http://palladion.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFENY+6+gerLs4ltQ4RAiB7AJ9s/Ntu6pkWN8nyiQ6ifNfCj7DgPwCgzg0g SwE3IdhmcFdEnDL5kIpIiYU= =26iW -----END PGP SIGNATURE-----
On Thu, Apr 06, 2006 at 06:01:30PM -0400, Tres Seaver wrote:
Paul Winkler wrote:
I know who originally added those tests,
That would be me.
I was hoping you'd pop in :-)
I don't see anything wrong with using a non-Zope2-app object for unit testing: in fact, I think it is *superior* practice. Most tests don't need to have the whole shooting match of the Zope startup dance done, and in fact will be better unit tests if they are tested *outside* of that environment. (snip) I think you will see lots of usage of the pattern in CMF and related products: the RequestTest and SecurityRequestTest base classes from CMFCore are used *everywhere*. The only caveat is that, if your tests *needs* to call 'getPhysicalPath' and get a "realistic" value, you need to ensure that the object used as the root does the Right Thing (TM).
Interesting. I'd forgotten that RequestTest et al. used this stuff; I had another look and what gets wrapped in makerequest there is whatever you get when you do Zope2.DB.open().root()['Application']. Is that not a Zope2.app? tangent: I really wish I'd looked at tests/base/testcase.py sooner, because I wasted time rewriting our LogInterceptor to work with the logging package - I didn't realize that a) it was a copy-paste from CMFCore, and b) the logging update was already done for CMF 2 if not sooner. Doh! Copy-paste is evil. Oh well, I learned some things about the logging package.
I don't beleive that "must be a Zope2.app()" is a real part of the contract of 'makerequest': "must be able to emulate one well enough for the rest of the test to succeed" is.
That's a good thing to note in the docstrings. I'll go do that. Also I'll remove the getPhysicalPath() hackery. I don't really agree with Stefan that it's dangerous, but certainly it smells bad :-) -- Paul Winkler http://www.slinkp.com
Paul Winkler wrote:
Using an Item or Folder as your root object for tests works fine except for this one issue, so why not allow that? My feeling is that setting up an app is unnecessary work when you don't need one; for one thing, your test module needs to call Zope2.startup() first; for another, afaict creating a Zope2.app is a couple orders of magnitude slower than creating a SimpleItem.
So maybe more people *should* use makerequest(NotAnApp) ;-)
FWIW usually in my tests I rarely use makerequest. On the other hand, when testing things that use traversal, it's very common that I have to define a fake root object whose getPhysicalPath returns ('',). It's only a few line, and makes my tests self-contained and much easier to understand than relying on the magic of a testing framework library (which I tend to hate). Florent -- Florent Guillaume, Nuxeo (Paris, France) Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
On Fri, Apr 07, 2006 at 02:35:57PM +0200, Florent Guillaume wrote:
Paul Winkler wrote:
Using an Item or Folder as your root object for tests works fine except for this one issue, so why not allow that? My feeling is that setting up an app is unnecessary work when you don't need one; for one thing, your test module needs to call Zope2.startup() first; for another, afaict creating a Zope2.app is a couple orders of magnitude slower than creating a SimpleItem.
So maybe more people *should* use makerequest(NotAnApp) ;-)
FWIW usually in my tests I rarely use makerequest.
On the other hand, when testing things that use traversal, it's very common that I have to define a fake root object whose getPhysicalPath returns ('',). It's only a few line, and makes my tests self-contained and much easier to understand than relying on the magic of a testing framework library (which I tend to hate).
I'm of two minds here. Those "only a few lines" add up very quickly. I hate wading through 50 lines of setup for every two lines of actually testing something. On the other hand, it's annoying when the test framework magic does something unexpected. You just can't win. :-( Whipping up my own request-wrapped root object for reuse internally seems like a good solution. -- Paul Winkler http://www.slinkp.com
participants (4)
-
Florent Guillaume -
Paul Winkler -
Stefan H. Holek -
Tres Seaver