Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 32 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,38 @@ true until the next version shipped.
with it. Removal proof, four mutations, each reddening named checks and each
mutant asserted to still parse -- reversing the difference, dropping the sort,
restoring the subtraction, and dropping one recorded name.
- `pgc_ledger merge` wrote a row covering one major and never said so (#1071).

A contributor adds checks, runs the suite on ONE major, merges that log. The row
lands with `majors = 18`. The gate considers a row only where its majors intersect
the run's, so `suites (PG 18)` matches it and is green while `suites (PG 17)` reads
it as a check the ledger has never seen and reddens -- naming the contributor's own
checks `(on major 17)`, which reads as though their suite is broken on 17 when it
passes there.

FIVE AUTHORS IN A ROW hit it, including the person who wrote the tool, on a PR that
was itself about ledger hygiene: #1039, #1063, #1065, #1068 and #1070 each wrote 6
to 10 rows at `18` against a ledger where every other row carried `15;16;17;18;19`.
When everyone makes the same mistake it is the tool's shape rather than five lapses.

THE TOOL ALREADY KNEW. The distribution it prints for its summary line is computed
from the same rows, so `merge` could see the new row was a strict subset of what the
rest of the ledger carries, and said nothing.

`merge` now warns, naming the rows, the set they carry, the set the rest of the
ledger carries, and the majors the gate will redden on. The predicate is STRICT
SUBSET rather than inequality, so a row naming a major the ledger has never carried
-- how a new major legitimately enters -- is not warned about. Only rows the merge
touched are candidates, or a partly-seeded ledger would reprint its own history on
every merge.

REPORTING, NOT A REFUSAL, deliberately. Seeding one major at a time is how a
contributor without five installed majors makes progress, so refusing would block
the honest case to catch the careless one. The gate still refuses later; this makes
that refusal predictable at the moment it is caused.

The gate's printed recipe said `<log>`, singular, so following it exactly produced
the broken row. It now names one log per gated major and says why.

- Four secret-leak claims over the PG server log could pass having read nothing
(#1032).
Expand Down
16 changes: 16 additions & 0 deletions test/check_ledger.tsv
Original file line number Diff line number Diff line change
Expand Up @@ -1210,6 +1210,22 @@ harness_selftest 510-a-residual-must-be-counted the residual count agrees with t
harness_selftest 510-a-residual-must-be-counted the skipped-but-accounted suites are named, not folded into the residual 15;16;17;18;19 never -
harness_selftest 510-a-residual-must-be-counted the summary prints the residual from the reader, not by subtraction 15;16;17;18;19 never -
harness_selftest 510-a-residual-must-be-counted the tally function records the name of every suite that ran 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors a pre-existing minority row is not re-reported on an unrelated merge 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors a row covering fewer majors than the ledger is warned about 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors a row naming a major the ledger has never seen is not a subset 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors and a correct merge does not warn at all 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors and it names exactly the majors the gate will redden on 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors and names the checks whose rows are short 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors and names the set that was merged, so both sides are visible 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors and seeding still succeeds 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors and the old singular recipe is gone from the tool 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors and the row is written anyway, so the warning is advice not a veto 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors premise: the five-log merge wrote the full major set 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors premise: the ledger tool this part drives is present 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors premise: the single-major merge really did write the minority set 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors seeding a ledger with no prevailing set is not warned 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors the gate's printed recipe names one log per gated major 15;16;17;18;19 never -
harness_selftest 520-a-merged-row-must-cover-the-majors the warning does not fail the merge 15;16;17;18;19 never -
index_fetch_penalty_crossover index_fetch_penalty_crossover a 50000-row correlated range uses the custom scan, not a fetching index 15;16;17;18;19 never -
index_fetch_penalty_crossover index_fetch_penalty_crossover a selective point lookup still uses the index 15;16;17;18;19 never -
index_fetch_penalty_crossover index_fetch_penalty_crossover both paths return the same aggregate at 50000 15;16;17;18;19 never -
Expand Down
15 changes: 14 additions & 1 deletion test/check_ledger_budget.txt
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,17 @@ suites_not_covered 249
# 1306 -> 1309: three arms pinning the collation of the runner's `comm` readers, added
# after @OffgridwithJD found the three helpers unpinned in review. Re-derived by the
# command above and by the gate's census in the same run.
checks_never_observed_red 1309
# 1277 -> 1293 when #1071's sixteen checks merged: selftest part 520, over the warning
# `merge` now prints when it writes a row covering fewer majors than the ledger carries.
# Derived by the command above on this tree, and by the gate's own census in the same
# run, which printed `never observed red=1293` independently.
#
# THE MERGE THAT WROTE THESE ROWS EXERCISED THE NEW WARNING AND IT STAYED SILENT, which
# is the control: five logs, all sixteen rows uniform at 15;16;17;18;19, no WARNING line.
# MERGED with #1110. Both sides moved this key from 1277 -- that branch to 1309 over
# three merges, this one to 1293 -- and neither is the merged truth. The two ledgers
# had NO key in common (28 rows against 16, union 44, zero shared), so the rows are a
# clean union and only the COUNT had to be re-derived. Both files conflicted loudly
# here, which is the safe half: the ledger is always right and only sometimes speaks.
# Re-derived by the command above on the merged tree, never by adding the deltas:
checks_never_observed_red 1325
75 changes: 74 additions & 1 deletion test/pgc_ledger.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,69 @@ def cmd_census(args):
return 0


def _warn_subset_majors(rows, touched):
"""Say so when this merge wrote a row covering fewer majors than the ledger does.

#1071. A contributor adds checks, runs the suite on ONE major, and merges that
log. The row lands with `majors = 18`. The gate considers a row only where its
majors intersect the run's, so `suites (PG 18)` matches it and is green while
`suites (PG 17)` reads it as a check the ledger has never seen and reddens --
naming the contributor's own checks `(on major 17)`, which reads as though their
suite is broken on 17 when it passes there.

FIVE AUTHORS IN A ROW, including the person who wrote this tool, on a PR that was
itself about ledger hygiene (#1039, #1063, #1065, #1068, #1070). When everyone
makes the same mistake it is the tool's shape rather than five lapses, and the
tool already had what it needed: the distribution below is computed for the
summary line, so `merge` knew the new row was anomalous and said nothing.

THE PREDICATE IS STRICT SUBSET, not inequality. A row naming a major the ledger
has never carried is how a new major legitimately enters, and warning about that
would make this wrong in the case the project wants to encourage.

ONLY ROWS THIS MERGE TOUCHED. Every untouched row in a partly-seeded ledger is a
subset of the prevailing set, so a sweep over the whole file would reprint the
ledger's history on every merge and bury the one row that matters.

REPORTING, NOT A REFUSAL, deliberately. Seeding a major at a time is legitimate --
it is how a contributor without five installed majors makes progress -- so a hard
refusal would block the honest case to catch the careless one. The gate still
refuses later; this only makes that refusal predictable at the moment it is caused.
"""
untouched = [v[0] for k, v in rows.items() if k not in touched and v[0]]
if not untouched:
# Nothing to be a subset OF. Seeding a fresh ledger must not warn, or the
# warning fires when nothing is wrong and stops being read.
return
# The PLURALITY set, matching the distribution the summary prints. A union would
# be wrong for the same reason it was wrong in the summary (#1048): it cannot
# represent what most rows actually carry.
prevailing = collections.Counter(frozenset(m) for m in untouched).most_common(1)[0][0]

offenders = {}
for key in sorted(touched):
got = frozenset(rows[key][0])
if got and got < prevailing:
offenders.setdefault(MAJOR_SEP.join(sorted(got)), []).append(key)
if not offenders:
return

have = MAJOR_SEP.join(sorted(prevailing))
n_prev = sum(1 for m in untouched if frozenset(m) == prevailing)
for got, keys in sorted(offenders.items()):
missing = MAJOR_SEP.join(sorted(prevailing - set(got.split(MAJOR_SEP))))
print(f" WARNING: {len(keys)} row(s) written carrying majors={got}, while "
f"{n_prev} other row(s) carry {have}.")
print(f" The gate will refuse these on every major they do not "
f"name, so this reddens on {missing}.")
print(f" Merge a log from each major -- `merge` takes several at "
f"once -- or seed the rest before this lands.")
for suite, part, name in keys[:6]:
print(f" {suite}\t{part}\t{name}")
if len(keys) > 6:
print(f" ... and {len(keys) - 6} more")


def cmd_merge(args):
rows = read_ledger(args.ledger)
runs = _by_run(args.logs)
Expand Down Expand Up @@ -441,9 +504,11 @@ def cmd_merge(args):
f"--mutation NAME if you broke it deliberately, or --reds-are-real if "
f"this is a genuine observation of the code under test.")

touched = set()
for path, seen in runs:
for key, pairs in sorted(seen.items()):
verdicts = [v for v, _m in pairs]
touched.add(key)
if key not in rows:
# A check this ledger has never seen enters as DEBT. A green run
# has observed nothing go red, so merging one must never record a
Expand All @@ -468,6 +533,8 @@ def cmd_merge(args):
print(f" duplicate check name in one run, so one ledger row covers "
f"{len(verdicts)}: {key[0]}\t{key[1]}\t{key[2]}")

_warn_subset_majors(rows, touched)

write_ledger(args.ledger, rows)
seen_all = {k for _, s in runs for k in s}
red = sum(1 for v in rows.values() if v[1] != NEVER)
Expand Down Expand Up @@ -1062,7 +1129,13 @@ def cmd_gate(args):
print(f" not in the ledger: {suite}\t{part}\t{name}\t(on major {major})")
if unknown:
print(f" {len(unknown)} check(s) the ledger has never seen. Regenerate it with:")
print(f" python3 test/pgc_ledger.py merge --ledger {args.ledger} --date <today> <log>")
# PLURAL, and it is the reason this defect kept recurring (#1071). The recipe
# said `<log>`, and following it exactly writes a row covering one major --
# which reddens on every other leg and names the contributor's own checks.
print(f" python3 test/pgc_ledger.py merge --ledger {args.ledger} --date <today> \\")
print(f" <log-pg15> <log-pg16> <log-pg17> <log-pg18> <log-pg19>")
print(f" One log per gated major. A row covers only the majors it was "
f"merged from, so a single log reddens the other legs.")
rc = 1

# A CENSUS, not a ceiling. Bounding it deadlocks: every new check enters as
Expand Down
33 changes: 33 additions & 0 deletions test/pytest/TESTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2698,6 +2698,39 @@ discrimination assertion, so what is load-bearing is the distribution and not th
Reporting only. Whether merge should **refuse** a non-uniform result is a live design
question and is deliberately not settled here (#1048).

### A merged row that covers fewer majors than the ledger (#1071)

That design question is now settled the other way round, and the answer is a WARNING
rather than a refusal.

`merge` writes a row covering only the majors it was handed. A contributor runs the
suite on ONE major, merges that log, and the row lands with `majors = 18`. The gate
considers a row only where its majors intersect the run's, so `suites (PG 18)` matches
it and is green while `suites (PG 17)` reads it as a check never seen and reddens —
naming the contributor's own checks `(on major 17)`, which reads as though their suite
is broken on 17 when it passes there.

Five authors in a row hit it, including the person who wrote the tool, on a PR that was
itself about ledger hygiene: #1039, #1063, #1065, #1068, #1070. The tool already had
what it needed — the distribution it prints for its summary is computed from the same
rows — so it could see the new row was anomalous and said nothing.

| test | what it holds |
| --- | --- |
| `test_merge_warns_when_it_writes_a_strict_subset_of_the_ledgers_majors` | a single-major merge into a five-major ledger warns exactly once, and the same checks merged from five logs do not warn at all |
| `test_the_warning_names_the_majors_the_gate_will_redden_on` | the warning names the missing majors, read out of the text rather than searched for loosely — `"15" in output` is also satisfied by the prevailing set printed beside it |
| `test_seeding_a_ledger_with_no_prevailing_set_is_not_warned` | an empty ledger has nothing to be a subset of, and a warning that fires when nothing is wrong stops being read |
| `test_a_row_carrying_a_major_the_ledger_has_never_seen_is_not_a_subset` | the predicate is strict subset, not inequality, so adding a new major is not warned about |
| `test_the_warning_is_not_a_refusal` | the merge still succeeds and the row is still written — seeding one major at a time is legitimate |
| `test_an_existing_rows_widening_is_not_reported_as_a_subset` | only rows the merge touched are candidates, or a partly-seeded ledger reprints its own history every time |

Held as a **discrimination**, like the arm above it: the same checks merged correctly
must not warn. An assertion on the bad output alone would pass against a tool that
warned unconditionally, which would train the warning out of being read.

The shell twin is `test/selftest/520-a-merged-row-must-cover-the-majors.sh`. Same
properties, own fixtures, and neither file names the other.

## 24. test_loop_coverage_premise.py: a loop that never ran asserted nothing

**Why this file exists.** `assert-inside-a-loop-over-zero-rows` in VACUITY_MODES.md 3.5
Expand Down
16 changes: 15 additions & 1 deletion test/pytest/expected_tests.txt
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,21 @@
# NO_CLUSTER is asserted against the corpus in both directions, so the declaration
# and the property had to agree before this collected here at all.
# Re-derived by collection on this tree, never by adding nine: `355 tests collected`.
guard_tests 355
# 346 -> 352 when #1071's six arms landed in test_mutation_ledger.py: the warning,
# the correct-merge control that keeps it a discrimination rather than a wording, the
# majors it must name, the empty-ledger case, the new-major case, and the two that hold
# it to reporting rather than refusing.
#
# NOTE FOR WHOEVER MERGES SECOND. #1110 moves this same key to 355 from the same 346,
# for six arms of its own in a different file. Neither number survives the merge and
# adding the deltas lands on a value no tree collects -- the file says this three times
# already and it is about to be true again. Re-derive by collection on the merged tree.
# MERGED with #1110's nine arms. Both sides moved this key from 346 -- this branch to
# 352, #1110 to 355 -- and neither survives. 352 + 9 and 355 + 6 both happen to reach
# 361 here, which is a coincidence of this merge rather than a method: the deltas were
# measured against different trees. Re-derived by collection on the merged tree, which
# is the only resolution this number has.
guard_tests 361

# The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran
# in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and
Expand Down
Loading
Loading