From 712f4c93367bc72d2e2d27eda90c52433d4bf230 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Sat, 19 Sep 2026 01:00:59 -0500 Subject: [PATCH] fix(conformance): "unit-tested" counted an alert MENTIONED in a comment as tested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit alert-rules-unit-tested decided whether an alert had a unit test with a bare word match over the whole test file. Any mention counted — including prose in a `#` comment. Hit for real while adding platform/monitoring/tests/vpn-egress.test.yaml. Its header explains that VPNTunnelDown stays quiet during an egress-steering failure, and that sentence alone made this check declare VPNTunnelDown "asserted on" and demand its removal from UNTESTED_ALERTS. Removing the entry would have recorded coverage that did not exist — the precise "looks fixed" outcome this check exists to prevent, produced by the check itself. (In that instance I made the coverage real instead, which also exposed that the fixtures were missing tun0's receive counter entirely.) Now an alert counts only when a test file names it in an assertion position, with comment lines stripped first so a commented-out assertion does not count either. BOTH promtool idioms are accepted, because both are real assertions: alert_rule_test: - alertname: CNPGPrimariesCoLocated promql_expr_test: - expr: 'ALERTS{alertname="CNPGPrimariesCoLocated"}' Matching only the first is not a stricter check, it is a WRONG one: it reported CNPGPrimariesCoLocated as untested when platform/cnpg/alerts.promtool-test.yaml pins it across four scenarios entirely through the second form. Believing that would have produced a redundant duplicate test for an alert already covered better than most — so the first version of this very fix had to be corrected before it landed. MEASURED BEFORE LANDING, because the registers are shrink-only and a prose-only alert would have required an entry this check forbids adding: of 61 alerts, 32 are asserted in one of the two idioms, 29 are not mentioned at all (already declared), and ZERO were prose-only. So this tightening failed nothing on the day it landed. My first measurement said 31/30/0 and was wrong in a way worth recording: it scanned only tests/*.test.yaml and missed the .promtool-test.yaml companion convention, which is exactly what hid the CNPGPrimariesCoLocated false negative. Verified in both directions: 134 assertions pass with the real coverage intact, and a temporary prose-only mention of NodeExporterDown (a registered untested alert) leaves it correctly listed as untested instead of triggering the stale-entry failure. Gated: offline l0,l1 --include-scope whole-repo — 24 cases, 0 failures, 0 skips, 0 unwired. Co-Authored-By: Claude Opus 5 --- .../alert-rules-unit-tested/check.sh | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/conformance/tests/l0-static/alert-rules-unit-tested/check.sh b/conformance/tests/l0-static/alert-rules-unit-tested/check.sh index 0db392b..eb31f03 100755 --- a/conformance/tests/l0-static/alert-rules-unit-tested/check.sh +++ b/conformance/tests/l0-static/alert-rules-unit-tested/check.sh @@ -481,7 +481,34 @@ for rel in sorted(alert_files): f"{rel} is listed in UNTESTED_FILES but is now covered by {tests} — the register is " f"shrink-only: delete the line so the next missing test is caught") text = "".join((ROOT / t).read_text() for t in tests if (ROOT / t).is_file()) - covered = [n for n in names if re.search(rf"\b{re.escape(n)}\b", text)] + # "COVERED" MUST MEAN ASSERTED, NOT MENTIONED (tightened 2026-09-19). + # This used to be a bare word match over the whole test file, so an alert named anywhere — + # including in a PROSE COMMENT — counted as tested. Hit for real while adding + # platform/monitoring/tests/vpn-egress.test.yaml: its header comment explains that VPNTunnelDown + # stays quiet during an egress-steering failure, and that sentence alone was enough to make this + # check declare VPNTunnelDown "asserted on" and demand its removal from UNTESTED_ALERTS. Deleting + # the register entry would then have recorded coverage that did not exist — the exact "looks fixed" + # outcome this check exists to prevent, produced BY the check. + # Now an alert counts only when a test file names it in an `alertname:` position, with comment + # lines stripped first so a commented-out assertion does not count either. + # MEASURED BLAST RADIUS BEFORE TIGHTENING: of 61 alerts across the PrometheusRule manifests, 32 are + # asserted in one of the two idioms below, 29 are not mentioned at all (already declared in the + # registers), and ZERO are prose-only — so this change failed nothing on the day it landed. That + # mattered because the registers are shrink-only: had any alert been prose-only, tightening would + # have needed a register entry it is not allowed to add. (The first measurement said 31/30/0 because + # it scanned only tests/*.test.yaml and missed the .promtool-test.yaml companions.) + text_no_comments = "\n".join( + l for l in text.split("\n") if not l.lstrip().startswith("#") + ) + # BOTH promtool idioms count, because both are real assertions: + # alert_rule_test: - alertname: CNPGPrimariesCoLocated + # promql_expr_test: - expr: 'ALERTS{alertname="CNPGPrimariesCoLocated"}' + # Matching only the first produced a FALSE NEGATIVE on CNPGPrimariesCoLocated, which + # platform/cnpg/alerts.promtool-test.yaml pins across FOUR scenarios entirely through the second + # form. Had that been believed, the "fix" would have been a redundant duplicate test for an alert + # already better covered than most. + covered = [n for n in names + if re.search(rf'(?:alertname:\s*|alertname="){re.escape(n)}\b', text_no_comments)] covered_total += len(covered) for n in names: if n in covered: