test(cuda.core): add note about using multiple conftest modules - #2596
test(cuda.core): add note about using multiple conftest modules#2596juenglin wants to merge 1 commit into
Conversation
|
rwgk
left a comment
There was a problem hiding this comment.
Review findings and suggested replacement section (gpt-5.6.-sol)
Overall assessment
The current section appears to contain two main agent-facing instructions:
- When adding a nested, per-directory
conftest.py, make that test subdirectory a Python package with__init__.py. - Put pytest-discovered fixtures and hooks in
conftest.py, and put shared code imported by test modules intests/helpers/.
The current wording leaves important scope and migration questions unanswered. The pytest explanation is technically sound for the one-level example under the repository's default prepend import mode; the main issues are how the requirements are stated and how they relate to the existing test suite.
Findings
P2: The __init__.py rule does not define its scope
cuda_core/tests/AGENTS.md:69 says that a per-directory conftest.py needs an
__init__.py. Read literally, that could include the root
cuda_core/tests/conftest.py, but the root is intentionally different:
cuda_core/tests/__init__.py does not exist, and adding it would change the
root modules to names such as tests.conftest and tests.helpers. That would
break the suite's current bare from conftest import ... and
from helpers... imports.
The rule should say "a conftest.py below the root tests/ directory." For a
deeper path, every intervening directory below tests/ should contain
__init__.py so the full nested path has stable, hierarchical module names.
The root tests/ directory should remain exempt under the current import
layout.
P2: The helper split conflicts with current code and has no migration policy
cuda_core/tests/AGENTS.md:91-94 says that nothing imports conftest.py by
name and that helper functions belong in tests/helpers/. However,
cuda_core/tests/AGENTS.md:81-86 acknowledges that many modules import from
the root tests/conftest.py, and 11 current test modules still do so.
An agent cannot tell whether it should preserve those imports as established
practice, migrate all of them immediately, or clean them up opportunistically
during unrelated work. The section should explicitly label them as legacy and
define an incremental policy. It should also distinguish reusable helpers that
test modules import from private implementation functions used only by a
fixture or hook; the latter can reasonably remain in conftest.py.
P3: The actionable requirements are buried under rationale
The heading at cuda_core/tests/AGENTS.md:69 implies the first requirement,
but approximately 20 lines of mechanism, examples, and consequences precede
the only imperative instruction at cuda_core/tests/AGENTS.md:91.
For an agent instruction file, the requirements should come first as a short,
normative checklist. A separate Why subsection can then preserve the
mechanism and rationale humans need when reviewing exceptions or evolving the
policy. The rationale remains useful to agents too, but it should be
subordinate to an unambiguous contract.
P3: "The name collision" refers to two different problems
The current section discusses both:
- a nested, non-package
conftest.pyshadowing the rootconftest.py; and - duplicate test-module basenames being imported as the same top-level module.
The final sentence at cuda_core/tests/AGENTS.md:94 says the helper split
keeps "the name collision" from mattering, but that split only eliminates
dependence on importing conftest.py. Package markers are still what prevent
duplicate test-module basenames from colliding under prepend mode. The two
problems and their respective mitigations should be named separately.
P3: The patch and the stated rule do not quite align
The PR adds cuda_core/tests/graph/__init__.py, although graph/ currently
has neither a nested conftest.py nor a duplicate test-module basename. The
file is defensible as preparation and makes graph test module names
hierarchical, but it makes the intended policy unclear: is the rule about
directories containing nested conftest.py files, or should every test
subdirectory be a package? If the broader policy is intended, existing
non-package directories such as cuda_core/tests/memory/ need an explicit
explanation or consistent treatment.
Suggested replacement section
The following is intended to replace the section beginning with
## A per-directory conftest.py needs an __init__.py in this PR.
## Shared test support and nested `conftest.py` files
### Rules
- Do not import a `conftest.py` from test modules.
- Put shared pytest fixtures and hooks in the nearest applicable
`conftest.py`. Helper code used only to implement those fixtures and hooks
may remain in the same `conftest.py`.
- Put constants, functions, and classes imported by test modules in
`tests/helpers/`, and import them explicitly from `helpers` or one of its
submodules.
- When adding a `conftest.py` below the root `tests/` directory, add an empty
`__init__.py` to its directory and to every intervening directory below
`tests/`. Do not add `tests/__init__.py`.
- Existing imports from the root `tests/conftest.py` are legacy and may remain
unchanged. Do not add new imports from it. If new code needs one of those
legacy helpers, move the helper to `tests/helpers/` and update all of its
callers in the same change.
### Why
This test suite uses pytest's default `prepend` import mode. In that mode,
pytest walks upward from a test module to find the first directory without an
`__init__.py`, prepends that directory to `sys.path`, and imports the test by
the module name derived from the remaining path.
For example, assume both `tests/memory/test_x.py` and
`tests/memory/conftest.py` exist:
| `tests/memory` | `sys.path` entry | test module name | a legacy `import conftest` finds |
|---|---|---|---|
| no `__init__.py` | `tests/memory` | `test_x` | `tests/memory/conftest.py` |
| with `__init__.py` | `tests` | `memory.test_x` | `tests/conftest.py` |
Packaging a directory that contains a nested `conftest.py` prevents that file
from being imported as the top-level module `conftest`, which preserves the
behavior of remaining legacy imports from the root `tests/conftest.py`.
Hierarchical test-module names also allow different directories to contain
test files with the same basename without causing pytest's "import file
mismatch" error.
Keeping code imported by tests in `tests/helpers/` removes the ambiguity
entirely for new code: pytest discovers fixtures and hooks from `conftest.py`
automatically, while test modules import reusable support code from a stable,
explicit module. The root `tests/` directory remains a non-package because the
current suite imports `helpers` and the legacy root `conftest` as top-level
modules.Supporting pytest guidance
Pytest documents that importing conftest is ambiguous when the file is not
under package scope and recommends either packaging the directory or never
importing from conftest.py:
|
I'm having a little trouble understanding the rationale. Can you point me to the issue that lead to this? |
Pull request was converted to draft
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
No description provided.