Skip to content

Commit adb2bbc

Browse files
committed
fix(ci): stop an env-gated all-skip package from outranking real evidence in the revert oracle
#4131 (closes #4108) correctly stops an all-skipped baseline from being scored a pass, but `aggregate()`'s severity walk ranked ALL_SKIPPED above PASS and ASSERTION_FAILURE unconditionally. CI's revert-oracle job installs Python/ifcopenshell only when Python files changed, so a TS-only PR touching even one env-gated all-skip file (e.g. packages/export/src/ifcopenshell-schema-conformance.test.ts) now aggregates to ALL_SKIPPED across the whole run, and verdict() falls into BASELINE-BROKEN/exit 3 -- blocking a PR whose other packages produced real, executed assertions that went red on revert. Adds severityCandidates() in revert-oracle-all-skipped.mjs: when at least one result in the run has kind PASS or ASSERTION_FAILURE, ALL_SKIPPED entries are dropped from the severity walk (their counts/evidence still fold into aggregate()'s totals, unfiltered). With no real evidence anywhere, every result stays a candidate, so a single all-skipped package, or a run where every package is all-skipped, still aggregates to ALL_SKIPPED and still blocks -- #4108's own case is unchanged. Verified both directions with synthetic aggregate()/verdict() calls before fixing: a PASS-plus-ALL_SKIPPED run aggregated to ALL_SKIPPED/BASELINE-BROKEN (exit 3) pre-fix, and to PASS/OBSERVED (exit 0) post-fix; a single- or every-package all-skipped run stayed ALL_SKIPPED/BASELINE-BROKEN (exit 3) in both cases. Mutated to confirm the tests hold: reverting the aggregate() change back to scanning `results` directly turns exactly the three new multi-package tests red; reverting severityCandidates() to unconditionally filter ALL_SKIPPED (dropping the single-signal guard) turns exactly the two single/every all-skipped tests red. All other tests stayed green in both mutations. scripts/lib/revert-oracle.mjs stays within its 528-line module-size budget (now flush against it); the new logic lives in the existing revert-oracle-all-skipped.mjs sibling module instead of growing the budget. node --test scripts/lib/revert-oracle*.test.mjs: 104 -> 108 tests, all passing.
1 parent e734b0c commit adb2bbc

3 files changed

Lines changed: 90 additions & 5 deletions

File tree

scripts/lib/revert-oracle-all-skipped.mjs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,3 +60,27 @@ export function classifyExecuted(parsed) {
6060
}
6161
return { kind: PASS, passed: parsed.passed, failed: parsed.failed, total: parsed.total, evidence: [] };
6262
}
63+
64+
/**
65+
* `aggregate()`'s severity walk (`revert-oracle.mjs`) ranks ALL_SKIPPED above
66+
* PASS, which is right when a run's ONLY signal is an all-skipped package —
67+
* #4108's own case: the baseline measured nothing, so nothing can be
68+
* concluded — but wrong when ANOTHER package in the same run produced real
69+
* evidence (a PASS or ASSERTION_FAILURE). An env-gated all-skip file (e.g.
70+
* `ifcopenshell-schema-conformance.test.ts` on a TS-only PR, where CI never
71+
* installs Python) must not outrank a genuinely observed change and block a
72+
* well-tested PR. That was a real regression #4108's fix (#4131) introduced.
73+
*
74+
* Returns the subset of `results` that should decide the aggregate's worst
75+
* kind: with real evidence present, ALL_SKIPPED entries are dropped from the
76+
* severity walk (their counts and evidence still fold into the caller's
77+
* totals via its own `sum`/`flatMap` over the FULL `results` array, unfiltered
78+
* — only the *verdict-deciding* kind ignores them). With no real evidence
79+
* anywhere, every result — ALL_SKIPPED included — stays a candidate, so a
80+
* single-package or an every-package all-skipped run still poisons to
81+
* ALL_SKIPPED exactly as before.
82+
*/
83+
export function severityCandidates(results) {
84+
const hasEvidence = results.some((r) => r.kind === PASS || r.kind === ASSERTION_FAILURE);
85+
return hasEvidence ? results.filter((r) => r.kind !== ALL_SKIPPED) : results;
86+
}

scripts/lib/revert-oracle.mjs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
*/
3636

