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

Add test for complicated scenario that was producing schema generation errors #7648

Merged
merged 3 commits into from Sep 26, 2023

Conversation

dmontagu
Copy link
Contributor

The test added in this PR fails before #7646, and I think is a fairly minimal example of the issue resolved in that PR.

@dmontagu dmontagu changed the title Add test for complicated scenario that was producing schema generatio… Add test for complicated scenario that was producing schema generation errors Sep 26, 2023
@dmontagu dmontagu added the relnotes-ignore Omit this PR from the release notes. label Sep 26, 2023
@cloudflare-pages
Copy link

cloudflare-pages bot commented Sep 26, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 66e30e6
Status: ✅  Deploy successful!
Preview URL: https://9d738883.pydantic-docs2.pages.dev
Branch Preview URL: https://dmontagu-complex-scenario-te.pydantic-docs2.pages.dev

View logs

first: Annotated[Root1 | Root2, Field(discriminator='kind')]
second: Annotated[Root1 | Root2, Field(discriminator='kind')]

assert Outer.model_json_schema() == {
Copy link
Contributor Author

@dmontagu dmontagu Sep 26, 2023

Choose a reason for hiding this comment

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

Prior to #7646, this test fails to even create the Outer class due to a SchemaError. I added the model_json_schema() call though to make sure the generated JSON schema is also correct (which it appears to be), as an additional layer of protection against other bugs that might get introduced in the future with this complex of a model.

@adriangb adriangb enabled auto-merge (squash) September 26, 2023 18:30
@dmontagu dmontagu enabled auto-merge (squash) September 26, 2023 18:44
Copy link

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround!

@dmontagu dmontagu merged commit 9f54ebd into main Sep 26, 2023
58 checks passed
@dmontagu dmontagu deleted the dmontagu/complex-scenario-test branch September 26, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-ignore Omit this PR from the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants