From 9c18cb35f63a2527d9870ec898bdbe849b8633df Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 11:29:33 -0400 Subject: [PATCH 01/11] Issue #130: Clear the collection errors that block --strict-markers 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- pyproject.toml | 5 +++++ .../real_world/test_container_registry_comprehensive.py | 9 ++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index e0246a5b..035ab862 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -83,6 +83,11 @@ cloud = [ dev = [ "pytest>=6.0", "pytest-cov>=2.0", + # tests/comprehensive/* and several tests/*_real.py modules import numpy and + # pandas at module scope to exercise array/dataframe serialization. Without + # them those modules raise ModuleNotFoundError during collection (see #130). + "numpy>=1.19", + "pandas>=1.1", "black==25.1.0; python_version >= '3.9'", # pinned - unbounded ">=" let CI pull 26.5.1 (see #110) "flake8>=3.8", "mypy>=0.812", diff --git a/tests/real_world/test_container_registry_comprehensive.py b/tests/real_world/test_container_registry_comprehensive.py index cd9aaf07..7077bb00 100644 --- a/tests/real_world/test_container_registry_comprehensive.py +++ b/tests/real_world/test_container_registry_comprehensive.py @@ -566,9 +566,12 @@ def test_container_runtime_environment_validation(self): ), "Core Python modules should be available" logger.info("โœ… Container runtime environment validation successful") - logger.info( - f"Environment details: {len(output.split('\\n'))} properties checked" - ) + # NB: the split() must stay outside the f-string -- an f-string + # expression may not contain a backslash before Python 3.12, and + # this project supports >=3.8, so inlining it is a SyntaxError that + # breaks collection of the whole module. + property_count = len(output.split("\n")) + logger.info(f"Environment details: {property_count} properties checked") except subprocess.TimeoutExpired: assert False, "Environment validation timed out" From 176d87ed520a557a861ab0fd3fe9d3e8a214c63f Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 11:30:54 -0400 Subject: [PATCH 02/11] Issue #130: Add failing regression tests for pytest config shadowing 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- tests/unit/test_pytest_config.py | 107 +++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 tests/unit/test_pytest_config.py diff --git a/tests/unit/test_pytest_config.py b/tests/unit/test_pytest_config.py new file mode 100644 index 00000000..c0c1b252 --- /dev/null +++ b/tests/unit/test_pytest_config.py @@ -0,0 +1,107 @@ +"""Guard the pytest configuration itself against silent shadowing. + +See issue #130. + +For a long time this repo carried two pytest config files, and *neither* was +in effect. ``pytest.ini`` opened with ``[tool:pytest]`` -- a section header +that is only valid inside ``setup.cfg``. pytest still *selected* ``pytest.ini`` +as the config file, and having selected one it stops searching, so +``pyproject.toml``'s ``[tool.pytest.ini_options]`` was never read either. + +The failure mode is nasty precisely because it is silent: ``addopts``, +``testpaths``, ``filterwarnings``, every ``markers`` entry and ``--strict-markers`` +were all declared and all inert. Nothing warns about it. The only way to notice +is to check which file pytest actually loaded. + +These tests do that check, so a stray ``pytest.ini`` / ``tox.ini`` / ``setup.cfg`` +added later cannot quietly take over again. +""" + +import pathlib + +import pytest + +EXPECTED_CONFIG_NAME = "pyproject.toml" + +# Config files pytest will prefer over pyproject.toml if they exist at the +# rootdir, in pytest's own precedence order. Any of these silently wins. +SHADOWING_CONFIG_NAMES = ("pytest.ini", ".pytest.ini", "tox.ini", "setup.cfg") + +# Markers this project defines and relies on. Registration is what makes +# ``-m `` a usable selector and what lets --strict-markers catch typos. +REQUIRED_MARKERS = ( + "real_world", + "expensive", + "integration", + "dartmouth_network", + "performance", + "slow", + "unit", +) + + +def test_pytest_reads_the_intended_config(pytestconfig): + """pytest must actually be loading pyproject.toml, not something else.""" + assert pytestconfig.inipath is not None, ( + "pytest loaded no config file at all -- pyproject.toml's " + "[tool.pytest.ini_options] is not being applied (see #130)" + ) + assert pytestconfig.inipath.name == EXPECTED_CONFIG_NAME, ( + f"pytest loaded {pytestconfig.inipath}; a stray config file is " + f"shadowing {EXPECTED_CONFIG_NAME} (see #130)" + ) + + +def test_no_shadowing_config_file_exists(pytestconfig): + """Fail loudly if a higher-precedence config file reappears in the repo. + + ``test_pytest_reads_the_intended_config`` only catches a shadow that is + live during *this* run. A config file that exists but happens not to win + (for instance because the run was launched from a subdirectory with a + different rootdir) would slip past it, so check the tree directly. + """ + rootdir = pathlib.Path(str(pytestconfig.rootpath)) + strays = [name for name in SHADOWING_CONFIG_NAMES if (rootdir / name).exists()] + assert not strays, ( + f"found {strays} at {rootdir}; pytest prefers these over " + f"{EXPECTED_CONFIG_NAME} and stops searching once one is selected, so " + f"{EXPECTED_CONFIG_NAME} would be silently ignored (see #130)" + ) + + +def test_project_markers_are_registered(pytestconfig): + """Every marker the suite uses must be declared in the live config.""" + # getini("markers") also returns plugin-provided markers such as + # "timeout(timeout, method=None, ...): ..." -- strip the argspec as well as + # the description before comparing names. + registered = { + entry.split(":")[0].split("(")[0].strip() + for entry in pytestconfig.getini("markers") + } + missing = [name for name in REQUIRED_MARKERS if name not in registered] + assert not missing, ( + f"markers {missing} are not registered. Unregistered markers are not " + f"usable as -m selectors and become hard errors under --strict-markers " + f"(see #130). Declare them in {EXPECTED_CONFIG_NAME}." + ) + + +def test_strict_markers_is_active(pytestconfig): + """--strict-markers must be live, or marker typos silently do nothing. + + This is the setting that makes the two tests above self-enforcing: without + it, a mistyped ``@pytest.mark.reel_world`` is accepted as a no-op and the + test it decorates quietly stops being selectable. + + --strict-markers reaches us through ``addopts``, so a run that deliberately + clears addopts (``pytest -o addopts=``, used when comparing collection + counts across config changes) legitimately has it off. Detect that override + rather than reporting a failure the config is not responsible for. + """ + overrides = getattr(pytestconfig.option, "override_ini", None) or [] + if any(str(entry).startswith("addopts=") for entry in overrides): + pytest.skip("addopts overridden on the command line with -o addopts=") + assert pytestconfig.option.strict_markers, ( + "--strict-markers is not active; a mistyped marker is silently ignored " + "instead of erroring (see #130)" + ) From 9c852969608b6f5f39a3baf0bbd450e627de0cae Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 11:32:44 -0400 Subject: [PATCH 03/11] Issue #130: Key the billable-test guard off argv, not resolved testpaths 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- tests/conftest.py | 12 +++++- tests/unit/test_billable_safety.py | 62 +++++++++++++++++++++++------- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index e5825815..db1eada5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -34,10 +34,20 @@ def pytest_configure(config): keep working and silently skip the directory, while someone who asked for these tests by name gets told why they got nothing, rather than an inscrutable empty run. + + Reads `config.invocation_params.args` -- what the operator actually typed -- + and NOT `config.args`. The two differ in exactly the case that matters: when + no path is given on the command line, pytest populates `config.args` from + `testpaths` in pyproject.toml, which lists `tests`. Keying off `config.args` + therefore made a bare `pytest` abort with this very error the moment the + project's config became effective (see #130). `invocation_params.args` + contains flags as well as paths, so entries starting with "-" are skipped. """ if _billable_tests_enabled(): return - for arg in config.args: + for arg in config.invocation_params.args: + if str(arg).startswith("-"): + continue # strip pytest's "::TestClass::test_name" node-id suffix candidate = pathlib.Path(str(arg).split("::")[0]) if not candidate.is_absolute(): diff --git a/tests/unit/test_billable_safety.py b/tests/unit/test_billable_safety.py index 9bd893b2..89eed3af 100644 --- a/tests/unit/test_billable_safety.py +++ b/tests/unit/test_billable_safety.py @@ -51,11 +51,21 @@ def _collected_count(output: str): return max(int(m) for m in matches) +# Sentinel for "run pytest with no path argument at all", which is a distinct +# case from "run it against the default target": with no path on the command +# line pytest fills its target list from `testpaths` in pyproject.toml. That +# path is what regressed in #130 and needs its own coverage. +NO_TARGET = object() + + def _collect_integration(opt_in: bool, tmp_home, target=None): """Run `pytest --collect-only` against tests/integration in a subprocess. + Pass `target=NO_TARGET` to omit the path argument entirely. + `-o addopts=` strips the project's default addopts so this does not depend - on xdist being installed. + on xdist being installed. It deliberately does not touch `testpaths`, so a + NO_TARGET run still exercises testpaths resolution. The subprocess gets a throwaway HOME and a scrubbed environment. This is deliberate: a test whose job is to prove the suite cannot spend money must @@ -106,19 +116,12 @@ def _collect_integration(opt_in: bool, tmp_home, target=None): "LAMBDA_CLOUD_API_KEY", ): env.pop(leaked, None) + argv = [sys.executable, "-m", "pytest"] + if target is not NO_TARGET: + argv.append(str(target if target is not None else INTEGRATION_DIR)) + argv += ["--collect-only", "-q", "-o", "addopts=", "-p", "no:cacheprovider"] return subprocess.run( - [ - sys.executable, - "-m", - "pytest", - str(target if target is not None else INTEGRATION_DIR), - "--collect-only", - "-q", - "-o", - "addopts=", - "-p", - "no:cacheprovider", - ], + argv, cwd=str(REPO_ROOT), env=env, capture_output=True, @@ -153,6 +156,39 @@ def test_default_suite_does_not_collect_integration_tests(tmp_path): ) +def test_bare_pytest_is_not_refused_by_the_guard(tmp_path): + """A bare `pytest` must still run. See #130. + + This is the counterpart to the refusal tests, and it guards a trap that + already sprang once. The guard originally inspected `config.args`, which + reads like "what the user asked for" but is not: when no path is given on + the command line, pytest populates `config.args` from `testpaths` in + pyproject.toml. `testpaths` used to list `tests/integration`, so the moment + the project's config actually took effect, a bare `pytest` matched the + guard and aborted the entire suite with the billable-resources refusal. + + The fix was to read `config.invocation_params.args` -- the real argv -- so + the guard fires on explicit targeting only. Reverting that change, or + putting `tests/integration` back into `testpaths`, breaks bare `pytest` + for everyone, and nothing else in the suite would notice. + """ + result = _collect_integration(opt_in=False, tmp_home=tmp_path, target=NO_TARGET) + combined = (result.stdout or "") + (result.stderr or "") + + assert "Refusing to run" not in combined, ( + "A bare `pytest` was refused by the billable-resources guard. The guard " + "is matching testpaths-derived arguments instead of what the operator " + "typed (see #130).\n" + combined[-2000:] + ) + assert result.returncode == 0, ( + f"A bare `pytest --collect-only` failed (exit {result.returncode}).\n" + + combined[-2000:] + ) + assert (_collected_count(combined) or 0) > 0, ( + "A bare `pytest` collected nothing at all.\n" + combined[-2000:] + ) + + def test_explicitly_targeting_integration_is_refused(tmp_path): """Naming the directory or a file in it must fail loudly, not silently. From d1d609078ea32e50232c5bf3eaadd1ce929f357e Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 11:54:29 -0400 Subject: [PATCH 04/11] Issue #130: Delete pytest.ini, make pyproject.toml the only pytest config 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- pyproject.toml | 11 ++++++++++- pytest.ini | 22 ---------------------- tests/unit/test_billable_safety.py | 20 ++++++++++++++++---- 3 files changed, 26 insertions(+), 27 deletions(-) delete mode 100644 pytest.ini diff --git a/pyproject.toml b/pyproject.toml index 035ab862..9a0fff5a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -186,7 +186,16 @@ module = "tests.*" ignore_errors = true [tool.pytest.ini_options] -testpaths = ["tests/unit", "tests/integration"] +# This is the project's ONLY pytest config. Do not add pytest.ini, tox.ini or +# setup.cfg: pytest selects the first config file it finds by precedence and +# then stops searching, so any of those would silently shadow this block -- +# which is exactly what happened for months under #130. Enforced by +# tests/unit/test_pytest_config.py. +# +# testpaths must NOT list tests/integration. Those tests provision billable +# cloud resources and are gated by tests/integration/conftest.py (#109); naming +# the directory here would make a bare `pytest` resolve to it. +testpaths = ["tests"] python_files = "test_*.py" python_classes = "Test*" python_functions = "test_*" diff --git a/pytest.ini b/pytest.ini deleted file mode 100644 index 05ebe5ec..00000000 --- a/pytest.ini +++ /dev/null @@ -1,22 +0,0 @@ -[tool:pytest] -testpaths = tests -python_files = test_*.py -python_classes = Test* -python_functions = test_* -addopts = -v --tb=short --strict-markers -markers = - unit: fast unit tests with no external dependencies - integration: tests that use external resources (databases, APIs, etc.) - expensive: tests that may incur costs (cloud APIs, etc.) - visual: tests requiring manual visual verification - ssh_required: tests requiring SSH access - aws_required: tests requiring AWS credentials - azure_required: tests requiring Azure credentials - gcp_required: tests requiring GCP credentials - real_world: tests using real external resources - slow: tests that take longer than 30 seconds -filterwarnings = - ignore::DeprecationWarning - ignore::PendingDeprecationWarning - ignore::UserWarning:paramiko.* - ignore::UserWarning:cryptography.* \ No newline at end of file diff --git a/tests/unit/test_billable_safety.py b/tests/unit/test_billable_safety.py index 89eed3af..109e2d28 100644 --- a/tests/unit/test_billable_safety.py +++ b/tests/unit/test_billable_safety.py @@ -167,10 +167,22 @@ def test_bare_pytest_is_not_refused_by_the_guard(tmp_path): the project's config actually took effect, a bare `pytest` matched the guard and aborted the entire suite with the billable-resources refusal. - The fix was to read `config.invocation_params.args` -- the real argv -- so - the guard fires on explicit targeting only. Reverting that change, or - putting `tests/integration` back into `testpaths`, breaks bare `pytest` - for everyone, and nothing else in the suite would notice. + Two independent changes fixed it: the guard now reads + `config.invocation_params.args` (the real argv), and `testpaths` no longer + names `tests/integration`. Either one alone is sufficient, which is why + both are kept -- belt and braces. + + Measured on this branch, undoing one is survivable and undoing both is not: + + guard source testpaths bare pytest + invocation_params.args ["tests"] ok + invocation_params.args ["tests/unit","tests/integration"] ok + config.args ["tests"] ok + config.args ["tests/unit","tests/integration"] REFUSED + + So this test is a guard on the *combination*, not on either edit alone. + That is the honest scope: it is the last line of defence rather than the + first, and it fails loudly the moment the suite becomes unrunnable. """ result = _collect_integration(opt_in=False, tmp_home=tmp_path, target=NO_TARGET) combined = (result.stdout or "") + (result.stderr or "") From 6d39bd464cce6c6355bd1cebe10999a9f459d667 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 11:54:42 -0400 Subject: [PATCH 05/11] Issue #130: Register every marker the suite actually uses 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- pyproject.toml | 7 +++++++ tests/real_world/test_kubernetes_performance_benchmarks.py | 1 - tests/unit/test_pytest_config.py | 6 ++++-- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 9a0fff5a..359a98a8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -200,11 +200,18 @@ python_files = "test_*.py" python_classes = "Test*" python_functions = "test_*" addopts = "-v --tb=short" +# Every marker used anywhere under tests/ must appear here: --strict-markers +# turns an undeclared marker into a hard collection error. Verify with +# grep -rhoE "@pytest\.mark\.[a-zA-Z_]+" tests/ | sort -u +# and note that tests/unit/test_pytest_config.py asserts these stay registered. markers = [ "real_world: marks tests as real world tests (deselect with '-m \"not real_world\"')", "slow: marks tests as slow (deselect with '-m \"not slow\"')", "unit: marks tests as unit tests", "integration: marks tests as integration tests", + "expensive: marks tests that provision billable resources (applied automatically to tests/integration; see #109)", + "dartmouth_network: marks tests that require access to the Dartmouth campus network", + "performance: marks performance benchmark tests", ] [tool.coverage.run] diff --git a/tests/real_world/test_kubernetes_performance_benchmarks.py b/tests/real_world/test_kubernetes_performance_benchmarks.py index 4111aa44..f059f33c 100644 --- a/tests/real_world/test_kubernetes_performance_benchmarks.py +++ b/tests/real_world/test_kubernetes_performance_benchmarks.py @@ -1035,7 +1035,6 @@ def _save_benchmark_results( logger.info(f"๐Ÿ“ Benchmark results saved: {json_filename}") - @pytest.mark.cleanup def test_benchmark_cleanup_verification(self, performance_results_dir): """Verify all benchmark resources have been cleaned up.""" logger.info("๐Ÿงช Verifying benchmark resource cleanup") diff --git a/tests/unit/test_pytest_config.py b/tests/unit/test_pytest_config.py index c0c1b252..a784bea9 100644 --- a/tests/unit/test_pytest_config.py +++ b/tests/unit/test_pytest_config.py @@ -90,8 +90,10 @@ def test_strict_markers_is_active(pytestconfig): """--strict-markers must be live, or marker typos silently do nothing. This is the setting that makes the two tests above self-enforcing: without - it, a mistyped ``@pytest.mark.reel_world`` is accepted as a no-op and the - test it decorates quietly stops being selectable. + it, a marker name mistyped as "reel_world" instead of "real_world" is + accepted as a no-op and the test it decorates quietly stops being + selectable. (Spelled without the decorator prefix on purpose, so the + grep-based marker audit in pyproject.toml does not trip over this line.) --strict-markers reaches us through ``addopts``, so a run that deliberately clears addopts (``pytest -o addopts=``, used when comparing collection From d93f933dad19fb3edba82d98358da6fa4c9fa983 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 11:54:53 -0400 Subject: [PATCH 06/11] Issue #130: Enable --strict-markers 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- pyproject.toml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 359a98a8..575e1ad3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -199,7 +199,13 @@ testpaths = ["tests"] python_files = "test_*.py" python_classes = "Test*" python_functions = "test_*" -addopts = "-v --tb=short" +# --strict-markers makes an undeclared marker a collection error rather than a +# warning. Keep it: without it a typo'd marker silently does nothing and the +# test it decorates stops being selectable with no signal at all. +# Do NOT add "-n" here without first moving pytest-xdist from the [test] extra +# into [dev] -- otherwise `pip install -e ".[dev]"` yields an env where pytest +# cannot start (see #110). +addopts = "-v --tb=short --strict-markers" # Every marker used anywhere under tests/ must appear here: --strict-markers # turns an undeclared marker into a hard collection error. Verify with # grep -rhoE "@pytest\.mark\.[a-zA-Z_]+" tests/ | sort -u From 43e7bf3c32b51ac6b02959f19be925a335b79c84 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 11:56:06 -0400 Subject: [PATCH 07/11] Issue #130: Update docs that still describe the shadowed pytest config 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- .claude/commands/testing/prime.md | 2 +- MIGRATION.md | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/.claude/commands/testing/prime.md b/.claude/commands/testing/prime.md index badc5550..6a62411c 100644 --- a/.claude/commands/testing/prime.md +++ b/.claude/commands/testing/prime.md @@ -92,7 +92,7 @@ environment: framework: pytest test_command: pytest test_directory: tests -config_file: pytest.ini +config_file: pyproject.toml # [tool.pytest.ini_options]; see #130 options: - -v - --tb=short diff --git a/MIGRATION.md b/MIGRATION.md index f50743e0..3fe0fb68 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -76,17 +76,34 @@ from clustrix.filesystem import cluster_ls, cluster_find **Pytest configuration updated** to properly discover tests: ```toml -# pyproject.toml +# pyproject.toml -- the project's only pytest config file [tool.pytest.ini_options] -testpaths = ["tests/unit", "tests/integration"] +testpaths = ["tests"] +addopts = "-v --tb=short --strict-markers" markers = [ "real_world: marks tests as real world tests", "slow: marks tests as slow", - "unit: marks tests as unit tests", + "unit: marks tests as unit tests", "integration: marks tests as integration tests", + "expensive: marks tests that provision billable resources", + "dartmouth_network: marks tests needing the Dartmouth campus network", + "performance: marks performance benchmark tests", ] ``` +> **Note (see #130).** This block only became effective later. A `pytest.ini` +> in the repo root used the section header `[tool:pytest]`, which is valid only +> in `setup.cfg`; pytest still selected that file and stopped searching, so +> nothing here was applied. `pytest.ini` has since been deleted and +> `pyproject.toml` is now the single source. Do not reintroduce `pytest.ini`, +> `tox.ini` or `setup.cfg` -- pytest prefers all three over `pyproject.toml` +> and would silently shadow it again. +> +> `testpaths` is `["tests"]`, not `["tests/unit", "tests/integration"]`: with no +> path on the command line pytest resolves its targets from `testpaths`, and +> naming the integration directory there points a bare `pytest` at tests that +> provision billable cloud resources. + ### CI/CD Updates **GitHub Actions workflows updated** for new structure: - Test paths fixed to use `tests/unit/` and `tests/integration/` From 14222d041d4261b4c3d5886bab8b73629a902c10 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 12:12:38 -0400 Subject: [PATCH 08/11] Issue #130: Make pre_push_check run the same tests CI runs 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- scripts/pre_push_check.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/scripts/pre_push_check.py b/scripts/pre_push_check.py index bb0f232c..e21f6409 100755 --- a/scripts/pre_push_check.py +++ b/scripts/pre_push_check.py @@ -45,7 +45,19 @@ def main(): ("black clustrix/ tests/", "Black formatting"), # Format, don't just check ("flake8 clustrix/ tests/ --max-line-length=88 --extend-ignore=E203,W503,F401,E722,F541,F841,F811,E731,E501,W291,W293,F824", "Flake8 linting"), ("mypy clustrix/", "MyPy type checking"), - ("pytest", "Tests"), + # Must mirror what GitHub Actions actually runs (see + # .github/workflows/tests.yml), or this script cannot deliver on + # the promise in its own docstring. + # + # This was a bare `pytest`, which was harmless only for as long as + # the project had no effective pytest config: pytest.ini shadowed + # pyproject.toml and neither applied, so `testpaths` did nothing + # (see #130). With the config live, a bare `pytest` resolves to + # testpaths and pulls in all 1670 tests -- including the 224 in + # tests/real_world/, which make live network and cloud calls and do + # not terminate in a reasonable time. Real-world tests are run + # deliberately via scripts/run_real_world_tests.py, not here. + ('pytest tests/unit/ -m "not real_world"', "Tests"), ] all_passed = True From 54839170f7490691045465fcb4f1632d8ab63ffc Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 12:14:53 -0400 Subject: [PATCH 09/11] Issue #130: Ignore test-generated artifacts; add session notes 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- .gitignore | 3 + notes/session_130_pytest_config_execution.md | 112 +++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 notes/session_130_pytest_config_execution.md diff --git a/.gitignore b/.gitignore index d03ef976..571e559a 100644 --- a/.gitignore +++ b/.gitignore @@ -26,8 +26,11 @@ test-reports/ .coverage .coverage.* coverage.json +coverage_detailed_report.txt htmlcov/ .tox/ +# Written by the benchmark tests in tests/real_world/ at run time. +performance_test_results/ # Personal configuration files clustrix.yml diff --git a/notes/session_130_pytest_config_execution.md b/notes/session_130_pytest_config_execution.md new file mode 100644 index 00000000..6999640f --- /dev/null +++ b/notes/session_130_pytest_config_execution.md @@ -0,0 +1,112 @@ +# Session log: executing the #130 plan + +**Date:** 2026-08-17 ยท **Branch:** `fix/130-pytest-config` ยท **Base:** `master` @ `a9393b7` +**Plan followed:** `notes/handoff_130_pytest_config.md` + +Every command below was run in a throwaway venv, since the repo has no usable +one by default (#110): + +```bash +python3 -m venv /tmp/v130 && /tmp/v130/bin/pip install -e ".[dev]" +``` + +`scripts/pre_push_check.py` shells out to bare `pytest`/`mypy`/`flake8` from +`PATH`, so it must be run as +`PATH="/tmp/v130/bin:$PATH" /tmp/v130/bin/python scripts/pre_push_check.py`. +Running it with the venv interpreter alone is not enough and fails with +`ModuleNotFoundError: paramiko`. + +## What landed, in order + +| Commit | Step | What | +|-|-|-| +| `9c18cb3` | 0.5 | Cleared 6 collection errors blocking Step 4 | +| `176d87e` | 5 (first) | Regression tests, TDD red | +| `9c85296` | 1 | Guard reads argv, not resolved testpaths | +| `d1d6090` | 2 | Deleted pytest.ini; `testpaths = ["tests"]` | +| `6d39bd4` | 3 | Registered all 7 markers; dropped `cleanup` | +| `d93f933` | 4 | Enabled `--strict-markers` | +| `43e7bf3` | 6 | MIGRATION.md + prime.md | +| `14222d0` | 6 | pre_push_check runs CI's test command | + +## Things the plan did not anticipate + +**Step 0 baseline had 6 collection errors.** The plan said to record the +number; it did not say the number came with errors attached. They had to be +fixed before Step 4 could be verified at all, because Step 4's gate is +"0 errors under `--strict-markers`". + +- `tests/real_world/test_container_registry_comprehensive.py:571` had a + backslash inside an f-string expression โ€” a `SyntaxError` on every Python + before 3.12, while `requires-python` says `>=3.8`. An AST parse over all of + `tests/` and `clustrix/` confirmed it was the only one in the tree. +- `numpy` and `pandas` were undeclared but imported at module scope by five + modules. Added to the `[dev]` extra. + +Result: 1214 collected + 6 errors โ†’ 1275 collected + 0 errors. + +**The plan's own regression test needed correcting.** It proposed asserting +`pytestconfig.inipath.name == "pyproject.toml"`, which is right, but the +docstring I first wrote for the bare-pytest guard test overclaimed. Measured: + +| guard reads | testpaths | bare pytest | +|-|-|-| +| `invocation_params.args` | `["tests"]` | ok | +| `invocation_params.args` | `["tests/unit","tests/integration"]` | ok | +| `config.args` | `["tests"]` | ok | +| `config.args` | `["tests/unit","tests/integration"]` | **REFUSED** | + +So the two fixes are each independently sufficient, and the test guards the +*combination*. Both are kept as belt and braces, and the table is recorded in +the test's docstring. + +**`--strict-markers` needs care with `-o addopts=`.** It reaches pytest through +`addopts`, so `test_strict_markers_is_active` skips when +`config.option.override_ini` contains an `addopts=` entry. Verified against +pytest 8.4.2: that attribute holds `['addopts=']`. + +**`pre_push_check.py` ran a bare `pytest`.** Harmless only while no config was +effective. Once `testpaths` went live it resolved to all 1670 tests including +the 224 network-bound ones in `tests/real_world/`, and stopped terminating. It +also left artifacts behind: it modified checked-in files under +`tests/real_world/screenshots/` and created an untracked +`performance_test_results/`. Now runs `pytest tests/unit/ -m "not real_world"`, +matching `.github/workflows/tests.yml`; the generated paths are gitignored. + +## Definition of done โ€” measured + +``` +configfile pyproject.toml +shadowing config files in repo 0 +bare pytest 1670 collected, no abort +pytest tests/integration/ refused (#109 holds) +tests/ -m "not real_world" integration node IDs 0 +markers registered 7 / 7 +collection errors under --strict-markers 0 +tests/unit/ 75 passed +fresh venv, [dev] only, pytest tests/ --co 1670 collected, 0 errors +``` + +## Left open deliberately + +**#133 (filed).** `scripts/pre_push_check.py` still cannot exit 0: flake8 +reports 92 findings on `master` (91 on this branch). CI runs the identical +flake8 command with `--exit-zero`, so it has never been green anywhere. 75 are +`E402` from deliberate `sys.path` setup. The rest are not stylistic โ€” the +notable one is `tests/integration/test_direct_gpu_detection.py:42`, which +builds a remote Python program inside an f-string without escaping its braces, +so `{torch.__version__}`, `{i}`, `{props.name}`, `{e}` are interpolated in the +*local* scope and the module raises `NameError` before the subprocess starts. +It survives because the #109 guard keeps `tests/integration/` out of CI. + +Not folded into this PR: different concern, gated directory, and it would bury +the #130 change. + +**#110 corrected**, not closed. Its `addopts`/xdist premise quoted the epic +branch, not `master`, and its `sklearn` collection-error claim is wrong (every +`sklearn` import under `tests/` is inside a function body). Two of its +acceptance criteria are now met. Comment: +https://github.com/ContextLab/clustrix/issues/110#issuecomment-5317453905 + +**#117** untouched, as instructed โ€” `tests/real_world/conftest.py:223` still +calls `is_dartmouth_network()` at collection time. From a5b665f84bbdbc2bac0eb9815295c13eee580d84 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 12:50:49 -0400 Subject: [PATCH 10/11] Issue #130: Close the guard bypasses red-teaming found, and fix my own 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- .github/workflows/fast_ci.yml | 6 +- .gitignore | 1 - pyproject.toml | 13 +++ scripts/pre_push_check.py | 21 ++-- tests/conftest.py | 91 ++++++++++++---- tests/unit/test_billable_safety.py | 163 ++++++++++++++++++++++++----- tests/unit/test_pytest_config.py | 26 +++-- 7 files changed, 255 insertions(+), 66 deletions(-) diff --git a/.github/workflows/fast_ci.yml b/.github/workflows/fast_ci.yml index c297fb98..cda92f9a 100644 --- a/.github/workflows/fast_ci.yml +++ b/.github/workflows/fast_ci.yml @@ -39,7 +39,11 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - pip install black flake8 mypy pytest + # pytest-timeout is required: the test step below passes + # --timeout=60, and without the plugin pytest exits 4 with + # "unrecognized arguments". This job had been failing on every run + # for that reason (see #130). + pip install black flake8 mypy pytest pytest-timeout pip install -e . - name: Format check with Black diff --git a/.gitignore b/.gitignore index 571e559a..40a1348b 100644 --- a/.gitignore +++ b/.gitignore @@ -26,7 +26,6 @@ test-reports/ .coverage .coverage.* coverage.json -coverage_detailed_report.txt htmlcov/ .tox/ # Written by the benchmark tests in tests/real_world/ at run time. diff --git a/pyproject.toml b/pyproject.toml index 575e1ad3..ce440e41 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -219,6 +219,19 @@ markers = [ "dartmouth_network: marks tests that require access to the Dartmouth campus network", "performance: marks performance benchmark tests", ] +# Carried over from the deleted pytest.ini, where it never took effect. These +# suppress third-party noise only; nothing here hides a warning raised by +# clustrix itself. +# NB: markers visual/ssh_required/aws_required/azure_required/gcp_required are +# deliberately NOT listed. tests/real_world/conftest.py registers them with +# addinivalue_line at the point they are used, so --strict-markers is satisfied +# there and they stay scoped to the suite that actually defines them. +filterwarnings = [ + "ignore::DeprecationWarning", + "ignore::PendingDeprecationWarning", + "ignore::UserWarning:paramiko.*", + "ignore::UserWarning:cryptography.*", +] [tool.coverage.run] source = ["clustrix"] diff --git a/scripts/pre_push_check.py b/scripts/pre_push_check.py index e21f6409..2962cf90 100755 --- a/scripts/pre_push_check.py +++ b/scripts/pre_push_check.py @@ -43,20 +43,23 @@ def main(): checks = [ ("black clustrix/ tests/", "Black formatting"), # Format, don't just check - ("flake8 clustrix/ tests/ --max-line-length=88 --extend-ignore=E203,W503,F401,E722,F541,F841,F811,E731,E501,W291,W293,F824", "Flake8 linting"), + ( + "flake8 clustrix/ tests/ --max-line-length=88 --extend-ignore=E203,W503,F401,E722,F541,F841,F811,E731,E501,W291,W293,F824", + "Flake8 linting", + ), ("mypy clustrix/", "MyPy type checking"), # Must mirror what GitHub Actions actually runs (see # .github/workflows/tests.yml), or this script cannot deliver on # the promise in its own docstring. # - # This was a bare `pytest`, which was harmless only for as long as - # the project had no effective pytest config: pytest.ini shadowed - # pyproject.toml and neither applied, so `testpaths` did nothing - # (see #130). With the config live, a bare `pytest` resolves to - # testpaths and pulls in all 1670 tests -- including the 224 in - # tests/real_world/, which make live network and cloud calls and do - # not terminate in a reasonable time. Real-world tests are run - # deliberately via scripts/run_real_world_tests.py, not here. + # This was a bare `pytest`, which gated nothing: on master it + # aborted in seconds with "Interrupted: 1 error during collection" + # (an f-string SyntaxError under Python < 3.12) and ran zero tests. + # Fixing that error is what made the problem visible -- collection + # then succeeded and a bare `pytest` ran all 1670 tests, including + # the 388 in tests/real_world/, which open live SSH and cloud + # connections. Real-world tests are run deliberately through + # scripts/run_real_world_tests.py, not from this gate. ('pytest tests/unit/ -m "not real_world"', "Tests"), ] diff --git a/tests/conftest.py b/tests/conftest.py index db1eada5..fbee2ee0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,6 +15,66 @@ def _billable_tests_enabled(): return os.environ.get(_OPT_IN_VAR, "").strip().lower() in _TRUTHY +def _iter_candidate_targets(config): + """Yield every path-ish string this run could end up collecting from. + + Reads `config.args` -- the *effective* target list -- rather than + `config.invocation_params.args`, which is only what the operator literally + typed. The difference is the whole ballgame, because there are three ways + to aim pytest at a directory without typing its path: + + PYTEST_ADDOPTS=tests/integration/test_x.py pytest + pytest -o testpaths=tests/integration/test_x.py + (testpaths in pyproject.toml) + + All three land in `config.args` and none appear in `invocation_params.args`. + Red-teaming confirmed the first two collected billable modules when the + guard read the typed argv. + + Reading `config.args` is only safe because `testpaths` is `["tests"]`. If + it ever names tests/integration again, a bare `pytest` will be refused -- + loudly and correctly, since testpaths would then be pointing every default + run at billable tests. + """ + for arg in config.args: + yield str(arg).split("::")[0] + # --pyargs addresses modules by dotted name, which never looks like a path. + if getattr(config.option, "pyargs", False): + for arg in config.args: + yield str(arg).split("::")[0].replace(".", os.sep) + # -p imports a plugin by dotted name, before collection begins. + for plugin in getattr(config.option, "plugins", None) or []: + yield str(plugin).replace(".", os.sep) + + +def _targets_integration_dir(candidate, config): + """True if `candidate` names tests/integration under any sane resolution. + + Paths on the command line resolve against the invocation directory, while + `testpaths` resolves against rootdir. Those differ whenever pytest is run + from a subdirectory, and resolving against only one of them was a real + hole: from `tests/`, `pytest integration/test_x.py` was not recognised. + + Both bases are tried. A guard protecting real money should over-match + rather than under-match. + """ + raw = pathlib.Path(candidate) + if raw.is_absolute(): + attempts = [raw] + else: + invocation_dir = getattr(config.invocation_params, "dir", None) + bases = [invocation_dir, pathlib.Path.cwd(), pathlib.Path(str(config.rootpath))] + attempts = [pathlib.Path(str(base)) / raw for base in bases if base] + for attempt in attempts: + try: + resolved = attempt.resolve() + except OSError: # pragma: no cover - defensive, unresolvable path + continue + if resolved == _INTEGRATION_DIR or _INTEGRATION_DIR in resolved.parents: + return True + return False + + def pytest_configure(config): """Refuse to start when a run explicitly targets tests/integration. @@ -35,31 +95,20 @@ def pytest_configure(config): these tests by name gets told why they got nothing, rather than an inscrutable empty run. - Reads `config.invocation_params.args` -- what the operator actually typed -- - and NOT `config.args`. The two differ in exactly the case that matters: when - no path is given on the command line, pytest populates `config.args` from - `testpaths` in pyproject.toml, which lists `tests`. Keying off `config.args` - therefore made a bare `pytest` abort with this very error the moment the - project's config became effective (see #130). `invocation_params.args` - contains flags as well as paths, so entries starting with "-" are skipped. + Target discovery is in `_iter_candidate_targets` and path matching is in + `_targets_integration_dir`; both carry the reasoning for why they look + where they do. In short: read the *effective* target list, and resolve + relative paths against every plausible base. """ if _billable_tests_enabled(): return - for arg in config.invocation_params.args: - if str(arg).startswith("-"): - continue - # strip pytest's "::TestClass::test_name" node-id suffix - candidate = pathlib.Path(str(arg).split("::")[0]) - if not candidate.is_absolute(): - candidate = (pathlib.Path(str(config.rootpath)) / candidate).resolve() - else: - candidate = candidate.resolve() - if candidate == _INTEGRATION_DIR or _INTEGRATION_DIR in candidate.parents: + for candidate in _iter_candidate_targets(config): + if _targets_integration_dir(candidate, config): raise pytest.UsageError( - f"Refusing to run {arg!r}: tests/integration provisions real, " - f"billable cloud resources (AWS EKS/EC2). Set {_OPT_IN_VAR}=1 to " - f"run them deliberately, e.g.\n" - f" {_OPT_IN_VAR}=1 pytest {arg}" + f"Refusing to run {candidate!r}: tests/integration provisions " + f"real, billable cloud resources (AWS EKS/EC2). Set " + f"{_OPT_IN_VAR}=1 to run them deliberately, e.g.\n" + f" {_OPT_IN_VAR}=1 pytest {candidate}" ) diff --git a/tests/unit/test_billable_safety.py b/tests/unit/test_billable_safety.py index 109e2d28..eb9bc67f 100644 --- a/tests/unit/test_billable_safety.py +++ b/tests/unit/test_billable_safety.py @@ -160,37 +160,31 @@ def test_bare_pytest_is_not_refused_by_the_guard(tmp_path): """A bare `pytest` must still run. See #130. This is the counterpart to the refusal tests, and it guards a trap that - already sprang once. The guard originally inspected `config.args`, which - reads like "what the user asked for" but is not: when no path is given on - the command line, pytest populates `config.args` from `testpaths` in - pyproject.toml. `testpaths` used to list `tests/integration`, so the moment - the project's config actually took effect, a bare `pytest` matched the - guard and aborted the entire suite with the billable-resources refusal. - - Two independent changes fixed it: the guard now reads - `config.invocation_params.args` (the real argv), and `testpaths` no longer - names `tests/integration`. Either one alone is sufficient, which is why - both are kept -- belt and braces. - - Measured on this branch, undoing one is survivable and undoing both is not: - - guard source testpaths bare pytest - invocation_params.args ["tests"] ok - invocation_params.args ["tests/unit","tests/integration"] ok - config.args ["tests"] ok - config.args ["tests/unit","tests/integration"] REFUSED - - So this test is a guard on the *combination*, not on either edit alone. - That is the honest scope: it is the last line of defence rather than the - first, and it fails loudly the moment the suite becomes unrunnable. + already sprang once. The guard reads `config.args` -- the *effective* + target list -- which is what makes it resistant to indirect targeting + (see `test_indirect_targeting_of_integration_is_refused`). The cost of + reading it is that when no path is given, pytest fills `config.args` from + `testpaths`. While `testpaths` named `tests/integration`, that made a bare + `pytest` match the guard and abort the whole suite. + + The fix is therefore in the config, not the guard: `testpaths` is `["tests"]`. + That keeps `config.args` readable (so PYTEST_ADDOPTS and `-o testpaths=` + cannot sneak past) while leaving a default run collectible. + + Note this is the opposite resolution from the one first attempted here. + Narrowing the guard to `invocation_params.args` also unblocked bare + `pytest`, but it opened three real bypasses, because what the operator + types is not what pytest collects. Reverting `testpaths` to name + `tests/integration` will make this test fail -- correctly, since every + default run would then be aimed at billable tests. """ result = _collect_integration(opt_in=False, tmp_home=tmp_path, target=NO_TARGET) combined = (result.stdout or "") + (result.stderr or "") assert "Refusing to run" not in combined, ( - "A bare `pytest` was refused by the billable-resources guard. The guard " - "is matching testpaths-derived arguments instead of what the operator " - "typed (see #130).\n" + combined[-2000:] + "A bare `pytest` was refused by the billable-resources guard, so the " + "default suite cannot run at all. `testpaths` is probably naming " + "tests/integration again (see #130).\n" + combined[-2000:] ) assert result.returncode == 0, ( f"A bare `pytest --collect-only` failed (exit {result.returncode}).\n" @@ -201,6 +195,123 @@ def test_bare_pytest_is_not_refused_by_the_guard(tmp_path): ) +def _run_pytest(tmp_home, argv_extra, env_extra=None, cwd=None): + """Run pytest in a scrubbed subprocess with arbitrary argv and env. + + Same isolation contract as `_collect_integration` (throwaway HOME, blocked + sockets, credentials stripped) but without assuming the shape of the + command, so indirect targeting routes can be exercised. + """ + env = dict(os.environ) + env.pop(OPT_IN_VAR, None) + env["HOME"] = str(tmp_home) + env["USERPROFILE"] = str(tmp_home) + sitecustomize = tmp_home / "sitecustomize.py" + sitecustomize.write_text( + "import socket\n" + "def _deny(*a, **k):\n" + " raise OSError('network disabled by test_billable_safety')\n" + "socket.socket.connect = _deny\n" + "socket.socket.connect_ex = _deny\n" + "socket.create_connection = _deny\n", + encoding="utf-8", + ) + env["PYTHONPATH"] = os.pathsep.join( + [str(tmp_home), env.get("PYTHONPATH", "")] + ).rstrip(os.pathsep) + for leaked in ( + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + "AWS_PROFILE", + "AZURE_CLIENT_SECRET", + "GOOGLE_APPLICATION_CREDENTIALS", + "LAMBDA_CLOUD_API_KEY", + ): + env.pop(leaked, None) + env.update(env_extra or {}) + return subprocess.run( + [sys.executable, "-m", "pytest", "--collect-only", "-q", "-o", "addopts="] + + list(argv_extra), + cwd=str(cwd or REPO_ROOT), + env=env, + capture_output=True, + encoding="utf-8", + errors="replace", + timeout=300, + ) + + +# Each entry is an *indirect* way to aim pytest at tests/integration -- one +# that does not put the path anywhere in the typed argv, or that spells it +# relative to something other than the repo root. Every one of these was a +# working bypass at some point during #130 and is now refused. See the module +# docstring of tests/conftest.py for why config.args is the right thing to read. +_TARGET = "tests/integration/test_timeout_mechanism.py" +INDIRECT_TARGETING = [ + pytest.param( + {"argv_extra": ["-o", f"testpaths={_TARGET}"]}, + id="testpaths-overridden-on-command-line", + ), + pytest.param( + {"env_extra": {"PYTEST_ADDOPTS": _TARGET}}, + id="path-injected-through-PYTEST_ADDOPTS", + ), + pytest.param( + {"argv_extra": ["--pyargs", "tests.integration.test_timeout_mechanism"]}, + id="dotted-module-name-via-pyargs", + ), + pytest.param( + { + "argv_extra": ["integration/test_timeout_mechanism.py"], + "from_tests_dir": True, + }, + id="path-relative-to-a-subdirectory-cwd", + ), +] + + +@pytest.mark.parametrize("case", INDIRECT_TARGETING) +def test_indirect_targeting_of_integration_is_refused(case, tmp_path): + """Aiming pytest at tests/integration without typing the path is refused. + + The guard originally read `config.invocation_params.args` -- literally what + the operator typed. That is not the same as what pytest will collect: + `testpaths`, `-o testpaths=` and `PYTEST_ADDOPTS` all feed `config.args` + without appearing in the typed argv, and a path spelled relative to a + subdirectory did not resolve against the repo root. Each of these collected + billable modules while the guard believed nothing had been targeted. + + Money is the reason this is parametrized rather than merged into one test: + a single assertion that stops at the first failure would hide the others. + """ + case = dict(case) + from_tests_dir = case.pop("from_tests_dir", False) + result = _run_pytest( + tmp_path, + argv_extra=case.get("argv_extra", []), + env_extra=case.get("env_extra"), + cwd=(REPO_ROOT / "tests") if from_tests_dir else None, + ) + combined = (result.stdout or "") + (result.stderr or "") + + collected = [ + line + for line in combined.splitlines() + if "integration/test_timeout_mechanism" in line.replace("\\", "/") + and "::" in line + ] + assert not collected, ( + "Billable integration tests were collected through an indirect route " + "the guard did not see:\n" + "\n".join(collected[:10]) + ) + assert "Refusing to run" in combined, ( + f"The guard did not refuse this run (exit {result.returncode}). It is " + f"reading something narrower than the effective target list.\n" + + combined[-2000:] + ) + + def test_explicitly_targeting_integration_is_refused(tmp_path): """Naming the directory or a file in it must fail loudly, not silently. diff --git a/tests/unit/test_pytest_config.py b/tests/unit/test_pytest_config.py index a784bea9..55670239 100644 --- a/tests/unit/test_pytest_config.py +++ b/tests/unit/test_pytest_config.py @@ -23,9 +23,18 @@ EXPECTED_CONFIG_NAME = "pyproject.toml" -# Config files pytest will prefer over pyproject.toml if they exist at the -# rootdir, in pytest's own precedence order. Any of these silently wins. -SHADOWING_CONFIG_NAMES = ("pytest.ini", ".pytest.ini", "tox.ini", "setup.cfg") +# Config files pytest prefers over pyproject.toml. +# +# pytest's search order (_pytest/config/findpaths.py) is: +# pytest.ini, .pytest.ini, pyproject.toml, tox.ini, setup.cfg +# +# Only the first two outrank pyproject.toml, so only those two can shadow it. +# tox.ini and setup.cfg are deliberately NOT listed: they are checked *after* +# pyproject.toml and cannot take precedence over it. Listing them would also +# forbid a perfectly ordinary pytest-free setup.cfg holding flake8 or metadata +# config -- verified: with such a file present pytest still reports +# `configfile: pyproject.toml`. +SHADOWING_CONFIG_NAMES = ("pytest.ini", ".pytest.ini") # Markers this project defines and relies on. Registration is what makes # ``-m `` a usable selector and what lets --strict-markers catch typos. @@ -57,13 +66,14 @@ def test_no_shadowing_config_file_exists(pytestconfig): ``test_pytest_reads_the_intended_config`` only catches a shadow that is live during *this* run. A config file that exists but happens not to win - (for instance because the run was launched from a subdirectory with a - different rootdir) would slip past it, so check the tree directly. + (for instance because the run was launched with ``--rootdir`` pointed + elsewhere) would slip past it, so check the repository tree directly rather + than trusting the resolved rootdir. """ - rootdir = pathlib.Path(str(pytestconfig.rootpath)) - strays = [name for name in SHADOWING_CONFIG_NAMES if (rootdir / name).exists()] + repo_root = pathlib.Path(__file__).resolve().parents[2] + strays = [name for name in SHADOWING_CONFIG_NAMES if (repo_root / name).exists()] assert not strays, ( - f"found {strays} at {rootdir}; pytest prefers these over " + f"found {strays} at {repo_root}; pytest prefers these over " f"{EXPECTED_CONFIG_NAME} and stops searching once one is selected, so " f"{EXPECTED_CONFIG_NAME} would be silently ignored (see #130)" ) From 6c36bf3e1bb60c98fb1bc0de887eef8923811dd9 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 17 Aug 2026 13:01:01 -0400 Subject: [PATCH 11/11] Issue #130: Revert the fast_ci.yml change; the diagnosis was wrong 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) Claude-Session: https://claude.ai/code/session_012gTBDPK16HUZ3kHQ2QyjuU --- .github/workflows/fast_ci.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/.github/workflows/fast_ci.yml b/.github/workflows/fast_ci.yml index cda92f9a..c297fb98 100644 --- a/.github/workflows/fast_ci.yml +++ b/.github/workflows/fast_ci.yml @@ -39,11 +39,7 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - # pytest-timeout is required: the test step below passes - # --timeout=60, and without the plugin pytest exits 4 with - # "unrecognized arguments". This job had been failing on every run - # for that reason (see #130). - pip install black flake8 mypy pytest pytest-timeout + pip install black flake8 mypy pytest pip install -e . - name: Format check with Black