compass(tests): one recursive walk for every spec site check - #272
Conversation
The site checks in test_spec_schema.py walked the spec package with
PACKAGE.glob("*.py") while the import check in the same file used rglob,
so a refusal site in a submodule was read by one and invisible to the
other: a genuine Rule.SHAPE raise at spec/sub/extra.py left the file green.
_spec_modules() is now defined once, above the first walk, and every walk
takes its modules from it. Site keys are the path relative to the package
(via _named), so a submodule file cannot share a key with a top-level
module of the same name, and _site_of names a site the same way.
Closes #218
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| # A site's file, relative to the package, so `sub/machine.py` is not | ||
| # `machine.py`. A module loaded from outside the package walked here | ||
| # refuses rather than being compared under a name that happens to match. | ||
| return pathlib.Path(path).resolve().relative_to(PACKAGE).as_posix() |
There was a problem hiding this comment.
Non-blocking, noted for the record (principle 6: working as intended). relative_to(PACKAGE) has one consequence beyond sub-module keying, and I want it recorded.
PACKAGE comes from this test file's __file__, but _site_of passes co_filename, which comes from wherever atom was actually imported. Suppose a run's atom resolves to a different root than the tests, for example a stale PYTHONPATH or an installed copy. At the tip, the four _site_of tests compared basenames across two different trees and could pass. At this head, recording raises ValueError: ... is not in the subpath of ... inside pytest.raises(SpecRefusal), so those tests error and name both paths.
That is the refusal the comment above promises. It also catches the node-18 PYTHONPATH-overrides-the-tree hazard for free. I'm accepting it as a refusal, not a fallback.
The next task in this area should know that this file cannot be run against an installed atom. tests/compass/test_memory_readings.py takes the other approach and derives its PACKAGE from the module's __file__.
| @@ -394,7 +394,6 @@ def written(path, value): | |||
| "one field written under two spellings": lambda: both_spellings(True), | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Nit, non-blocking (principle 3). This hunk deletes one of the two blank lines between BREAKAGES and the ELSEWHERE comment block. It has nothing to do with the walk. black and ruff accept either form, so leave it or restore it. It is not worth a cycle.
Review of PR #272 (issue #218, finding A)Verdict: APPROVE at head
|
| probe | tip ff9617f30 |
head f70f15c7a |
|---|---|---|
| null control | 139 passed, rc=0 | 139 passed, rc=0 |
SHAPE raise in spec/sub/extra.py:7, through a local |
141 passed, rc=0 (blind) | rc=1: test_every_site_that_declines_a_document_is_driven_here FAILED, Extra items in the right set: ('sub/extra.py', 7) |
SHAPE raise in spec/sub/machine.py:119, the same basename and line as the real SHAPE site machine.py:119 |
141 passed, rc=0 (blind) | rc=1: same node id, Extra items in the right set: ('sub/machine.py', 119) |
same collision, head with _named mutated to Path(path).name (rglob kept) |
n/a | 141 passed, rc=0. The collision is masked. |
The last row settles the path-keying question. A plain glob to rglob swap would have stayed green on a real uncovered site, so keying by relative path is load-bearing (principle 6).
In every red run, the other site tests at the head still passed: resolves_to_a_rule, explain_declines..., and the_version_is_the_one_site....
There is one further tip defect that the head fixes. With sub/machine.py present, the tip's import check parametrised as [machine.py0], [machine.py1], [__init__.py0] and [__init__.py1]: pytest's de-dup of two identical ids. The head names them [machine.py], [sub/machine.py] and [sub/__init__.py].
3. B is already fixed at the tip: confirmed
I appended a stray def _stray_probe(value): raise ValueError(f"cannot read {value!r}") to validate.py, landing at line 417. The result was identical on both sides: rc=1, with test_every_refusal_site_in_the_package_resolves_to_a_rule FAILED on Extra items in the left set: ('validate.py', 417), and 138 passed on each.
Extra probe: the same stray raise ValueError(value) in spec/sub/stray.py.
- Tip: 141 passed, blind.
- Head: the same node id FAILED, on
('sub/stray.py', 2).
So the recursion also covers the count check, not only the SHAPE partition.
4. Key stability for the flat package
I loaded the tip's and the head's test_spec_schema.py as two modules against the same staged head tree, and compared every key by value:
_refusal_sites(): 45 = 45, equal dicts._built_where_thrown(): 43 = 43, equal._mentions_the_walk_cannot_follow():[]=[]._site_offor all 7BREAKAGESand bothELSEWHEREentries: identical tuples on both sides. For example,('merge.py', 114)and('validate.py', 283)forELSEWHERE, and('machine.py', 119)for the missing required field._sites_naming, all identical:- SHAPE has 9 sites;
- ADDRESSING is
explain.py:207andrules.py:117; - TOTALITY is
rules.py:145andrules.py:170; - VERSION is
machine.py:154.
- Parametrise ids: identical lists, from
__init__.pythroughvalidate.py. --collect-onlynode ids for the file: 139 = 139,diffempty.
No partition entry moves.
5. No design-doc references
I checked the added lines for D-numbers, principle numbers, gate labels and issue or PR numbers: none found. The comments say what the code does.
Gate: the tree that will land
- Tip re-read after
git fetch:fork/feature/atomcompass_new=ff9617f3052c93067a2f56a663ef2cea5add4c28, unchanged. git merge-tree --write-tree ff9617f30 f70f15c7agivesf979c3f47293b6aa5179dd695e30fb510beeb02e, clean. The merge-base is the tip, and the head's own tree is the same object, so the head archive is the tree that lands.- Gated once, on node 18, with the tree's own
scripts/compass/gate_cpu.sh:- run with
timeout -k 10 2400and no pipe; - stamps:
.compass-commit= the head sha, and.compass-changed=tests/compass/test_spec_schema.py; - the gate printed
commit: f70f15c7a (stamp)andgpu: not required; atom.__file__=/tmp/p272rev/head/ATOM/atom/__init__.py.- 5120 passed, 149 skipped, 3 xfailed, 0 failed,
GATE_CPU_RC=0(165 s).
- run with
This matches the developer's control and branch measurements of 5120 and 0 delta. There were no failures, so the test_stream_marker_properties.py timing classes did not come into play.
The staging was removed afterwards, and nothing was written to the shared mount.
What the next task in this area should watch
- This file now refuses to run when
atomimports from a root other than the test file's own. See the inline note at line 419. That is intended, but it rules out running against an installed copy. atom/compass/spec/is still flat. The probes above are the only evidence that the recursive walk reaches a second level.
Closes #218
Which findings were still live at the tip
Measured on node 18 (
xiaobizh_n18_cpu),tests/compass/test_spec_schema.pyalone, ongit archivecopies withatom.__file__printed under each staged root. First at308922c5c, then re-measured atff9617f30after #263 and #268 landed mid-task (both touchatom/compass/spec/, which the guard walks). The two tips gave the same results.ff9617f30f70f15c7araise SpecRefusal(Rule.SHAPE, ...)atatom/compass/spec/sub/extra.py:6, relative import like the package's own modulesrglobimport check picking the module uptest_every_site_that_declines_a_document_is_driven_here:Extra items in the right set: ('sub/extra.py', 6)atom/compass/spec/sub/machine.py:119, the same basename and line as the real SHAPE sitemachine.py:119Extra items in the right set: ('sub/machine.py', 119)raise ValueError(value)appended atmachine.py:338test_every_refusal_site_in_the_package_resolves_to_a_rule:Extra items in the left set: ('machine.py', 338)_refusal_sites,_mentions_the_walk_cannot_follow,_built_where_thrown) usedPACKAGE.glob("*.py"). The import check about 700 lines further down usedrglob.assert thrown <= set(_refusal_sites()), and pytest diffs that item by item. This PR does not change it.How the walks now agree by construction
_spec_modules()is now defined once, above the first walk, and is the only place the package is listed. All four readers of the package source take their modules from it: the three site walks and the parametrised import check. Before, there were four independentglob/rglobcalls. For the walks to diverge again, someone would have to write a second listing next to one that already exists.source.namecan no longer key a site, becausesub/machine.py:119andmachine.py:119would be the same key. That is the second probe above: green on the tip only because the walk never got that far. With a plainglob→rglobswap, that site would collide with a real one and stay green._named(path)keys every site by its path relative to the package._site_ofnames the site it records in the same way, so the driven set and the walked set use one naming scheme. Flat modules keep their bare file name, sounknown[0] == "explain.py"still holds.relative_toraise, instead of being compared under a name that happens to match.sub/extra.pyis not shown asextra.py.Gates
Gate 1: ATOM's suite, unmodified.
scripts/compass/gate_cpu.sh, each tree's own copy, run with stamps on node 18, one gate at a time, each bounded withtimeout -k 10 900.GATE_CPU_RCff9617f30f70f15c7a--junitxmlon both sides: 5272 ids each, none added, none removed, none changed state.tests/compass/test_spec_schema.py+24/−15.blackandruffare clean on the file.git merge-tree --write-tree ff9617f30 f70f15c7agivesf979c3f47293b6aa5179dd695e30fb510beeb02e, clean.Gate 2: CPU-only tests.
test_spec_schema.pyis the test. It is 139 passed on both sides, inside the gate run above.Gate 3: named result. See the table above: a submodule SHAPE site reddens the guard and is named by file and line, and the same site leaves the tip green.
Gate 4: independent review. Pending; the coordinator dispatches the reviewer.
Left undone
atom/compass/spec/is still flat, so the recursive walk has no second module to read today. The probes above are the only evidence that it reaches one.🤖 Generated with Claude Code