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

Use dexterity instead of AT for PloneTestCase #50

Merged
merged 2 commits into from
Jul 3, 2018
Merged

Conversation

pbauer
Copy link
Member

@pbauer pbauer commented Jul 1, 2018

For now this is just a crazy experiment. Porting all tests away from PloneTestCase seems way more work that migrating the layer and fixing the tests. Also we have no replacement for self.publish in the modern testlayers.

…d. Porting all tests away from PloneTestCase seems less work that migrating the layer and fixing the tests.
@coveralls
Copy link

coveralls commented Jul 1, 2018

Coverage Status

Coverage remained the same at ?% when pulling a0d77f9 on use_dexterity into 7d5eb0f on py3.

@pbauer
Copy link
Member Author

pbauer commented Jul 1, 2018

This change increased the number of running test in python 3 by +748 and added 310 failing tests (https://jenkins.plone.org/view/PLIPs/job/plip-py3/463). So in raw numbers that seems like a good idea.

I'm willing to fix all these tests (mosty replace obj.setFoo('bar') with obj.foo = 'bar').
One alternative would be to provide a self.publish method in in plone.app.testing. Another is to migrate all tests that use self.publish to now use restrictedTraverse or the testbrowser.

Opinions or other options?

@pbauer pbauer requested review from davisagli and gforcada July 1, 2018 13:36
@pbauer
Copy link
Member Author

pbauer commented Jul 2, 2018

Some more things:

  • I have started fixing the failing PloneTestCase tests in CMFPlone and it is going pretty good. I will commit some of the first changes to a separate branch today.

  • During fixing the tests I found a couple of issues with Dexterity. Some tests of features that work with AT do not work with DX because the features are different or missing. That is generally good because we want to fix these issues. But: even more work.

Most importantly:

  • When we switch all PloneTestCase tests to use dexterity like this we are reducing the test-coverage of Archetypes in Plone 5.2. The right™ way would be to duplicate all these tests and run them with a new DX-based layer in py2 and py3 and leave the old AT-based test as they are but only run them in py2. I'm no fan of that since it means even more additional work and duplication but doing the other way could mean we break Archetypes on Plone 5.2 with py2 and not find out because the test are only running with DX. We could also move s couple of these tests to ATContentTypes and let them die with it.

Copy link
Member

@gforcada gforcada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 that would be great to merge!

@gforcada
Copy link
Member

gforcada commented Jul 2, 2018

Regarding your comments:

  • I would move to restrictedTraverse rather than adding yet even more indirection with a self.publish method
  • about archetypes coverage... you could see it the other way around: by switching to dexterity we increase the coverage on dexterity which is the default and long term supported content type system, while we decrease the coverage of the dated about-to-get-expired content type. So why would you think this is a bad move? 😄

You are doing a great job! If you need more hands, it might be a good idea to create a tracker ticket to give bite sized chunks of things to port, something that one could do in some free hours during an afternoon or weekend.

I myself tried to fix some failing tests the other day and was a bit let down as there was someone else already working on it as well and it was a bit confusing how thing should get merged and how to actually check that what you are doing is actually improving the situation.

These examples that you are giving now about "just" changing this or that to fix some tests, might be a good idea to have them as these tasks I was mentioning.

@jensens jensens merged commit a0d77f9 into py3 Jul 3, 2018
@jensens jensens deleted the use_dexterity branch July 3, 2018 12:53
pbauer added a commit to plone/Products.CMFPlone that referenced this pull request Jul 3, 2018
@pbauer
Copy link
Member Author

pbauer commented Jul 4, 2018

I just realized that this will break many tests in Archetypes, ATContentTypes and related packages since they now get DX as content. We should create a py2-only testlayer for these Tests exactly like the old PloneTestCase and have all test in AT-packages and ATCTSiteTestCase use that.

@gforcada
Copy link
Member

gforcada commented Jul 4, 2018

@pbauer we can create a layer on those packages to override the changes done here. I guess this is possible, right?

@pbauer
Copy link
Member Author

pbauer commented Jul 5, 2018

I'm not sure if that's possible without changing the imports of PloneTestCase for all tests. And if we need to do that it does not really matter if we overwrite the existing one or use a new one. Luckly many tests already use ATCTTypeTestCase and changing that is easy.

@pbauer
Copy link
Member Author

pbauer commented Jul 5, 2018

I can start working on that next week since I'm currently busy with client-work. So if you want to tackle that please go ahead 😄

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.

4 participants