collective / buildout.plonetest

Testing/development buildouts for Plone
6 stars 12 forks source link

Inconsistency between buildout.plonetest and buildout.jenkins test eggs #15

Open idgserpro opened 9 years ago

idgserpro commented 9 years ago

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 buildout

[buildout]
extends =
    https://raw.github.com/collective/buildout.plonetest/master/test-4.3.x.cfg
    https://raw.github.com/collective/buildout.plonetest/master/qa.cfg
    https://raw.github.com/plone/plone.app.robotframework/master/versions.cfg

everything went fine. But when running bin/buildout -c jenkins.cfg -n && bin/jenkins-test with our jenkins.cfg:

[buildout]
extends =
    buildout.cfg
    https://raw.github.com/plone/buildout.jenkins/master/jenkins.cfg

We got "ImportError: No module named CMFPlacefulWorkflow". Although this is fixed in plone.app.upgrade in https://github.com/plone/plone.app.upgrade/commit/0da7c6cc10279bcbc7b478a349b874231ec0c729, we were trying to figure out why the error was only happening when using jenkins.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:

[test]
recipe = zc.recipe.testrunner
defaults = ['-s', '${buildout:package-name}', '--auto-color', '--auto-progress']
eggs =
    Plone
    plone.app.upgrade
   (...)

...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.

gforcada commented 9 years ago

Just as a side note (maybe) but http://jenkins.plone.org does not use buildout.plonetest nor buildout.jenkins. It's using only cfg files from buildout.coredev itself.

Basically versions.cfg, sources.cfg and core.cfg.

idgserpro commented 9 years ago

@gforcada the buildout.coredev use buildout.jenkins.

idgserpro commented 9 years ago

@maartenkling opinion?

maartenkling commented 9 years ago

@idgserpro i think they should be the same and p.a.upgrade should be removed

idgserpro commented 9 years ago

@hvelarde opinion?

hvelarde commented 9 years ago

@idgserpro Plone and plone.app.upgrade are listed in eggs for historical reason only; I think we can remove them from there and yes, your package should depend on Products.CMFPlone, that's the best practice, IMO.

the only problem with removing them is that all packages that do not declare Products.CMFPlone as a dependency will start failing in tests, but that's probably good at the end.

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).

idgserpro commented 9 years ago

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 http://svn.plone.org/svn/collective/buildout/plonetest/ (and they may change it in the foreseeable future), https://raw.github.com/collective/buildout.plonetest/ still has 1,441 occurences. This is only on github: who knows how many private organizations may be using it.

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 https://github.com/plone/plone.app.upgrade/commit/0da7c6cc10279bcbc7b478a349b874231ec0c729) 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.

hvelarde commented 9 years ago

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.