diff --git a/CHANGES.md b/CHANGES.md index 666fc4b..3ae14af 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -20,6 +20,55 @@ their date-only headings. --- +## 4.1.1 - 2026-09-03 - Testisolation: kb_dir repointet config.ROOT, lint löst Kollektionen gegen den übergebenen Baum auf + +**Author:** Torben Nehmer + +Issue #44, gefunden beim Bau der Migrations-Gate-Tests für 4.1.0: die `kb_dir`-Fixture baute +ihren Baum unter `tmp_path`, ließ `config.ROOT` aber auf dem echten Checkout stehen. Jeder +Codepfad, der eine Datei über `config.ROOT`/`config.KB_DIR` auflöst statt über das übergebene +Verzeichnis, traf damit das echte Repository. + +**Der laute Fall** war ein Test, der `kb_state.write_kb_state()` rief und dabei das +`.wikitool-kb.json` des Repos überschrieb — Applied-Ledger leer statt zwei Einträgen. In +`git status` sofort sichtbar und reversibel; bei einer gitignorierten Datei wäre es das nicht +gewesen. + +**Der stillere Fall** ist der teurere. `lint`s Kollektions-Lookup löste eine Seite gegen +`config.KB_DIR` auf. Für eine Seite unter `tmp_path/kb/` warf das `ValueError`, die Funktion +antwortete „keine Kollektion", und die Label-Autorisierung übersprang die Kante wortlos. +`unauthorised_labels` war damit faktisch ungetestet — jeder Test, der das Finding hätte +auslösen können, bekam eine leere Liste und behauptete nichts. Ein grüner Lauf, der wie eine +Zusicherung aussah. + +**Der Fix ist der Codepfad, nicht die Fixture.** `run_lint()` bekommt ein Verzeichnis +übergeben und löst jetzt auch intern dagegen auf; `authorised_labels()` bekommt denselben Baum +gereicht, statt auf `config.KB_DIR` zurückzufallen. Der Regressionstest lintet einen Baum, von +dem `ROOT` bewusst wegzeigt — genau der Fall, den die alte Auflösung verschluckte. Eine Funktion, +die ein Verzeichnis entgegennimmt, löst dagegen auf: keine Fixture kann diese Form von außen +reparieren. + +**Beide Korpus-Fixturen repointen jetzt.** `kb_dir` tut, was `raw_dir` längst tat — `ROOT` auf +das eigene `tmp_path`, plus `use_shipped_type_specs()`. Der Suite-Lauf kippte dadurch keinen +einzigen Test. Die lokale `rooted_kb`-Umgehung aus 4.1.0 entfällt damit; die Auswahl zwischen +zwei fast gleichen Fixturen war Wissen, das nirgends stand. + +**Und ein Wächter für die ganze Klasse.** `repository_tree_guard` (session-scoped, autouse) +vergleicht `git status --porcelain` vor und nach dem Lauf und lässt die Suite scheitern, wenn +sich im Checkout etwas bewegt hat — zwei `git status`-Aufrufe pro Lauf, deshalb per Default an. +Er vergleicht vorher gegen nachher statt einen sauberen Baum zu verlangen, sagt also nichts über +die unveröffentlichte Arbeit des Entwicklers. Den Verursacher benennt er nicht; +`CHEMENU_TREE_GUARD=each` prüft nach jedem Test und tut es. Ohne git oder außerhalb eines +Repositorys sind beide still. + +Was der Wächter nicht sieht: eine Prüfung, die unter Test nichts tut, schreibt keine Datei. +Dagegen hilft nur ein Test, der das Finding tatsächlich auslöst — der neue tut das. + +`instructions/dev/testing-conventions.md` hat dafür einen eigenen Abschnitt („Which tree a test +writes into"), einen Schritt in der Checkliste und die Regel für neue Fixturen. + +--- + ## 4.1.0 - 2026-09-03 - Link-Taxonomie: Lint-Findings hart ab kb_version 4.0.0, outbound: an das Type-Spec gebunden, part-of/composition als Inversenpaar **Author:** Torben Nehmer diff --git a/VERSION b/VERSION index ee74734..627a3f4 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -4.1.0 +4.1.1 diff --git a/instructions/dev/testing-conventions.md b/instructions/dev/testing-conventions.md index 7cf02b5..fa9c2df 100644 --- a/instructions/dev/testing-conventions.md +++ b/instructions/dev/testing-conventions.md @@ -40,6 +40,48 @@ resolved paths and `conventions`' parsed `kb/CONVENTIONS.md`. A test that *rewri conventions file mid-test calls `conventions.reset_cache()` itself - the fixture answers for the boundary between tests, not for one inside a test. +## Which tree a test writes into + +The environment is one half of the isolation; `config.ROOT` is the other. With `CHEMENU_ROOT` +cleared, `ROOT` falls back to the checkout pytest is running from - deliberately, because most +tests want the shipped `types/`. It also means that any code path resolving a file through +`config.ROOT` or `config.KB_DIR` reaches **the real repository**, no matter which tree the +fixture built. + +Both corpus fixtures therefore repoint it: `raw_dir` and `kb_dir` each set +`config.ROOT` to their `tmp_path` and re-declare the shipped `types/` through +`use_shipped_type_specs()`. `config`'s module `__getattr__` resolves the derived paths on +access, so repointing `ROOT` carries `KB_DIR`, `RAW_DIR` and the rest with it. A new fixture +that builds a tree does the same thing - that is the rule here, not a per-test judgment. + +`kb_dir` did not, until Gitea #44. Two things came of that. A test calling +`kb_state.write_kb_state()` overwrote the real `.wikitool-kb.json`, which `git status` made +visible within the minute. Quieter and worse: `lint`'s collection lookup resolved a page +against `config.KB_DIR`, so every fixture page read back as "no collection" and the +`unauthorised_labels` check skipped every edge in silence - the finding had no working test at +all, and its green run read like an assurance. + +Two guards came out of it, both in `conftest.py`: + +| Guard | Default | Cost | +|---|---|---| +| `repository_tree_guard` (session) | on | two `git status --porcelain` calls per run | +| `per_test_tree_guard` | off, `CHEMENU_TREE_GUARD=each` turns it on | one `git status` per test | + +The session guard compares the working tree before against after and fails the run if anything +moved, so it says nothing about uncommitted work a developer already had. It cannot name the +test that did it; `CHEMENU_TREE_GUARD=each` can, and is the way to bisect once it fires. Where +git is unavailable or the checkout is not a repository, both are silently inert. + +Neither guard sees the second, quieter half: a check that silently *does nothing* under test +writes no file. That one is only caught by a test that asserts the finding actually fires - +which is why `test_unauthorised_label_is_judged_in_a_tree_that_is_not_the_configured_kb` +lints a tree `ROOT` deliberately points away from. + +**A function that takes a directory resolves against that directory.** `run_lint(kb_dir)` +reading `config.KB_DIR` for one of its own lookups was the defect behind the quiet half, and +no fixture can fix that shape from the outside. + ## When to run Whenever you add or change a test under `tools/chemenu/tests/`. @@ -80,7 +122,12 @@ Whenever you add or change a test under `tools/chemenu/tests/`. `conftest.py` in the same change. A variable the tool reads and the fixture does not clear is the exact hole this whole file is about, reopened. -5. **Verify against an empty machine before publishing**, not only in your own shell: +5. **Writing a fixture that builds a tree?** Repoint `config.ROOT` at it and call + `use_shipped_type_specs(monkeypatch)`, as `raw_dir` and `kb_dir` do - see + [Which tree a test writes into](#which-tree-a-test-writes-into). A fixture that returns a + path without repointing hands the code under test the real repository. + +6. **Verify against an empty machine before publishing**, not only in your own shell: ```bash cd tools && env -i PATH="$PATH" HOME="$(mktemp -d)" \ @@ -92,7 +139,7 @@ Whenever you add or change a test under `tools/chemenu/tests/`. `.venv/bin/python -m pytest -q`. A difference between the two is a leak, and the leaking variable belongs in step 4's list. -6. **Check the coverage report when adding tests to close a gap**, rather than guessing which +7. **Check the coverage report when adding tests to close a gap**, rather than guessing which lines were uncovered: ```bash diff --git a/tools/chemenu/lint_core.py b/tools/chemenu/lint_core.py index ad3a89b..cb51c76 100644 --- a/tools/chemenu/lint_core.py +++ b/tools/chemenu/lint_core.py @@ -165,9 +165,15 @@ def run_lint(kb_dir: Path) -> dict: # propagated, a deleted page, or a URL pasted where a title belongs - used # to pass every check. Which fields hold page titles is declared by each # type-spec's `page_ref_fields:`, not hardcoded here. + # Resolved against the directory `run_lint()` was handed, not against + # `config.KB_DIR`. A page under a tree that is not the configured corpus - + # every fixture tree, and any `lint ` aimed elsewhere - raised + # `ValueError` here and read as "no collection", which made the label + # authorisation below skip the edge in silence rather than judge it + # (Gitea #44). def _collection_of(page): try: - return page.path.relative_to(config.KB_DIR).parts[0] + return page.path.relative_to(kb_dir).parts[0] except (ValueError, IndexError): return None @@ -211,7 +217,9 @@ def run_lint(kb_dir: Path) -> dict: destination = _collection_of(target_page) if destination is None: continue - allowed = kb_collections.authorised_labels(source_collection, destination) + allowed = kb_collections.authorised_labels( + source_collection, destination, kb_dir + ) if edge.label not in allowed: unauthorised_labels.append( { diff --git a/tools/chemenu/tests/conftest.py b/tools/chemenu/tests/conftest.py index fc1cbc3..08a4dce 100644 --- a/tools/chemenu/tests/conftest.py +++ b/tools/chemenu/tests/conftest.py @@ -1,4 +1,5 @@ import os +import subprocess from pathlib import Path import pytest @@ -37,6 +38,78 @@ _GIT_ENV = ( ) +def _working_tree_state() -> str | None: + """`git status --porcelain` for the checkout the tests live in, or None if + there is no git available to ask.""" + try: + result = subprocess.run( + ["git", "-C", str(config._PACKAGE_ROOT), "status", "--porcelain"], + capture_output=True, + text=True, + timeout=60, + ) + except (OSError, subprocess.SubprocessError): + return None + return result.stdout if result.returncode == 0 else None + + +_TREE_GUARD_MESSAGE = ( + "A test wrote into the repository checkout instead of into its tmp_path.\n" + "`git status --porcelain` moved while the suite ran:\n\n" + " before:\n{before}\n" + " after:\n{after}\n\n" + "This is the class of bug Gitea #44 describes: code under test resolves a " + "path through `config.ROOT`/`config.KB_DIR` rather than through the " + "directory the fixture handed it, so the write lands in the real tree. Fix " + "the fixture (repoint `config.ROOT`, as `raw_dir` and `kb_dir` do) or the " + "code path, never the symptom.\n" + "To find the test that did it, re-run with CHEMENU_TREE_GUARD=each - the " + "guard then checks after every test and fails on the first one that moves " + "the tree." +) + + +@pytest.fixture(scope="session", autouse=True) +def repository_tree_guard(): + """Fail the run if the suite moved a file in the real checkout. + + Two `git status` calls for the whole session, which is why this is on by + default: it catches the whole class rather than the one case that was + noticed. It compares before against after rather than demanding a clean + tree, so it says nothing about a developer's own uncommitted work. + + It cannot name the culprit - set `CHEMENU_TREE_GUARD=each` for that, which + trades a `git status` per test for a failure on the test that did it. + """ + before = _working_tree_state() + yield + after = _working_tree_state() + if before is None or after is None or before == after: + return + raise AssertionError( + _TREE_GUARD_MESSAGE.format(before=before or "(clean)", after=after or "(clean)") + ) + + +@pytest.fixture(autouse=True) +def per_test_tree_guard(repository_tree_guard): + """The bisect half of `repository_tree_guard`, off unless asked for. + + `CHEMENU_TREE_GUARD=each` turns the session-wide "something moved the tree" + into "this test moved the tree", at the cost of a `git status` per test. + """ + if os.environ.get("CHEMENU_TREE_GUARD") != "each": + yield + return + before = _working_tree_state() + yield + after = _working_tree_state() + if before is not None and after is not None and before != after: + raise AssertionError( + _TREE_GUARD_MESSAGE.format(before=before or "(clean)", after=after or "(clean)") + ) + + @pytest.fixture(autouse=True) def hermetic_environment(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): """Cut every test off from the machine it runs on. @@ -159,13 +232,25 @@ def raw_dir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: @pytest.fixture -def kb_dir(tmp_path: Path) -> Path: +def kb_dir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: """A minimal fixture kb/ with the standard collection layout, populated with a handful of pages covering entities/concepts/sources/comparisons. Every collection carries a COLLECTION.md, both because that is what makes it a collection and because the scanner must prove it skips them at a depth the - kb-root meta files never reach.""" + kb-root meta files never reach. + + `config.ROOT` is repointed for the same reason `raw_dir` does it, one + collection over: code under test that resolves a path through + `config.ROOT`/`config.KB_DIR` rather than through the directory it was + handed otherwise reaches the *real* repository. That was not theoretical + either - a test calling `kb_state.write_kb_state()` overwrote this + checkout's `.wikitool-kb.json`, and `lint`'s collection lookup answered + "no collection" for every fixture page, which left `unauthorised_labels` + with no working test at all (Gitea #44). + """ + monkeypatch.setattr(config, "ROOT", tmp_path) + use_shipped_type_specs(monkeypatch) kb = tmp_path / "kb" for sub in ("entities/projects", "entities/systems", "entities/tools", "entities/technologies", "entities/people", diff --git a/tools/chemenu/tests/test_hermetic_env.py b/tools/chemenu/tests/test_hermetic_env.py index c557bd0..fea26da 100644 --- a/tools/chemenu/tests/test_hermetic_env.py +++ b/tools/chemenu/tests/test_hermetic_env.py @@ -74,3 +74,23 @@ def test_wiki_author_overrides_the_git_identity(tmp_path: Path, monkeypatch.setattr(config, "ROOT", tmp_path) monkeypatch.setenv("WIKI_AUTHOR", "Env Override") assert config.default_author() == "Env Override" + + +def test_kb_dir_repoints_the_configured_root_at_its_own_tree(kb_dir: Path, tmp_path: Path): + """The other half of the isolation, and the one `kb_dir` was missing until + Gitea #44: a fixture that builds a corpus but leaves `config.ROOT` on the + real checkout hands every `config.KB_DIR` lookup the developer's own wiki - + which is how a test overwrote the repository's `.wikitool-kb.json`.""" + assert config.ROOT == tmp_path + assert config.KB_DIR == kb_dir + + +def test_raw_dir_repoints_the_configured_root_at_its_own_tree(raw_dir: Path, tmp_path: Path): + assert config.ROOT == tmp_path + assert config.RAW_DIR == raw_dir + + +def test_both_corpus_fixtures_keep_the_shipped_type_specs_reachable(kb_dir: Path): + """Repointing `ROOT` moves `TYPES_DIR` with it, so the repoint has to be + paired with `use_shipped_type_specs()` or no page type resolves at all.""" + assert (config.TYPES_DIR / "entity.md").is_file() diff --git a/tools/chemenu/tests/test_lint.py b/tools/chemenu/tests/test_lint.py index 61032ad..a75f604 100644 --- a/tools/chemenu/tests/test_lint.py +++ b/tools/chemenu/tests/test_lint.py @@ -15,7 +15,6 @@ from chemenu.commands.lint import ( ) from chemenu.frontmatter_io import write_page from chemenu.provenance import cite_id, render_cite_block -from chemenu.tests.conftest import use_shipped_type_specs from chemenu.version import Version @@ -482,26 +481,18 @@ def test_lint_does_not_count_a_shell_prompt_as_a_quote(kb_dir): # --- the migration gate on `unlabelled_edges` / `unauthorised_labels` ------- # -# These four repoint `config.ROOT` at the fixture tree, which the `kb_dir` -# fixture alone does not do. Two things need it: `write_kb_state()` writes -# `.wikitool-kb.json` relative to `ROOT`, and `lint`'s collection lookup -# resolves a page against `config.KB_DIR` rather than against the directory it -# was handed - so without the repoint the gate would be read off the real -# repository's state file. +# These read and write `.wikitool-kb.json`, which `kb_state` resolves relative +# to `config.ROOT`. The `kb_dir` fixture repoints `ROOT` at its own tmp_path +# (Gitea #44), so the gate is read off the fixture tree; before it did, these +# four ran against the real repository's state file and one of them overwrote +# it. -@pytest.fixture -def rooted_kb(kb_dir, tmp_path, monkeypatch): - monkeypatch.setattr(config, "ROOT", tmp_path) - use_shipped_type_specs(monkeypatch) - return kb_dir - - -def _page_with_an_unlabelled_edge(rooted_kb): +def _page_with_an_unlabelled_edge(kb_dir): """A `related:` entry that is a bare title rather than a `label: title` mapping - the shape every page was in before the 4.0.0 migration.""" write_page( - rooted_kb / "entities/tools/bare-edge.md", + kb_dir / "entities/tools/bare-edge.md", {"type": "types/entity.md", "entity_type": "tool", "tags": [], "created": "2026-09-03", "modified": "2026-09-03", "related": ["Modbus"], "sources": [], "confidence": 0.8, "provenance": "general", "summary": "One edge whose label was never declared."}, @@ -509,14 +500,14 @@ def _page_with_an_unlabelled_edge(rooted_kb): ) -def test_unlabelled_edge_is_advisory_below_kb_version_4(rooted_kb): +def test_unlabelled_edge_is_advisory_below_kb_version_4(kb_dir): """The window the migration document describes: the machinery has landed, the corpus has not been converted yet, and `lint --fail-on-error` must not refuse the very tree the migration tells the instance to publish unit by unit.""" - _page_with_an_unlabelled_edge(rooted_kb) + _page_with_an_unlabelled_edge(kb_dir) kb_state.write_kb_state(Version(3, 0, 0), []) - report = run_lint(rooted_kb) + report = run_lint(kb_dir) assert report["unlabelled_edges"] != [] assert "unlabelled_edges" not in hard_error_keys() # Narrowed to the finding under test: the fixture corpus carries unrelated @@ -525,34 +516,64 @@ def test_unlabelled_edge_is_advisory_below_kb_version_4(rooted_kb): assert has_hard_errors({"unlabelled_edges": report["unlabelled_edges"]}) is False -def test_unlabelled_edge_is_hard_at_kb_version_4(rooted_kb): +def test_unlabelled_edge_is_hard_at_kb_version_4(kb_dir): """Once the migration is recorded, a bare title is no longer a page waiting its turn - it is an edge whose author did not say what it asserts.""" - _page_with_an_unlabelled_edge(rooted_kb) + _page_with_an_unlabelled_edge(kb_dir) kb_state.write_kb_state(Version(4, 0, 0), []) - report = run_lint(rooted_kb) + report = run_lint(kb_dir) assert report["unlabelled_edges"] != [] assert "unlabelled_edges" in hard_error_keys() assert has_hard_errors({"unlabelled_edges": report["unlabelled_edges"]}) is True -def test_unauthorised_label_is_hard_at_kb_version_4(rooted_kb): +def test_unauthorised_label_is_hard_at_kb_version_4(kb_dir): """The fixture contracts authorise `depends-on` but not `contradicts`.""" write_page( - rooted_kb / "entities/tools/off-menu.md", + kb_dir / "entities/tools/off-menu.md", {"type": "types/entity.md", "entity_type": "tool", "tags": [], "created": "2026-09-03", "modified": "2026-09-03", "related": [{"contradicts": "Modbus"}], "sources": [], "confidence": 0.8, "provenance": "general", "summary": "A label off this menu."}, "\n# off-menu\n\nA label the source collection never authorised.\n", ) kb_state.write_kb_state(Version(4, 0, 0), []) - report = run_lint(rooted_kb) + report = run_lint(kb_dir) assert report["unauthorised_labels"] != [] assert "unauthorised_labels" in hard_error_keys() assert has_hard_errors({"unauthorised_labels": report["unauthorised_labels"]}) is True -def test_a_tree_that_never_declared_a_kb_version_keeps_every_key(rooted_kb): +def test_unauthorised_label_is_judged_in_a_tree_that_is_not_the_configured_kb( + kb_dir, tmp_path, monkeypatch +): + """`run_lint()` judges the tree it was handed, not the configured corpus. + + The collection lookup used to resolve a page against `config.KB_DIR`; a + page anywhere else raised `ValueError`, read back as "no collection", and + the label check skipped the edge without a word. That is why + `unauthorised_labels` was untested in practice before Gitea #44 - every + fixture tree was somewhere else. Here `ROOT` deliberately points away from + the tree under lint, which is the case the old code got wrong. + """ + write_page( + kb_dir / "entities/tools/off-menu.md", + {"type": "types/entity.md", "entity_type": "tool", "tags": [], "created": "2026-09-03", + "modified": "2026-09-03", "related": [{"contradicts": "Modbus"}], "sources": [], + "confidence": 0.8, "provenance": "general", "summary": "A label off this menu."}, + "\n# off-menu\n\nA label the source collection never authorised.\n", + ) + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + monkeypatch.setattr(config, "ROOT", elsewhere) + assert config.KB_DIR != kb_dir + + report = run_lint(kb_dir) + assert { + "page": "off-menu", "target": "Modbus", "label": "contradicts", "destination": "concepts" + } in report["unauthorised_labels"] + + +def test_a_tree_that_never_declared_a_kb_version_keeps_every_key(kb_dir): """No `.wikitool-kb.json` means a fresh instance, which starts at the current shape rather than migrating into it - so there is no outstanding migration for a gated finding to be the noise of."""