fix: exclude PR-modified tests from the flaky count

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('');

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)) {

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);
+  });
+});