[Zope-dev] zope.component test isolation (was: Zope test layers, pytest, and test isolation)

[Martin Aspeli]

Hi,

On 25 March 2011 13:17, Jim Fulton <jim at zope.com> wrote:
> On Fri, Mar 25, 2011 at 4:24 AM, Wolfgang Schnerring <ws at gocept.com> wrote:
>> Hello Uli,
>>
>> I've spent quite some time thinking (and partly coding) about the same
>> issues you mention (but didn't feel ready to talk about it here, yet),
>> so I'm glad that maybe we can start thinking about them together
>>
>> I think your email addresses two quite different topics, so I'll split
>> my reply. First up: test support for zope.component. Second part:
>> about the concept of test layers.
>>
>> * Uli Fouquet <uli at gnufix.de> [2011-03-24 01:05]:
>>> Right now we have a problem with pytest integration when it comes to
>>> ZCA setups [...] all tests share the same global ZCA registrations
>>> and changes to the registrations in one test will affect other tests
>>> run thereafter. We have a lack of test isolation.
>>
>> Exactly. This issue has bitten me too in various places, and as far as
>> I know there are no solutions for it, yet.
>
> The classic solution is to start tests with empty registries, or, if
> you're using layers, with some baseline registries.

plone.testing (which is Plone non-specific and will shortly be BSD
licensed) allows for stacking of ZCA registries. It has to do some
ugly hacking to achieve this, since zope.component stores handles to
the global registry in *three* different modules and update them in
weird ways depending on the registry hooking, but it's well tested and
robust now.

See http://dev.plone.org/plone/browser/plone.testing/trunk/src/plone/testing/zca.py
and http://dev.plone.org/plone/browser/plone.testing/trunk/src/plone/testing/zca.txt.

>> What I envision to solve this issue is that test support for
>> zope.component should work the same way as with the ZODB. There, we
>> have a *stack* of Databases (DemoStorages, to be precise) that wrap
>> each other, where each one delegates reads downwards, but keeps writes
>> for itself. So you might have one database for the layer that provides
>> the baseline, and each test (in its setUp) gets its own database where
>> it can do whatever it wants, because it is thrown away in its
>> tearDown.
>>
>> In principle, quite a few of the mechanics to do the same thing with
>> zope.component registries are already there (since a registry keeps a
>> list of base registries it will delegate to when something can not be
>> found in itself). And as Hanno and Godefroid mentioned, plone.testing
>> does something in this direction already. (And, it bears repeating,
>> in its core has no dependencies on Plone or Zope2.)
>
> I like the idea of stacking registries.

plone.testing implements this.

If we could fix zope.component to make the implementation less ugly,
that'd be a big win.

>> But as far as I see, there are issues that plone.testing does not
>> address:
>>
>> 1. I've been going over this stuff with my colleague Thomas Lotze, and
>> we realized that just squeezing in a new registry and bending its
>> bases to the previously active one is not enough for full isolation,
>> since this does not cover *deleting* registrations (one, you can only
>> delete the registration from the precise registry it was created in,
>> and two, in the just-bend-the-bases approach, once you delete a
>> registration, it's gone forever).

Correct, although this is in practice extremely rare. I'd say it's
much better to control setup more carefully and not have to "undo" in
a child layer.

>> I think to provide full isolation, we need to make *copies*. And since
>> zope.component in general supports a chain of registries, we probably
>> need to make copies of each of them.

Copying is very hard. It was my first attempt in plone.testing and
didn't work out well. You need to support pickling of registries for
local/persistent component registries. I cannot begin to tell you how
many weird pickling errors I found and had to work around.

> Is deleting registrations important?  This seems like an odd use case.
> If it's needed, I would suggest starting with a baseline (e.g. stack)
> that doesn't include the component you want to test deleting, then
> adding in setup.

+1

>> 2. zope.component has two entry points, the global site registry and
>> the current registry (getGlobalSiteManager and getSiteManager).
>> The current registry can be anything, or more precisely, you can call
>> getSiteManager.sethook(callable) and provide a callable that returns
>> the current registry.
>>
>> I think to provide test support for zope.component (i. e. generally,
>> at the "library level"), we need to support both entry points.
>
> Why?  Why would someone care about anything other than the current
> effective configuration.

Agree. There is a problem in that provideAdapter() and friends don't
use getSiteManager() - the always use the global site manager. And
there are parts of zope.component that use module level variables
directly, ignoring hooks.

>> The
>> global one is not hard, but the getSiteManager one gets nasty really
>> fast, because of course we have to rely on bending getSiteManager to
>> return the current "test registry"
>
> But as you point out, there's a hook for that.

I don't think you need to do that. plone.testing doesn't, for sure, it
just changes the variable that getSiteManager() looks at.

You do need to be careful about when you set hooks and when you don't.
Again, plone.testing is the result of hours and hours of finding weird
problems, so I'd recommend you don't discard the knowledge there. I
think a best case scenario would be for plone.testing.zca to just be a
delegate for something more robust in zope.component.testing.

>> -- but anyone could at any time
>> call getSiteManager.sethook to change it!
>
> Seriously?  Nobody calls that but deep infrastructure code.

People do call zope.site.hooks.setHooks() sometimes, though, e.g. upon
traversal. The getSiteManager.sethook() functino is of course voodoo
that no-one knows about.

>> Which means we need to
>> intercept that and a) prevent our hook from being replaced and b)
>> inject the new registry into the test stack somehow.
>
> I think you're making this more complicated than it needs to be.

Agree with Jim. Are you going to stop people monkey patching
getSiteManager() too? ;-)

>> As far as I understand, plone.testing sidesteps these issues by only
>> dealing with the global registry, and specially munging the two known
>> cases in the Zope world where getSiteManager is changed (zope.site and
>> five.something).
>>
>> **
>>
>> I'd like to know what people think about this plan.
>> Thomas and I have been over this quite a bit and think it's sound, not
>> overly complicated, and (after we did some experiments) definitely
>> doable. Please do point out stuff we missed. :-)
>
> I think a stack-based approach is very appealing.  I think anything
> more complex is likely to cause more problems than it solves.

Agree. The DemoStorage analogy is a useful one.

Another advantage is that the work is already done. :) I think the
implementation in plone.testing has some warts caused by a lack of
support in zope.component. The number one win for me would be to get
rid of the proliferation of module level variables. Number two would
be the __reduce__ hack to make unpickling of local componenet
registries (which hold a reference to the global registry in their
__bases__) work without monkey patching.

>> I'd very much like to put this functionality into zope.component
>> itself, which of course raises backwards compatibility issues
>> galore,
>
> Not sure why this would have to be backward incompatible, but I'm
> unconvinced that the complexity comes close to being justified by the
> benefit.

I think some tidying of zope.component internals would mean pushing
some of the plone.testing.zca stuff down a level to zope.component
feasible, which would have no BBB issues except perhaps for
plone.testing itself, which we could manage quite easily.

>> but any code for this definitely isn't wasted since we can always
>> package it separately if we don't find a way to integrate it.
>>
>> Thomas and I taken up implementing this, but we can't devote a lot of
>> time to it (about one session per week), so realistically I'm afraid I
>> guess it will take a few months until we have something of substance.
>
> OMG and then who gets to maintain it? Not it!
>
> You and Thomas have obviously thought a lot about this and I
> appreciate the effort you've put into this, but I really don't think
> it's worth the added complexity.

I would suggest a good starting point would be to see if you can make
plone.testing.zca "clean". It should be pretty obvious what is
currently a hack. If you got there, you effectively have stacking of
component registries. plone.testing would be simpler, and
zope.component.testing would be able to provide a simple way to stack
configuration by layer without a plone.testing dependency.

If you ignore the "delete" use case (which I think is a non-use case),
I think this is quite achievable in a sprint, say.

Martin