Skip to content

Commit fc17bc4

Browse files
easonliang28easonliang28
committed
fix(mutation-gate): keep the stacked map out of the pull request's own tree
- .github/workflows/mutation-testing.yml downloaded the stacked map into .mutation/, inside the checkout. A pull request can commit .mutation/stacked-map.json itself, and with continue-on-error a missing artifact leaves that committed file exactly where the gate reads it, so the pull request picks the base it is measured against. The map now lands under RUNNER_TEMP: a missing artifact means no file there, which degrades to the event base - stricter than the intended measurement, never looser. - reinstallAlignedDependencies built its failure detail with ??. pnpm killed or terminated by a signal leaves stderr empty, and an empty string is a valid ?? operand, so the report ended in a blank detail and lost the exit status. The chain now lives in reinstallFailureDetail() and falls through with ||. - summarizeReinstallFailure received only stackedOn, so the job summary named the parent PR but not the base the gate had selected. It takes the resolved invocation now, and formatSummary turns baseSha + headSha into the "Reproduce locally" command. Each change has a control with it removed and the file restored byte-identically: - detail chain back to ?? -> "AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:" with actual: '' and expected: 'exit 1' (test line 616), - summary back to { stackedOn } -> "AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:" because the reproduce line disappears (test line 598), - map path back to .mutation -> the same falsy-value assertion on the workflow check. Patched suite: 122 tests / 23 suites / 122 pass / 0 fail. tsc from src: 0 errors. eslint --max-warnings=0 clean, src/eslint-suppressions.json restored to its committed blob.
1 parent f133c58 commit fc17bc4

3 files changed

Lines changed: 59 additions & 12 deletions

File tree

‎.github/workflows/mutation-testing.yml‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,13 +106,16 @@ jobs:
106106

107107
- name: Read the stacked map
108108
if: github.event_name == 'pull_request'
109-
# A missing artifact degrades to the event base, which charges this unit every unmerged
110-
# ancestor - stricter than the intended measurement, never looser - and the gate logs it.
109+
# Downloaded outside the checkout on purpose: a pull request can commit .mutation/stacked-map.json
110+
# itself, and with continue-on-error a missing artifact would leave that committed file in place for
111+
# the gate to read as if it were the map. Under RUNNER_TEMP a missing artifact means no file, which
112+
# degrades to the event base - stricter than the intended measurement, never looser - and the gate
113+
# logs it.
111114
continue-on-error: true
112115
uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2
113116
with:
114117
name: mutation-stacked-map
115-
path: .mutation
118+
path: ${{ runner.temp }}/mutation-stacked-map
116119

117120
- name: Enforce executable-line scope and run advisory mutation testing
118121
if: github.event_name == 'pull_request'
@@ -128,7 +131,7 @@ jobs:
128131
# The event already carries that head for provenance, so it wins here; deriving it from
129132
# the merge commit stays as the fallback for a manual run with no pull_request payload.
130133
PR_HEAD_SHA="${PR_HEAD_SHA:-$(git rev-parse "$HEAD_SHA^2" 2>/dev/null || git rev-parse "$HEAD_SHA")}"
131-
node scripts/stryker-diff.mjs ci --base "$BASE_SHA" --head "$HEAD_SHA" --pr-head "$PR_HEAD_SHA" --pr-base-ref "$PR_BASE_REF" --stacked-map-file .mutation/stacked-map.json
134+
node scripts/stryker-diff.mjs ci --base "$BASE_SHA" --head "$HEAD_SHA" --pr-head "$PR_HEAD_SHA" --pr-base-ref "$PR_BASE_REF" --stacked-map-file "${RUNNER_TEMP}/mutation-stacked-map/stacked-map.json"
132135
133136
- name: Upload mutation reports
134137
id: mutation_report

‎scripts/stryker-diff.mjs‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -394,14 +394,25 @@ export function dependencyManifestDrift(repoRoot, installedSha, alignedSha) {
394394
return changed.trim().split(/\n+/).filter(Boolean)
395395
}
396396

397+
/**
398+
* The detail of a failed `pnpm install --frozen-lockfile`. pnpm writes its diagnostics to stderr, but a
399+
* killed or signal-terminated run leaves stderr empty, and "" is a valid ?? operand: with ?? the failure
400+
* would report a blank detail and lose the exit status that is the only remaining fact. || treats the
401+
* empty string as absent, so the chain reaches the exit status.
402+
*/
403+
export function reinstallFailureDetail(result) {
404+
return result.error?.message || result.stderr?.trim() || `exit ${result.status}`
405+
}
406+
397407
function reinstallAlignedDependencies(repoRoot, driftedManifests) {
398408
console.log(
399409
`Reinstalling dependencies from the aligned tree: ${driftedManifests.join(", ")} differ from the tree they were installed from.`,
400410
)
401411
const result = spawnSync("pnpm", ["install", "--frozen-lockfile"], { cwd: repoRoot, encoding: "utf8" })
402412
if (result.error || result.status !== 0) {
403-
const detail = result.error?.message ?? result.stderr?.trim() ?? `exit ${result.status}`
404-
throw new Error(`The aligned tree changed ${driftedManifests.join(", ")} and the reinstall failed: ${detail}`)
413+
throw new Error(
414+
`The aligned tree changed ${driftedManifests.join(", ")} and the reinstall failed: ${reinstallFailureDetail(result)}`,
415+
)
405416
}
406417
}
407418

