diff --git a/lib/ci/build-types/pr_build.js b/lib/ci/build-types/pr_build.js index 87ccdd41..3129c3f4 100644 --- a/lib/ci/build-types/pr_build.js +++ b/lib/ci/build-types/pr_build.js @@ -21,6 +21,7 @@ export class PRBuild extends TestBuild { super(cli, request, path, tree); this.skipMoreThan = skipMoreThan; this.commitBuild = null; + this.targetBranch = undefined; } // Get the failures and their reasons of this build @@ -71,6 +72,12 @@ export class PRBuild extends TestBuild { // Get result from the sub build // assert.strictEqual(commitBuild.jobName, 'node-test-commit'); const allBuilds = commitBuild.build?.subBuilds; + const targetBranch = commitBuild.build?.actions + ?.flatMap(action => action.parameters ?? []) + .find(parameter => parameter.name === 'PR_BASE_BRANCH')?.value; + if (typeof targetBranch === 'string' && targetBranch.length > 0) { + this.targetBranch = targetBranch; + } // TODO: fetch result, builtOn, timestamp in the commit build's own data // ..or maybe they do not worth an additional API call? // Note that we have to pass the actions down to detect resume builds. @@ -96,6 +103,14 @@ export class PRBuild extends TestBuild { return { result, builds, failures }; } + formatAsJson() { + const failures = super.formatAsJson(); + if (this.targetBranch === undefined) { + return failures; + } + return failures.map(failure => ({ ...failure, targetBranch: this.targetBranch })); + } + formatAsMarkdown() { if (!this.commitBuild) { let result = 'Failed to trigger node-test-commit'; diff --git a/lib/ci/failure_aggregator.js b/lib/ci/failure_aggregator.js index ca530f52..9f94b39e 100644 --- a/lib/ci/failure_aggregator.js +++ b/lib/ci/failure_aggregator.js @@ -19,6 +19,23 @@ function uniqBy(array, key) { return array.filter((item) => !seen.has(item[key]) && seen.add(item[key])); } +function formatSource(source) { + const parsed = parsePRFromURL(source); + return parsed ? `${parsed.owner}/${parsed.repo}#${parsed.prid}` : source; +} + +function formatTargetBranches(pr, markdown = false) { + if (!parsePRFromURL(pr.source)) { + return ''; + } + const branches = pr.targetBranches + .filter(branch => branch !== 'main') + .map(branch => markdown + ? branch.replace(/[&<>"'\\`*_[\]{}()!|~]/g, char => `&#${char.charCodeAt(0)};`) + : branch); + return branches.length ? ` (target: ${branches.join(', ')})` : ''; +} + export class FailureAggregator { constructor(cli, data) { this.cli = cli; @@ -37,8 +54,20 @@ export class FailureAggregator { // If multiple sub builds of one PR are failed by the same reason, // we'll only take one of those builds, as that might be a genuine failure + const branchesBySource = new Map(); + for (const { source, targetBranch } of failures) { + if (!branchesBySource.has(source)) { + branchesBySource.set(source, new Set()); + } + branchesBySource.get(source).add(targetBranch || 'unknown'); + } const prs = uniqBy(failures, 'source') - .map(({ source, upstream }) => ({ source, upstream, _id: parseJobFromURL(upstream).jobid })) + .map(({ source, upstream }) => ({ + source, + upstream, + targetBranches: [...branchesBySource.get(source)], + _id: parseJobFromURL(upstream).jobid + })) .sort((a, b) => a._id - b._id); const machines = uniqBy( failures.map(f => ({ hostname: f.builtOn, url: f.url })), @@ -89,7 +118,8 @@ export class FailureAggregator { output += markdownRow('Reason', `${reason}`); output += markdownRow('-', ':-'); output += markdownRow('Type', type); - const source = prs.map(f => `[${f.source}](${f.upstream})`); + const source = prs.map(f => + `[${formatSource(f.source)}](${f.upstream})${formatTargetBranches(f, true)}`); output += markdownRow( 'Failed PR', `${source.length} (${source.join(', ')})` ); @@ -129,11 +159,7 @@ export class FailureAggregator { const { reason, type, prs, failures, machines } = item; cli.table('Reason', reason); cli.table('Type', type); - const source = prs - .map(f => { - const parsed = parsePRFromURL(f.source); - return parsed ? `#${parsed.prid}` : f.source; - }); + const source = prs.map(f => `${formatSource(f.source)}${formatTargetBranches(f)}`); cli.table('Failed PR', `${source.length} (${source.join(', ')})`); cli.table('Appeared', machines.map(m => m.hostname).join(', ')); if (prs.length > 1) { diff --git a/lib/ci/jenkins_constants.js b/lib/ci/jenkins_constants.js index ca896d15..291d36ed 100644 --- a/lib/ci/jenkins_constants.js +++ b/lib/ci/jenkins_constants.js @@ -6,7 +6,7 @@ const CHANGE_FIELDS = 'commitId,author[absoluteUrl,fullName],authorEmail,' + const CHANGE_TREE = `changeSet[items[${CHANGE_FIELDS}]]`; export const PR_TREE = `result,url,number,${ACTION_TREE},${CHANGE_TREE},builtOn,` + - `subBuilds[${BUILD_FIELDS},build[subBuilds[${BUILD_FIELDS}]]]`; + `subBuilds[${BUILD_FIELDS},build[${ACTION_TREE},subBuilds[${BUILD_FIELDS}]]]`; export const COMMIT_TREE = `result,url,number,${ACTION_TREE},${CHANGE_TREE},builtOn,` + `subBuilds[${BUILD_FIELDS}]`; diff --git a/test/unit/ci_failure_aggregator.test.js b/test/unit/ci_failure_aggregator.test.js new file mode 100644 index 00000000..043db40a --- /dev/null +++ b/test/unit/ci_failure_aggregator.test.js @@ -0,0 +1,115 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +import { FailureAggregator } from '../../lib/ci/failure_aggregator.js'; +import TestCLI from '../fixtures/test_cli.js'; + +function failure(prid, jobid, targetBranch, reason = 'Example failure') { + return { + source: `https://github.com/nodejs/node/pull/${prid}/`, + upstream: `https://ci.nodejs.org/job/node-test-pull-request/${jobid}/`, + url: `https://ci.nodejs.org/job/node-test-commit-linux/${jobid}/console`, + type: 'BUILD_FAILURE', + reason, + highlight: 0, + builtOn: 'test-worker', + ...(targetBranch === undefined ? {} : { targetBranch }) + }; +} + +const health = { formatAsMarkdown: () => 'Health summary\n' }; + +describe('Failure aggregation target branches', () => { + it('preserves branches across runs without changing PR counts or CI ordering', () => { + const cli = new TestCLI(); + const failures = [ + failure(1, 105, 'main'), + failure(1, 104, 'v24.x'), + failure(1, 103, 'main'), + failure(2, 102, 'v22.x'), + failure(3, 101, 'main', 'Another failure') + ]; + const aggregator = new FailureAggregator(cli, [health, ...failures]); + const groups = aggregator.aggregate().BUILD_FAILURE; + assert.deepEqual(groups.map(group => group.prs.length), [2, 1]); + assert.deepEqual(groups[0].prs.map(pr => ({ + source: pr.source, upstream: pr.upstream, targetBranches: pr.targetBranches + })), [ + { source: failures[3].source, upstream: failures[3].upstream, targetBranches: ['v22.x'] }, + { + source: failures[0].source, + upstream: failures[0].upstream, + targetBranches: ['main', 'v24.x'] + } + ]); + + const markdown = aggregator.formatAsMarkdown(); + assert.ok(markdown.includes('| Failed PR | 2 (' + + `[nodejs/node#2](${failures[3].upstream}) (target: v22.x), ` + + `[nodejs/node#1](${failures[0].upstream}) (target: v24.x)) |`)); + assert.ok(markdown.includes(`| First CI | ${failures[3].upstream} |`)); + assert.ok(markdown.includes(`| Last CI | ${failures[0].upstream} |`)); + assert.ok(markdown.includes('Health summary\n')); + assert.ok(markdown.endsWith('### Progress\n\n- [ ] `Example failure` (2)\n')); + assert.equal(markdown.includes('Another failure'), false); + + aggregator.display(); + assert.deepEqual(cli._calls.table.filter(([key]) => key === 'Failed PR'), [ + ['Failed PR', '2 (nodejs/node#2 (target: v22.x), nodejs/node#1 (target: v24.x))'], + ['Failed PR', '1 (nodejs/node#3)'] + ]); + }); + + it('reports missing metadata as unknown alongside recorded branches', () => { + const cli = new TestCLI(); + const aggregator = new FailureAggregator(cli, [health, + failure(1, 104), failure(1, 103, 'main'), failure(1, 102), failure(2, 101) + ]); + const markdown = aggregator.formatAsMarkdown(); + assert.equal(markdown.includes('target: unknown, main'), false); + assert.ok(markdown.includes('(target: unknown)')); + aggregator.display(); + assert.deepEqual(cli._calls.table.find(([key]) => key === 'Failed PR'), + ['Failed PR', '2 (nodejs/node#2 (target: unknown), nodejs/node#1 (target: unknown))']); + }); + + it('hides main targets and abbreviates PR labels without changing CI links', () => { + const cli = new TestCLI(); + const failures = [failure(1, 102, 'main'), failure(2, 101, 'main')]; + failures[1].source = 'https://github.com/nodejs-private/node-private/pull/2/'; + const aggregator = new FailureAggregator(cli, [health, ...failures]); + const markdown = aggregator.formatAsMarkdown(); + assert.ok(markdown.includes('| Failed PR | 2 (' + + `[nodejs-private/node-private#2](${failures[1].upstream}), ` + + `[nodejs/node#1](${failures[0].upstream})) |`)); + assert.equal(markdown.includes('target:'), false); + aggregator.display(); + assert.deepEqual(cli._calls.table.find(([key]) => key === 'Failed PR'), + ['Failed PR', '2 (nodejs-private/node-private#2, nodejs/node#1)']); + }); + + it('escapes Markdown and HTML in branch names while preserving terminal text', () => { + const branch = 'feature/a|b`c&_e_(f)'; + const cli = new TestCLI(); + const aggregator = new FailureAggregator(cli, [health, + failure(1, 102, branch), failure(2, 101, 'main') + ]); + const markdown = aggregator.formatAsMarkdown(); + const escaped = 'feature/a|b`c&<d>_e_(f)'; + assert.ok(markdown.includes(`(target: ${escaped})`)); + assert.equal(markdown.includes(branch), false); + aggregator.display(); + assert.ok(cli._calls.table.find(([key]) => key === 'Failed PR')[1].includes(branch)); + }); + + it('leaves non-PR sources unannotated', () => { + const failures = [failure(1, 102), failure(2, 101)]; + for (const [index, item] of failures.entries()) { + item.source = `https://api.github.com/repos/nodejs/node/git/commit-${index}`; + } + const aggregator = new FailureAggregator(new TestCLI(), [health, ...failures]); + const markdown = aggregator.formatAsMarkdown(); + assert.equal(markdown.includes('target:'), false); + assert.ok(markdown.includes(`[${failures[0].source}](${failures[0].upstream})`)); + }); +}); diff --git a/test/unit/ci_pr_target_branch.test.js b/test/unit/ci_pr_target_branch.test.js new file mode 100644 index 00000000..e8a46bba --- /dev/null +++ b/test/unit/ci_pr_target_branch.test.js @@ -0,0 +1,93 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +import { PRBuild } from '../../lib/ci/build-types/pr_build.js'; +import TestCLI from '../fixtures/test_cli.js'; + +function buildData() { + return { + result: 'FAILURE', + actions: [{ + parameters: [ + { name: 'TARGET_GITHUB_ORG', value: 'nodejs' }, + { name: 'TARGET_REPO_NAME', value: 'node' }, + { name: 'PR_ID', value: '123' }, + { name: 'REBASE_ONTO', value: '' } + ] + }], + changeSet: { items: [] }, + timestamp: 0, + subBuilds: [{ buildNumber: 456, build: { subBuilds: [] } }] + }; +} + +describe('PR target branch reporting', () => { + for (const targetBranch of ['main', 'v24.x', 'release/example']) { + it(`retains the recorded target ${targetBranch} in failure JSON`, async() => { + const data = buildData(); + data.subBuilds[0].build.actions = [{}, { + parameters: [ + { name: 'REBASE_ONTO', value: 'origin/another-branch' }, + { name: 'PR_BASE_BRANCH', value: targetBranch } + ] + }]; + const request = { + async json(url) { + assert.ok(new URL(url).searchParams.get('tree') + .includes('build[actions[parameters[name,value]],subBuilds[')); + return data; + } + }; + const build = new PRBuild(new TestCLI(), request, 123); + await build.getResults(); + + const failures = build.formatAsJson(); + assert.equal(failures.length, 1); + assert.equal(failures[0].targetBranch, targetBranch); + assert.equal(failures[0].source, 'https://github.com/nodejs/node/pull/123/'); + assert.equal(failures[0].upstream, build.jobUrl); + assert.equal(failures[0].reason, 'Failed to trigger sub builds'); + // Nested parameters must not replace the PR job's parameters or actions. + assert.equal(build.commitBuild.params.REBASE_ONTO, ''); + assert.equal(Object.hasOwn(build.failures[0], 'targetBranch'), false); + }); + } + + for (const scenario of [ + 'missing actions', 'missing parameter', 'empty branch', 'invalid branch', + 'unavailable commit build', 'untriggered commit build' + ]) { + it(`omits targetBranch for ${scenario}`, async(t) => { + const data = buildData(); + // Parent parameters must not substitute for the nested build's metadata. + data.actions[0].parameters.push({ name: 'PR_BASE_BRANCH', value: 'wrong' }); + switch (scenario) { + case 'missing parameter': + data.subBuilds[0].build.actions = [{}, { parameters: [] }]; + break; + case 'empty branch': + case 'invalid branch': + data.subBuilds[0].build.actions = [{ + parameters: [{ + name: 'PR_BASE_BRANCH', + value: scenario === 'empty branch' ? '' : null + }] + }]; + break; + case 'unavailable commit build': + data.subBuilds[0].build = null; + break; + case 'untriggered commit build': + data.subBuilds = []; + break; + } + const build = new PRBuild(new TestCLI(), {}, 123); + t.mock.method(build, 'getBuildData', async() => data); + await build.getResults(); + + const failures = build.formatAsJson(); + assert.equal(failures.length, 1); + assert.equal(Object.hasOwn(failures[0], 'targetBranch'), false); + }); + } +});