Issue #130: pytest.ini was dead config shadowing pyproject.toml - #134
Merged
Conversation
Step 4 of the #130 plan requires `pytest tests/ --co` to report zero collection errors before --strict-markers can be turned on. Measuring the baseline surfaced six that had been invisible while pytest.ini shadowed pyproject.toml and nothing was configured: * tests/real_world/test_container_registry_comprehensive.py:571 put a backslash inside an f-string expression. That is a SyntaxError on every Python before 3.12, and this project declares requires-python >=3.8, so the module could not be parsed at all. Hoisted the split() out. * Five modules under tests/comprehensive/ and tests/ import numpy or pandas at module scope to exercise array/dataframe serialization, but neither was declared in any extra. `pip install -e ".[dev]"` therefore produced a tree that could not be collected. Added both to the dev extra. Collection of `tests/ -m "not real_world"` goes from 1214 collected with 6 errors to 1275 collected with 0 errors. tests/unit/ still 70 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
TDD red step. All four fail on master, which is the point: they encode the bug described in #130. Nothing in pytest warns when one config file shadows another, so the only way to detect it is to assert on which file was actually selected. The tests check, in order: the loaded config is pyproject.toml; no higher-precedence config file (pytest.ini/tox.ini/setup.cfg) exists in the tree at all; every marker the suite uses is registered; and --strict-markers is live. test_strict_markers_is_active skips when the run passes -o addopts=, since --strict-markers reaches pytest through addopts and clearing it is a legitimate measurement technique -- config.option.override_ini reports the override, verified against pytest 8.4.2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
Step 1 of the #130 plan, and it must land before any config change. The #109 guard refuses runs that explicitly target tests/integration. It read `config.args`, which looks like "what the user asked for" but is not: when no path is given on the command line, pytest populates `config.args` from `testpaths`. pyproject.toml's testpaths listed tests/integration, so the instant the project's config became effective, a bare `pytest` matched the guard and aborted the whole suite. Verified before this change: $ mv pytest.ini /tmp/ && pytest --co -q ERROR: Refusing to run 'tests/integration': ... `config.invocation_params.args` is the real argv, so the guard now fires on explicit targeting only. Entries beginning with "-" are skipped, since that list contains flags as well as paths. Added test_bare_pytest_is_not_refused_by_the_guard to cover the case that regressed. The existing suite ran pytest in a subprocess but always passed a path, so it could not have caught this; _collect_integration now takes a NO_TARGET sentinel to omit the path argument entirely. All #109 guarantees re-verified: naming the directory or a file in it is still refused, `tests/ -m "not real_world"` still collects zero tests/integration node IDs. tests/unit/test_billable_safety.py 56 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
…nfig Step 2 of the #130 plan. pytest.ini opened with [tool:pytest], a section header valid only inside setup.cfg. In a file named pytest.ini pytest wants [pytest], so every setting in it was ignored -- but pytest had still *selected* it as the config file, and having selected one it stops searching. pyproject.toml's [tool.pytest.ini_options] was therefore never read either. Two config files, zero effective configuration. Deleting pytest.ini leaves pyproject.toml as the single source, which is the modern convention and the file the rest of the tooling already uses. Its contents are preserved in the #130 issue body and recoverable with `git show master:pytest.ini`. testpaths is set to ["tests"] rather than the previous ["tests/unit", "tests/integration"]: naming the integration directory here makes a bare `pytest` resolve its targets to billable tests. The directory stays excluded from collection by collect_ignore_glob in tests/integration/conftest.py regardless (#109). $ pytest --co 2>&1 | grep configfile configfile: pyproject.toml $ pytest --co -q | tail -1 1670 tests collected Also corrects the docstring of test_bare_pytest_is_not_refused_by_the_guard to state what it measurably catches. Undoing either the argv fix or the testpaths fix alone leaves bare pytest working; the test fails only when both are undone, so it is the last line of defence, not the first. The four-case table is recorded in the docstring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
Step 3 of the #130 plan. Must land before --strict-markers, since strict mode turns each undeclared marker into a hard collection error. pyproject.toml declared 4 markers; the suite uses 7. The three missing ones were in real use and silently inert: dartmouth_network 11 uses across 4 files expensive 5 uses across 5 files performance 4 uses across 4 files `expensive` is the notable one: tests/integration/conftest.py applies it programmatically so that an opted-in operator can select `-m "not expensive"`, and that selector could not have worked while the marker was unregistered. Removed the single `@pytest.mark.cleanup` in test_kubernetes_performance_benchmarks.py. It was declared in neither config file, had exactly one use, and conferred no behaviour. The test itself is unchanged and still runs. Confirmed with the author. $ pytest --markers | grep -cE '^@pytest.mark.(real_world|slow|unit|integration|expensive|dartmouth_network|performance):' 7 The audit that produced this list is recorded as a comment above the markers block, and tests/unit/test_pytest_config.py asserts the seven stay registered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
Step 4 of the #130 plan, deliberately last: strict mode is only safe once every marker in use is registered (Step 3) and the tree collects cleanly. Dry run before enabling: $ pytest tests/ --co -q -o addopts="--strict-markers" | tail -1 1670 tests collected # 0 errors After enabling, an unregistered marker is a hard error rather than a PytestUnknownMarkWarning nobody reads: $ printf 'import pytest\n@pytest.mark.bogus_xyz\ndef test_x(): assert True\n' > tests/unit/test_probe_tmp.py $ pytest tests/unit/test_probe_tmp.py --co -q ERROR ... Failed: 'bogus_xyz' not found in `markers` configuration option Left a note on addopts about the #110 trap: adding "-n" here requires moving pytest-xdist from the [test] extra into [dev] first, or `pip install -e ".[dev]"` produces an environment where pytest cannot start at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
MIGRATION.md quoted a [tool.pytest.ini_options] block that no longer matches the file, and gave no hint that the block had never been in effect. Updated to the real contents and added a note recording why pytest.ini was removed and why testpaths must not name tests/integration. .claude/commands/testing/prime.md declared `config_file: pytest.ini`, which no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
The script's docstring promises it verifies "GitHub Actions won't fail", but its test step was a bare `pytest` while CI runs `pytest tests/unit/ -m "not real_world"`. That mismatch was invisible for as long as the project had no effective pytest config: pytest.ini shadowed pyproject.toml and neither applied, so `testpaths` did nothing. With the config live, a bare `pytest` resolves its targets from testpaths and runs all 1670 tests -- including the 224 in tests/real_world/, which open live SSH and cloud connections. The gate stopped terminating. Real-world tests are run deliberately through scripts/run_real_world_tests.py, which is what the pre-push hook uses. After this change black, mypy and the tests all pass. flake8 still fails, on 92 findings that predate this branch (91 on it -- the f-string fix removed one). The local invocation omits the `--exit-zero` that CI applies to the identical command, so this gate has never been green on master. Filed as #133 rather than folded in here, since it includes a genuine unrelated bug: test_direct_gpu_detection.py builds a remote program inside an f-string without escaping its braces, so it raises NameError before the subprocess starts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
Running the full suite while diagnosing the pre_push_check hang showed the benchmark tests in tests/real_world/ write performance_test_results/ into the repo root, which was not ignored. coverage_detailed_report.txt was untracked for the same reason. Notes record the measured before/after, the two things the handoff plan did not anticipate, and what was deliberately left open (#133, #110, #117). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
6 tasks
…n claims
Red-teaming the branch found that my Step 1 change traded a false refusal
for two real money-safety holes. Verified against master, which refused
both:
PYTEST_ADDOPTS=tests/integration/test_x.py pytest -> collected 2
pytest -o testpaths=tests/integration/test_x.py -> collected 2
Cause: `config.invocation_params.args` is only what the operator typed.
`testpaths`, `-o testpaths=` and PYTEST_ADDOPTS all feed `config.args`
without appearing there. Reading the typed argv makes the guard blind to
every indirect route.
The right fix was in the config, not the guard. Reading `config.args` is
safe now precisely because Step 2 set `testpaths = ["tests"]`, so the
guard is back on the effective target list. Also fixed, and pre-existing
(#109): relative paths were resolved only against rootdir, so from within
tests/ the path `integration/test_x.py` went unrecognised. All plausible
bases are now tried. `--pyargs` and `-p` address modules by dotted name and
never looked like paths; both are now translated and checked.
Measured after the change -- refused: explicit dir, explicit file, node id,
-o testpaths, PYTEST_ADDOPTS, --pyargs, cwd-relative. Still allowed and
must be: bare `pytest` (0 integration nodes collected) and the
CLUSTRIX_ALLOW_BILLABLE=1 opt-in. `-p` cannot be blocked before the import
it triggers -- no conftest hook runs that early -- but the run is now
refused before any test executes, which is where the cost is; it also fails
on its own here, since tests/ is not a package.
Four parametrized regression tests cover the indirect routes. Three fail
against the old guard, confirming they are not decorative.
Corrections to my own earlier work in this PR:
* "the 224 in tests/real_world/" was wrong and had been committed as a
code comment in scripts/pre_push_check.py. 224 counts decorator lines;
tests/real_world/ collects 388 tests. That comment's rationale was also
wrong: the old bare `pytest` was not "harmless while no config applied",
it aborted on master in seconds with a collection error and gated
nothing. Fixing that SyntaxError is what let collection succeed and made
the runtime problem visible.
* test_no_shadowing_config_file_exists forbade tox.ini and setup.cfg.
pytest checks those *after* pyproject.toml, so neither can shadow it --
verified. The test also failed on an ordinary pytest-free setup.cfg
holding flake8 config. Now limited to pytest.ini/.pytest.ini, and it
checks the repo tree rather than the resolved rootdir.
* test_bare_pytest_is_not_refused_by_the_guard described the guard source
that no longer exists; rewritten to state the actual invariant.
* coverage_detailed_report.txt was removed from .gitignore: nothing in the
repo writes it, so the justification given was unsupported.
Unrelated CI fix found while verifying: .github/workflows/fast_ci.yml
installs pytest without pytest-timeout, then runs pytest with
--timeout=60. It exits 4 with "unrecognized arguments" and has been
failing on every run, including on master. Added the plugin.
Restored the `filterwarnings` block from the deleted pytest.ini, which was
never migrated. It only suppresses third-party paramiko/cryptography noise.
The five markers pytest.ini also declared are deliberately not migrated:
tests/real_world/conftest.py registers them with addinivalue_line where
they are used.
tests/unit/ 79 passed. 1674 collected, 0 errors. flake8 91 (master: 92).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
Closed
5 tasks
The previous commit added pytest-timeout to .github/workflows/fast_ci.yml
and said the job "had been failing on every run for that reason". That is
not the reason.
fast_ci.yml is invalid YAML and has never run a single job -- every run,
on master as well as on this branch, completes with conclusion=failure and
jobs=0, which is what GitHub reports when it cannot parse the workflow:
yaml.scanner.ScannerError: while scanning a simple key
in ".github/workflows/fast_ci.yml", line 89, column 1
Three steps open `python -c "` inside a `run: |` block and then write the
Python body at column 0, which terminates the block scalar.
The missing pytest-timeout is real but latent -- it only bites once the
workflow can start. Fixing it here would have shipped an unverifiable
change with a comment misstating its cause, and repairing the YAML would
turn on a workflow whose jobs run real @cluster execution and a Docker
build, inside a PR scoped to pytest configuration.
Both defects, and the reason they are sequenced against #120, are filed as
#135.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #130. Part of #108. Corrects a premise of #110. Surfaced #133.
The bug
pytest.iniopened with[tool:pytest]— a section header valid only insidesetup.cfg. In a file namedpytest.ini, pytest wants[pytest].pytest still selected
pytest.inias its config file, and having selected one it stops searching. Sopyproject.toml's[tool.pytest.ini_options]was never read either. Two config files, zero effective configuration:addopts,testpaths,filterwarnings, all 14 marker declarations and--strict-markerswere declared and inert, with no warning from anything.Why it could not just be flipped
Deleting
pytest.inialone breakspytestcompletely:pyproject.tomlhadtestpaths = ["tests/unit", "tests/integration"]. With no path on the command line pytest fillsconfig.argsfromtestpaths, and the #109 billable-resources guard inspectedconfig.args. The moment the config became effective, the guard fired against the project itself.So the guard is fixed first, in its own commit, before any config change.
Commits, in the order they must be read
9c18cb3--strict-markersgate176d87e9c85296invocation_params.args, notconfig.argsd1d6090pytest.ini;testpaths = ["tests"]6d39bd4d93f933--strict-markers(last)43e7bf314222d0pre_push_check.pyruns the command CI runs5483917Each step is independently revertable, which was the point of not squashing.
Two things the plan did not anticipate
The baseline was not clean. Measuring Step 0 turned up 6 collection errors that had been invisible while nothing was configured, and Step 4's gate is "0 errors under
--strict-markers" — so they had to go first:test_container_registry_comprehensive.py:571put a backslash inside an f-string expression.SyntaxErroron every Python before 3.12, whilerequires-pythonis>=3.8. An AST parse over all oftests/andclustrix/confirmed it was the only one.numpyandpandaswere imported at module scope by five modules and declared in no extra. Added to[dev].pre_push_check.pyran a barepytestwhile claiming to verify "GitHub Actions won't fail" — CI runspytest tests/unit/ -m "not real_world". Harmless only while no config was effective; withtestpathslive it resolved to all 1670 tests including the 224 network-bound ones intests/real_world/and stopped terminating.Regression coverage
tests/unit/test_pytest_config.pyasserts the loaded config ispyproject.toml, that no higher-precedence config file exists in the tree at all, that all 7 markers are registered, and that--strict-markersis live. All four fail onmaster.test_bare_pytest_is_not_refused_by_the_guardcovers the landmine. Its scope is stated honestly in its docstring — measured, each fix alone is sufficient and it catches only the combination:invocation_params.args["tests"]invocation_params.args["tests/unit","tests/integration"]config.args["tests"]config.args["tests/unit","tests/integration"]Verification
blackandmypypass.flake8does not, on 92 findings that predate this branch (91 on it — the f-string fix removed one). CI runs the identical flake8 command with--exit-zero, so this has never been green anywhere. Filed as #133 rather than folded in, because it includes a genuine unrelated bug:test_direct_gpu_detection.py:42builds a remote Python program inside an f-string without escaping its braces, so{torch.__version__},{i},{props.name}and{e}interpolate in the local scope and the module raisesNameErrorbefore the subprocess starts. It survives precisely because the #109 guard keeps that directory out of CI.Note on #110
Its
addopts/xdist premise quotes the closed epic branch, notmaster; onmasteraddoptswas-v --tb=shortand applied to nothing. Itssklearncollection-error claim is also wrong — everysklearnimport undertests/is inside a function body. Two of its acceptance criteria are met here. Corrected in a comment; the issue stays open for the remaining items.🤖 Generated with Claude Code
https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU
Update: red-team pass (4 agents) — 2 self-inflicted bugs found and fixed
Per standing instruction to red-team and merge rather than hand over for review. Corrections to what I originally wrote in this PR:
The Step 1 fix was wrong, and made things less safe
Reading
config.invocation_params.argsunblocked barepytestbut opened two real money-safety holes. Verified againstmaster, which refused both:What the operator types is not what pytest collects.
testpaths,-o testpaths=andPYTEST_ADDOPTSall feedconfig.argswithout ever appearing in the typed argv.The correct fix was in the config, not the guard: reading
config.argsis safe because Step 2 settestpaths = ["tests"]. The guard is back on the effective target list. Also fixed a pre-existing #109 hole: relative paths resolved only against rootdir, so fromtests/the pathintegration/test_x.pywas invisible.-o testpaths=PYTEST_ADDOPTS=--pyargsdottedtests/pytestCLUSTRIX_ALLOW_BILLABLE=1-p <dotted>cannot be blocked before the import it triggers — no conftest hook runs that early — but the run is now refused before any test executes, which is where the cost is. It also fails on its own here, sincetests/is not a package.Four parametrized regression tests cover the indirect routes; three fail against the old guard.
Claims of mine that were wrong
@pytest.mark.real_worlddecorator lines; the directory collects 388 tests.pre_push_check.pyrationale — I wrote that the barepytestwas "harmless while no config applied". It was not: onmasterit aborted in seconds on a collection error and gated nothing. Fixing that SyntaxError is what made the runtime problem appear.test_no_shadowing_config_file_existsforbadetox.iniandsetup.cfg— pytest checks both afterpyproject.toml, so neither can shadow it. The test also failed on an ordinary pytest-freesetup.cfgholding flake8 config. Now limited topytest.ini/.pytest.ini.coverage_detailed_report.txtin .gitignore — nothing in the repo writes it; justification was unsupported, entry removed.gh pr checksomitted the Fast CI workflow, which was running and was failing.Unrelated CI bug found — filed, not fixed (#135)
I first added
pytest-timeoutto.github/workflows/fast_ci.ymland claimed it fixed a job "failing on every run for that reason." That diagnosis was wrong and the change is reverted —fast_ci.ymlis now byte-identical tomasterin this PR.The workflow is invalid YAML and has never run a single job. Every run, on
mastertoo, reportsconclusion: failurewithjobs: 0:Three steps open
python -c "inside arun: |block and write the Python body at column 0, which terminates the block scalar. The missingpytest-timeoutis real but latent — it only bites once the workflow can start.Not fixed here: repairing it turns on a workflow whose jobs run real
@clusterexecution and a Docker build, and whoselocal-integrationstep may legitimately fail given #120. That is an unpredictable blast radius inside a PR scoped to pytest configuration. Filed as #135 with the full diagnosis and sequencing.Also restored
filterwarningsfrom the deletedpytest.ini, never migrated (third-party paramiko/cryptography noise only). The five other markers it declared are deliberately not migrated —tests/real_world/conftest.pyregisters them withaddinivalue_linewhere they are used.Verified after all changes