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

A [test] extra? #1121

Open
gforcada opened this issue Apr 15, 2023 · 12 comments
Open

A [test] extra? #1121

gforcada opened this issue Apr 15, 2023 · 12 comments

Comments

@gforcada
Copy link
Contributor

While testing the new pip version that came out today, I noticed that when it installs new packages it explains why, e.g:

Collecting WebOb>=1.2 (from WebTest>=2.0.30->zope.testbrowser->Zope>=5.5->plone.app.upgrade->Plone)

a pip install Plone pulls plone.app.upgrade, which in turn pulls Zope and then zope.testbrowser, etc ...

And that's the topic of this issue: I see that on setup.py in install_requires there is at least zope.testbrowser and zope.testing which I assume to be distributions meant only for testing.

Given that there's at least a docs and a wsgi extras, would it make sense to move at least these two distributions mentioned above to it, possibly(?) others as well? 🤔

Or is there some historical reason not to do so?

@d-maurer
Copy link
Contributor

d-maurer commented Apr 15, 2023 via email

@mauritsvanrees
Copy link
Member

For me it would make sense to create a test extra though. That would be for Zope 6 then. We could create an empty one in Zope 5 as preparation. Or have these dependencies duplicate for now.

@d-maurer
Copy link
Contributor

d-maurer commented Apr 17, 2023 via email

@mauritsvanrees
Copy link
Member

The advantage would be to get rid of packages like zope.testbrowser and WebTest which should not be needed on a production system.

@gforcada
Copy link
Contributor Author

This Testing subpackage might get moved outside of Zope then, thus reducing also the amount of code is being shipped with Zope.

Not sure how much tangled is Testing with the other parts of Zope, or rather how much Testing is being called from within other parts of Zope. Hopefully not so much 🤞🏾

@dataflake
Copy link
Member

"Patches welcome"?

@mauritsvanrees
Copy link
Member

I searched for import Testing and from Testing in all the code that Plone 6 pulls in. It is used a lot in tests, naturally. I saw only one production use:
https://github.com/zopefoundation/Zope/blob/5.8.1/src/Zope2/utilities/zconsole.py#L7

from Testing.makerequest import makerequest

Ah, and that same import is used in plone.recipe.zope2instance as well in some circumstances:
https://github.com/plone/plone.recipe.zope2instance/blob/6.12.0/src/plone/recipe/zope2instance/ctl.py#L673

makerequest.py is small and only needs ZPublisher:
https://github.com/zopefoundation/Zope/blob/5.8.1/src/Testing/makerequest.py
So maybe that could be moved to ZPublisher, with backwards compatibility import staying behind in Testing.

@dataflake wrote:

"Patches welcome"?

That seems the main question here: would a patch to split off Testing and add it in a test extra in Zope be accepted? Sounds like that is the case.

I don't know who will work on this, and when. There is a Beethoven sprint in May in Bonn, that could be a good time, but no promises. But how about the following plan?

In Zope 5:

  • Copy makerequest.py to ZPublisher.
  • Let Testing.makerequest import it from there, keeping it for backwards compatibility.
  • Move Testing to a separate repo.
  • Add Testing in Zope/setup.py both to the install_requires and a new test extra. Then add-ons or core Zope packages can continue to work without change.
  • Change other core Zope packages to either use the Zope[test] extra or explicitly add Testing to their own test extra, so they are ready for the next step.

In Zope 6:

  • Remove Testing from the install_requires list.

These are the affected namespaces:

App/tests
OFS/tests
Products/CMFCore
Products/CMFPlone
Products/CMFUid
Products/DCWorkflow
Products/Five
Products/GenericSetup
Products/MailHost
Products/PageTemplates
Products/PluggableAuthService
Products/PluginIndexes
Products/PythonScripts
Products/Sessions
Products/SiteAccess
Products/SiteErrorLog
Products/Transience
Products/ZCatalog
Products/ZopeVersionControl
ZPublisher/tests
Zope2/App
Zope2/utilities
five/customerize
five/intid
five/localsitemanager
plone/app/content
plone/app/z3cform
plone/app/testing
plone/app/contenttypes
plone/schemaeditor
webdav
zmi

@d-maurer
Copy link
Contributor

d-maurer commented Apr 19, 2023 via email

@dataflake
Copy link
Member

Dieter is right, that is a lot of work just so loading Zope doesn't pull zope.testbrowser and its dependencies. IMHO not worth it.

@icemac
Copy link
Member

icemac commented Jul 10, 2023

I see a benefit from not installing test dependencies (and their dependencies) on a production system, so I support this change request even though it requires changes in many places.
As we can implement it using BBB imports and deprecation warnings there is no need to do most of the changes right now. So I am open to further steps.

@dataflake
Copy link
Member

I'm open to this as well, but don't want to do the work. So my earlier remark, "patches welcome", still stands.

The package name "Testing" is so generic that if it were to be moved outside of Zope I'd either rename it or merge it into another package like zope.testing?

@icemac
Copy link
Member

icemac commented Jul 14, 2023

I'm open to this as well, but don't want to do the work.

Same here.

So my earlier remark, "patches welcome", still stands.
The package name "Testing" is so generic that if it were to be moved outside of Zope I'd either rename it or merge it into another package like zope.testing?

I think we need a new package, as Testing is too specific for Zope 5 and zope.testing should not depend on it. So the first challenge would be to find a good name. Suggestions are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

5 participants