From e9847b8ac989c018a7f741bd7ce0a3c3a9cbcd8c Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 11:44:46 +0200 Subject: [PATCH 1/2] fix(tests): one honest failure instead of twenty-five misleading ones (C-104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 2026-08-16 a pytest run here reported 25 failures across five modules and none was a defect. The virtualenv held views-pipeline-core 2.3.0 and pyarrow 23.0.1 while poetry.lock pinned 3.0.1 and 16.1.0. Two majors of drift produce: - ModuleNotFoundError on views_pipeline_core.modules.dataloaders. datafactory_contract, imported at module scope by BOTH managers, which takes out every test that constructs or inspects one — the only coverage the two largest modules in the package have; - five byte-parity failures against the ADR-013 §10 fixture, because parquet bytes are not stable across pyarrow majors (C-72). From the failure output neither is distinguishable from a real regression, and "you broke the wire contract" is the reading that costs an afternoon. tests/test_locked_environment.py compares the installed versions of the runtime dependencies DECLARED in pyproject.toml against poetry.lock. Declared, not hardcoded: a literal list goes stale the first time a dependency is added, which is the failure conftest.PARTNER_PACKAGES exists to prevent one level up. The failure names each drifted package, both versions, the command, and says plainly that the other failures are consequences rather than defects — and that if the drift is deliberate, the rest of the suite is not testing the locked contract. A second check catches the other direction: declared in pyproject but absent from the lock, which would otherwise read as a virtualenv problem when it is a stale lock. It fixes nothing and skips nothing. The 25 stay until `poetry install` runs. What changes is that a contributor can tell in one line which kind of problem they have — the failures were never wrong, they were unreadable. Dev-group tools are out of scope on purpose: ruff's reported version varies with how it was installed, and what actually broke CI on 2026-08-03 was its rule set, already pinned explicitly in pyproject.toml. Natural experiment rather than a mutation: this FAILS here, against the real drift, and must PASS in CI, which installs from the lock. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 8 ++- tests/test_locked_environment.py | 106 +++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 tests/test_locked_environment.py diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 0cf8b12..d45073e 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -1104,7 +1104,13 @@ Measured 2026-08-16 in the project venv: 458 collected, **433 passed, 25 failed* The consequence is that in a drifted checkout the two largest modules in the package — 387 lines each, 21% of the source — are not merely under-covered but **entirely unexercised**, and the suite reports that in a form indistinguishable from a real break. CI runs `poetry install` and gets the locked versions, so this is a local condition rather than a CI one — **C-81**'s asymmetry running in the other direction, with the laptop the weaker seat rather than the stronger. **C-36** is the precedent for what a suite that is red for a known reason costs: it stops being read. -Cross-refs: **C-72** (owns the pyarrow half — that half is not re-registered here), **C-81** (CI-versus-local coverage asymmetry), **C-36** (a permanently-red suite cannot detect new regressions). +**Partial mitigation, 2026-08-17.** `tests/test_locked_environment.py` compares the installed versions of the runtime dependencies **declared in `pyproject.toml`** (read from there, not hardcoded) against `poetry.lock`, and fails with one message naming each drifted package, both versions, the command to run, and — the part that matters — that the other failures in the run are consequences rather than defects. Verified against the live drift: it reports `views-pipeline-core installed=2.3.0 locked=3.0.1` and `pyarrow installed=23.0.1 locked=16.1.0`. A second check catches the other direction, a dependency declared in `pyproject.toml` but absent from the lock, which would otherwise be reported as a virtualenv problem when it is a stale lock. + +It does **not** fix the drift and does not skip. The 25 failures remain until someone runs `poetry install`; what changes is that a contributor can now tell in one line which kind of problem they have. That is the whole of the entry's cost — the failures were never wrong, they were unreadable — so the entry stays open only until the environment is actually reconciled, which is a machine action rather than engineering work. + +Dev-group tools are deliberately out of scope: `ruff`'s reported version varies with how it was installed, and the thing that actually broke CI on 2026-08-03 was its *rule set*, which `pyproject.toml` already pins explicitly. + +Cross-refs: **C-72** (owns the pyarrow half — that half is not re-registered here), **C-81** (CI-versus-local coverage asymmetry), **C-36** (a permanently-red suite cannot detect new regressions), **C-102** (the same argument in the other direction: a guard that never runs proves nothing, and a failure nobody can read is not a signal). --- diff --git a/tests/test_locked_environment.py b/tests/test_locked_environment.py new file mode 100644 index 0000000..bd4f6dd --- /dev/null +++ b/tests/test_locked_environment.py @@ -0,0 +1,106 @@ +"""The environment the suite runs in is the environment the lockfile describes (C-104). + +**Why this exists.** On 2026-08-16 a `pytest` run in this repository reported 25 +failures across five modules. None was a defect. The virtualenv held +`views-pipeline-core 2.3.0` and `pyarrow 23.0.1` while `poetry.lock` pinned `3.0.1` +and `16.1.0`, and the two majors of drift produced: + +* `ModuleNotFoundError: views_pipeline_core.modules.dataloaders.datafactory_contract` + at the import of both managers — which takes out **every** test that constructs or + inspects one, i.e. the only coverage the two largest modules in the package have; +* five byte-parity failures against the ADR-013 §10 golden fixture, because parquet + bytes are not stable across pyarrow majors (C-72). + +Neither is distinguishable, from the failure output, from a real regression. A +contributor reading that wall has no way to tell "your venv is stale" from "you broke +the wire contract", and the second reading is the one that costs an afternoon. + +**What this does not do.** It does not fix the drift and it does not skip. It replaces +25 misleading failures with one that names the packages, the two versions, and the +command. The other 25 stay until `poetry install` runs — they are the *symptom*, this +is the *diagnosis*, and the repo's position (ADR-014 §1) is that a diagnosis belongs in +a check rather than in a register entry nobody reads at the moment it would help. + +**Scope: the declared runtime dependencies, read from `pyproject.toml`.** Not a +hardcoded list — that would go stale the first time a dependency is added, which is +the failure mode `tests/conftest.py::PARTNER_PACKAGES` exists to prevent one level up. +Dev-group tools are deliberately out: `ruff`'s version genuinely varies by how it was +installed, and its *rule set* — the thing that actually broke CI on 2026-08-03 — is +already pinned explicitly in `pyproject.toml`. + +CI runs `poetry install` before `pytest`, so this passes there by construction. It is +aimed squarely at the developer machine, which is the seat that had the problem. +""" + +from __future__ import annotations + +import tomllib +from importlib import metadata +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parent.parent +_PYPROJECT = _REPO / "pyproject.toml" +_LOCK = _REPO / "poetry.lock" + + +def _declared_runtime_dependencies() -> list[str]: + """The distributions `pyproject.toml` declares this package needs at runtime.""" + project = tomllib.loads(_PYPROJECT.read_text()) + deps = project["tool"]["poetry"]["dependencies"] + return [name for name in deps if name != "python"] + + +def _locked_versions() -> dict[str, str]: + return {p["name"]: p["version"] for p in tomllib.loads(_LOCK.read_text())["package"]} + + +def test_the_lockfile_and_pyproject_agree_on_what_is_declared(): + """A declared dependency absent from the lock means the lock was never regenerated. + + Checked before the version comparison below, because that comparison would + otherwise report a missing package as "installed X, locked None" and send the + reader looking at their virtualenv rather than at `poetry lock`. + """ + locked = _locked_versions() + missing = [name for name in _declared_runtime_dependencies() if name not in locked] + assert not missing, ( + f"declared in pyproject.toml but absent from poetry.lock: {missing}. The lock " + "is stale with respect to the declaration — run `poetry lock` and commit it." + ) + + +def test_the_installed_runtime_dependencies_match_the_lockfile(): + """One honest failure instead of twenty-five misleading ones (C-104).""" + locked = _locked_versions() + drift = [] + for name in _declared_runtime_dependencies(): + try: + installed = metadata.version(name) + except metadata.PackageNotFoundError: + installed = None + expected = locked.get(name) + if installed != expected: + drift.append((name, installed, expected)) + + if not drift: + return + + table = "\n".join( + f" {name:24} installed={installed or 'NOT INSTALLED':<12} locked={expected}" + for name, installed, expected in drift + ) + pytest.fail( + "this virtualenv is not the one poetry.lock describes:\n" + f"{table}\n\n" + "Run `poetry install`.\n\n" + "Any OTHER failures in this run are most likely consequences of the drift " + "above, not defects. Two majors of views-pipeline-core remove " + "`modules.dataloaders.datafactory_contract`, which both managers import at " + "module scope — so every test that touches a manager dies at collection. A " + "pyarrow major changes emitted parquet bytes, which fails the ADR-013 §10 " + "byte-parity fixture (C-72). Fix the environment before reading them as bugs.\n\n" + "If the drift is deliberate — trialling an upgrade — this test is telling you " + "the truth and the rest of the suite is not testing the locked contract." + ) From b5c4991067fc23e8d4c76f37380a86acac8ccd91 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 11:54:27 +0200 Subject: [PATCH 2/2] fix(tests): /code-review findings on the lock-drift guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight findings. The first is the one that matters: the guard whose entire purpose is a readable diagnosis stated something measurably false. 1. "every test that touches a manager dies at collection" — it does not. The manager tests import lazily (tests/test_framework_contract.py:50), so the drift produces ordinary failures and the session runs to completion. Measured in the drifted venv: 26 failed, 441 passed, 0 errors. The claim was also self-defeating — pytest interrupts on collection errors, so had it been true this diagnostic would never have run in the scenario it was written for. Corrected in the message and the module docstring. 2. The consequences paragraph was hardcoded to the two packages that drifted on 2026-08-16 but fired verbatim for any drift: a views-frames bump would have been explained in terms of pipeline-core and parquet bytes. Now conditional on which names are actually in the drift set — a diagnosis naming the wrong packages is the failure this file exists to remove. 3. No PEP 503 normalization between pyproject keys and lock names. `PyYAML` or `views_frames` are legal declarations that would never match a lock entry, and the failure would have read "absent from poetry.lock, run `poetry lock`" — which fixes nothing, because the lock is fine. Both sides normalized. 4. Dependencies gated by `optional`, `python` or `markers` were treated as must-be-installed. They are legitimately absent from a given environment, and the message would have said "NOT INSTALLED — run `poetry install`", advice that cannot work. It would also have gone red in CI on the 3.11 runner for a python-gated dep. Skipped. Verified that `extras`-carrying deps are still KEPT, since that is our real views-pipeline-core declaration. 5. The claimed ordering guarantee did not hold: running the lock check first does not stop the version check reporting the same omission as `locked=None`. The version check now skips names absent from the lock and defers. 6. `installed=None, expected=None` was a silent pass. Same fix as 5 — unlocked names are the other test's business, and it says so better. 7. `project["tool"]["poetry"]["dependencies"]` raised a bare KeyError on a PEP 621 migration — the unreadable failure this module was written against. Now fails with the form it expects and an instruction not to delete it, matching tests/test_release_version.py's precedent. 8. Register `Last Updated` was still 2026-08-16, stale since C-103's additions. Verified: normalization on PyYAML/views_frames/Foo.Bar_baz; the conditional-skip rule against optional/python/markers/extras shapes; the message now naming only the packages that drifted. ruff clean; register and doc-accuracy guards green. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 6 +- tests/test_locked_environment.py | 126 +++++++++++++++++++++-------- 2 files changed, 98 insertions(+), 34 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index d45073e..0db40c2 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -4,7 +4,7 @@ |-------------------|--------------------------------------| | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | -| Last Updated | 2026-08-16 | +| Last Updated | 2026-08-17 | | Total Concerns | 108 | | Open Concerns | 28 | | Resolved Concerns | 80 | @@ -1104,7 +1104,9 @@ Measured 2026-08-16 in the project venv: 458 collected, **433 passed, 25 failed* The consequence is that in a drifted checkout the two largest modules in the package — 387 lines each, 21% of the source — are not merely under-covered but **entirely unexercised**, and the suite reports that in a form indistinguishable from a real break. CI runs `poetry install` and gets the locked versions, so this is a local condition rather than a CI one — **C-81**'s asymmetry running in the other direction, with the laptop the weaker seat rather than the stronger. **C-36** is the precedent for what a suite that is red for a known reason costs: it stops being read. -**Partial mitigation, 2026-08-17.** `tests/test_locked_environment.py` compares the installed versions of the runtime dependencies **declared in `pyproject.toml`** (read from there, not hardcoded) against `poetry.lock`, and fails with one message naming each drifted package, both versions, the command to run, and — the part that matters — that the other failures in the run are consequences rather than defects. Verified against the live drift: it reports `views-pipeline-core installed=2.3.0 locked=3.0.1` and `pyarrow installed=23.0.1 locked=16.1.0`. A second check catches the other direction, a dependency declared in `pyproject.toml` but absent from the lock, which would otherwise be reported as a virtualenv problem when it is a stale lock. +**Partial mitigation, 2026-08-17.** `tests/test_locked_environment.py` compares the installed versions of the runtime dependencies **declared in `pyproject.toml`** (read from there, not hardcoded) against `poetry.lock`, and fails with one message naming each drifted package, both versions, the command to run, and — the part that matters — that the other failures in the run are consequences rather than defects. Verified against the live drift: it reports `views-pipeline-core installed=2.3.0 locked=3.0.1` and `pyarrow installed=23.0.1 locked=16.1.0`. A second check owns the other direction — declared in `pyproject.toml` but absent from the lock — and the version check *skips* names it cannot find rather than reporting them as `locked=None`, so a stale lock is diagnosed once, correctly, instead of twice with one of the two sending the reader at their virtualenv. + +Three things it deliberately does not treat as drift: dependencies gated by `optional`, `python` or `markers` (legitimately absent from a given environment — reporting one as "run `poetry install`" would be advice that cannot work), name spellings that differ only by case or separator (both sides are PEP 503-normalized, so `PyYAML` and `views_frames` match their lock entries), and dev-group tools. The failure text names only the packages that actually drifted: the 2026-08-16 incident was pipeline-core and pyarrow, a future one will not be, and a diagnosis describing the wrong packages is the failure this file exists to remove. It does **not** fix the drift and does not skip. The 25 failures remain until someone runs `poetry install`; what changes is that a contributor can now tell in one line which kind of problem they have. That is the whole of the entry's cost — the failures were never wrong, they were unreadable — so the entry stays open only until the environment is actually reconciled, which is a machine action rather than engineering work. diff --git a/tests/test_locked_environment.py b/tests/test_locked_environment.py index bd4f6dd..7f09632 100644 --- a/tests/test_locked_environment.py +++ b/tests/test_locked_environment.py @@ -6,8 +6,8 @@ and `16.1.0`, and the two majors of drift produced: * `ModuleNotFoundError: views_pipeline_core.modules.dataloaders.datafactory_contract` - at the import of both managers — which takes out **every** test that constructs or - inspects one, i.e. the only coverage the two largest modules in the package have; + wherever a test imports a manager — which is every test that constructs or inspects + one, i.e. the only coverage the two largest modules in the package have; * five byte-parity failures against the ADR-013 §10 golden fixture, because parquet bytes are not stable across pyarrow majors (C-72). @@ -15,18 +15,27 @@ contributor reading that wall has no way to tell "your venv is stale" from "you broke the wire contract", and the second reading is the one that costs an afternoon. +**These are ordinary test failures, not collection errors** — the manager tests import +lazily (`tests/test_framework_contract.py:50`), so the session runs to completion. +Measured 2026-08-17 in the drifted venv: 26 failed, 441 passed, **0 errors**. That +distinction is not pedantry: a collection error would interrupt the session and this +diagnostic would never get to run in the scenario it was written for. + **What this does not do.** It does not fix the drift and it does not skip. It replaces -25 misleading failures with one that names the packages, the two versions, and the -command. The other 25 stay until `poetry install` runs — they are the *symptom*, this -is the *diagnosis*, and the repo's position (ADR-014 §1) is that a diagnosis belongs in -a check rather than in a register entry nobody reads at the moment it would help. - -**Scope: the declared runtime dependencies, read from `pyproject.toml`.** Not a -hardcoded list — that would go stale the first time a dependency is added, which is -the failure mode `tests/conftest.py::PARTNER_PACKAGES` exists to prevent one level up. -Dev-group tools are deliberately out: `ruff`'s version genuinely varies by how it was -installed, and its *rule set* — the thing that actually broke CI on 2026-08-03 — is -already pinned explicitly in `pyproject.toml`. +a wall of misleading failures with one that names the packages, both versions, and the +command. The others stay until `poetry install` runs — they are the *symptom*, this is +the *diagnosis*, and the repo's position (ADR-014 §1) is that a diagnosis belongs in a +check rather than in a register entry nobody reads at the moment it would help. + +**Scope: the unconditional runtime dependencies declared in `pyproject.toml`**, read +from there rather than hardcoded — a literal list goes stale the first time a +dependency is added, which is the failure `tests/conftest.py::PARTNER_PACKAGES` exists +to prevent one level up. Dependencies gated by `optional`, `python` or `markers` are +skipped: they are legitimately absent from some environments, and reporting one as +"NOT INSTALLED — run `poetry install`" would be advice that cannot work. Dev-group +tools are out too: `ruff`'s reported version varies with how it was installed, and the +thing that actually broke CI on 2026-08-03 was its *rule set*, which `pyproject.toml` +already pins explicitly. CI runs `poetry install` before `pytest`, so this passes there by construction. It is aimed squarely at the developer machine, which is the seat that had the problem. @@ -34,6 +43,7 @@ from __future__ import annotations +import re import tomllib from importlib import metadata from pathlib import Path @@ -44,43 +54,74 @@ _PYPROJECT = _REPO / "pyproject.toml" _LOCK = _REPO / "poetry.lock" +#: Spec keys that make a dependency conditional, so its absence is not drift. +_CONDITIONAL = ("optional", "python", "markers") + + +def _normalize(name: str) -> str: + """PEP 503 normalization. + + `poetry.lock` stores normalized names; `pyproject.toml` stores whatever the author + typed. `PyYAML` and `views_frames` are both legal declarations that would never + match a lock entry compared raw — and the failure would read "absent from + poetry.lock, run `poetry lock`", which would fix nothing because the lock is fine. + """ + return re.sub(r"[-_.]+", "-", name).lower() + def _declared_runtime_dependencies() -> list[str]: - """The distributions `pyproject.toml` declares this package needs at runtime.""" + """Unconditional runtime dependency names, normalized.""" project = tomllib.loads(_PYPROJECT.read_text()) - deps = project["tool"]["poetry"]["dependencies"] - return [name for name in deps if name != "python"] + try: + deps = project["tool"]["poetry"]["dependencies"] + except KeyError: + pytest.fail( + "pyproject.toml no longer declares [tool.poetry.dependencies] in the form " + "this guard reads — a PEP 621 [project] migration would do this. Teach the " + "test the new form; do not delete it, or nothing compares the environment " + "to the lock again (C-104). Same treatment as tests/test_release_version.py." + ) + return [ + _normalize(name) + for name, spec in deps.items() + if name != "python" + and not (isinstance(spec, dict) and any(k in spec for k in _CONDITIONAL)) + ] def _locked_versions() -> dict[str, str]: - return {p["name"]: p["version"] for p in tomllib.loads(_LOCK.read_text())["package"]} + packages = tomllib.loads(_LOCK.read_text())["package"] + return {_normalize(p["name"]): p["version"] for p in packages} -def test_the_lockfile_and_pyproject_agree_on_what_is_declared(): +def test_every_declared_dependency_is_in_the_lockfile(): """A declared dependency absent from the lock means the lock was never regenerated. - Checked before the version comparison below, because that comparison would - otherwise report a missing package as "installed X, locked None" and send the - reader looking at their virtualenv rather than at `poetry lock`. + Owned here rather than by the version check below, which skips names it cannot + find — otherwise the same omission is reported twice, once correctly as a stale + lock and once misleadingly as "installed X, locked None, run `poetry install`". """ locked = _locked_versions() - missing = [name for name in _declared_runtime_dependencies() if name not in locked] + missing = sorted(set(_declared_runtime_dependencies()) - set(locked)) assert not missing, ( f"declared in pyproject.toml but absent from poetry.lock: {missing}. The lock " - "is stale with respect to the declaration — run `poetry lock` and commit it." + "is stale with respect to the declaration — run `poetry lock` and commit it. " + "This is not a virtualenv problem and `poetry install` will not fix it." ) def test_the_installed_runtime_dependencies_match_the_lockfile(): - """One honest failure instead of twenty-five misleading ones (C-104).""" + """One honest failure instead of a wall of misleading ones (C-104).""" locked = _locked_versions() drift = [] for name in _declared_runtime_dependencies(): + expected = locked.get(name) + if expected is None: + continue # not locked at all — the test above owns that, and says so better try: installed = metadata.version(name) except metadata.PackageNotFoundError: installed = None - expected = locked.get(name) if installed != expected: drift.append((name, installed, expected)) @@ -91,16 +132,37 @@ def test_the_installed_runtime_dependencies_match_the_lockfile(): f" {name:24} installed={installed or 'NOT INSTALLED':<12} locked={expected}" for name, installed, expected in drift ) + + # Say what THIS drift causes, not what some remembered drift once caused. The 2026 + # incident was pipeline-core and pyarrow; a future one will not be, and a diagnosis + # that describes the wrong packages is the failure this file exists to remove. + drifted = {name for name, _, _ in drift} + consequences = [] + if "views-pipeline-core" in drifted: + consequences.append( + " - views-pipeline-core: the managers import " + "`modules.dataloaders.datafactory_contract` at module scope, so every test " + "that imports a manager fails. Those are ordinary failures, not collection " + "errors — the whole suite still runs." + ) + if "pyarrow" in drifted: + consequences.append( + " - pyarrow: emitted parquet bytes are not stable across majors, so the " + "ADR-013 §10 byte-parity fixture fails (C-72)." + ) + detail = ( + "Known consequences of the packages above:\n" + "\n".join(consequences) + "\n\n" + if consequences + else "" + ) + pytest.fail( "this virtualenv is not the one poetry.lock describes:\n" f"{table}\n\n" "Run `poetry install`.\n\n" - "Any OTHER failures in this run are most likely consequences of the drift " - "above, not defects. Two majors of views-pipeline-core remove " - "`modules.dataloaders.datafactory_contract`, which both managers import at " - "module scope — so every test that touches a manager dies at collection. A " - "pyarrow major changes emitted parquet bytes, which fails the ADR-013 §10 " - "byte-parity fixture (C-72). Fix the environment before reading them as bugs.\n\n" + f"{detail}" + "Other failures in this run are likely consequences of the drift above rather " + "than defects. Fix the environment before reading them as bugs.\n\n" "If the drift is deliberate — trialling an upgrade — this test is telling you " - "the truth and the rest of the suite is not testing the locked contract." + "the truth, and the rest of the suite is not testing the locked contract." )