-
Notifications
You must be signed in to change notification settings - Fork 765
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
Improve pytest completions and goto def #3727
Comments
Pytest features should support goto definition (like described here: #2576) @gramster had an idea to generate virtual type stubs in memory to 'trick' pyright into thinking the feature has a type Another example: |
Pyright has 'builtin' fixtures. Type completions on these (or shipped stubs) would be nice: https://docs.pytest.org/en/7.2.x/builtin.html cache -- .venv\lib\site-packages_pytest\cacheprovider.py:510 capsys -- .venv\lib\site-packages_pytest\capture.py:905 capsysbinary -- .venv\lib\site-packages_pytest\capture.py:933 capfd -- .venv\lib\site-packages_pytest\capture.py:961 capfdbinary -- .venv\lib\site-packages_pytest\capture.py:989 doctest_namespace [session scope] -- .venv\lib\site-packages_pytest\doctest.py:738 pytestconfig [session scope] -- .venv\lib\site-packages_pytest\fixtures.py:1351 record_property -- .venv\lib\site-packages_pytest\junitxml.py:282 record_xml_attribute -- .venv\lib\site-packages_pytest\junitxml.py:305 record_testsuite_property [session scope] -- .venv\lib\site-packages_pytest\junitxml.py:343 tmpdir_factory [session scope] -- .venv\lib\site-packages_pytest\legacypath.py:302 tmpdir -- .venv\lib\site-packages_pytest\legacypath.py:309 caplog -- .venv\lib\site-packages_pytest\logging.py:491 monkeypatch -- .venv\lib\site-packages_pytest\monkeypatch.py:29 recwarn -- .venv\lib\site-packages_pytest\recwarn.py:30 tmp_path_factory [session scope] -- .venv\lib\site-packages_pytest\tmpdir.py:188 tmp_path -- .venv\lib\site-packages_pytest\tmpdir.py:203 |
Pytest raises exceptions. Message regex might be computable? https://docs.pytest.org/en/7.2.x/reference/reference.html#pytest.raises |
|
Fixtures are basically auto matched parameter names based on the name of a fixture function. At test time the value passed is the result (or yield) of the function call. I believe this is where Graham's idea of generating typestubs might work. Generate a typestub next to the test file with the types filled in based on the fixture return value. More about fixtures:
|
Markers allow for:
|
Switch algorithm to look for test file names first |
TypeEval class changes
|
Npm Run tests results (with Node 16.15.1)
|
Principles of Pyright and Pylance:
Alternative ideas without changing the TypeEvaluator: Idea 1: Second language server (started from a different extension)
Pros:
Cons:
Idea 2: Separate extension that injects middleware in python core extension?
Pros:
Cons:
Idea 3: Outside command that adds types for features as a quick action
Pros:
Cons:
Idea 4: Pyright somehow knows about pytest fixtures
Pros:
Cons:
|
I think the pytest weirdness can be summed up as:
If you were going to codify that it such a way that a static type checker could figure it out, how would you do it? Maybe something like a 'implicit_parameter_types' decorator? def implicit_parameter_types(
function_has_name_pattern: str)
def implicit_parameter_types(
function_has_decorator_type: Type) Then the hard part would be how to apply this to pytest. For fixtures, we could probably just have some mapping between pytest.fixture and one or more of these, but for 'test_' functions, that doesn't work. Users don't do anything except name functions 'test_' Not sure how to apply some rule that a type checker can follow to the 'test_' functions. Is there any precedence for general pattern matching and implying something from it? |
Pytest fixtures are typically not explicitly imported by the test modules that consume them. (This is a terrible design, IMO. I find pytest tests to be incredibly difficult to understand because of this. It's almost impossible to look at the code and determine what's going on in the test.) Because fixtures are not explicitly referenced in the modules in which they're used, the normal dependency-tracking mechanisms in pyright do not apply. What are your thoughts on how to handle this aspect of the problem? If someone modifies a fixture, how will that change be propagated? Are you assuming that each project will have only one pytest configuration? What about mono-repos where there are multiple configurations? Have you thought about how to map specific pytest configurations and fixture sets to specific subtrees in the code base? |
Maybe I missed something, but I thought the pytest fixtures are always either:
The 'conftest.py' file can be anywhere in the workspace AFAIK, so the plan is to just look up the directory tree to find all the conftest.py files. They're added to the 'tracked' file list in the server. I need to reread the docs, but maybe there's other ways to define fixtures too. |
Oh yeah, there's this too. Plugins |
Ah, so fixtures are discovered based on searching for a hard-coded file name ( |
AFAIK, yes. There might be a way with some sort of outside configuration that I don't know about because @bschnurr mentioned something special that Visual Studio does for pytest. Haven't looked into it yet. |
You mentioned that there's a way to invoke a test with a fixture without including a decorator — that pattern matching is also supported. Where is that pattern defined? Is it hard-coded, or is it configurable in some manner? |
Sorry I meant this: def test_foo(request):
pass The fixture has no decorator here because it's a builtin one. Conftest.py fixtures are similar too: # conftest.py
@pytest.fixture
def my_fixture():
return "4" # test.py
def test_stuff(my_fixture)
pass
|
Maybe you meant the pattern I mentioned here?
Pytest thinks anything starting with the string |
Is Obviously, Is it common to rely on I'm trying to get a sense for how much of pytest's internal behaviors need to be assumed here. The problem gets much easier (and the solution less fragile) if we don't need to encode these behaviors. |
AFAICT, a test with a fixture is any test function that takes a parameter other than self.
Test functions don't have any decorators. Fixtures do. If you want to create a new fixture, you add a decorator. That's the hard part I think in trying to distinguish fixtures as parameters. There's nothing other than the pattern of I don't know what we'd tell users other than figure out the types yourself if we don't want pyright to compute them. I did have an idea above (Idea 3: Outside command that adds types for features as a quick action) about having a separate action that does the computation for them and then adds the type annotations for them. That way pyright wouldn't have to know about the types, but that seems crummy to me. |
Looks like 'test_' isn't harcoded either. You can have a pytest configuration file too: https://docs.pytest.org/en/7.2.x/example/pythoncollection.html#changing-naming-conventions |
I think we need a more complete functional spec. It looks like you're discovering requirements on the fly, which makes it difficult to understand the scope and bound of the problem that you're trying to solve. Which pytest functionality do you think is important to support (and which is not), and how does that translate into pyright/pylance features? |
I don't think we're attempting to fully define everything we want to do or all the scenarios it will cover. The goal is to just get an MVP that we can put in front of customers. What do you think is missing? |
Perhaps I should have more examples and what the behavior could be? Like maybe it's hard to imagine what we'd want to have happen for test functions and fixtures. I'll add some gifs to the top outlining what it could look like. |
I'm trying to get a sense for which pytest behaviors need to be understood and replicated in pyright or pylance logic to achieve the desired outcome. Without that understanding, it's difficult to evaluate our technical options. I'm asking basic questions about how fixtures are applied and evaluated, which behaviors are hard-coded vs configurable, etc., and it sounds like you haven't explored those questions. In my experience, a functional spec is a good way to establish the contours of a problem and make sure that everyone has that shared context. For example, looking at the pytest test cases in my team's code base, we often use My understanding of pytest is very incomplete. It sounds like yours is too. We need to obtain a much more complete understanding. Otherwise we risk pursuing a technical approach that falls apart when we discover some new requirement or behavior that needs to be supported. |
A word doc would probably be a better spot to put all of this down. Then I could have stated a bunch of things that I didn't think were going to affect the MVP. I didn't think pytest.mark.usefixtures would affect the features proposed here, but now that I think about it more, the It'd probably be easier to comment on these things there instead of just having my random thoughts on what I thought the MVP should be. And it could certainly be more exhaustive than what I'm listing here. |
Re:
If I'm understanding this suggestion correctly, Graham's suggesting generating *.pyi files and leaning on Pylance picking up completions from the generated PEP-571 stub files? That sounds clever at first glance (simple/elegant) but for navigation I don't think anyone would want code navigation to land them in the type stub file. Just an off-the-hip thought while reading over your notes so far. Re:
If design thoughts steer out of this public thread and into a Word document somewhere, can we follow along somehow? |
That was the idea, but the stubs would be in 'virtual file system'. Goto def would likely not use those stubs and would just find the real .py files (as that's what goto def does today for functions that are both in a pyi and a py file).
The word doc (at the moment) is more about internal architecture required to implement the ideas outlined here. The intent of this issue is to be a first stab at the multiple discussion items, so if the MVP changes, we'll update it here. |
If that's all viable, it sounds like it could really simplify the MVP here. It may also provide unique avenues to separate the "pytest"-specific concerns from this project, if warranted. Is this "generate stubs on the fly" approach used anywhere else in "Pylance/Pyright land" so to speak? Where are the other (Forgive my ignorance on the above two questions, although I've been enjoying using these projects for a while, I haven't tried to dig into their internals yet, but I figured you might know off hand.) Pylance principle #2 in your ideas pros-and-cons list comment seems reasonable in that if baking The |
Not at the moment, no. I didn't end up using it for the prototype either as it was more involved than just injecting code into pyright. You'd have to generate one for every test function, as the user types them. So for example, you have this test func: def test_foo(my_fixture):
assert(my_fixture.length == 2) The stub for that might be something like: def test_foo(my_fixture: list[str]): pass
Some are in the vscode-python extension. Some are in 3rd party extensions.
Interesting idea that we should keep in mind when designing this. If plugins are used a lot, how do we handle them? I don't know if a plugin would be able to tell us the types of fixtures or not. That's what we'd need in order to generate the type stubs. I don't think the plugin could generate the type stubs because it'd be conflicting with the stubs we're generating. |
Did some more thinking about the type stub generation. There's a bit of a problem with the idea. Generated type stubs are like a cache of what type information pyright might generate. Pyright does not currently cache anything in between edits. It recomputes everything on every edit. This is because it's famously hard to update the 'right' parts of the cache. So instead it regenerates as much as necessary after every edit. JIT type evaluation is a core tenet of pyright and why it's so damn fast compared to what we had before. Generating type stubs for test functions would essentially have to be done on every edit. (Same cache hit detection problem). That might be way too slow. And it would have to be done before pyright was asked to compute any types (otherwise pyright might go through some point that those stubs affected). So it would have to block all other operations. That makes it seem like to me as not the way to do this. Granted it might be worth a try just to see how slow it is in practice. All of the types for the predefined fixtures could remain cached (as we can probably assume the user isn't going to update pytest, or we can detect when they do). But types for all user fixtures would have to be transitively computed on every edit. |
Agreed, that looks right. I experimented a bit with it after I wrote my last post and it didn't quite feel as awesome as I thought it would. It's not that we can't inspect, ascertain the types, and generate the stubs, but that like you mention, I think there are some devils in the details including:
For that second point, what I mean is if you are actively editing Edit:
Agreed, the more I looked at it, the less I liked it. And I wouldn't be inclined to sacrifice speed unless it was marginal and truly reduced complexity for maintenance of the feature set. I don't think you're at all off base on your reply above. Also, thanks both to you and @brettcannon for engaging with the community, I think some combination of the continual skilled active development/maintenance coupled with public/open evolution of this editor and its supporting ecosystem (Pylance/etc.) is what makes it a cut above most. Briefly circling back to the extension/injection approach vs. the stub ideas, I may revisit tinker with it a bit if I can get some time to think on it more. I still need to read up on your earlier suggestion about hacking together an implementation off Pyright. Having this group of features will be awesome in day to day work. I totally appreciate @erictraut's cautionary sentiments that it may be hard to scope and get it right, but I do think there's got to be one or more reasonable ways to get it off the ground. It saddens me that PyCharm leads the pack on this feature set at the moment. |
Yeah, I don't see how type stubs help here — for the reasons you mention above (especially point 2). To get intellisense for parameters in any other function, we require that developers provide type annotations for input parameters. Increasingly, Python developers are adopting this habit because they see the benefits not only for completion suggestions but also for static type checking. Pytest unit tests are simply functions, so I would think that we'd want to follow the same pattern here and require developers to add inlined type annotations for parameters. The big difference is that we have some additional type information about the arguments that are likely to be passed to each parameter (by using type inference on the return type of the fixture or parameterization marker). That means we could offer a code action to insert the inferred type as a type annotation for the parameter, saving the developer the extra work. This approach would maintain consistency of experience (since test functions would be like every other function), and it wouldn't violate any of the other core principles that we established with pylance and pyright. In particular, we want parity between diagnostics produced in pylance and in pyright (assuming the two are configured the same). This is critical for developers who want to automate testing & type checking, for example through the use of CI scripts. |
The
In my inline response above I'm assuming by "code action to insert" you mean as I've described above, which has the aforementioned pitfall(s). If you mean to insert the inferred type dynamically as part of IntelliSense despite the test function not specifying a type hint, disregard my above concern. Can you clarify your idea if I misinterpreted it? |
Linking some past related issues in case we make changes that could allow us to revisit them: Discussion: #2576 We've also had this issue opened about 5 times, but it's different to fixture type inference and involves code flow, so not related to 3727. Looks like it was addressed ultimately anyway (so we could potentially go back to some of the older versions and change the resolution). |
This issue has been fixed in prerelease version 2023.2.11, which we've just released. You can find the changelog here: CHANGELOG.md |
Mini-spec:
Example - Hover over a parameter on a test function or a fixture shows the type of the parameter:
Example - Go to def works for parameters for tests and fixtures:
Example - completions are listed in the parameters for a test function or a fixture
Example - error for parameters that don't match an existing fixture:
The text was updated successfully, but these errors were encountered: