Skip to content

Commit 22ff79b

Browse files
committed
fix: ignore make exit summaries in resume checks
A failed make target names its Makefile even when an unrelated test failed. Exclude these summaries from file attribution while retaining direct Makefile diagnostics and failures in changed tests. Assisted-by: Codex Signed-off-by: Filip Skokan <panva.ip@gmail.com>
1 parent db7a98b commit 22ff79b

5 files changed

Lines changed: 116 additions & 0 deletions

File tree

‎docs/ncu-ci.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,8 @@ Before resuming, the command streams failed-job console output and compares
247247
failure diagnostics with the PR's changed files. When recovering through resume
248248
ancestry, it checks the latest run and every ancestor visited. It refuses to resume
249249
if a failed test or a file referenced in a failure diagnostic is changed by the PR.
250+
Generic `make` recipe-failure summaries do not count as file references; direct
251+
Makefile diagnostics, such as syntax errors, still do.
250252
The refusal includes the first matching filename, failure excerpt, and console
251253
log URL. Large excerpts are truncated with an explicit marker.
252254
Logs are scanned one at a time with bounded memory. HTTP compression is decoded as the

‎lib/ci/failure_file_scanner.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,11 @@ const diagnostic = createMatcher(FAILURE_PATTERNS.diagnostic);
1313
const infrastructure = createMatcher(FAILURE_PATTERNS.infrastructure);
1414
const gitStart = createMatcher(FAILURE_MARKERS.git.map(({ start }) => start));
1515
const gitEnd = createMatcher(FAILURE_MARKERS.git.map(({ end }) => end));
16+
const makeRecipeFailure = createMatcher([
17+
/^\s*(?:\[(?:out|err)\]\s*)?g?make(?:\[\d+\])?: \*\*\* \[[^\r\n]+\] .+$/,
18+
/^\s*(?:\[(?:out|err)\]\s*)?[^\r\n]+:\d+: recipe for target '.+' failed$/
19+
]);
20+
const makefileDiagnostic = /^\s*(?:\[(?:out|err)\]\s*)?[^\r\n]+:\d+: \*\*\* /;
1621

