Skip to content

compass(gates): join the README's exclude-list counts to the list #310

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(gates): join the README's exclude-list counts to the list #310

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
28 changes: 28 additions & 0 deletions tests/compass/test_cpu_gate_exclude.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,15 @@
`regen_cpu_gate_exclude.sh`'s job, not a unit test's.
"""

import re
from pathlib import Path

import pytest

REPO = Path(__file__).resolve().parents[2]
EXCLUDE = REPO / "scripts" / "compass" / "cpu_gate_exclude.txt"
TRIGGERS = REPO / "scripts" / "compass" / "gpu_gate_triggers.txt"
README = REPO / "scripts" / "compass" / "README.md"

GEN_BEGIN, GEN_END = "# BEGIN GENERATED", "# END GENERATED"
MAN_BEGIN, MAN_END = "# BEGIN MANUAL", "# END MANUAL"
Expand Down Expand Up @@ -187,6 +189,32 @@ def test_the_manual_guard_finds_nothing_when_the_section_empties(monkeypatch, tm
assert not _manual_entries()


@pytest.mark.parametrize(
"stated, derive",
[
(r"\*\*(\d+)\*\* excluded test files", _entries),
(r"\*\*(\d+) GENERATED\*\*", lambda: _paths(_section(GEN_BEGIN, GEN_END))),
(r"\*\*(\d+) MANUAL\*\*", _manual_entries),
],
ids=["total", "generated", "manual"],
)
def test_the_readme_states_the_counts_the_list_holds(stated, derive):
# README.md restates this file's counts, and the pin above holds the MANUAL

@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.

Non-blocking (principles 3 and 8). The PR body says the MANUAL pin (test_the_manual_section_holds_the_entries_it_is_pinned_to_hold, L147-168) is kept because "the join does not replace it". I measured that claim, and it does not hold. The pin's stated job is to stop an emptied MANUAL section from passing silently, with its parametrisation collecting nothing. This join does that job already.

tree edit (line count preserved) result, tests/compass/test_cpu_gate_exclude.py only
a821914ad = head + MANUAL emptied cpu_gate_exclude.txt:51 commented out 3 failed, 68 passed, 1 skipped: the pin, [total], [manual]
a833627b9 = the same + pin neutralised also assert len(entries) == 1 -> >= 0 2 failed, 69 passed, 1 skipped: test_the_readme_states_the_counts_the_list_holds[total] and [manual] (states **1 MANUAL** but ... holds 0: [])

After this PR, the MANUAL count is stated twice by hand: **1 MANUAL** in the README and == 1 in the pin. Both are joined to the list, and every edit that fails one fails the other. Reaching zero on purpose now takes two edits instead of one.

Either choice is fine:

  • Delete the pin (about -24 lines). Then point the control test's docstring (L172) and the module docstring (L28-30) at this join.
  • Keep the pin with a reason that holds. The only real difference is locality: the pin's number sits in the file the list is tested from. The PR body's sentence should then say that, not that the join cannot replace it.

# one in this file only. Without this join, an entry added here reddens the
# pin and leaves the README wrong in silence. Same shape as gate_gpu.sh's
# BASE_FAILED against gpu_gate_known_failures.txt: the stated number stays,
# the list is counted, and a disagreement names both rather than picking one.
row = [ln for ln in _lines(README) if ln.startswith(f"| `{EXCLUDE.name}` |")]
assert len(row) == 1, f"{README.name} has {len(row)} rows for {EXCLUDE.name}"
found = re.search(stated, row[0])
assert found, f"{README.name}'s {EXCLUDE.name} row no longer states /{stated}/"
entries = derive()
assert int(found[1]) == len(entries), (
f"{README.name} states {found[0]} but {EXCLUDE.name} holds {len(entries)}: "
f"{entries}. Update the README row to match the list."

@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.

Non-blocking (principle 6). This sentence picks a side, which the comment at L206 says the join does not do ("a disagreement names both rather than picking one"). gate_gpu.sh's message is the model this test copies, and it only states the two numbers: BASE_FAILED=%s but %s names %s node-id(s).

The side it picks is wrong in exactly the case the MANUAL pin exists for. Emptying the MANUAL section by accident (mutation a821914ad: cpu_gate_exclude.txt:51 commented out) fails [manual] with README.md states **1 MANUAL** but cpu_gate_exclude.txt holds 0: []. Update the README row to match the list. Here the list is the wrong side, and the message tells the reader to make the README agree with it. For GENERATED the list usually is right, because a regenerator wrote it. For MANUAL it is a hand edit, and either side can be the mistake.

Suggested fix: end the message at {entries}., or replace the last sentence with a neutral one such as One of the two is wrong; fix that one. This matters more if the pin goes (see the comment on L202), because the pin's "Add or remove one deliberately" wording is then no longer printed beside it.

)


@pytest.mark.parametrize("entry", _manual_entries())
def test_every_manual_entry_states_why(entry):
# A manual entry is an assertion no script can check, so the reason is the
Expand Down