Repository navigation
fix(ci): stabilize nightly-mutation — guard tap.testFiles drift + anti-flake eps - #5658
diegosouzapw wants to merge 1 commit into
Conversation
…i-flake eps Root cause (NOT a timeout): the nightly-mutation run fails on cold-cache nights because the blocking mutation-ratchet job measures modules below baseline, while warm-cache nights pass — the verdict tracked GitHub Actions cache state, not code quality. Proven via a local Stryker probe on headers.ts: covering unit tests (no-memory-header, strip-reasoning) had drifted OUT of stryker.conf.json tap.testFiles, so their mutants went covered-but-unkilled = Survived on a cold full run (COVERED score 61.73 vs 94.29 baseline); adding them restores the kills. - Add scripts/check/check-mutation-test-coverage.mjs: guards that every UNIT test importing a Stryker-mutated module is listed in tap.testFiles. Advisory by default, --strict in CI (wired in quality.yml fast-gates). Prevents recurrence. - Add the 38 drifted covering unit tests to stryker.conf.json tap.testFiles (138 -> 176). Monotonically safe: more covering tests only raise/hold the score. - Add MUTATION_RATCHET_EPS (1.0pt) anti-flake tolerance to check-mutation-ratchet so sub-point tap-runner jitter no longer false-fails the gate. Lowers no baseline. - Tests: check-mutation-test-coverage (3) + eps cases in check-mutation-ratchet. Residual: a clean post-merge nightly confirms scores return to/above baseline; any marginal residual gets a baseline re-seed (operator).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new script, check-mutation-test-coverage.mjs, to guard against drift in stryker.conf.json's tap.testFiles by ensuring all unit tests importing mutated modules are listed. It also introduces an anti-flake tolerance (MUTATION_RATCHET_EPS) to the mutation ratchet script to absorb minor score fluctuations. The review feedback highlights several key improvement opportunities: enhancing cross-platform compatibility on Windows by using fileURLToPath for entry point checks, updating the import detection regex to support backticks in dynamic imports, optimizing performance by pre-compiling regular expressions outside of loops, and replacing the git child process with native fs.readdirSync to list test files more robustly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import fs from "node:fs"; | ||
| import { execFileSync } from "node:child_process"; |
There was a problem hiding this comment.
Import fileURLToPath from node:url to support cross-platform path comparison at the entry point check.
| import fs from "node:fs"; | |
| import { execFileSync } from "node:child_process"; | |
| import fs from "node:fs"; | |
| import { execFileSync } from "node:child_process"; | |
| import { fileURLToPath } from "node:url"; |
| export function testImportsModule(content, fragment) { | ||
| const esc = fragment.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const re = new RegExp( | ||
| "(?:from\\s+|\\bimport\\s*\\(\\s*|\\brequire\\s*\\(\\s*)[\"'][^\"']*" + esc | ||
| ); | ||
| return re.test(content); | ||
| } |
There was a problem hiding this comment.
The current regular expression only matches single (') and double (") quotes for imports, which misses dynamic imports using backticks (template literals), e.g., await import(\...`)`. This can lead to false negatives where covering tests are not detected.
We can update the regex to support backticks and also allow testImportsModule to accept a pre-compiled RegExp to avoid redundant compilations in the loop.
export function testImportsModule(content, fragmentOrRegExp) {
const re = fragmentOrRegExp instanceof RegExp
? fragmentOrRegExp
: new RegExp(
"(?:from\\s+|\\bimport\\s*\\(\\s*|\\brequire\\s*\\(\\s*)[\"'`][^\"'`]*" +
fragmentOrRegExp.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")
);
return re.test(content);
}| export function findCoverageDrift({ mutate, tapTestFiles, unitTests }) { | ||
| const tap = new Set(tapTestFiles); | ||
| const drift = {}; | ||
| for (const mod of mutate) { | ||
| if (mod.startsWith("_") || !mod.endsWith(".ts")) continue; // skip comment/non-ts entries | ||
| const frag = moduleFragment(mod); | ||
| const missing = unitTests | ||
| .filter((t) => testImportsModule(t.content, frag) && !tap.has(t.path)) | ||
| .map((t) => t.path) | ||
| .sort(); | ||
| if (missing.length > 0) drift[mod] = missing; | ||
| } | ||
| return drift; | ||
| } |
There was a problem hiding this comment.
In findCoverageDrift, calling testImportsModule for every test file compiles a new RegExp every time. With ~31 modules and ~2230 test files, this results in over 69,000 RegExp compilations, which is highly inefficient.
We can pre-compile the RegExp once per module and pass it to testImportsModule to drastically improve performance.
export function findCoverageDrift({ mutate, tapTestFiles, unitTests }) {
const tap = new Set(tapTestFiles);
const drift = {};
for (const mod of mutate) {
if (mod.startsWith("_") || !mod.endsWith(".ts")) continue; // skip comment/non-ts entries
const frag = moduleFragment(mod);
const esc = frag.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
const re = new RegExp(
"(?:from\\s+|\\bimport\\s*\\(\\s*|\\brequire\\s*\\(\\s*)[\"'`][^\"'`]*" + esc
);
const missing = unitTests
.filter((t) => testImportsModule(t.content, re) && !tap.has(t.path))
.map((t) => t.path)
.sort();
if (missing.length > 0) drift[mod] = missing;
}
return drift;
}| function listUnitTests() { | ||
| // Static argv — no shell, no interpolation. | ||
| const out = execFileSync("git", ["ls-files", "tests/unit"], { encoding: "utf8" }); | ||
| return out | ||
| .split("\n") | ||
| .filter((f) => /\.test\.ts$/.test(f)) | ||
| // Exclude tests/unit/build/: these test the build TOOLING (scripts/), not the | ||
| // mutated runtime modules. They legitimately embed module paths as fixture | ||
| // strings (e.g. this gate's own test), which would otherwise false-match. | ||
| .filter((f) => !f.startsWith("tests/unit/build/")) | ||
| .map((path) => ({ path, content: fs.readFileSync(path, "utf8") })); | ||
| } |
There was a problem hiding this comment.
Spawning a git child process to list test files is slower and can fail in environments where git is not installed or when running inside a minimal container/CI runner. Additionally, git ls-files will not detect newly created, untracked test files during local development, which can lead to unexpected CI failures.
Since Node.js >= 22 is guaranteed, we can use the native fs.readdirSync with { recursive: true } to list files synchronously, which is faster, more robust, and correctly handles untracked files.
function listUnitTests() {
return fs.readdirSync("tests/unit", { recursive: true })
.filter((f) => typeof f === "string" && /\.test\.ts$/.test(f))
.map((f) => `tests/unit/${f.replace(/\\/g, "/")}`)
// Exclude tests/unit/build/: these test the build TOOLING (scripts/), not the
// mutated runtime modules. They legitimately embed module paths as fixture
// strings (e.g. this gate's own test), which would otherwise false-match.
.filter((f) => !f.startsWith("tests/unit/build/"))
.map((path) => ({ path, content: fs.readFileSync(path, "utf8") }));
}| process.exit(0); | ||
| } | ||
|
|
||
| if (import.meta.url === `file://${process.argv[1]}`) main(); |
There was a problem hiding this comment.
The check import.meta.url === \file://${process.argv[1]}`` will fail on Windows platforms due to differences in path separators (backslashes vs forward slashes) and URL formatting.
To ensure cross-platform compatibility, we should import fileURLToPath from node:url and use it to normalize the path comparison, matching the robust pattern already used in check-mutation-ratchet.mjs.
| if (import.meta.url === `file://${process.argv[1]}`) main(); | |
| if ( | |
| import.meta.url === `file://${process.argv[1]}` || | |
| process.argv[1] === fileURLToPath(import.meta.url) | |
| ) { | |
| main(); | |
| } |
|
Superseded by #5682 — rehomed para |
Problema (BLOCO C — pré-requisito: estabilizar a medição de mutação)
O
nightly-mutationfalha quase toda noite. Não é timeout — os 12 batches Stryker completam (cold ~200min, warm ~8min). Quem falha é o jobMutation score ratchet (blocking), de forma não-determinística: passa em noites warm (cache incremental reusa veredictos antigos → super-reporta) e falha em noites cold (re-mede o score verdadeiro, menor). O veredito do gate bloqueante seguia o estado do cache do GitHub, não a qualidade do código.Causa-raiz (provada)
Probe local do Stryker no
headers.ts(65 linhas, código imutável desde v3.8.37): com otap.testFilesatual o COVERED score cai porque testes cobridores (no-memory-header,strip-reasoning-header) derivaram para fora da lista — seus mutantes ficam covered-but-unkilled = Survived no run cold (61.73 vs baseline 94.29). Adicionando-os, killed 35→75 e o score volta a ~92. A listatap.testFiles(138, hand-mantida) deriva silenciosamente quando testes são adicionados/divididos, e não havia gate pegando isso.Fix
scripts/check/check-mutation-test-coverage.mjs(NOVO gate) — prova que todo teste unit que importa um módulo mutado está notap.testFiles. Advisory por default,--strictno CI (wired emquality.ymlfast-gates). Previne recorrência. Excluitests/unit/build/(testes da tooling, não cobrem runtime).stryker.conf.jsontap.testFiles(138→176). Monotonicamente seguro: mais testes cobridores só elevam/mantêm o score (Stryker roda cada teste só contra mutantes que cobre).MUTATION_RATCHET_EPS(1.0pt) anti-flake nocheck-mutation-ratchet.mjs— absorve jitter run-to-run do tap-runner (concurrency + fronteira de timeout). Não baixa nenhum baseline.Validação
check:mutation-test-coverage --strict→ 0 drift (exit 0; 2230 unit tests × 31 módulos).check-mutation-test-coverage.test.ts(3) + casos de eps emcheck-mutation-ratchet.test.ts→ 12/12.check:test-discovery,check:test-runner-api, lint → verdes.Residual (follow-up — operador)
A confirmação definitiva (scores cold voltam a/acima do baseline para os 7 módulos) é um nightly-mutation pós-merge. Se algum módulo ficar marginalmente abaixo, re-semear o baseline daquele módulo (decisão do operador — não mexi em baseline neste PR).