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

Fix random failures on test_services_types #1232

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

wesleybl
Copy link
Member

Fix #1018

The problem is that plone.dexterity 2.10+ now uses the _p_mtime variable in the schema names. See:

plone/plone.dexterity#137

As this variable is related to date/time and the tests are executed very fast, some schema had the same name, when they shouldn't
be. This was causing problems in the tests. So i introduce sleeps in Plone 5.2 with Python 2, so that the schemas don't have the same names.

In Python 3 we don't have this problem because the precision of _p_mtime is higher in Python 3.

plone.dexterity 2.10+ now uses the _p_mtime variable in the schema
names. As this variable is related to date/time and the tests are
executed very fast, some schema had the same name, when they shouldn't
be. This was causing problems in the tests. So we introduce sleeps in
Plone 5.2 with Python 2, so that the schemas don't have the same names.
In Python 3 we don't have this problem because the precision of _p_mtime
is higher in Python 3.
@wesleybl wesleybl requested a review from avoinea September 22, 2021 03:55
@mister-roboto
Copy link

@wesleybl thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@wesleybl
Copy link
Member Author

@jenkins-plone-org please run jobs

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know that p_mtime is more precise on Python 3. That would explain the difference.

The change should be good.
But locally it does not always solve it. I have seen this failure when running these tests a couple of times:

Failure in test test_types_document_patch_properties (plone.restapi.tests.test_services_types.TestServicesTypes)
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_services_types.py", line 218, in test_types_document_patch_properties
    self.assertEqual("[email protected]", response.json().get("default"))
  File "/Users/maurits/.pyenv/versions/2.7.18/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/Users/maurits/.pyenv/versions/2.7.18/lib/python2.7/unittest/case.py", line 506, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: '[email protected]' != None

And this:

Failure in test test_types_document_get_fieldset (plone.restapi.tests.test_services_types.TestServicesTypes)
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_services_types.py", line 139, in test_types_document_get_fieldset
    self.assertEqual("Contact Info", response.json().get("title"))
  File "/Users/maurits/.pyenv/versions/2.7.18/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/Users/maurits/.pyenv/versions/2.7.18/lib/python2.7/unittest/case.py", line 506, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 'Contact Info' != None

But it should hopefully prevent some errors. So for me it is fine to merge.
Maybe try a transaction commit anyway, but we could do this in a new PR.

@wesleybl
Copy link
Member Author

But locally it does not always solve it.

Yes. As it is a time-related issue, I agree that some failures may occur. Machine performance can influence.

But it should hopefully prevent some errors.

Yes. On my machine, with these settings, I was able to run the tests, without -t parameter, several times without error. Since the bug only occurs in Python 2, and on a branch that is primarily for backporting, running these tests locally will be rare. The important thing is that the error doesn't occur (or at least decreases) in the CI. I think the most annoying thing is when someone does a PR on a package that has nothing to do with plone.restapi and Jenkins failure because of this problem plone.restapi.

We can watch Jenkins to see if the crashes have stopped/decreased. If not, we can put more sleeps.

@mauritsvanrees mauritsvanrees merged commit 0954fe9 into 7.x.x Sep 23, 2021
@mauritsvanrees mauritsvanrees deleted the fix_test_services_types branch September 23, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants