diff --git a/bin/ncu-ci.js b/bin/ncu-ci.js index 80803a7e..ddf7cdf4 100755 --- a/bin/ncu-ci.js +++ b/bin/ncu-ci.js @@ -460,8 +460,8 @@ class WalkCommand extends CICommand { if (this.queue.length === 0) { return; } - const aggregator = new FailureAggregator(cli, this.json); - this.json = aggregator.aggregate(); + const aggregator = new FailureAggregator(cli, this.json, this.request); + this.json = await aggregator.aggregate(); cli.log(''); cli.separator('Stats'); cli.log(''); @@ -541,8 +541,8 @@ class DailyCommand extends CICommand { async aggregate() { const { argv, cli } = this; - const aggregator = new FailureAggregator(cli, this.json); - this.json = aggregator.aggregate(); + const aggregator = new FailureAggregator(cli, this.json, this.request); + this.json = await aggregator.aggregate(); cli.log(''); cli.separator('Stats'); cli.log(''); diff --git a/lib/ci/failure_aggregator.js b/lib/ci/failure_aggregator.js index 8c421915..512562cb 100644 --- a/lib/ci/failure_aggregator.js +++ b/lib/ci/failure_aggregator.js @@ -20,14 +20,47 @@ function uniqBy(array, key) { } export class FailureAggregator { - constructor(cli, data) { + constructor(cli, data, request) { this.cli = cli; + this.request = request; this.health = data[0]; this.failures = data.slice(1); this.aggregates = null; } - aggregate() { + /** + * Tells whether the pull request that triggered the run also modified the + * test that failed. Such a failure is likely caused by the change itself, + * so it should not count as an independent flaky occurrence. + */ + async isSelfInflicted(failure) { + const { file, source } = failure; + if (!file || !this.request) { + return false; + } + + const pr = parsePRFromURL(source); + if (!pr) { + return false; + } + + const path = `test/${file}.js`; + try { + for await (const changed of this.request.getPullRequestFiles(pr)) { + if (changed.filename === path) { + return true; + } + } + } catch { + // Not being able to fetch the changed files is not fatal: keep the + // occurrence rather than dropping it on incomplete information. + this.cli.warn(`Could not determine the files changed by ${source}`); + } + + return false; + } + + async aggregate() { const groupedByReason = Object.groupBy(this.failures, getHighlight); const data = []; for (const reason of Object.keys(groupedByReason).sort()) { @@ -37,7 +70,11 @@ 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 prs = uniqBy(failures, 'source') + const candidates = uniqBy(failures, 'source'); + const selfInflicted = await Promise.all( + candidates.map(failure => this.isSelfInflicted(failure))); + const prs = candidates + .filter((_, index) => !selfInflicted[index]) .map(({ source, upstream }) => ({ source, upstream, _id: parseJobFromURL(upstream).jobid })) .sort((a, b) => a._id - b._id); const machines = uniqBy( @@ -57,9 +94,9 @@ export class FailureAggregator { } formatAsMarkdown() { - let { aggregates } = this; + const { aggregates } = this; if (!aggregates) { - aggregates = this.aggregates = this.aggregate(); + throw new Error('aggregate() must be awaited before formatAsMarkdown()'); } const last = parseJobFromURL(this.failures[0].upstream); @@ -118,9 +155,9 @@ export class FailureAggregator { } display() { - let { cli, aggregates } = this; + const { cli, aggregates } = this; if (!aggregates) { - aggregates = this.aggregates = this.aggregate(); + throw new Error('aggregate() must be awaited before display()'); } for (const type of Object.keys(aggregates)) { diff --git a/test/unit/failure_aggregator.test.js b/test/unit/failure_aggregator.test.js new file mode 100644 index 00000000..12efcf0a --- /dev/null +++ b/test/unit/failure_aggregator.test.js @@ -0,0 +1,122 @@ +import assert from 'node:assert'; +import { describe, it } from 'node:test'; + +import { FailureAggregator } from '../../lib/ci/failure_aggregator.js'; + +const health = { type: 'health' }; + +/** + * Builds a JS test failure as produced by the CI parsers, where `file` is the + * test name reported by the runner and `source` is the pull request that + * triggered the run. + */ +function failure(prid, file, jobid) { + return { + type: 'JS_TEST_FAILURE', + reason: `not ok 1 ${file}\n ---\n severity: fail\n`, + highlight: 0, + file, + source: `https://github.com/nodejs/node/pull/${prid}/`, + upstream: `https://ci.nodejs.org/job/node-test-pull-request/${jobid}/`, + builtOn: `test-machine-${jobid}`, + url: `https://ci.nodejs.org/job/node-test-commit/${jobid}/console` + }; +} + +/** + * Stubs the parts of the request client the aggregator relies on. `changed` + * maps a pull request number to the files it modified. + */ +function requestStub(changed) { + return { + async * getPullRequestFiles({ prid }) { + for (const filename of changed[prid] ?? []) { + yield { filename }; + } + } + }; +} + +const cli = { warn() {} }; + +describe('FailureAggregator', () => { + it('should not count a failure in a test the pull request modified', async() => { + const request = requestStub({ + 65113: ['lib/fs.js'], + 65233: ['test/ffi/test-ffi-fast-buffer.js'] + }); + + const aggregator = new FailureAggregator(cli, [ + health, + failure(65113, 'ffi/test-ffi-fast-buffer', 75793), + failure(65233, 'ffi/test-ffi-fast-buffer', 75799) + ], request); + + const aggregates = await aggregator.aggregate(); + const [entry] = aggregates.JS_TEST_FAILURE; + + assert.strictEqual(entry.prs.length, 1); + assert.strictEqual( + entry.prs[0].source, + 'https://github.com/nodejs/node/pull/65113/' + ); + }); + + it('should keep failures in tests the pull request left alone', async() => { + const request = requestStub({ + 65113: ['lib/fs.js'], + 65233: ['src/ffi/fast.cc'] + }); + + const aggregator = new FailureAggregator(cli, [ + health, + failure(65113, 'ffi/test-ffi-fast-buffer', 75793), + failure(65233, 'ffi/test-ffi-fast-buffer', 75799) + ], request); + + const aggregates = await aggregator.aggregate(); + const [entry] = aggregates.JS_TEST_FAILURE; + + assert.strictEqual(entry.prs.length, 2); + }); + + it('should keep the occurrence when the changed files cannot be fetched', async() => { + const request = { + getPullRequestFiles() { + throw new Error('network is down'); + } + }; + + const aggregator = new FailureAggregator(cli, [ + health, + failure(65233, 'ffi/test-ffi-fast-buffer', 75799) + ], request); + + const aggregates = await aggregator.aggregate(); + const [entry] = aggregates.JS_TEST_FAILURE; + + assert.strictEqual(entry.prs.length, 1); + }); + + it('should leave failures without a test file untouched', async() => { + const request = requestStub({ 65233: ['test/ffi/test-ffi-fast-buffer.js'] }); + + const buildFailure = { + type: 'BUILD_FAILURE', + reason: 'fatal: could not read Username', + highlight: 0, + source: 'https://github.com/nodejs/node/pull/65233/', + upstream: 'https://ci.nodejs.org/job/node-test-pull-request/75799/', + builtOn: 'test-machine', + url: 'https://ci.nodejs.org/job/node-test-commit/75799/console' + }; + + const aggregator = new FailureAggregator( + cli, [health, buildFailure], request); + + const aggregates = await aggregator.aggregate(); + const [entry] = aggregates.BUILD_FAILURE; + + assert.strictEqual(entry.prs.length, 1); + }); +});