1722
// Keep both ends of large diagnostics without retaining whole lines or TAP blocks.
1823
class FailureExcerpt {
@@ -97,6 +102,13 @@ async function * failureLines(windows, matchFile) {
97102
line.gitEnd ||= gitEnd(text);
98103
if (lineEnd) {
99104
line.text = excerpt.toString();
105+
if (makeRecipeFailure(line.text.trimEnd())) {
106+
// The recipe location reports a child's exit, not the source of its
107+
// failure. Keep the output without attributing it through TAP or history.
108+
line = { text: line.text, todo: line.todo };
109+
} else {
110+
line.diagnostic ||= makefileDiagnostic.test(line.text);
111+
}
100112
yield line;
101113
line = {};
102114
excerpt = new FailureExcerpt();
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"pr": "https://github.com/nodejs/node/pull/66228",
3+
"url": "https://ci.nodejs.org/job/node-test-commit-linux/nodes=fedora-latest-x64/73461/consoleText",
4+
"filename": "test/ffi/test-ffi-calls.js",
5+
"failure": "not ok 81 ffi/test-ffi-calls\n ---\n duration_ms: 123029.13600\n severity: fail\n exitcode: -15\n stack: |-\n timeout\n (node:1962772) ExperimentalWarning: FFI is an experimental feature and might change at any time\n (Use `node --trace-warnings ...` to show where the warning was created)\n ...",
6+
"summary": "make[1]: *** [Makefile:660: test-ci] Error 1"
7+
}

‎test/unit/ci_failure_file_scanner.test.js‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ async function scan(...args) {
3131
// Complete console diagnostics and truncated report excerpts from September 23–25, 2026.
3232
const reliabilityFailures = JSON.parse(readFileSync(
3333
new URL('../fixtures/ci-reliability-failures.json', import.meta.url), 'utf8'));
34+
const makeFailure = JSON.parse(readFileSync(
35+
new URL('../fixtures/ci-resume-make-failure.json', import.meta.url), 'utf8'));
3436

3537
describe('Reliability report diagnostics', () => {
3638
for (const { name, kind, filenames, log } of reliabilityFailures) {
@@ -49,6 +51,74 @@ describe('Reliability report diagnostics', () => {
4951
}
5052
});
5153

54+
describe('Make recipe failure summaries', () => {
55+
const summary = makeFailure.summary;
56+
57+
it('attributes a test timeout to the test rather than the make recipe', async() => {
58+
const log = `${makeFailure.failure}\n${summary}\n`;
59+
for (const size of [1, 31, 8192]) {
60+
assert.equal(await scan(log, ['Makefile'], size), undefined);
61+
assert.deepEqual(await scanFailure(log, ['Makefile', makeFailure.filename], size),
62+
{ filename: makeFailure.filename, reason: makeFailure.failure });
63+
}
64+
});
65+
66+
for (const [name, footer] of [
67+
['recursive make', summary],
68+
['top-level make', 'make: *** [Makefile:660: test-ci] Error 2'],
69+
['gmake', 'gmake[2]: *** [Makefile:660: test-ci] Error 1'],
70+
['segmentation fault', 'make[1]: *** [Makefile:660: test-ci] Segmentation fault (core dumped)'],
71+
['abort', 'make[1]: *** [Makefile:660: test-ci] Aborted (core dumped)'],
72+
['failed recipe', "Makefile:660: recipe for target 'test-ci' failed"],
73+
['prefixed stdout', ' [out] make[1]: *** [Makefile:660: test-ci] Error 1'],
74+
['prefixed stderr', " [err] Makefile:660: recipe for target 'test-ci' failed"]
75+
]) {
76+
it(`ignores a propagated exit summary: ${name}`, async() => {
77+
for (const size of [1, 31, 8192]) {
78+
assert.equal(await scan(`${footer}\n`, ['Makefile'], size), undefined);
79+
}
80+
});
81+
}
82+
83+
for (const [name, log] of [
84+
['preceding error', `error: unrelated failure\n${summary}\n`],
85+
['following filesystem error', `${summary}\nRead-only file system\n`],
86+
['following C++ failure', `${summary}\n[ FAILED ] Example\n`],
87+
['following filename', `${summary}\nMakefile\n`],
88+
['TAP block', tap(` ${summary}`)],
89+
['git failure block',
90+
`Changes not staged for commit:\n${summary}\nno changes added to commit\n`]
91+
]) {
92+
it(`does not use a propagated exit summary as failure context: ${name}`, async() => {
93+
for (const size of [1, 31, 8192]) {
94+
assert.equal(await scan(log, ['Makefile'], size), undefined);
95+
}
96+
});
97+
}
98+
99+
it('continues to later failures and preserves the summary as output context', async() => {
100+
const failure = tap(` ${summary}\n AssertionError: failure`);
101+
const log = `${summary}\n${failure}`;
102+
assert.deepEqual(await scanFailure(log, ['Makefile', filename], 1),
103+
{ filename, reason: failure.trimEnd() });
104+
});
105+
106+
for (const [name, reason] of [
107+
['missing separator', 'Makefile:123: *** missing separator. Stop.'],
108+
['unterminated variable', 'Makefile:123: *** unterminated variable reference. Stop.'],
109+
['recipe before target', 'Makefile:123: *** recipe commences before first target. Stop.'],
110+
['invalid recipe', 'Makefile:123: error: invalid recipe'],
111+
['unreadable makefile', 'fatal: Unable to read Makefile: No such file or directory']
112+
]) {
113+
it(`retains a genuine Makefile diagnostic: ${name}`, async() => {
114+
for (const size of [1, 31, 8192]) {
115+
assert.deepEqual(await scanFailure(`${reason}\n`, ['Makefile'], size),
116+
{ filename: 'Makefile', reason });
117+
}
118+
});
119+
}
120+
});
121+
52122
describe('Streaming failure file scanner', () => {
53123
it('returns the matching TAP failure without duplicating chunk overlaps', async() => {
54124
const failure = tap(' severity: fail\n AssertionError: expected true, received false');

‎test/unit/ci_resume.test.js‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ const walkFailures = JSON.parse(readFileSync(
5959
new URL('../fixtures/ci-resume-walk.json', import.meta.url), 'utf8'));
6060
const reliabilityFailures = JSON.parse(readFileSync(
6161
new URL('../fixtures/ci-reliability-failures.json', import.meta.url), 'utf8'));
62+
const makeFailure = JSON.parse(readFileSync(
63+
new URL('../fixtures/ci-resume-make-failure.json', import.meta.url), 'utf8'));
6264

6365
describe('Resume file checks against real CI diagnostics', () => {
6466
for (const { name, kind, filenames, log, url } of reliabilityFailures) {
@@ -178,6 +180,29 @@ describe('Jenkins resume', () => {
178180
assert.deepEqual(cli._calls.stopSpinner.at(-1), ['PR CI job successfully resumed']);
179181
});
180182

183+
it('allows resuming when only a make exit summary references a changed file', async() => {
184+
request.json.withArgs(filesURL).resolves([
185+
{ filename: 'Makefile' },
186+
{ filename: '.github/workflows/build-tarball.yml' },
187+
{ filename: '.github/workflows/test-shared.yml' }
188+
]);
189+
request.text.resolves(`${makeFailure.failure}\n${makeFailure.summary}\n`);
190+
assert.equal(await jobRunner.resume(), true);
191+
sinon.assert.calledOnce(resumeRequest);
192+
assert.deepEqual(cli._calls.error, []);
193+
});
194+
195+
it('refuses resuming when the failing test changed despite a make exit summary', async() => {
196+
request.json.withArgs(filesURL).resolves([
197+
{ filename: 'Makefile' }, { filename: makeFailure.filename }
198+
]);
199+
request.text.resolves(`${makeFailure.failure}\n${makeFailure.summary}\n`);
200+
assert.equal(await jobRunner.resume(), false);
201+
sinon.assert.notCalled(resumeRequest);
202+
assert.deepEqual(cli._calls.error, [[makeFailure.filename]]);
203+
assert.deepEqual(cli._calls.log, [[makeFailure.failure]]);
204+
});
205+
181206
for (const result of ['FAILURE', 'ABORTED']) {
182207
it(`does not scan failures or POST when a ${result} job has no resume action`, async() => {
183208
request.json.withArgs(apiURL).resolves({ ...resumeBuildData, result });

0 commit comments

Comments
 (0)