3737
import { parsePython, PYTEST_MISSING_PATTERN } from './revert-oracle-python.mjs';
38-
import { ALL_SKIPPED, classifyExecuted } from './revert-oracle-all-skipped.mjs';
38+
import { ALL_SKIPPED, classifyExecuted, severityCandidates } from './revert-oracle-all-skipped.mjs';
3939
// ---------------------------------------------------------------------------
4040
// Diff classification
4141
// ---------------------------------------------------------------------------
@@ -511,8 +511,9 @@ export function aggregate(results) {
511511
if (!Array.isArray(results) || results.length === 0) {
512512
return { kind: UNPARSEABLE, passed: null, failed: null, total: null, evidence: ['no packages were run'] };
513513
}
514-
let worst = results[0];
515-
for (const r of results) {
514+
const candidates = severityCandidates(results);
515+
let worst = candidates[0];
516+
for (const r of candidates) {
516517
if (KIND_SEVERITY.indexOf(r.kind) > KIND_SEVERITY.indexOf(worst.kind)) worst = r;
517518
}
518519
const sum = (key) =>

scripts/lib/revert-oracle.test.mjs

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -950,13 +950,73 @@ test('aggregate: an assertion failure alongside passes stays an assertion failur
950950
assert.equal(a.total, 8);
951951
});
952952

953-
test('#4108: aggregate: one all-skipped package outranks a genuinely green one', () => {
953+
test('#4131 regression: aggregate: an all-skipped package no longer outranks a genuinely green one', () => {
954+
// #4131's own fix for #4108 made this the false positive: an env-gated
955+
// all-skip file (ifcopenshell absent on a TS-only PR) must not poison a run
956+
// that also contains a package with real, executed evidence.
954957
const a = aggregate([
955958
{ kind: PASS, passed: 5, failed: 0, total: 5, evidence: [] },
956959
{ kind: ALL_SKIPPED, passed: 0, failed: 0, total: 2, evidence: ['all skipped'] },
957960
]);
958-
assert.equal(a.kind, ALL_SKIPPED);
961+
assert.equal(a.kind, PASS);
959962
assert.equal(a.total, 7);
963+
assert.equal(a.passed, 5);
964+
});
965+
966+
test('#4131 regression: aggregate: an all-skipped package no longer outranks a real assertion failure', () => {
967+
const a = aggregate([
968+
{ kind: ASSERTION_FAILURE, passed: 2, failed: 1, total: 3, evidence: ['assertion X failed'] },
969+
{ kind: ALL_SKIPPED, passed: 0, failed: 0, total: 2, evidence: ['all skipped'] },
970+
]);
971+
assert.equal(a.kind, ASSERTION_FAILURE);
972+
assert.equal(a.failed, 1);
973+
assert.equal(a.total, 5);
974+
});
975+
976+
test('#4131 regression: multi-package all-skipped-plus-evidence run does not block CI', () => {
977+
// The end-to-end shape: a well-tested PR whose reverted run turns real
978+
// assertions RED in one package while an unrelated env-gated file stays
979+
// all-skipped in both the baseline and the reverted run.
980+
const baseline = aggregate([
981+
{ kind: PASS, passed: 12, failed: 0, total: 12, evidence: [] },
982+
{ kind: ALL_SKIPPED, passed: 0, failed: 0, total: 2, evidence: ['all skipped'] },
983+
]);
984+
const reverted = aggregate([
985+
{ kind: ASSERTION_FAILURE, passed: 10, failed: 2, total: 12, evidence: ['assertion X failed'] },
986+
{ kind: ALL_SKIPPED, passed: 0, failed: 0, total: 2, evidence: ['all skipped'] },
987+
]);
988+
const v = verdict({ baseline, reverted });
989+
assert.equal(v.verdict, OBSERVED);
990+
assert.equal(v.exitCode, 0);
991+
});
992+
993+
test('#4108 (still fixed): aggregate: a SINGLE all-skipped package is still ALL_SKIPPED and still blocks', () => {
994+
// With no other package producing real evidence, an all-skipped package
995+
// must still poison the run -- this is #4108's actual case and #4131's
996+
// fix for it must not be weakened.
997+
const results = [{ kind: ALL_SKIPPED, passed: 0, failed: 0, total: 2, evidence: ['all skipped'] }];
998+
const baseline = aggregate(results);
999+
const reverted = aggregate(results);
1000+
assert.equal(baseline.kind, ALL_SKIPPED);
1001+
const v = verdict({ baseline, reverted });
1002+
assert.equal(v.verdict, BASELINE_BROKEN);
1003+
assert.equal(v.exitCode, 3);
1004+
});
1005+
1006+
test('#4108 (still fixed): aggregate: EVERY package all-skipped is still ALL_SKIPPED and still blocks', () => {
1007+
// No package anywhere produced real evidence, so the "another package has
1008+
// evidence" exception must not apply -- this must not slip through as a pass.
1009+
const results = [
1010+
{ kind: ALL_SKIPPED, passed: 0, failed: 0, total: 2, evidence: ['all skipped'] },
1011+
{ kind: ALL_SKIPPED, passed: 0, failed: 0, total: 3, evidence: ['all skipped'] },
1012+
];
1013+
const baseline = aggregate(results);
1014+
const reverted = aggregate(results);
1015+
assert.equal(baseline.kind, ALL_SKIPPED);
1016+
assert.equal(baseline.total, 5);
1017+
const v = verdict({ baseline, reverted });
1018+
assert.equal(v.verdict, BASELINE_BROKEN);
1019+
assert.equal(v.exitCode, 3);
9601020
});
9611021

9621022
test('aggregate: zero packages is UNPARSEABLE, never a pass', () => {

0 commit comments

Comments
 (0)