10BC0 Remove some cases of test_union_literal_with_other_type by alexdrydew · Pull Request #8436 · pydantic/pydantic · GitHub
[go: up one dir, main page]
Skip to content

Remove some cases of test_union_literal_with_other_type #8436

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.

Sign up for GitHub

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

Jump to bottom
Closed
alexdrydew wants to merge 1 commit into from
Closed

Remove some cases of test_union_literal_with_other_type #8436

alexdrydew wants to merge 1 commit into from

Conversation

@alexdrydew
Copy link
Contributor
@alexdrydew alexdrydew commented Dec 23, 2023

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

@alexdrydew alexdrydew mentioned this pull request Dec 23, 2023
Merged
4 tasks
@codspeed-hq
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
Contributor
sydney-runkle commented Jan 5, 2024

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
davidhewitt commented Jan 9, 2024

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
alexdrydew commented Jan 9, 2024

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
d AEAD avidhewitt commented Jan 10, 2024

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
davidhewitt commented Jan 10, 2024

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

Reviewers

@davidhewitt davidhewitt Awaiting requested review from davidhewitt

Assignees

No one assigned

Labels

relnotes-ignore Omit this PR from the release notes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alexdrydew @sydney-runkle @davidhewitt
0