| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] type inference/checking test framework #13636
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
Conversation
|
carljm
force-pushed
the
cjm/test-framework
branch
3 times, most recently
from
6df46f5 to
62103f4
Compare
carljm
changed the title
carljm
force-pushed
the
cjm/test-framework
branch
from
62103f4 to
f5ffc61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on parser.rs (the first commit). Mostly nitpicks. In general, the code looks really well written, but I'd love some more doc-comments for some of these methods :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on your second commit. Again, nothing major!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! We should reduce the pulled in dependencies and I suggest moving the tests itself to the red_knot_python_semantic crate. See my inline comment. We should also add a README explaining the test structure.
The one design change that I propose is to make the pragma comments lower case because that's the most commonly used style used in the python community and to rename Type to revealed-type or similar to disambiguate it from type: ignore.
I think it should work as expected but could you add a test for a parenthesized expression:
a = b + (
# comment
5 # revealed-type: Literal[5]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on your third commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on your last commit!
Co-authored-by: Micha Reiser <micha@reiser.io> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
carljm
force-pushed
the
cjm/test-framework
branch
from
69697a1 to
9f900c8
Compare
- move tests to red_knot_python_semantic crate - use strip_prefix to relativize paths - use BTreeMap / custom struct instead of FxOrderMap - provide better context on parse errors (Markdown and Python) - rearrange dependencies in Cargo.toml - don't pull in default features of rstest
carljm
force-pushed
the
cjm/test-framework
branch
from
9f900c8 to
d6ec7b8
Compare
|
I haven't made it through quite all the comments yet, but you're welcome to review the additional commits I've made so far. I haven't modified the initial commits (other than to rebase on main, which was a clean rebase), just added new ones for various review comments. So you should be able to just review the new commits rather than re-reviewing everything. |
|
Ok, now I think all comments are addressed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more nits, but nothing blocking! This is great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more nits, mostly on docs and naming. Feel free to respond/tackle some/all of these as followups after landing the PR, nothing blocking
|
Thanks for all the great review comments! Planning to merge once signal is green. |
Summary
Adds a markdown-based test framework for writing tests of type inference and type checking. Fixes #11664.
Implements the basic required features. A markdown test file is a suite of tests, each test can contain one or more Python files, with optionally specified path/name. The test writes all files to an in-memory file system, runs red-knot, and matches the resulting diagnostics against
Type:andError:assertions embedded in the Python source as comments.We will want to add features like incremental tests, setting custom configuration for tests, writing non-Python files, testing syntax errors, capturing full diagnostic output, etc. There's also plenty of room for improved UX (colored output?).
This is a large PR, but I split the commits carefully for easier review. I'm preferring this over stacked PRs, since the GH UX for stacked PRs is so bad. I encourage reviewers to use the GH feature to narrow review per-commit, for less daunting review.
Test Plan
Lots of tests!
Sample of the current output when a test fails: