[red-knot] Support custom typeshed Markdown tests #15683

sharkdp · 2025-01-23T08:48:24Z

Summary

Add feature to specify a custom typeshed from within Markdown-based tests
Port "builtins" unit tests from infer.rs to Markdown tests, part of [red-knot] port tests in infer.rs to mdtest #13696

Test Plan

Tests for the custom typeshed feature
New Markdown tests for deleted Rust unit tests

sharkdp · 2025-01-23T08:48:51Z

crates/red_knot_python_semantic/src/module_resolver/testing.rs

@@ -73,7 +73,7 @@ pub(crate) struct UnspecifiedTypeshed;
 ///
 /// For tests checking that standard-library module resolution is working
 /// correctly, you should usually create a [`MockedTypeshed`] instance
-/// and pass it to the [`TestCaseBuilder::with_custom_typeshed`] method.
+/// and pass it to the [`TestCaseBuilder::with_mocked_typeshed`] method.


Drive-by fix. Unrelated to this PR.

MichaReiser

Sweet

crates/red_knot_test/src/lib.rs

AlexWaygood

Brilliant!

AlexWaygood · 2025-01-23T10:02:51Z

crates/red_knot_python_semantic/resources/mdtest/mdtest_custom_typeshed.md

+When providing a custom typeshed directory, basic things like `reveal_type` will stop working
+because we rely on being able to import it from `typing_extensions`. The actual definition of
+`reveal_type` in typeshed is slightly involved (depends on generics, `TypeVar`, etc.), but a very
+simple untyped definition is enough to make `reveal_type` work in tests:


heh, that's annoying 😆 but fine for now, I think, since I imagine making reveal_type work even if it's not defined in a custom typeshed would require much more invasive special casing for the function.

Mypy's approach might be interesting to you though -- it by default uses a custom typeshed for its tests, which you can find in https://github.com/python/mypy/tree/master/test-data/unit/lib-stub. The reason for this is largely to improve the runtime of the tests -- the default custom typeshed is very simple, and mypy's tests would be much slower if they all used the much-more-complex "real" typeshed in every test. Mypy's approach has many downsides: there have often been PRs that appear to fix bugs, and pass all tests, but don't actually fix the bug when mypy runs on real user code, because of the difference between mypy's mock typeshed and the real typeshed mypy uses when checking user code. But it also has upsides apart from performance:

Mypy's tests are better isolated from random typeshed changes that cause tests to fail

It's very easy to override a single file or two from mypy's custom typeshed in a test case, without having to provide definitions for reveal_type in every test case.

Overall I'm definitely not advocating for mypy's approach; I think the costs outweigh the benefits given how much faster our tests are than mypy's. But it is an interesting approach.

Mypy's tests are better isolated from random typeshed changes that cause tests to fail

To clarify my understanding: This part we're fairly well insulated from, since we're vendoring specific SHAs of typeshed. We'll get test failures when we update our vendored copy, if there have been incompatible changes — but that's a good thing, since those are real things we have to fix, and CI forces us to fix them before we can commit the typeshed update. Do I have all of that right?

It's very easy to override a single file or two from mypy's custom typeshed in a test case, without having to provide definitions for reveal_type in every test case.

Does mypy's ability to override individual files depend on it using a custom typeshed by default? i.e. I could imagine an additional environment.typeshed_override = true mdtest config that lets us override individual files but retain the rest of our vendored real typeshed.

Mypy's tests are better isolated from random typeshed changes that cause tests to fail

To clarify my understanding: This part we're fairly well insulated from, since we're vendoring specific SHAs of typeshed. We'll get test failures when we update our vendored copy, if there have been incompatible changes — but that's a good thing, since those are real things we have to fix, and CI forces us to fix them before we can commit the typeshed update. Do I have all of that right?

Yes, that's all correct! Our tests are more vulnerable to breaking whenever we do a typeshed sync, and already have broken several times when we've tried to sync typeshed. So mypy's tests are closer to being unit tests, in that they control the input to their tests totally; ours are more like integration tests. But as you say, this makes our tests more representative of how we'll actually type-check our users' code, so it's a good thing!

Does mypy's ability to override individual files depend on it using a custom typeshed by default? i.e. I could imagine an additional environment.typeshed_override = true mdtest config that lets us override individual files but retain the rest of our vendored real typeshed.

That's true, I suppose that could work!

crates/red_knot_python_semantic/src/db.rs

AlexWaygood · 2025-01-23T10:12:00Z

crates/red_knot_python_semantic/resources/mdtest/mdtest_custom_typeshed.md

it took me a second to clock that this was actually an integration test for the feature, as well as documentation 😆

crates/red_knot_test/README.md

crates/red_knot_test/src/lib.rs

crates/red_knot_test/src/parser.rs

AlexWaygood · 2025-01-23T10:26:30Z

I guess this feature again suffers from the problem that, when rendered, you don't see the filenames at all :/

Screenshot

But, not a problem new to this PR, and this PR obviously doesn't have to fix it!

## Summary - Port "deferred annotations" unit tests to Markdown - Port `implicit_global_in_function` unit test to Markdown - Removed `resolve_method` and `local_inference` unit tests. These seem like relics from a time where type inference was in it's early stages. There is no way that these tests would fail today without lots of other things going wrong as well. part of #13696 based on #15683 ## Test Plan New MD tests for existing Rust unit tests.

carljm

This is awesome!

carljm · 2025-01-23T17:01:12Z

crates/red_knot_python_semantic/resources/mdtest/import/builtins.md

+```pyi path=/typeshed/stdlib/typing_extensions.pyi
+def reveal_type(obj, /): ...
+```
+


