-
Notifications
You must be signed in to change notification settings - Fork 12
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
Inconsistency between buildout.plonetest and buildout.jenkins test eggs #15
Comments
Just as a side note (maybe) but http://jenkins.plone.org does not use Basically |
@gforcada the |
@maartenkling opinion? |
@idgserpro i think they should be the same and p.a.upgrade should be removed |
@hvelarde opinion? |
@idgserpro the only problem with removing them is that all packages that do not declare feel free to fix that here (starting from the Plone 4.1 configuration) and please post a thread on https://community.plone.org/ informing about this change (tweeting could be nice, also). |
Thanks for the links and more info @hvelarde. Before doing these changes, I decided to do some research and just got to the conclusion that we need more thought. Doing a simple search of buildout.plonetest on github, we got 2,646 references in code. Although lots of them still uses I don't know how much breakage this change is going to cause. We already have some complaints from @giacomos and one from @tisto about buildout.plonetest volatile behavior. I don't think just posting a thread informing about the change is enough. I really think if we're going to follow this approach, we should wait for a new Plone release (since the error "ImportError: No module named CMFPlacefulWorkflow" is what motivated this issue and was fixed in plone/plone.app.upgrade@0da7c6c) or even Plone 5, since it's a brand new major version, and it's expected to have at least some kind of breakage (I know Plone team is working hard to not have the same problem we had in 2>3 transition, but everyone who is upgrading to Plone 5 should know that some adaptation, as effortless as it may be, is going to be needed nonetheless). To @giacomos and @tisto who just got into the discussion, feel free to give your 0.02c about the subject. |
I wouldn't care too much about those statistics; most add-ons must be stable or abandoned; life should go on and making a change like this is not big deal if you just inform the community. buildout.plonetest does what it does, I don't see any issue with it. I use it for testing in dozens of add-ons and it works fine; I never use it in production. |
We were having some problems when testing some packages in our local infrastructure that we would like do discuss here in buildout.plonetest.
When running
bin/buildout -n && bin/test
with our buildouteverything went fine. But when running
bin/buildout -c jenkins.cfg -n && bin/jenkins-test
with our jenkins.cfg:We got "ImportError: No module named CMFPlacefulWorkflow". Although this is fixed in
plone.app.upgrade
in plone/plone.app.upgrade@0da7c6c, we were trying to figure out why the error was only happening when usingjenkins.cfg
and not when using cfgs from buildout.plonetest alone.It seems the inconsistency is that in https://github.com/collective/buildout.plonetest/blob/master/test-4.3.x.cfg we have Plone and plone.app.upgrade in eggs:
...but we don't have them in jenkins.cfg. And adding them to the namespace is really what the fix is about (since Plone has Products.CMFPlacefulWorkflow in setup.py).
This is a tricky subject, when asking @tomgross about "easing" needed imported stuff in plone.app.testing, he convinced us that "the general philosophy of unittests is to start with the minimal possible configuration and add stuff as needed" is the best approach, so we're opening this issue to try solve this inconsistency between plonetest and jenkins. Our questions:
Following the testing principle, we vote for the first, specially because not having it in jenkins was what made it able to detect this dependency problem.
Still don't know the full implications of choosing the first one so we need your support.
The text was updated successfully, but these errors were encountered: