Skip to content

Commit e97ab7f

Browse files
committed
fix: handle unavailable Jenkins commit build data
Jenkins can retain a successful PR build result while returning null for its nested commit build. Accessing that build's subBuilds crashes git node metadata and aborts commit queue filtering. Preserve the recorded result when child build data is unavailable. Report unavailable details for non-successful builds without throwing. Refs: https://github.com/nodejs/node/actions/runs/34948903953/job/104314954048 Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex
1 parent d4bbaeb commit e97ab7f

3 files changed

Lines changed: 79 additions & 1 deletion

File tree

‎lib/ci/build-types/pr_build.js‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { PR_TREE } from '../jenkins_constants.js';
2+
import { statusType } from '../ci_utils.js';
23
import CIFailureParser from '../ci_failure_parser.js';
34
import { TestBuild } from './test_build.js';
45
import { CommitBuild } from './commit_build.js';
@@ -69,7 +70,7 @@ export class PRBuild extends TestBuild {
6970

7071
// Get result from the sub build
7172
// assert.strictEqual(commitBuild.jobName, 'node-test-commit');
72-
const allBuilds = commitBuild.build.subBuilds;
73+
const allBuilds = commitBuild.build?.subBuilds;
7374
// TODO: fetch result, builtOn, timestamp in the commit build's own data
7475
// ..or maybe they do not worth an additional API call?
7576
// Note that we have to pass the actions down to detect resume builds.
@@ -78,6 +79,15 @@ export class PRBuild extends TestBuild {
7879
};
7980
const commitBuildId = commitBuild.buildNumber;
8081
this.commitBuild = new CommitBuild(cli, request, commitBuildId, this.skipMoreThan);
82+
// Jenkins can retain the result after the nested build is no longer available.
83+
if (!commitBuild.build) {
84+
this.commitBuild.setBuildData(buildData);
85+
this.failures = result === statusType.SUCCESS
86+
? []
87+
: [new NCUFailure({ url: this.commitBuild.jobUrl }, 'Build data is unavailable')];
88+
this.commitBuild.failures = this.failures;
89+
return { result, builds: this.builds, failures: this.failures };
90+
}
8191
const { builds, failures } = await this.commitBuild.getResults(buildData);
8292

8393
// Set up aliases for display

‎test/unit/ci_result_parser.test.js‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,42 @@ describe('Jenkins', () => {
9494
assert.strictEqual(markdown, expected);
9595
});
9696

97+
for (const result of ['SUCCESS', 'FAILURE', null]) {
98+
it(`should handle unavailable commit build data with result ${result}`, async(t) => {
99+
const data = fixtures.readJSON(
100+
'jenkins', 'success', 'node-test-pull-request-15237.json'
101+
);
102+
data.result = result;
103+
data.subBuilds[0].result = result;
104+
data.subBuilds[0].build = null;
105+
106+
const cli = new TestCLI();
107+
const prBuild = new PRBuild(cli, {}, 15237);
108+
t.mock.method(prBuild, 'getBuildData', async() => data);
109+
const results = await prBuild.getResults();
110+
111+
assert.strictEqual(results.result, result);
112+
assert.deepStrictEqual(results.builds, {
113+
failed: [], aborted: [], pending: [], unstable: []
114+
});
115+
assert.strictEqual(prBuild.commitBuild.result, result);
116+
assert.deepStrictEqual(prBuild.commitBuild.failures, prBuild.failures);
117+
if (result === 'SUCCESS') {
118+
assert.deepStrictEqual(results.failures, []);
119+
assert.deepStrictEqual(prBuild.formatAsJson(), []);
120+
assert.strictEqual(prBuild.formatAsMarkdown(), `Job ${prBuild.jobUrl} is green.`);
121+
} else {
122+
assert.strictEqual(results.failures.length, 1);
123+
assert.strictEqual(results.failures[0].type, 'NCU_FAILURE');
124+
assert.strictEqual(results.failures[0].url, prBuild.commitBuild.jobUrl);
125+
assert.strictEqual(results.failures[0].reason, 'Build data is unavailable');
126+
assert.match(prBuild.formatAsMarkdown(), /Build data is unavailable/);
127+
assert.strictEqual(prBuild.formatAsJson()[0].reason, 'Build data is unavailable');
128+
}
129+
prBuild.display();
130+
});
131+
}
132+
97133
it('should handle node-test-commit trigger failure', async() => {
98134
tmpdir.refresh();
99135
const prefix = ['jenkins', 'trigger-failure'];

‎test/unit/pr_checker.test.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1386,6 +1386,38 @@ describe('PRChecker', () => {
13861386
});
13871387
});
13881388

1389+
for (const result of ['SUCCESS', 'FAILURE', null]) {
1390+
it(`should check CI with unavailable commit build data and result ${result}`, async() => {
1391+
const jenkins = structuredClone(jenkinsCI['success/node-test-pull-request-15237']);
1392+
jenkins.result = result;
1393+
jenkins.subBuilds[0].result = result;
1394+
jenkins.subBuilds[0].build = null;
1395+
const data = {
1396+
pr: firstTimerPR,
1397+
comments: commentsWithSuccessCI,
1398+
reviews: approvingReviews,
1399+
commits: githubCI['check-suite-success'],
1400+
getThread() {
1401+
return PRData.prototype.getThread.call(this);
1402+
}
1403+
};
1404+
const checker = new PRChecker(new TestCLI(), data, {
1405+
async json(url) {
1406+
assert(url.startsWith(jenkins.url));
1407+
return jenkins;
1408+
}
1409+
}, argv);
1410+
1411+
assert.strictEqual(Boolean(await checker.checkCI()), result === 'SUCCESS');
1412+
assert.deepStrictEqual(checker.reasons.map(({ code }) => code),
1413+
result === 'SUCCESS'
1414+
? []
1415+
: [result === null
1416+
? PR_CHECK_REASON_CODES.PENDING_JENKINS_CI
1417+
: PR_CHECK_REASON_CODES.FAILED_JENKINS_CI]);
1418+
});
1419+
}
1420+
13891421
it('should error if failed Jenkins CI', async() => {
13901422
const cli = new TestCLI();
13911423

0 commit comments

Comments
 (0)