search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
246 stars 186 forks source link

Savepoint required in PloneFixture for tests to pass #3467

Closed pbauer closed 2 years ago

pbauer commented 2 years ago

Test are failing unless there is a transaction.savepoint(optimistic=True) after creating the portal (addPloneSite) while the contextmanager with zope.zopeApp() as app: is still active.

Up to now that savepoint was triggered rather accidentally while adding a large js-file (>128 kB) for the logged-in bundle during combine_bundles. The logic for that savepoint is in OFS.Image.File._read_data).

I will remove all the obsolete code (the bundle and combine_bundles stuff). Instead I'll add the saveopint explicitly in PloneFixture.

The question is now: Why do we need a savepoint here? It seems that the connection to the zodb is dropped if the savepoint is not there.

See also: https://github.com/plone/Products.CMFPlone/pull/3420, https://github.com/plone/plone.app.users/pull/107, https://github.com/plone/Products.CMFPlone/pull/3396#issuecomment-1079461434, https://github.com/plone/buildout.coredev/pull/725#issuecomment-877139105

pbauer commented 2 years ago

I created three pull-requests:

I also triggered a build with these three: https://jenkins.plone.org/job/pull-request-6.0-3.9/990

pbauer commented 2 years ago

New build with all four pull-requests: https://jenkins.plone.org/job/pull-request-6.0-3.9/991/

mauritsvanrees commented 2 years ago

The Python 3.9 job has succeeded. I don't expect problems with other Python versions, but let's check anyway. If there is a freak error on 3.7, I do not want to undo four merges.

davisagli commented 2 years ago

Oh my, that sounds like a nightmare to debug. Maybe creating a savepoint holds a reference somewhere that prevents the connection from getting garbage collected, or something like that?

pbauer commented 2 years ago

Oh my, that sounds like a nightmare to debug. Maybe creating a savepoint holds a reference somewhere that prevents the connection from getting garbage collected, or something like that?

Yes, it was quite a ride going from the logged-in bundle (that accidentally triggered the savepoint beacuse it was really large). At one point I checked all events, all subrequests and subprocesses for issues. Duh!

sneridagh commented 2 years ago

For the record. I just stumbled upon this trying to put together the Volto tests using 6.0.0a4 and the official image plone/plone-backend.

This fails:

docker run -i --rm -e ZSERVER_HOST=0.0.0.0 -e ZSERVER_PORT=55001 -p 55001:55001 -e 'ADDONS=plone.restapi==8.22.0 plone.volto==4.0.0a4 plone.rest==2.0.0a5 plone.app.iterate==4.0.2 plone.app.vocabularies==4.3.0 plone.app.robotframework==2.0.0a3 plone.app.contenttypes' -e APPLY_PROFILES=plone.app.contenttypes:plone-content,plone.restapi:default,plone.volto:default-homepage -e CONFIGURE_PACKAGES=plone.app.contenttypes,plone.restapi,plone.volto,plone.volto.cors plone/plone-backend:6.0.0a4 ./bin/robot-server plone.app.robotframework.testing.PLONE_ROBOT_TESTING

Throws a thousand of lines complaining about ZODBtransactions errors. I had to manually state to use plone.app.testing==7.0.0a2 (same workaround @ericof found for local tests).

docker run -i --rm -e ZSERVER_HOST=0.0.0.0 -e ZSERVER_PORT=55001 -p 55001:55001 -e 'ADDONS=plone.restapi==8.22.0 plone.volto==4.0.0a4 plone.rest==2.0.0a5 plone.app.iterate==4.0.2 plone.app.vocabularies==4.3.0 plone.app.robotframework==2.0.0a3 plone.app.testing==7.0.0a2 plone.app.contenttypes' -e APPLY_PROFILES=plone.app.contenttypes:plone-content,plone.restapi:default,plone.volto:default-homepage -e CONFIGURE_PACKAGES=plone.app.contenttypes,plone.restapi,plone.volto,plone.volto.cors plone/plone-backend:6.0.0a4 ./bin/robot-server plone.app.robotframework.testing.PLONE_ROBOT_TESTING

