Tests: make RelativeSession transaction aware. [7.x.x]

[mauritsvanrees]

This should fix some random test failures.
This fixes #1018
Summary of my latest comments there:

• In a functional test layer using zope.testbrowser, a request that changes something in Plone leads to a transaction commit.
• plone.restapi uses RelativeSession instead, and this missed such an integration: changes did not really end up in the database.

It somehow mostly worked so far, but to me that seems luck.

Once approved, I can forward port to master.

[mauritsvanrees]

@jenkins-plone-org please run jobs

[mauritsvanrees]

@jenkins-plone-org please run jobs

[wesleybl]

When I have some time I'll take a look at this PR but I don't think it fixes #1018.

The big question I have is: Why only happens in Python 2?

I don't think commit issues are related to Python 2.

[wesleybl]

@mauritsvanrees as I expect, this PR didn't fix #1018. See:

https://github.com/plone/plone.restapi/pull/1227/checks?check_run_id=3603953265

I'm wondering if these changes are really necessary. The tests in Python 3 work fine. And the tests in Python 2 also work well, apart from the problems of #1018.

I removed the lines:

plone.restapi/src/plone/restapi/tests/test_services_types.py

Lines 55 to 62 in 4bca687

	self.api_session.post(
	"/@types/Document",
	json={
	"factory": "URL",
	"title": "Author url",
	"description": "Website of the author",
	},
	)

and the tests failed as expected.

I tend to think that these changes are not necessary.

[mauritsvanrees]

I tend to think that they are needed and make sense.
Except that, as you say, in the GitHub Actions on 5.2-27 exactly such a failure happens as we try to avoid... So it does not really help. :-(

I think this change can help when you do a api_session.request and then do a check in 'standard' code. For example you make a restapi request to create a document, and then you check if self.portal.document exists. Without this PR, the check will fail. But seemingly no such checks are done in the current tests.

[wesleybl]

For example you make a restapi request to create a document, and then you check if self.portal.document exists. Without this PR, the check will fail. But seemingly no such checks are done in the current tests.

Can you please create a branch and create a test like this, showing that it fails?

[mauritsvanrees]

For example you make a restapi request to create a document, and then you check if self.portal.document exists. Without this PR, the check will fail. But seemingly no such checks are done in the current tests.

Can you please create a branch and create a test like this, showing that it fails?

Just comment out transaction.begin in test_content_post.test_post_to_folder_creates_document:

$ bin/test -t test_post_to_folder_creates_document
...
Error in test test_post_to_folder_creates_document (plone.restapi.tests.test_content_post.TestFolderCreate)
Traceback (most recent call last):
  File "/Users/maurits/.pyenv/versions/2.7.18/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/Users/maurits/community/plone-coredev/5.2/src/plone.restapi/src/plone/restapi/tests/test_content_post.py", line 48, in test_post_to_folder_creates_document
    self.assertEqual("My Document", self.portal.folder1.mydocument.Title())
AttributeError: 'RequestContainer' object has no attribute 'mydocument'

This does not actually use RelativeSession, but uses requests.post directly, with the same effect.

[wesleybl]

Just comment out transaction.begin in test_content_post.test_post_to_folder_creates_documen

I can confirm that by commenting out this line the test fails. I even substitutes to use RelativeSession and it failed too.

Wouldn't these changes make a test include in others? I've had problems committing in a test and objects created in a test appeared in other tests.

It would be nice to do a cleanup of transaction.begin and transaction.commit, which would no longer be needed.

And check on Jenkins.

[wesleybl]

Another thing that should be taken into account:

For performance reasons, wouldn't it be better to call the commit when necessary, as it's being done today, rather than every request?

[mauritsvanrees]

Another thing that should be taken into account:

For performance reasons, wouldn't it be better to call the commit when necessary, as it's being done today, rather than every request?

Every request should already do a commit automatically. But somehow there is discrepancy between what the request sees and what is actually synced to the database. It is rather strange, and failures are not deterministic. We may need to call transaction begin/commit/sync more often, but is hard for me to say when and where.

I see a sync calls transaction.begin and this aborts the current transaction, if any. So this seems not what we would want, but it is what plone.testing is doing.

Maybe we should add a transaction.commit() at the beginning and end of all setUp methods that use functional layers. But it is a guess, and I have put far more hours into plone.restapi testing lately than is healthy, so I don't want to go down another rabbit hole.
Let me close this PR, as it does not actually prevent these failures.