Repository navigation
fix pytest goto definition support conftest #3775 - #3830
asukaminato0721 wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This pull request has been imported. If you are a Meta employee, you can view this in D117298097. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
334b78d to
e569239
Compare
This comment has been minimized.
This comment has been minimized.
e569239 to
bc77c67
Compare
This comment has been minimized.
This comment has been minimized.
bc77c67 to
981c052
Compare
This comment has been minimized.
This comment has been minimized.
981c052 to
4dd8dc9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi! Thanks for working on this feature! I'm using Pyrefly with Django and pytest, and go-to-definition support for fixtures defined in I'd be happy to help get this feature merged sooner, whether that's through testing, addressing outstanding issues, or contributing code. Is there anything I could help with to move this forward? |
stroxler
left a comment
There was a problem hiding this comment.
Thanks!
I think this PR has a number of flaws we probably should fix before we merge, based on AI review.
The central issue seems to be that the new conftest navigation target is returned by the shared definition resolver without corresponding cross-file fixture-reference support.
1. [P1] Conftest fallback changes rename targets and loses parameter references
Location: pyrefly/lib/state/lsp/pytest.rs:124–130
Given @pytest.fixture def answer(): ... in conftest.py and def test_thing(answer): assert answer == 42 in a test module, resolving the parameter now returns the conftest function rather than the parameter. This is not limited to go-to-definition: rename, references and document highlighting also consume find_definition.
local_references_from_definition routes an external target through local_references_from_external_definition, which recognizes import/attribute references, not pytest's implicit parameter injection. Consequently the parameter and its body uses disappear from local references/highlighting. Rename can edit the conftest fixture declaration without updating the test's fixture request. With a matching conftest.pyi, rename's default prefer_pyi: true instead selects the stub.
Suggested change: make conftest navigation opt-in for definition/declaration/hover, preserving parameter identity for reference-based operations until cross-file fixture references are implemented. Add rename and highlight tests, including the .pyi case.
Supporting code: state/lsp.rs, functions find_local_references_with_preference, local_references_from_definition, local_references_from_external_definition, and find_global_references_from_definition_impl; server.rs, functions rename and async_find_references_helper.
2. [P2] Direct parametrization is mistaken for fixture injection
Location: state/lsp/pytest.rs:113–115
import pytest
@pytest.mark.parametrize("answer", [42])
def test_value(answer):
assert answer == 42If conftest defines an answer fixture, this parameter now navigates to that unrelated fixture. Pytest excludes directly parametrized arguments from fixture resolution. Previously, in this example with no same-file fixture, navigation retained the parameter definition.
Suggested change: exclude statically identifiable direct parametrization arguments before conftest fallback; preserve fixture lookup for indirect parametrization. Add a negative definition test with a same-named conftest fixture.
3. [P2] Explicit fixture names can select the wrong ancestor
Location: state/pytest.rs:303–305
A child conftest containing @pytest.fixture(name="answer") def child_answer(): ... overrides an ancestor's answer fixture. The new scanner compares the requested name only with the Python function name, skips child_answer, and can navigate to the overridden ancestor instead. Conversely, the Python name can be treated as an available fixture even when name= exposes a different name.
Suggested change: match the effective fixture name, including literal name=, while returning the defining function's source range. Test a renamed child fixture overriding an ancestor.
4. [P2] Imported fixtures are skipped before ancestor fallback
Location: state/pytest.rs:299–305
With the repository root on the import path, a child conftest can expose a fixture using from shared_fixtures import answer. Pytest registers that module attribute, so it overrides an ancestor's answer. This scanner accepts only direct function definitions; it ignores the imported fixture and continues upward to the wrong definition, despite already computing the child's exports.
Suggested change: resolve imported/re-exported fixtures in the nearest conftest before considering an ancestor, following the import to its definition. Add an imported-fixture override test.
5. [P2] Missing matches introduce repeated parsing and unnecessary dependency resolution
Location: state/lsp/pytest.rs:115–123
Each lookup without a same-file fixture discovers candidate conftests, computes their complete export maps, and unconditionally reparses their contents. Built-in fixture names such as tmp_path normally match none of these files, so every candidate is reparsed on every request, including hover's definition lookup. Test-named functions now take this path even without a pytest import.
The discarded get_exports result also resolves wildcard imports: a conftest containing from support import * can demand that dependency's wildcard exports even when this syntactic fixture scan needs none of them.
Suggested change: prefer retained ASTs or cached, module-owned fixture metadata; avoid full export-map resolution merely to access source contents. Resolve imports only as needed for the requested fixture. Cover the missing-match/dependency-absent path with demand or parse-count tests. No timing measurements were made.
Supporting code: export/exports.rs, Exports::exports, specifically wildcard-import resolution.
6. [P2] The import root is not the conftest discovery boundary
Location: state/lsp/pytest.rs:38–40
For a project with search-path = ["tests"], a test at tests/test_main.py can have module name test_main, making root_of(handle.module()) equal to tests. The walk stops there and never sees a repository-root conftest.py, even when pytest loads it. The current new test only exercises a conftest in the test's own directory, so it does not catch this central ancestor-discovery case.
This boundary already existed in the quick-fix helper; reusing it for definition lookup introduces the same limitation into the feature this PR adds.
Suggested change: use an appropriate project/pytest discovery boundary rather than the Python import root, and add parent-conftest tests with an explicit nested search path.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes #3775
Pytest fixture go-to-definition now falls back from same-file fixture lookup to visible conftest.py / conftest.pyi files, walking upward from the test file directory and stopping at the first matching conftest fixture.
The shared conftest discovery is now reused by the fixture annotation quick fix too.
Test Plan