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 test_documentation (ReadConflictError) #543

Merged
merged 2 commits into from Jun 23, 2018

Conversation

Projects
None yet
4 participants
@sunew
Contributor

sunew commented Jun 22, 2018

Fixes #470

The issue was the generation of new transaction ids in ZODB.utils.newTid.
For the documentation test only we patch the newTid to use the unpatched time and gmtime methods, in a new testing layer.

from plone.restapi.testing import PLONE_RESTAPI_DX_PAM_FUNCTIONAL_TESTING
from plone.restapi.testing import (
PLONE_RESTAPI_DX_PAM_FUNCTIONAL_TESTING_FREEZETIME,
PLONE_RESTAPI_DX_FUNCTIONAL_TESTING_FREEZETIME

This comment has been minimized.

@lukasgraf

lukasgraf Jun 22, 2018

Member

Could you please split these imports onto seperate lines? That allows people to quickly keep imports sorted by just sorting lines.

This comment has been minimized.

@sunew

sunew Jun 22, 2018

Contributor

yep - I had to make them two-line because of the line length requirement.

I had this, but thought it ugly:

from plone.restapi.testing import (
    PLONE_RESTAPI_DX_FUNCTIONAL_TESTING_FREEZETIME
)
from plone.restapi.testing import (
    PLONE_RESTAPI_DX_PAM_FUNCTIONAL_TESTING_FREEZETIME
)

This comment has been minimized.

@sunew

sunew Jun 22, 2018

Contributor

btw I use isort - it handles the sorting for me ... :)

This comment has been minimized.

@sunew

sunew Jun 22, 2018

Contributor

and I added the standard plone .isort config in #537
makes things easier...

This comment has been minimized.

@lukasgraf

lukasgraf Jun 22, 2018

Member

You can just slap a # noqa at the end of the import. See here for example. Having a as-stupid-as-they-get sort-tool being able to get the job done instead of requiring isort is preferable :)

This comment has been minimized.

@sunew

sunew Jun 22, 2018

Contributor

yep I know # noqa - added.

@@ -49,5 +49,6 @@ Content-Type: application/json
"table_of_contents": null,
"text": null,
"title": "My Document",
"version": "current"
"version": "current",
"versioning_enabled": true

This comment has been minimized.

@lukasgraf

lukasgraf Jun 22, 2018

Member

I'm a bit confused about why these changes in the req/resp files happen here, even though this PR

  • doesn't bump the Plone version
  • doesn't remove the skipTest() from test_documentation.py

This comment has been minimized.

@sunew

sunew Jun 22, 2018

Contributor

I was running it with plone 5.1 - that explains it perhaps.
Currently builds 4 and 5.1 succeeds, while 5.0 fails. (because of documentation artifacts)

This comment has been minimized.

@lukasgraf

lukasgraf Jun 22, 2018

Member

Ah, righ - that's what I would have expected to happen. I would recommend to keep 5.0 as the reference-build for the documentation artifacts (that's defined here, though the comment didn't get updated when the reference build got switched from 4.3 to 5.0).

I would address actually moving to 5.1 (for the default buildout, and the reference build for docs artifacts, and updating them / checking changes) in a separate PR.

@@ -23,8 +23,10 @@
from plone.namedfile.file import NamedBlobImage

This comment has been minimized.

@lukasgraf

lukasgraf Jun 22, 2018

Member

Hmm - the skipTest for Plone >= 5.1 still doesn't get removed in this PR though, no? Is this intentional?

This comment has been minimized.

@sunew

sunew Jun 22, 2018

Contributor

no - I'll look at that now

This comment has been minimized.

@sunew

sunew Jun 22, 2018

Contributor

@lukasgraf what skipTest fo you mean? There is no such thing.
There is

    @unittest.skipIf(not PLONE5, 'Just Plone 5 currently.')
    def test_controlpanels_get_item(self):

but this is not related to 5.1 or not.

This comment has been minimized.

@lukasgraf

lukasgraf Jun 22, 2018

Member

This one, and two more further down - those are the ones that skip the tests that previously failed with the ReadConflict, but only for Plone >= 5.1

@sunew sunew changed the title from Fix test_documentation (ReadConflictError) to WIP: Fix test_documentation (ReadConflictError) Jun 23, 2018

Fix test_documentation (ReadConflictError) - #470
The issue was the generation of new transaction ids in ZODB.utils.newTid.
For the documentation test only we patch the newTid to use the unpatched time and gmtime methods, in a new testing layer.
@coveralls

This comment has been minimized.

coveralls commented Jun 23, 2018

Coverage Status

Coverage decreased (-0.6%) to 95.786% when pulling 05e3bea on fix-documentation-test-on-51 into fb94ad7 on master.

@sunew sunew changed the title from WIP: Fix test_documentation (ReadConflictError) to Fix test_documentation (ReadConflictError) Jun 23, 2018

@lukasgraf

This comment has been minimized.

Member

lukasgraf commented Jun 23, 2018

Awesome, nice work! 👍 🎉 🍰

@tisto I think this one is ready.

@tisto

tisto approved these changes Jun 23, 2018

LGTM. Thank you @sunew!

@tisto tisto merged commit 6df9f76 into master Jun 23, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on master at 96.118%
Details

@tisto tisto deleted the fix-documentation-test-on-51 branch Jun 23, 2018

@sunew

This comment has been minimized.

Contributor

sunew commented Jun 24, 2018

also coredev builds are failing...for instance https://jenkins.plone.org/job/pull-request-5.2/891/consoleFull

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment