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

Use checkout on 5.2, normalise http-examples [7xx] #1218

Merged
merged 12 commits into from
Sep 14, 2021

Conversation

mauritsvanrees
Copy link
Member

This is a backport of my PR #1215 to branch 7.x.x.
It also updates 5.2.x to use Plone 5.2.5, including using the newer plone.dexterity. We normalise its unpredictable behavior schema names in the http examples.

This PR is best viewed with ?w=1 at the end of the url, to ignore whitespace changes, especially with the commit that updates the http-examples, which is actually a whitespace-only change.

This was already done correctly for the other Plone versions.
Done for all versions in the same way now:
first extend the dist.plone.org versions and then extend base.cfg, which unpins plone.restapi.

Also: test with latest Plone 5.2.5.
And remove pins that are already in 5.2.5 with a good version.
When writing the http-examples, use the original simpler generated schema name.
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).
…mparison.

This avoids many uninteresting differences.
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.
These are only whitespace changes.
@mister-roboto
Copy link

@mauritsvanrees 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!

@mauritsvanrees
Copy link
Member Author

@jenkins-plone-org please run jobs

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.

I left some comments.

src/plone/restapi/tests/test_documentation.py Show resolved Hide resolved
src/plone/restapi/tests/test_documentation.py Outdated Show resolved Hide resolved
This is no longer needed, since we update normalize the http-examples output in test_documentation.py.
Also, without hardcoded port, it becomes possible to introduce tox and run it in parallel, if we want.
…expected name.

Uglier, but closer to reality.
code-analysis did not complain, so I assumed it was fine, but apparently not.
Comment on lines 544 to 546
document_schema_re = re.compile(
r"^plone.dexterity.schema.generated.plone_.*_Document$"
)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can be less invasive here. That is, replacing only the really dynamic part. I suggest:

Suggested change
document_schema_re = re.compile(
r"^plone.dexterity.schema.generated.plone_.*_Document$"
)
document_schema_re = re.compile(
r"^plone.dexterity.schema.generated.plone_5_\d*_2_\d*_0_Document$"
)

So, if we have changes like what happened in plone.dexterity 2.10+, the tests will fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would we do this?
I have no idea if the 5, 2 and 0 are always here or if they can differ as well. Seems pretty random to me.
This is an internal dexterity detail. If it changes, I do not want these tests to fail.

Copy link
Member

@wesleybl wesleybl Sep 14, 2021

Choose a reason for hiding this comment

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

They will only change if we have structural changes like the ones that occurred on plone.dexterity 2.10. They don't change "via parameter". In this case, I find it valid for the test to fail, as failed. If we had this regular expression, before the change from plone.dexterity 2.10, the tests would not fail, even with the name pattern changing. And I think that in this case it should fail.

schemaName after this line:

https://github.com/plone/plone.dexterity/blob/35a12ab726b76ee5527ee467974917bc1fbdca0c/plone/dexterity/fti.py#L262

it's always something like:

plone_5_1631640310_2_6213937_0_Document

Copy link
Member Author

Choose a reason for hiding this comment

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

I see the 5 and 0 come from the SchemaNameEncoder.
I see no value in having a test in plone.restapi fail when this class changes.

Copy link
Member

Choose a reason for hiding this comment

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

We can have more similar values to the real ones in the documentation. I think that's why we have testing to generate documentation, rather than just writing the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine, since no one stepped up to argue in my favour, I have changed it: commit 8921bec.

I think it is fine to merge now, right? Well, once everything is green.

@mauritsvanrees
Copy link
Member Author

@jenkins-plone-org please run jobs

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.

It would be nice if the changes only made on this branch are also made on the master.

@mauritsvanrees mauritsvanrees merged commit 9d8d0df into 7.x.x Sep 14, 2021
@mauritsvanrees
Copy link
Member Author

It would be nice if the changes only made on this branch are also made on the master.

Will do.

@mauritsvanrees mauritsvanrees deleted the maurits/use-plone-restapi-checkout-52-7xx branch September 14, 2021 20:33
@mauritsvanrees
Copy link
Member Author

Done: #1226

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.

5 participants