Skip to content

compass(tests): hold the profiler-reply assertion to the roots REPLY_SURFACE names #302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jgong5 merged 1 commit into from
Sep 23, 2026
Merged

compass(tests): hold the profiler-reply assertion to the roots REPLY_SURFACE names #302

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions tests/compass/test_runner_rpc_surface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,19 @@ def test_the_scan_ignores_a_compass_package_the_reply_never_reaches(tmp_path):
assert _mentions(roots, "trace_dir", tmp_path) == {PRODUCER}


def test_the_reply_assertion_scans_the_roots_the_constant_names(monkeypatch):
"""The tests above hold `REPLY_SURFACE`; this one holds its reader.

With the surface emptied, the profiler-reply assertion has nowhere to find
the producer and must fail. An assertion that scans roots written out at
its own call site ignores the constant and passes here, and it is the
whole-tree scan that call site used to hold.
"""
monkeypatch.setitem(globals(), "REPLY_SURFACE", ())

@jgong5 jgong5 Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding 1: non-blocking. Principle 8, a claim broader than its measurement. This test does not hold a root that is added at the call site when that root does not contain the producer.

The PR body says the test "also catches spellings that keep the name but add roots, such as REPLY_SURFACE + (REPO / "atom",)". That example is caught only because atom/ contains model_engine/model_runner.py. Add a root that does not contain the producer and the emptied surface still finds nothing, so the assertion still raises and this test stays green.

Measured on node 18 (xiaobizh_n18_cpu). The whole file was run with line 705 replaced and the line count kept:

line 705 tip b3078336d head c42404b67
_mentions(REPLY_SURFACE + (REPO / "atom" / "compass",), "trace_dir") 62 passed, rc=0 63 passed, rc=0
same, plus # trace_dir appended to atom/compass/spec/__init__.py:1 1 failed 1 failed, 62 passed: ...::test_the_profiler_replies_are_forwarded_whole_and_never_unpacked, Extra items in the left set: 'atom/compass/spec/__init__.py'

The second row is #185's symptom, on a tree where every scope test and this test are green. The scope tests read the constant, so none of them sees a widening written at the call site.

For comparison, I measured an argument spy with the same mutants: _mentions patched to record roots, then assert seen == [REPLY_SURFACE]. It catches the additive root, but it misses REPLY_SURFACE or (REPO / "atom",), which this test catches. So the two mechanisms are complementary, and neither one covers both.

Why this does not block: the brief's defect is the re-inlined whole-atom/ scan, and this test holds it (see the verdict comment). The additive root is a different mutation. It belongs with the brief's second gap as one follow-up. Please narrow that sentence in the PR body to what was measured.

with pytest.raises(AssertionError):
test_the_profiler_replies_are_forwarded_whole_and_never_unpacked()


def test_the_two_names_no_caller_waits_for_and_what_replying_costs():
"""A reply nobody reads sits on the queue for whoever asks next.

Expand Down