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.

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

[red-knot] More comprehensive is_assignable_to tests #15353

Merged
merged 8 commits into from
Jan 8, 2025
Merged

[red-knot] More comprehensive is_assignable_to tests #15353

merged 8 commits into from
Jan 8, 2025
+368 −81

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Jan 8, 2025 •
edited
Loading

Summary

This changeset migrates all existing is_assignable_to tests to a Markdown-based test. It also increases our test coverage in a hopefully meaningful way (not claiming to be complete in any sense). But at least I found and fixed one bug while doing so.

Test Plan

Ran property tests to make sure the new test succeeds after fixing it.

@sharkdp
[red-knot] More comprehensive is_assignable_to tests
cdcf139 
This changeset migrates all existing `is_assignable_to` tests to a
Markdown-based test. It also increases our test coverage in a hopefully
meaningful way. At least I found (and fixed) one bug while doing so.
@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Jan 8, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 8, 2025 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

AlexWaygood
AlexWaygood reviewed Jan 8, 2025
View reviewed changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Fantastic!

crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/property_tests.rs Show resolved Hide resolved
@AlexWaygood AlexWaygood added the testing Related to testing Ruff itself label Jan 8, 2025
carljm
carljm approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Love it, thank you!!

AlexWaygood
AlexWaygood approved these changes Jan 8, 2025
View reviewed changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Some suggested commentary. But I don't mind if you want to leave it to a followup! (And I'm happy to do the followup.) Feel free to merge!

crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md
static_assert(is_assignable_to(type[Unknown], Meta))
```

## Tuple types
Copy link
Member

Choose a reason for hiding this comment

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

we don't have any gradual tuples in this seciton, and I wonder if we should (e.g. tuple[Any, Literal[2]] should be assignable to tuple[int, int] but not to tuple[int, str]). Doesn't need to be done in this PR, though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that now, and also added tests with gradual types in the union and intersection types sections.

crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
crates/red_knot_python_semantic/resources/mdtest/type_properties/is_assignable_to.md Show resolved Hide resolved
@sharkdp sharkdp force-pushed the david/migrate-assignable_to-tests branch from cd753c6 to 71aab38 Compare January 8, 2025 18:33
@sharkdp sharkdp force-pushed the david/migrate-assignable_to-tests branch from 71aab38 to ffaecb8 Compare January 8, 2025 19:18
@sharkdp sharkdp merged commit beb8e2d into main Jan 8, 2025
21 checks passed
@sharkdp sharkdp deleted the david/migrate-assignable_to-tests branch January 8, 2025 19:25
This was referenced Jan 10, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexWaygood AlexWaygood AlexWaygood approved these changes

@carljm carljm carljm approved these changes

@MichaReiser MichaReiser Awaiting requested review from MichaReiser MichaReiser is a code owner

Assignees
No one assigned
Labels
red-knot Multi-file analysis & type inference testing Related to testing Ruff itself
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sharkdp @carljm @AlexWaygood