I wonder if mdtest should just automatically include this in any custom typeshed (if you don't specify your own typing_extensions), so we don't have to specify it in each test?

But I guess there's value in simple, explicit behavior; we can wait and see how painful this is; maybe not too bad if we don't have too many tests using custom typeshed.

(I think ideally we would have at least a few more tests using custom typesheds, so I wouldn't want to make it too painful to write these tests)

Anyway, we already discussed this at #15683 (comment) ;)

I saw that convo but I didn't see any specific proposal there to do something to make this easier. I guess we could do the override-files-instead-of-replace-entirely thing, but I kind of like the idea that we can write tests that more fully control typeshed contents; I think replacing just one file without causing lots of cascading import errors would be hard (though I guess maybe import errors are fine, we won't surface them in typeshed.)

Replacing files is probably also more expensive to implement because it requires reading and copying all vendored files to the memory file system (not hugely expensive but not free).

I do like the idea of an extra environment option that e.g. includes whatever's necessary for reveal_type

* main: [red-knot] MDTests: Do not depend on precise public-symbol type inference (#15691) [red-knot] Make `infer.rs` unit tests independent of public symbol inference (#15690) Tidy knot CLI tests (#15685) [red-knot] Port comprehension tests to Markdown (#15688) Create Unknown rule diagnostics with a source range (#15648) [red-knot] Port 'deferred annotations' unit tests to Markdown (#15686) [red-knot] Support custom typeshed Markdown tests (#15683) Don't run the linter ecosystem check on PRs that only touch red-knot crates (#15687) Add `rules` table to configuration (#15645) [red-knot] Make `Diagnostic::file` optional (#15640) [red-knot] Add test for nested attribute access (#15684) [red-knot] Anchor relative paths in configurations (#15634) [`pyupgrade`] Handle multiple base classes for PEP 695 generics (`UP046`) (#15659) [`pyflakes`] Treat arguments passed to the `default=` parameter of `TypeVar` as type expressions (`F821`) (#15679) Upgrade zizmor to the latest version in CI (#15649) [`pyupgrade`] Add rules to use PEP 695 generics in classes and functions (`UP046`, `UP047`) (#15565) [red-knot] Ensure a gradual type can always be assigned to itself (#15675)

…5712) ## Summary Correcting a small oversight by me (#15683 (comment)).

sharkdp added the red-knot Multi-file analysis & type inference label Jan 23, 2025

sharkdp requested review from carljm, MichaReiser and AlexWaygood as code owners January 23, 2025 08:48

sharkdp commented Jan 23, 2025

View reviewed changes

sharkdp force-pushed the david/mdtest-custom-typeshed branch from dd51df7 to a624bcd Compare January 23, 2025 08:52

MichaReiser approved these changes Jan 23, 2025

View reviewed changes

crates/red_knot_test/src/lib.rs Outdated Show resolved Hide resolved

crates/red_knot_test/src/lib.rs Outdated Show resolved Hide resolved

sharkdp force-pushed the david/mdtest-custom-typeshed branch from a624bcd to 744598b Compare January 23, 2025 08:56

This comment was marked as resolved.

Sign in to view

sharkdp force-pushed the david/mdtest-custom-typeshed branch 2 times, most recently from 89b4134 to 66e3e8b Compare January 23, 2025 09:34

sharkdp mentioned this pull request Jan 23, 2025

[red-knot] Port 'deferred annotations' unit tests to Markdown #15686

Merged

AlexWaygood approved these changes Jan 23, 2025

View reviewed changes

AlexWaygood mentioned this pull request Jan 23, 2025

Don't run the linter ecosystem check on PRs that only touch red-knot crates #15687

Merged

sharkdp added 10 commits January 23, 2025 12:30

[red-knot] Support custom typeshed Markdown tests

b47842e

Remove dbg(…)

60f9bea

Use Program::update_from_settings

4ada2a6

Support stubs like sys/__init__.pyi

100cbc6

Fix clippy suggestions

ebb2c13

Rename to {SearchPathSettings,Environment}::custom_typeshed

7ee5c2c

Add link to VERSIONS file format

194619a

Add link to custom-typeshed test

c7d0688

Module *name*

44c058b

Add link to VERSIONS file format

49fc7ce

sharkdp force-pushed the david/mdtest-custom-typeshed branch from f1e6aaa to 49fc7ce Compare January 23, 2025 11:31

sharkdp merged commit 7855f03 into main Jan 23, 2025
20 checks passed

sharkdp deleted the david/mdtest-custom-typeshed branch January 23, 2025 11:36

carljm reviewed Jan 23, 2025

View reviewed changes

sharkdp mentioned this pull request Jan 24, 2025

[red-knot] Rename TestDbBuilder::typeshed to .custom_typeshed #15712

Merged

sharkdp added a commit that referenced this pull request Jan 24, 2025

[red-knot] Rename TestDbBuilder::typeshed to .custom_typeshed (#1…

fb58a9b

…5712) ## Summary Correcting a small oversight by me (#15683 (comment)).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] Support custom typeshed Markdown tests #15683

[red-knot] Support custom typeshed Markdown tests #15683

sharkdp commented Jan 23, 2025 •

edited

Loading

sharkdp Jan 23, 2025

MichaReiser left a comment

This comment was marked as resolved.

AlexWaygood left a comment

AlexWaygood Jan 23, 2025

dcreager Jan 23, 2025

AlexWaygood Jan 23, 2025

AlexWaygood Jan 23, 2025

AlexWaygood commented Jan 23, 2025

carljm left a comment

carljm Jan 23, 2025

AlexWaygood Jan 23, 2025

AlexWaygood Jan 23, 2025

carljm Jan 23, 2025

MichaReiser Jan 23, 2025

[red-knot] Support custom typeshed Markdown tests #15683

[red-knot] Support custom typeshed Markdown tests #15683

Conversation

sharkdp commented Jan 23, 2025 • edited Loading

Summary

Test Plan

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

AlexWaygood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood commented Jan 23, 2025

carljm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp commented Jan 23, 2025 •

edited

Loading