Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with mro in zodb #19

Closed
wants to merge 1 commit into from
Closed

Fix issue with mro in zodb #19

wants to merge 1 commit into from

Conversation

do3cc
Copy link
Member

@do3cc do3cc commented Jan 11, 2015

@mgedmin Thanks for point me to the failing zodb tests.
I am not happy with this solution, but checking for the mro attribute failed, probably because of the python trick to make __ private, and isinstance(layer, type) also failed, maybe because of old style layer classes.
I was also wondering, if it is legal to have an object as a layer, but the docs don't say.
I manually tests zodb with 3.4 and this change, and my tests then ran through

@mgedmin
Copy link
Member

mgedmin commented Jan 12, 2015

Layers don't have to be classes. They're objects with a __bases__ attribute listing base layers. I've been confused by this in the past.

  1. We should have an explicit test of this use-case.
  2. I fear layer.__class__.__mro__ is wrong and we need to do a manual traversal of layer.__bases__.

@do3cc
Copy link
Member Author

do3cc commented Jan 12, 2015

I fear it must be both classes and mro
I fear I fell into a deep pit

@do3cc
Copy link
Member Author

do3cc commented Jan 12, 2015

I have reviewed code and docs a bit more. zope.testrunner keeps it simple for itself.
They do not state if a layer has to be a class, but they state that Layers can be extended giving an example of extending classes via subclassing. http://docs.zope.org/zope.testrunner/index.html#layers
In the implementation, base classes are looked up by going over the elements of the __bases__ property which is a python property. https://docs.python.org/2/library/stdtypes.html#special-attributes
Since it is not part of the zope.testrunner docs, I would say that it is reasonable to expect a layer to be a class like attribute. If one wants to fake it, depending on internal knowledge of how resolution works, one has to suck it up that a new patch release changes internal details and one has to provide a __mro__ method too now.
Sorting by mro is safe because layer setup is done breath-first. __mro__ works the same, mostly.

@mgedmin
Copy link
Member

mgedmin commented Jan 12, 2015

zope.testrunner's documentation says "Layers are generally implemented as classes using class methods" (emphasis mine), which can be read to imply that they don't have to be.

Martin Aspeli describes some downsides of using classes themselves as test layers in this email from 2010.

Some market research:

In short, I don't think zope.testrunner can afford to drop support of object instances as test layers (or require that everyone go implement a __mro__ property on their test layers) at this time, without a painful transition affecting many users.

I suggest that reimplementing the MRO algorithm inside zope.testrunner to compute the MRO from layer.__bases__ is the right thing to do. I volunteer to do it if nobody else steps forward. ;)

mgedmin added a commit to zopefoundation/ZODB that referenced this pull request Jan 12, 2015
@do3cc
Copy link
Member Author

do3cc commented Jan 12, 2015

Just for the record, I volunteered too. For reverting 4.4.5...

@mgedmin
Copy link
Member

mgedmin commented Jan 14, 2015

Oh my, this is a can of worms.

I'm now worrying about the implications of zope.testrunner not using the C3 MRO algorithm when running layer setup.

(AFAICS the optimal layer ordering solution that minimizes the total number of layer setUp/tearDown calls is NP-hard.)

@mgedmin
Copy link
Member

mgedmin commented Jan 14, 2015

#20 is my attempt to fix this by moving forwards rather than backwards. Review would be appreciated!

@mgedmin
Copy link
Member

mgedmin commented Jan 20, 2015

Fixed by #20.

@mgedmin mgedmin closed this Jan 20, 2015
@mgedmin mgedmin mentioned this pull request May 19, 2015
mgedmin referenced this pull request May 19, 2015
Commit 3a68ee9 improved layer sorting
but accidentally dropped support for instance-based layers.  These turn
out to be rather widely used (in ZODB, in zope.app.testing, in
plone.testing etc).

This change fixes the above problem in a different way from PR #6: it
replacing inspect.getmro(layer) with a recursive sort key computation
that traverses layer.__bases__.

It doesn't actually use the C3 algorithm for MRO computation.  I tried
to find an example where that would matter and couldn't.

This commit also adds tests to prevent this from regressing in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants