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 testing of a checkout instead of a released package. #1214

Closed
wants to merge 5 commits into from

Conversation

mauritsvanrees
Copy link
Member

No description provided.

@mauritsvanrees mauritsvanrees requested a review from tisto September 2, 2021 09:51
@mister-roboto

This comment has been minimized.

@mauritsvanrees
Copy link
Member Author

There is no change in actual code, so let's not bother Jenkins with this.

Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

@mauritsvanrees It keeps getting the released version, because in plone-5.2.x.cfg https://dist.plone.org/release/5.2.5/versions.cfg takes precedence over base.cfg:

extends =
base.cfg
https://dist.plone.org/release/5.2.5/versions.cfg

It is necessary to change there too.

Maybe it's good to change in

https://github.com/kitconcept/buildout/blob/5.2/plone-5.2.x.cfg

also, since plone.restapi uses this repository as base.

@tisto
Copy link
Member

tisto commented Sep 2, 2021

@mauritsvanrees we ran into this problem quite a few times already. If I am not mistaken buildout only takes the empty version pin into account on the buildout file that is called via command line. Not in any of the included buildout files. Therefore we need to add this to plone-5.2.x.cfg. I think @wesleybl is correct here.

@wesleybl
Copy link
Member

wesleybl commented Sep 2, 2021

@tisto empty pin works in base.cfg. It isn't working because https://dist.plone.org/release/5.2.5/versions.cfg takes precedence over base.cfg. It's necessary to change the order in extends:

 extends = 
-     base.cfg 
-     https://dist.plone.org/release/5.2.5/versions.cfg 
+     https://dist.plone.org/release/5.2.5/versions.cfg 
+     base.cfg 

@mauritsvanrees mauritsvanrees force-pushed the maurits/actually-test-a-checkout branch from 3a70469 to da73ac5 Compare September 2, 2021 15:46
@mauritsvanrees
Copy link
Member Author

Sorry, I should have checked.
I have added the pin to plone-5.2.x.cfg now. This command shows that it is working now:

$ bin/buildout -c plone-5.2.x.cfg annotate | grep -A1 restapi
...
plone.restapi=
    plone-5.2.x.cfg

Wesley is right though: a possibly better alternative would be to let plone-5.2.x.cfg first extend the 5.2 dist.plone.org versions, and then base.cfg, and add versions in base.cfg. But that might interfere with the standard kitconcept setup and the Makefile, I'm not sure.

Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

Please change the order in extends and put the empty pin in base.cfg. Leaving in the order it is now may cause more pin problems in the future.

Let plone-5.2.x.cfg first extend the default Plone 5.2.5 versions, and then base.cfg.
Let base.cfg contain version overrides, including `plone.restapi =` to unpin ourselves.

Also remove version pin from buildout.cfg which are already the same or newer in Plone 5.2.5.
@mauritsvanrees mauritsvanrees force-pushed the maurits/actually-test-a-checkout branch from da73ac5 to 9d85b86 Compare September 2, 2021 16:27
@mauritsvanrees
Copy link
Member Author

@wesleybl

Please change the order in extends and put the empty pin in base.cfg. Leaving in the order it is now may cause more pin problems in the future.

Done.
I have squashed the commits and force pushed.

Looks like the test for uncommitted changes will fail though, at least this line for the lock token.

We already ignored this when checking if there are any differences at all.
But when there are differences, we still showed *all* differences, including white space,
which makes it hard to see the real problems.
Locally I also get a different 'created' date: '1995-07-31T19:30:00' instead of '1995-07-31T17:30:00',
as I am in GMT+2, even though the tests set the timezone to UTC.
Remaining problem: the token in this same snippet is random.

Also, in a few of the other files, instead of
'plone.dexterity.schema.generated.plone_0_Document'
I get:
'plone.dexterity.schema.generated.plone_5_1630600102_2_680259_0_Document'
I saw these before locally, but assumed it was something in my setup,
because the GH Actions kept passing.
That seems to have been because on old release of plone.restapi was being tested.
Copy link
Member

@wesleybl wesleybl left a comment

Choose a reason for hiding this comment

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

Looks like the test for uncommitted changes will fail though, at least this line for the lock token.

@mauritsvanrees Take a look at:

#1096

You can pin plone.dexterity = 2.9.8 in plone-5.2.x.cfg.

But there may be other errors that were hidden because of the wrong pin.

We need to pin an older dexterity because the new dynamic schemas conflict with the expected output in our http-examples.
See #1096
@mauritsvanrees
Copy link
Member Author

Ah, thanks. I have re-added the plone.dexterity pin, with a comment on why we have it.

Only remaining problem that I see, is the random lock token in the http examples.
We might need to do some normalisation of response.text in the save_request_and_response_for_docs function. That could help for random lock tokens and random schema names.

@wesleybl
Copy link
Member

wesleybl commented Sep 2, 2021

I recommend commenting out the documentation testing in this PR so we can merge. The version problem is more serious than the documentation problem. Fix of documentation can be done in another PR.

@mauritsvanrees
Copy link
Member Author

I recommend commenting out the documentation testing in this PR so we can merge. The version problem is more serious than the documentation problem. Fix of documentation can be done in another PR.

Done. I will start on a separate PR for the documentation.

@wesleybl
Copy link
Member

wesleybl commented Sep 2, 2021

The 7.x.x branch has the same problem.

@wesleybl
Copy link
Member

wesleybl commented Sep 2, 2021

@tisto consider making these changes to

https://github.com/kitconcept/buildout/blob/5.2/plone-5.2.x.cfg

All repositories that use it will have the same problem.

env:
PYTHON_VERSION: ${{ matrix.python-version }}
PLONE_VERSION: ${{ matrix.plone-version }}
# TODO Temporarily commented out.
Copy link
Member

Choose a reason for hiding this comment

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

@mauritsvanrees I am somehow reluctant to merge this PR because we comment out this check here. I know there are quirks but this works reliably for quite a while now. Can we try to fix things step by step? First the checkout, then the normalization?

Copy link
Member

Choose a reason for hiding this comment

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

@tisto as the wrong version was being used, there were errors in the documentation that were "hidden". I think the best thing to do is to merge this PR with the documentation tests commented on and then fix the tests. Using the wrong version is a more serious mistake than not running the documentation tests

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we try to fix things step by step? First the checkout, then the normalization?

No. If we only fix the checkout, we run into test failures for the part that checks if there are differences in the http-examples.

I have already fixed the documentation tests in PR #1215 which builds on the current PR.

Problem is that the follow-up PR changes 146 files, making it virtually impossible to review. That is why I split it into two PRs. But that is optional. You can also ignore the current PR and merge the other, as it includes all commits.

@tisto
Copy link
Member

tisto commented Sep 3, 2021

@mauritsvanrees how did you build plone.restapi? I just did a checkout, ran "make", then amended a test to fail, ran "bin/test" and the test fails. To make sure this is not just test runner magic, I broke code in restapi and ran "bin/test". Tests fail, as expected.

@wesleybl
Copy link
Member

wesleybl commented Sep 3, 2021

@tisto GA uses plone-5.2.x.cfg:

run: buildout -t 10 -c plone-${{ matrix.plone-version }}.x.cfg code-analysis:return-status-codes=True

You have to use it too to see the problem:

./bin/buildout -c plone-5.2.x.cfg

@tisto
Copy link
Member

tisto commented Sep 3, 2021

I figured out what's wrong. I am always running "make" which runs "bin/buildout". You folks run "bin/buildout -c plone-5.2.x.cfg".

tisto pushed a commit that referenced this pull request Sep 3, 2021
* Fix testing of a checkout instead of a released package.

Let plone-5.2.x.cfg first extend the default Plone 5.2.5 versions, and then base.cfg.
Let base.cfg contain version overrides, including `plone.restapi =` to unpin ourselves.

Also remove version pin from buildout.cfg which are already the same or newer in Plone 5.2.5.

* Test no uncommitted doc changes: ignore space at end of line.

We already ignored this when checking if there are any differences at all.
But when there are differences, we still showed *all* differences, including white space,
which makes it hard to see the real problems.

* Updated time of lock in http-examples/workingcopy_baseline_get.resp.

Locally I also get a different 'created' date: '1995-07-31T19:30:00' instead of '1995-07-31T17:30:00',
as I am in GMT+2, even though the tests set the timezone to UTC.
Remaining problem: the token in this same snippet is random.

Also, in a few of the other files, instead of
'plone.dexterity.schema.generated.plone_0_Document'
I get:
'plone.dexterity.schema.generated.plone_5_1630600102_2_680259_0_Document'
I saw these before locally, but assumed it was something in my setup,
because the GH Actions kept passing.
That seems to have been because on old release of plone.restapi was being tested.

* Downgrade to plone.dexterity 2.9.8 again.

We need to pin an older dexterity because the new dynamic schemas conflict with the expected output in our http-examples.
See #1096

* Temporarily comment out running bin/test-no-uncommitted-doc-changes in CI.

See #1214 (comment)

* Revert "Temporarily comment out running bin/test-no-uncommitted-doc-changes in CI."

This reverts commit 6b265ff.

* Test fix: save a stable token in workingcopy_baseline_get.

* Revert "Downgrade to plone.dexterity 2.9.8 again."

This reverts commit def2340.

* Test fix: report stable behavior name in test_documentation_types_document_crud.

plone.dexterity 2.10+ has dynamic schemas, so we get for example plone.dexterity.schema.generated.plone_5_1630611587_2_523689_0_Document as behavior name.
Instead, we save what we got in older dexterity: plone.dexterity.schema.generated.plone_0_Document

* Let save_request_and_response_for_docs call save_request_for_docs.

The first part of save_request_and_response_for_docs was an exact copy save_request_for_docs
(before my change in the previous commit).

* Doc tests: right strip the pretty printed http examples for easier comparison.

This avoids many uninteresting differences.

* Test fix: normalize random port to standard 55001.

This avoids differences when running from the Plone coredev buildout, which uses a random port.

A few differences remain, at least when running the test in Plone 6.
On Plone 5.2, only workingcopy_baseline_get.resp has a difference, and this depends on your timezone.

* black

* Updated all http-examples.

This is a very big diff, but 'git diff -w' is empty.

Note: when you view the diff on github, you can add '?w=1' after the url to ignore whitespace differences.
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Sep 3, 2021
Branch: refs/heads/master
Date: 2021-09-03T21:12:18+02:00
Author: Maurits van Rees (mauritsvanrees) <m.van.rees@zestsoftware.nl>
Commit: plone/plone.restapi@c284b56

Normalize http examples (#1215)

* Fix testing of a checkout instead of a released package.

Let plone-5.2.x.cfg first extend the default Plone 5.2.5 versions, and then base.cfg.
Let base.cfg contain version overrides, including `plone.restapi =` to unpin ourselves.

Also remove version pin from buildout.cfg which are already the same or newer in Plone 5.2.5.

* Test no uncommitted doc changes: ignore space at end of line.

We already ignored this when checking if there are any differences at all.
But when there are differences, we still showed *all* differences, including white space,
which makes it hard to see the real problems.

* Updated time of lock in http-examples/workingcopy_baseline_get.resp.

Locally I also get a different 'created' date: '1995-07-31T19:30:00' instead of '1995-07-31T17:30:00',
as I am in GMT+2, even though the tests set the timezone to UTC.
Remaining problem: the token in this same snippet is random.

Also, in a few of the other files, instead of
'plone.dexterity.schema.generated.plone_0_Document'
I get:
'plone.dexterity.schema.generated.plone_5_1630600102_2_680259_0_Document'
I saw these before locally, but assumed it was something in my setup,
because the GH Actions kept passing.
That seems to have been because on old release of plone.restapi was being tested.

* Downgrade to plone.dexterity 2.9.8 again.

We need to pin an older dexterity because the new dynamic schemas conflict with the expected output in our http-examples.
See plone/plone.restapi#1096

* Temporarily comment out running bin/test-no-uncommitted-doc-changes in CI.

See plone/plone.restapi#1214 (comment)

* Revert "Temporarily comment out running bin/test-no-uncommitted-doc-changes in CI."

This reverts commit 6b265ff8f8dd1ba521f2c2bb0cd628738c136520.

* Test fix: save a stable token in workingcopy_baseline_get.

* Revert "Downgrade to plone.dexterity 2.9.8 again."

This reverts commit def23408cec96e58a7164e7f27d07c2bfacf05a4.

* Test fix: report stable behavior name in test_documentation_types_document_crud.

plone.dexterity 2.10+ has dynamic schemas, so we get for example plone.dexterity.schema.generated.plone_5_1630611587_2_523689_0_Document as behavior name.
Instead, we save what we got in older dexterity: plone.dexterity.schema.generated.plone_0_Document

* Let save_request_and_response_for_docs call save_request_for_docs.

The first part of save_request_and_response_for_docs was an exact copy save_request_for_docs
(before my change in the previous commit).

* Doc tests: right strip the pretty printed http examples for easier comparison.

This avoids many uninteresting differences.

* Test fix: normalize random port to standard 55001.

This avoids differences when running from the Plone coredev buildout, which uses a random port.

A few differences remain, at least when running the test in Plone 6.
On Plone 5.2, only workingcopy_baseline_get.resp has a difference, and this depends on your timezone.

* black

* Updated all http-examples.

This is a very big diff, but 'git diff -w' is empty.

Note: when you view the diff on github, you can add '?w=1' after the url to ignore whitespace differences.

Files changed:
A news/1213.bugfix
M base.cfg
M buildout.cfg
M plone-5.2.x-performance.cfg
M plone-5.2.x.cfg
M src/plone/restapi/tests/http-examples/404_not_found.resp
M src/plone/restapi/tests/http-examples/batching.resp
M src/plone/restapi/tests/http-examples/breadcrumbs.resp
M src/plone/restapi/tests/http-examples/collection.resp
M src/plone/restapi/tests/http-examples/collection_fullobjects.resp
M src/plone/restapi/tests/http-examples/comments_add_root.req
M src/plone/restapi/tests/http-examples/comments_add_sub.req
M src/plone/restapi/tests/http-examples/comments_get.resp
M src/plone/restapi/tests/http-examples/comments_update.req
M src/plone/restapi/tests/http-examples/content_get.resp
M src/plone/restapi/tests/http-examples/content_get_folder.resp
M src/plone/restapi/tests/http-examples/content_patch.req
M src/plone/restapi/tests/http-examples/content_patch_representation.req
M src/plone/restapi/tests/http-examples/content_patch_representation.resp
M src/plone/restapi/tests/http-examples/content_post.req
M src/plone/restapi/tests/http-examples/content_post.resp
M src/plone/restapi/tests/http-examples/contextnavigation.resp
M src/plone/restapi/tests/http-examples/controlpanels_get.resp
M src/plone/restapi/tests/http-examples/controlpanels_get_dexterity.resp
M src/plone/restapi/tests/http-examples/controlpanels_get_dexterity_item.resp
M src/plone/restapi/tests/http-examples/controlpanels_get_item.resp
M src/plone/restapi/tests/http-examples/controlpanels_patch_dexterity_item.req
M src/plone/restapi/tests/http-examples/controlpanels_post_dexterity_item.req
M src/plone/restapi/tests/http-examples/controlpanels_post_dexterity_item.resp
M src/plone/restapi/tests/http-examples/copy.req
M src/plone/restapi/tests/http-examples/copy.resp
M src/plone/restapi/tests/http-examples/copy_multiple.req
M src/plone/restapi/tests/http-examples/copy_multiple.resp
M src/plone/restapi/tests/http-examples/document.resp
M src/plone/restapi/tests/http-examples/event.resp
M src/plone/restapi/tests/http-examples/expansion.resp
M src/plone/restapi/tests/http-examples/expansion_expanded.resp
M src/plone/restapi/tests/http-examples/expansion_expanded_full.resp
M src/plone/restapi/tests/http-examples/file.resp
M src/plone/restapi/tests/http-examples/folder.resp
M src/plone/restapi/tests/http-examples/groups.resp
M src/plone/restapi/tests/http-examples/groups_created.req
M src/plone/restapi/tests/http-examples/groups_created.resp
M src/plone/restapi/tests/http-examples/groups_filtered_by_groupname.resp
M src/plone/restapi/tests/http-examples/groups_get.resp
M src/plone/restapi/tests/http-examples/groups_update.req
M src/plone/restapi/tests/http-examples/history_get.resp
M src/plone/restapi/tests/http-examples/history_revert.req
M src/plone/restapi/tests/http-examples/history_revert.resp
M src/plone/restapi/tests/http-examples/image.resp
M src/plone/restapi/tests/http-examples/jwt_logged_in.resp
M src/plone/restapi/tests/http-examples/jwt_login.req
M src/plone/restapi/tests/http-examples/jwt_login.resp
M src/plone/restapi/tests/http-examples/jwt_login_renew.resp
M src/plone/restapi/tests/http-examples/link.resp
M src/plone/restapi/tests/http-examples/lock.resp
M src/plone/restapi/tests/http-examples/lock_get.resp
M src/plone/restapi/tests/http-examples/lock_nonstealable_timeout.req
M src/plone/restapi/tests/http-examples/lock_nonstealable_timeout.resp
M src/plone/restapi/tests/http-examples/lock_update.req
M src/plone/restapi/tests/http-examples/move.req
M src/plone/restapi/tests/http-examples/move.resp
M src/plone/restapi/tests/http-examples/navigation.resp
M src/plone/restapi/tests/http-examples/navigation_tree.resp
M src/plone/restapi/tests/http-examples/newsitem.resp
M src/plone/restapi/tests/http-examples/principals.resp
M src/plone/restapi/tests/http-examples/querystring_get.resp
M src/plone/restapi/tests/http-examples/querystringsearch_post.req
M src/plone/restapi/tests/http-examples/querystringsearch_post.resp
M src/plone/restapi/tests/http-examples/refresh_lock.resp
M src/plone/restapi/tests/http-examples/registry_get.resp
M src/plone/restapi/tests/http-examples/registry_get_list.resp
M src/plone/restapi/tests/http-examples/registry_update.req
M src/plone/restapi/tests/http-examples/roles.resp
M src/plone/restapi/tests/http-examples/search.resp
M src/plone/restapi/tests/http-examples/search_fullobjects.resp
M src/plone/restapi/tests/http-examples/search_metadata_fields.resp
M src/plone/restapi/tests/http-examples/search_multiple_paths.resp
M src/plone/restapi/tests/http-examples/search_options.resp
M src/plone/restapi/tests/http-examples/sharing_folder_get.resp
M src/plone/restapi/tests/http-examples/sharing_folder_post.req
M src/plone/restapi/tests/http-examples/sharing_search.resp
M src/plone/restapi/tests/http-examples/siteroot.resp
M src/plone/restapi/tests/http-examples/sources_get.resp
M src/plone/restapi/tests/http-examples/translated_messages_object_history.resp
M src/plone/restapi/tests/http-examples/translated_messages_object_workflow.resp
M src/plone/restapi/tests/http-examples/translated_messages_types.resp
M src/plone/restapi/tests/http-examples/translated_messages_types_folder.resp
M src/plone/restapi/tests/http-examples/translation_locator.resp
M src/plone/restapi/tests/http-examples/translations_delete.req
M src/plone/restapi/tests/http-examples/translations_get.resp
M src/plone/restapi/tests/http-examples/translations_link_on_post.req
M src/plone/restapi/tests/http-examples/translations_link_on_post.resp
M src/plone/restapi/tests/http-examples/translations_post.req
M src/plone/restapi/tests/http-examples/translations_post.resp
M src/plone/restapi/tests/http-examples/translations_post_by_id.req
M src/plone/restapi/tests/http-examples/translations_post_by_id.resp
M src/plone/restapi/tests/http-examples/translations_post_by_uid.req
M src/plone/restapi/tests/http-examples/translations_post_by_uid.resp
M src/plone/restapi/tests/http-examples/types.resp
M src/plone/restapi/tests/http-examples/types_document.resp
M src/plone/restapi/tests/http-examples/types_document_get_field.resp
M src/plone/restapi/tests/http-examples/types_document_get_fieldset.resp
M src/plone/restapi/tests/http-examples/types_document_patch_field.req
M src/plone/restapi/tests/http-examples/types_document_patch_fieldset.req
M src/plone/restapi/tests/http-examples/types_document_patch_fieldsets.req
M src/plone/restapi/tests/http-examples/types_document_patch_properites.req
M src/plone/restapi/tests/http-examples/types_document_post_field.req
M src/plone/restapi/tests/http-examples/types_document_post_field.resp
M src/plone/restapi/tests/http-examples/types_document_post_fieldset.req
M src/plone/restapi/tests/http-examples/types_document_post_fieldset.resp
M src/plone/restapi/tests/http-examples/unlock.resp
M src/plone/restapi/tests/http-examples/unlock_force.req
M src/plone/restapi/tests/http-examples/unlock_force.resp
M src/plone/restapi/tests/http-examples/users.resp
M src/plone/restapi/tests/http-examples/users_add.req
M src/plone/restapi/tests/http-examples/users_add.resp
M src/plone/restapi/tests/http-examples/users_anonymous.resp
M src/plone/restapi/tests/http-examples/users_anonymous_get.resp
M src/plone/restapi/tests/http-examples/users_authorized_get.resp
M src/plone/restapi/tests/http-examples/users_created.req
M src/plone/restapi/tests/http-examples/users_created.resp
M src/plone/restapi/tests/http-examples/users_filtered_by_username.resp
M src/plone/restapi/tests/http-examples/users_get.resp
M src/plone/restapi/tests/http-examples/users_unauthorized.resp
M src/plone/restapi/tests/http-examples/users_unauthorized_get.resp
M src/plone/restapi/tests/http-examples/users_update.req
M src/plone/restapi/tests/http-examples/users_update_portrait.req
M src/plone/restapi/tests/http-examples/users_update_portrait_get.req
M src/plone/restapi/tests/http-examples/users_update_portrait_get.resp
M src/plone/restapi/tests/http-examples/users_update_portrait_scale.req
M src/plone/restapi/tests/http-examples/vocabularies.resp
M src/plone/restapi/tests/http-examples/vocabularies_get.resp
M src/plone/restapi/tests/http-examples/vocabularies_get_fields.resp
M src/plone/restapi/tests/http-examples/vocabularies_get_filtered_by_title.resp
M src/plone/restapi/tests/http-examples/vocabularies_get_filtered_by_token.resp
M src/plone/restapi/tests/http-examples/workflow_get.resp
M src/plone/restapi/tests/http-examples/workflow_post.resp
M src/plone/restapi/tests/http-examples/workflow_post_with_body.req
M src/plone/restapi/tests/http-examples/workflow_post_with_body.resp
M src/plone/restapi/tests/http-examples/workingcopy_baseline_get.resp
M src/plone/restapi/tests/http-examples/workingcopy_get.resp
M src/plone/restapi/tests/http-examples/workingcopy_post.resp
M src/plone/restapi/tests/http-examples/workingcopy_wc_get.resp
M src/plone/restapi/tests/test_documentation.py
M test-no-uncommitted-doc-changes.in
@tisto
Copy link
Member

tisto commented Sep 3, 2021

@mauritsvanrees I merged #1215. Will close this one.

@tisto tisto closed this Sep 3, 2021
@tisto tisto deleted the maurits/actually-test-a-checkout branch September 3, 2021 19:13
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.

4 participants