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

Remove some cases of test_union_literal_with_other_type #8436

Conversation

alexdrydew
Copy link
Contributor

Change Summary

This PR removes two test cases for pydantic-core integration due to the behavior being changed in pydantic/pydantic-core#1132

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Copy link

codspeed-hq bot commented Dec 23, 2023

CodSpeed Performance Report

Merging #8436 will not alter performance

Comparing alexdrydew:remove_pydantic_core_serialization_integration_test (707be60) with main (19fa86c)

Summary

✅ 10 untouched benchmarks

@sydney-runkle
Copy link
Member

Assigning this to @davidhewitt, as he'll likely review your other PR once he's back from vacation.

Thanks for your contribution!

@davidhewitt davidhewitt added the relnotes-ignore Omit this PR from the release notes. label Jan 8, 2024
@davidhewitt
Copy link
Contributor

I don't completely follow why these test cases are no longer wanted? They still seem to contain correct behaviour.

@alexdrydew
Copy link
Contributor Author

I don't completely follow why these test cases are no longer wanted? They still seem to contain correct behaviour.

This is a case of serialization against Union[int, Literal['False']]. The test cases assume that the result depends on the order of the arguments (False is serialized to 0 if int type is first in the union type arguments list and to "false" otherwise). pydantic/pydantic-core#1132 has conflicting changes: resulting value is now depends only on the type of the serialized value.

Honestly, I am not sure how should I handle this case: pydantic/pydantic-core#1132 can't be merged without failing tests either on pydantic-core main or pydantic main (in the case of tests updated to the new behavior)

@davidhewitt
Copy link
Contributor

I've merged the pydantic-core PR; the integration tests failing is not a blocker but rather just a signal that things are changing and we need to consider their impact.

For now let's leave this one as-is and when we cut a new pydantic-core release and update pydantic main I'll see if we need to pull this commit in.

@davidhewitt
Copy link
Contributor

Right, now that I've opened #8533 I understand what this was changing. The last commit on that PR addresses these items here; rather than remove them I preferred to just update the test parameters. Many thanks again 👍

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