test(adr): check the code citations in the ADR corpus against the tree - #807
Conversation
74 citations of the form File.php:NNN across Documentation/Adr, and nothing read them. AdrLifecycleTest checks form inside the corpus, AdrReferenceIntegrityTest checks that ADR filenames named elsewhere resolve, and its docblock says outright that a drifted line number still points at a line that exists. Three assertions, each seen to fail before being trusted: - a cited file that is in neither the tree nor the declared list - a citation past the end of its file, or at a blank line - a change to the set of citations that resolve to nothing here The census turned up something the issue did not: 21 of the 74 point into .Build/vendor — TYPO3 core, cms-install, nr-vault. That code is not committed and its line numbers move with every patch release of a dependency this repository does not pin, so no test here can check them. They are listed by hand instead, which makes adding one deliberate rather than something the resolver quietly skips. What it does not catch is in the docblock: a line that moved onto different but non-blank code. That is the common case and the dangerous one, and nothing mechanical can tell the difference. The rot #793 names — ADR-171 citing ResumeCoordinator.php:204 for a check that d3a8d71 pushed to :205 — is already repaired on main. This test does not fix a live defect; it stops the next one from being invisible. Refs #793 Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #807 +/- ##
=========================================
Coverage 87.43% 87.43%
Complexity 9710 9710
=========================================
Files 566 566
Lines 31418 31418
=========================================
Hits 27471 27471
Misses 3947 3947
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Review record — no Copilot review exists for this diff (monthly, account-wide quota exhausted), so the green gate is not a read of the change. What was checked: The test was built from a census, not from the issue. #793 said 74 citations and one confirmed rot. Re-derived against Each of the three assertions was seen to fail — a missing file, a blank line, a past-EOF line, an undeclared vendor path, and an ambiguous basename each produce the failure they exist for. Reverted, green, What it does not catch is in the docblock, not only in the PR: a line that moved onto different but non-blank code. That is the common case and the dangerous one, and nothing mechanical distinguishes it. Cross-checked against the siblings in flight, because this test reads the whole corpus and two other open PRs edit ADR text: none of #808, #810 or #811 adds a
Merging on that basis. |
The ADR corpus carries 74 citations of the form
File.php:NNN, and nothing read any of them.AdrLifecycleTestchecks form inside the corpus;AdrReferenceIntegrityTestchecks that ADR filenames named from outside it resolve, and its docblock says outright that this is the part it does not cover — "a line number that has drifted still points at a line that exists".What the census found that #793 did not
21 of the 74 citations point into
.Build/vendor: TYPO3 core (DataHandler.php,RootLevelCapability.php,BackendUtility.php, …),cms-install, and the sibling extensionnr-vault. 18 of them are in ADR-169 alone. That code is not committed and its line numbers move with every patch release of a dependency this repository does not pin, so no test here can check them — and they are the most fragile citations in the corpus, not the least.They are declared in a hand-maintained list instead. A new one fails the third assertion rather than being silently skipped by the resolver, which is the difference between a list and a blind spot.
The remaining 53 resolve to exactly one file each. Basename resolution is unambiguous today, and the resolver asserts that rather than picking the first match.
Assertions, each seen to fail
A guard that has only been seen green proves it runs, not that it catches anything. Every branch was broken on purpose and the failure observed:
NoSuchFileAnywhere.php:12everyCitationNamesAFileThatStillExists+ the list assertion failResumeCoordinator.php:204(blank line)noCitationPointsPastTheEndOfItsFileOrAtABlankLinefailsResumeCoordinator.php:99999(past EOF)DataHandler.php:1, undeclared for that recordResumeCoordinator.phpadded to the treeEach was reverted and the suite returned to green;
git statuswas clean afterwards.What it does not catch
Stated in the docblock, not only here: a line that moved onto different but non-blank code. That is the common case and the dangerous one — the record then asserts something the cited code does not say, and the citation's precision is what makes a reviewer trust it. Nothing mechanical can tell the difference, which is the argument for citing a symbol rather than a line number wherever the prose allows.
One correction to the issue
#793 gives ADR-171 citing
ResumeCoordinator.php:204as confirmed rot. That is already repaired onmain— the record cites:205, which is whered3a8d718moved the check. So this test does not fix a live defect. It stops the next one from being invisible, and the corpus is clean as of this commit.I found that by re-running the census against
origin/mainrather than against themain/worktree, which was ten commits behind when I first measured.#793 stays open: it also asks for the convention (symbol over line number) and covers the two counting issues, #791 and #792. This is the enforceable part.
Tests:
unit(exit 0),phpstanlevel 10 clean,cglandrector -nboth stable at PHP 8.2 after applying.Refs #793