@@ -840,13 +851,16 @@ export function appendSummary(rows, advisories, manifest, commentFile) {
840851
* trace of the run is a step log: the author cannot see which base the gate chose, which is the fact
841852
* that explains a stacked unit's numbers. The caller rethrows - this only makes the failure visible.
842853
*/
843-
export function summarizeReinstallFailure(stackedOn, driftedManifests, error) {
854+
export function summarizeReinstallFailure(invocation, driftedManifests, error) {
844855
appendSummary(
845856
[],
846857
[
847858
`Dependency reinstall failed for the aligned tree (${driftedManifests.join(", ")}): ${error.message} The gate measured nothing; fix the lockfile drift and re-run.`,
848859
],
849-
{ stackedOn },
860+
// The base and the diff head belong in this summary too: once the parent PR's head advances, the
861+
// selected base is the only way to trace which commit the run measured, and formatSummary turns
862+
// these two into the "Reproduce locally" command.
863+
{ stackedOn: invocation.stackedOn, baseSha: invocation.baseSha, headSha: invocation.diffHead },
850864
)
851865
}
852866

@@ -1094,7 +1108,7 @@ function main() {
10941108
try {
10951109
reinstallAlignedDependencies(repoRoot, driftedManifests)
10961110
} catch (error) {
1097-
summarizeReinstallFailure(invocation.stackedOn, driftedManifests, error)
1111+
summarizeReinstallFailure(invocation, driftedManifests, error)
10981112
throw error
10991113
}
11001114
}

‎scripts/stryker-diff.test.mjs‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import {
3232
preferDirectTestFiles,
3333
parseStackedMap,
3434
readStackedMap,
35+
reinstallFailureDetail,
3536
summarizeReinstallFailure,
3637
dependencyManifestDrift,
3738
alignExecutionTree,
@@ -84,7 +85,12 @@ describe("mutation testing workflow", () => {
8485
// The map crosses jobs as a file, never as an argument or an environment string: Linux caps one
8586
// argument/env string at 128 KiB (MAX_ARG_STRLEN), and a map past that cap makes the step
8687
// unstartable rather than merely large.
87-
assert.ok(workflow.includes("--stacked-map-file .mutation/stacked-map.json"))
88+
assert.ok(workflow.includes('--stacked-map-file "${RUNNER_TEMP}/mutation-stacked-map/stacked-map.json"'))
89+
// The map must land outside the checkout: a pull request can commit .mutation/stacked-map.json,
90+
// and with continue-on-error a missing artifact would leave that committed file in front of the
91+
// gate, which then measures against whatever base the pull request chose for itself.
92+
assert.ok(workflow.includes("path: ${{ runner.temp }}/mutation-stacked-map"))
93+
assert.ok(!workflow.includes("path: .mutation"))
8894
assert.ok(!workflow.includes('--stacked-map "$STACKED_MAP"'))
8995
assert.ok(workflow.includes("name: mutation-stacked-map"))
9096
assert.ok(workflow.includes('--pr-head "$PR_HEAD_SHA"'))
@@ -580,10 +586,19 @@ describe("stacked unit base resolution", () => {
580586
const warn = console.warn
581587
console.warn = (message) => warnings.push(message)
582588
try {
583-
summarizeReinstallFailure(7, ["src/package.json", "pnpm-lock.yaml"], new Error("pnpm install exited 1"))
589+
summarizeReinstallFailure(
590+
{ stackedOn: 7, baseSha: "1".repeat(40), diffHead: "2".repeat(40) },
591+
["src/package.json", "pnpm-lock.yaml"],
592+
new Error("pnpm install exited 1"),
593+
)
584594
const summary = fs.readFileSync(summaryPath, "utf8")
585-
// The whole point of the summary here: which base this unit was measured against.
595+
// The whole point of the summary here: which base this unit was measured against, and the
596+
// command that reproduces it - the parent head moves, the selected base is what is left.
586597
assert.ok(summary.includes("Stacked unit: measured against the head of parent PR #7"))
598+
assert.ok(
599+
summary.includes(`node scripts/stryker-diff.mjs ci --base ${"1".repeat(40)} --head ${"2".repeat(40)}`),
600+
)
601+
assert.ok(summary.includes("git checkout " + "2".repeat(40)))
587602
assert.ok(summary.includes("src/package.json, pnpm-lock.yaml"))
588603
assert.ok(summary.includes("pnpm install exited 1"))
589604
assert.equal(warnings.length, 1)
@@ -595,6 +610,21 @@ describe("stacked unit base resolution", () => {
595610
}
596611
})
597612

613+
it("keeps the exit status when the reinstall writes nothing to stderr", () => {
614+
// A killed or signal-terminated pnpm leaves stderr empty, and "" is a valid ?? operand: with ?? the
615+
// report would end in a blank detail and lose the only fact that is left.
616+
assert.equal(reinstallFailureDetail({ status: 1, stderr: "" }), "exit 1")
617+
assert.equal(reinstallFailureDetail({ status: null, stderr: undefined }), "exit null")
618+
assert.equal(
619+
reinstallFailureDetail({ status: 1, stderr: " ERR_PNPM_OUTDATED_LOCKFILE " }),
620+
"ERR_PNPM_OUTDATED_LOCKFILE",
621+
)
622+
assert.equal(
623+
reinstallFailureDetail({ status: null, stderr: "", error: new Error("spawnSync pnpm ENOENT") }),
624+
"spawnSync pnpm ENOENT",
625+
)
626+
})
627+
598628
it("degrades to the event base for an unparsable stacked map", () => {
599629
assert.deepEqual(parseStackedMap("not json"), [])
600630
assert.deepEqual(parseStackedMap('{"number": 1}'), [])

0 commit comments

Comments
 (0)