We are using PIP-based approach everywhere now in frontend stack testing, in Volto we use the PIP-based plone/plone-backend images. Might seem innocent, but that means that the KGS for Plone5/6 is different now, so we have to split it up everywhere because of this unfortunate issue and

https://github.com/plone/plone.app.testing/pull/75

So. Could we make plone.app.testing 7 work in both 5 and 6? It's been so many years making the stack work in 5/6 that now because of the deprecation of CMFController now we are going to break it.

/cc @tisto @ericof

mauritsvanrees commented 2 years ago

But plone.app.testing 7.0.0a2 is in the versions.cfg and constraints.txt of 6.0.0a4. So I don't see what is wrong here.

Maybe the plone-backend image overrides the pin with an older one?

And the fix that went in plone.app.testing 7.0.0a2 is not needed on Plone 5, although the fix should work fine there as well.

sneridagh commented 2 years ago

The plone/plone-backend image does not include plone.app.testing so you have to provide it as a dependency (ADDONS env var) in the call, along with the update of other packages:

-e 'ADDONS=plone.restapi==8.22.0 plone.volto==4.0.0a4 plone.rest==2.0.0a5 plone.app.iterate==4.0.2 plone.app.vocabularies==4.3.0 plone.app.robotframework==2.0.0a3 plone.app.testing==7.0.0a2 plone.app.contenttypes'

these ADDONS that will be installed can't be constrained (the purpose of this env var is also to be able to override the existing ones).

So if you do not provide the version, it will simply download the latest one.

One solution could be to include plone.app.testing in the image as well, so you don't have to include it manually. Actually I don't think that would be a problem.

The other is to make v7 compatible with Plone5, so we can use it in both.

@ericof and I are planning to work in an image for testing purposes too during the Beethoven sprint, so we might address it in there already, but nonetheless it's worth to think about it. As said, https://github.com/plone/plone.app.testing/pull/75 didn't help either.

mauritsvanrees commented 2 years ago

ADDONS is used in this line:

  /app/bin/pip install ${ADDONS} ${PIP_PARAMS}

If that lines includes -c https://dist.plone.org/release/6.0.0a4/constraints.txt it should work. You can add it to ADDONS or PIP_PARAMS or the script could explicitly add it on this line. It may interfere with editable installs though.

Alternatively, the docker script could get an option WITH_TESTING, default False. When true, run something like this:

  /app/bin/pip install plone.app.testing plone.app.robotframework -c https://dist.plone.org/release/6.0.0a4/constraints.txt ${PIP_PARAMS}

But what is it that you try to achieve? Is your end goal that you want to use the exact same ADDONS value both on 5.2 and 6.0? Sure, I agree this would be nice. And this does not work because with plone.app.testing you get a too old version for 6.0 and with plone.app.testing==7.0.0a2 you get a too new version for Plone 5.2?

Is it okay to have a different PIP_PARAMS for the two versions? Because if you call the Plone 6 docker with PIP_PARAMS=--pre, you would get the latest alpha, where on Plone 5 without this option you would get the latest stable version. That would work. Or make this the default value for PIP_PARAMS in Plone 6, at least while Plone is in alpha/beta. Various packages may already have an alpha release which works with Plone 6.

As said, https://github.com/plone/plone.app.testing/pull/75 didn't help either.

True. It should not be too hard to only load the CMFFormController zcml when the package is there, and not complain when it isn't. In fact, I was surprised that something like that was not already in there in general. But the last change in plone.app.testing that was relevant for Plone 5 was in late 2020, so it made sense to make a hard break.

BTW, I would not be surprised if we run into the same problem soon with plone.app.robotframework. Plone still uses an ancient robotframework 3.1(latest is 5.0) because 3.2 has breaking changes that we need to adapt to. When running the core Plone robot tests you already see thousands of warnings because of this. This could very well require changes in plone.app.robotframework that we will only do for Plone 6. 'We' could be anyone from the Plone community. I hope both Volto/restapi and Classic UI people will step up for this task, which could be quite small or really big.

Small question: for the packages where you want to use this approach, you are only interested in testing on Python 3, right?

sneridagh commented 2 years ago

If that lines includes -c https://dist.plone.org/release/6.0.0a4/constraints.txt it should work. You can add it to ADDONS or PIP_PARAMS or the script could explicitly add it on this line. It may interfere with editable installs though.

Indeed we can't do it this way...

Alternatively, the docker script could get an option WITH_TESTING, default False. When true, run something like this:

  /app/bin/pip install plone.app.testing plone.app.robotframework -c https://dist.plone.org/release/6.0.0a4/constraints.txt ${PIP_PARAMS}

Could be an option, as said @ericof we are talking about this "testing image". Let's hope we can work on it soon-ish (or during the sprint)

But what is it that you try to achieve? Is your end goal that you want to use the exact same ADDONS value both on 5.2 and 6.0? Sure, I agree this would be nice. And this does not work because with plone.app.testing you get a too old version for 6.0 and with plone.app.testing==7.0.0a2 you get a too new version for Plone 5.2?

We did that during all these years... the "frontend stack" works both in 5 and 6 using the same KGS.

Is it okay to have a different PIP_PARAMS for the two versions? Because if you call the Plone 6 docker with PIP_PARAMS=--pre, you would get the latest alpha, where on Plone 5 without this option you would get the latest stable version. That would work. Or make this the default value for PIP_PARAMS in Plone 6, at least while Plone is in alpha/beta. Various packages may already have an alpha release which works with Plone 6.

That would work only until we release an stable for p.a.testing :(

As said, plone/plone.app.testing#75 didn't help either.

True. It should not be too hard to only load the CMFFormController zcml when the package is there, and not complain when it isn't. In fact, I was surprised that something like that was not already in there in general. But the last change in plone.app.testing that was relevant for Plone 5 was in late 2020, so it made sense to make a hard break.

I thought you issue the breaking because of the CMFFormController deprecation (which is a breaking).

BTW, I would not be surprised if we run into the same problem soon with plone.app.robotframework. Plone still uses an ancient robotframework 3.1(latest is 5.0) because 3.2 has breaking changes that we need to adapt to. When running the core Plone robot tests you already see thousands of warnings because of this. This could very well require changes in plone.app.robotframework that we will only do for Plone 6. 'We' could be anyone from the Plone community. I hope both Volto/restapi and Classic UI people will step up for this task, which could be quite small or really big.

Noted. We will have to prepare for it then (and maybe look for it during the sprint.

/cc @ericof @tisto

Small question: for the packages where you want to use this approach, you are only interested in testing on Python 3, right?

Yes, Python 3 only, there's no other way.

So as summary, I will work with @ericof on this and let you know the outcome. Thanks!

mauritsvanrees commented 2 years ago

We did that during all these years... the "frontend stack" works both in 5 and 6 using the same KGS.

I have never heard of such a KGS. What is in it? Is that only the packages that you have in ADDONS here or is the list bigger?

Other candidates for such a list are:

plone.app.tiles = 4.0.0a1
plone.namedfile = 6.0.0a3
plone.scale = 4.0.0a2

These are Python 3 only, but I intend to keep them working on both Plone 5.2 and 6.0. They all have a tox.ini and Github actions to test those versions. Let me link to the tox.ini files:

We could perhaps setup a Jenkins job, similar to the PLIP jobs, to test such a KGS on 5.2.

True. It should not be too hard to only load the CMFFormController zcml when the package is there, and not complain when it isn't. In fact, I was surprised that something like that was not already in there in general. But the last change in plone.app.testing that was relevant for Plone 5 was in late 2020, so it made sense to make a hard break.

I thought you issue the breaking because of the CMFFormController deprecation (which is a breaking).

Yes, that is correct. The point I tried to make was that I did not mind making this a breaking change because no development for 5.2 was happening in this package. But I have just now made a small PR that would restore 5.2 compatibility if we want it. Please review https://github.com/plone/plone.app.testing/pull/77