Skip to content

test(cuda.core): add note about using multiple conftest modules - #2596

Draft
juenglin wants to merge 1 commit into
NVIDIA:mainfrom
juenglin:tests-agents-md
Draft

test(cuda.core): add note about using multiple conftest modules#2596
juenglin wants to merge 1 commit into
NVIDIA:mainfrom
juenglin:tests-agents-md

Conversation

@juenglin

Copy link
Copy Markdown
Contributor

No description provided.

@juenglin juenglin added this to the cuda.core 1.2.0 milestone Aug 10, 2026
@juenglin juenglin added P2 Low priority - Nice to have test Improvements or additions to tests labels Aug 10, 2026
@juenglin juenglin self-assigned this Aug 10, 2026
@github-actions github-actions Bot added the cuda.core Everything related to the cuda.core module label Aug 10, 2026
@juenglin
juenglin requested a review from rwgk August 10, 2026 21:05
@juenglin
juenglin enabled auto-merge (squash) August 10, 2026 21:08
@github-actions

Copy link
Copy Markdown

@rwgk rwgk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review findings and suggested replacement section (gpt-5.6.-sol)

Overall assessment

The current section appears to contain two main agent-facing instructions:

  1. When adding a nested, per-directory conftest.py, make that test subdirectory a Python package with __init__.py.
  2. Put pytest-discovered fixtures and hooks in conftest.py, and put shared code imported by test modules in tests/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.py shadowing the root conftest.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:

@mdboom

mdboom commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

I'm having a little trouble understanding the rationale. Can you point me to the issue that lead to this?

@juenglin
juenglin marked this pull request as draft August 11, 2026 22:12
auto-merge was automatically disabled August 11, 2026 22:12

Pull request was converted to draft

@copy-pr-bot

copy-pr-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda.core Everything related to the cuda.core module P2 Low priority - Nice to have test Improvements or additions to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants