fix error on App teardown, somehow ZPublisher doesnt have get_module_info function with arguments #5

Merged
merged 2 commits into from Jan 2, 2012

Projects

None yet

3 participants

@vipod
Member
vipod commented Dec 31, 2011

fix error on App teardown, somehow ZPublisher doesnt have get_module_info function with arguments

@vipod
Member
vipod commented Dec 31, 2011

This is error I was getting before my fix. Not sure, maybe I'm missing something with my fix. I was running tests on plone 3.3.5:

Running choosehelp.theme.testing.ChoosehelpThemeUnitTestsFixture tests:
Tear down choosehelp.theme.testing.ChoosehelpTheme:Integration in 0.000 seconds.
Tear down plone.app.testing.layers.Plone:Integration in 0.000 seconds.
Tear down choosehelp.theme.testing.ChoosehelpThemeFixture in 0.018 seconds.
Tear down plone.app.testing.layers.PloneFixture in 0.348 seconds.
Tear down plone.testing.z2.Startup
Traceback (most recent call last):
File "./bin/instance", line 225, in ?
plone.recipe.zope2instance.ctl.main(
File "/data/work/buildouts/schoel/ch_tests/eggs/plone.recipe.zope2instance-3.6-py2.4.egg/plone/recipe/zope2instance/ctl.py", line 347, in main
c.onecmd(" ".join(options.args))
File "/data/work/buildouts/python/parts/opt/lib/python2.4/cmd.py", line 219, in onecmd
return func(arg)
File "/data/work/buildouts/schoel/ch_tests/eggs/plone.recipe.zope2instance-3.6-py2.4.egg/plone/recipe/zope2instance/ctl.py", line 314, in do_test
zope.testing.testrunner.run(defaults, args)
File "/data/work/buildouts/schoel/ch_tests/eggs/zope.testing-3.6.0-py2.4.egg/zope/testing/testrunner/init.py", line 33, in run
runner.run()
File "/data/work/buildouts/schoel/ch_tests/eggs/zope.testing-3.6.0-py2.4.egg/zope/testing/testrunner/runner.py", line 141, in run
self.run_tests()
File "/data/work/buildouts/schoel/ch_tests/eggs/zope.testing-3.6.0-py2.4.egg/zope/testing/testrunner/runner.py", line 216, in run_tests
setup_layers, self.failures, self.errors)
File "/data/work/buildouts/schoel/ch_tests/eggs/zope.testing-3.6.0-py2.4.egg/zope/testing/testrunner/runner.py", line 349, in run_layer
tear_down_unneeded(options, needed, setup_layers)
File "/data/work/buildouts/schoel/ch_tests/eggs/zope.testing-3.6.0-py2.4.egg/zope/testing/testrunner/runner.py", line 505, in tear_down_unneeded
l.tearDown()
File "/data/work/buildouts/schoel/ch_tests/src/plone.testing/src/plone/testing/z2.py", line 287, in tearDown
self.tearDownApp()
File "/data/work/buildouts/schoel/ch_tests/src/plone.testing/src/plone/testing/z2.py", line 557, in tearDownApp
d = list(ZPublisher.Publish.get_module_info.func_defaults)
TypeError: iteration over non-sequence

@optilude
Member

The patch is sane enough, but I can't really understand why it should be needed. I've not seen this before, either.

Can you debug a bit more? Perhaps this is caused by plone.app.linkintegrity's monkey patch of this method?

@vipod
Member
vipod commented Dec 31, 2011

I did further debug and found that plone.app.linkintegrity patch is not applied in testing environment. But the problem is that I'm using plone.postpublicationhook addon which depends on ZPublisherEventsBackport, which in turn calls linkintegrity's patch on purpose.

What would be the best solution to resolve the issue? I want to use postpublicationhook in my development environment and still be able to add tests using plone.testing.

Possible approach would be to override tearDownApp method in all my testing layers where apply above fix. Any better options?

@optilude
Member

I think your patch is fine, I just wanted to understand why it was needed. Can you please add a comment just to explain this is to work around issues with monkeypatching of that method?

Also, have you signed a Plone contributor agreement?

Martin

@vipod
Member
vipod commented Jan 2, 2012

Ok, note is added and I got Plone contributor agreement. May I do a merge by myself?

@optilude
Member
optilude commented Jan 2, 2012

Please!

@vipod vipod merged commit fcb08d6 into plone:3.x Jan 2, 2012
@davisagli
Member

Hmm, that's weird. Maybe get_module_info had been improperly monkey patched from somewhere else?

Member
vipod replied Jan 3, 2012

In my project I'm using plone.postpublicationhook that depends on ZPublisherEventsBackport, which in turn applies plone.app.linkintegrity's get_module_info patch on purpose. As I understand, patch is done using positional and keywords arguments (_args, *_kw) which makes func_defaults equal to None.

When I'm not using plone.postpublicationhook then plone.app.linkintegrity's patch is not installed in testing environment, so no issues encountered then.

Member

@davisagli, it's patched with something that takes _args and *_kwargs, so the defaults dict is not set properly.

Member

I see. Then I think that should be fixed in p.a.linkintegrity's patch rather than here, since merely skipping the teardown like this could can lead to a really hard to debug problem (tests using a cached db connection from a previously set up test layer)

Member
vipod replied Jan 3, 2012

I can take a look into plone.app.linkintegrity, but this won't completely resolve the problem. I just found that PloneFormGen does similar get_module_info patch with (_args, *_kwargs) arguments. So it'll cause the same issue.

For me it looks like patching code is doing it right using (_args, *_kwargs)-like parameters, trying to stay valid for possible future updates to original get_module_info function. And the main problem is that python's default mutable arguments are not cleared between function calls.

I think that because plone.testing is trying to keep arguments clean, it should try to do it a bit better. Could it be possible to have it remember all default mutable values inside setUpApp and then re-store it in tearDownApp? Or would it be too complicated and excessive?

Member

Well, the fact that Python's default mutable kwargs aren't cleared between calls is actually a feature that get_module_info is trying to take advantage of for performance reasons. Having plone.testing remember the func_defaults of get_module_info and then restore them later won't work when get_module_info has been patched, because it remembers/restores the defaults of the wrapper, not of the original function.

I guess maybe what we need is some convention for storing the original function under a reliable name in the module so that plone.testing can find it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment