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/.gitignore b/.gitignore index d03ef976..40a1348b 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,8 @@ test-reports/ coverage.json 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/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/` 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. diff --git a/pyproject.toml b/pyproject.toml index e0246a5b..ce440e41 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", @@ -181,16 +186,51 @@ 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_*" -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 +# 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", +] +# 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] 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/scripts/pre_push_check.py b/scripts/pre_push_check.py index bb0f232c..2962cf90 100755 --- a/scripts/pre_push_check.py +++ b/scripts/pre_push_check.py @@ -43,9 +43,24 @@ 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"), - ("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 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"), ] all_passed = True diff --git a/tests/conftest.py b/tests/conftest.py index e5825815..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. @@ -34,22 +94,21 @@ 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. + + 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.args: - # 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/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" 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_billable_safety.py b/tests/unit/test_billable_safety.py index 9bd893b2..eb9bc67f 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,162 @@ 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 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, 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" + + combined[-2000:] + ) + assert (_collected_count(combined) or 0) > 0, ( + "A bare `pytest` collected nothing at all.\n" + combined[-2000:] + ) + + +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 new file mode 100644 index 00000000..55670239 --- /dev/null +++ b/tests/unit/test_pytest_config.py @@ -0,0 +1,119 @@ +"""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 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. +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 with ``--rootdir`` pointed + elsewhere) would slip past it, so check the repository tree directly rather + than trusting the resolved rootdir. + """ + 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 {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)" + ) + + +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 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 + 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)" + )