Conversation
A failure in a test that the pull request itself modified is likely caused by that change, so counting it as an independent flaky occurrence overstates the number of affected pull requests. Compare the failing test against the files the pull request touched and drop the occurrence when they match. When the file list cannot be fetched the occurrence is kept, so incomplete information never removes a genuine failure. Fixes: nodejs#1164 Signed-off-by: Avocado <ujubongbong@gmail.com>
00626ea to
1a93536
Compare
| const selfInflicted = await Promise.all( | ||
| candidates.map(failure => this.isSelfInflicted(failure))); | ||
| const prs = candidates | ||
| .filter((_, index) => !selfInflicted[index]) |
There was a problem hiding this comment.
This can leave prs empty, but display() still reads prs[prs.length - 1].upstream.
| return false; | ||
| } | ||
|
|
||
| const path = `test/${file}.js`; |
There was a problem hiding this comment.
Node's test runner strips both .js and .mjs extensions from the reported name. For example, test/parallel/test-cli-print-promise.mjs is reported as parallel/test-cli-print-promise, so this always looks for the wrong file and keeps the occurrence in the flaky count.
|
|
||
| const path = `test/${file}.js`; | ||
| try { | ||
| for await (const changed of this.request.getPullRequestFiles(pr)) { |
There was a problem hiding this comment.
This fetches the current PR's changed files, which may differ from the revision CI tested. If a contributor pushes a fix to the failing test after the run, the earlier failure gets excluded even though the tested revision never touched that file. That can reduce the count from two PRs to one and remove the reliability entry. Compare against the tested revision, or keep the occurrence when we cannot verify that the current PR head matches it.
2 participants
What
A failure in a test that the pull request itself modified is counted as an independent flaky occurrence, which overstates the number of affected pull requests.
Taking the example from the issue,
ffi/test-ffi-fast-bufferis listed with two failed PRs in the 2026-08-15 report:src/ffi/fast.cc,test/ffi/test-ffi-fast-buffer.jslib/fs.js, unrelated fs testsOnly #65113 is a genuine occurrence.
How
Compare the failing test against the files the pull request touched, and drop the occurrence when they match.
formatAsMarkdownalready skips entries with fewer than two PRs, so such an entry leaves the report on its own.When the file list cannot be fetched the occurrence is kept, following
lib/benchmark.js: incomplete information should never remove a genuine failure.This makes
aggregate()async. Both call sites inbin/ncu-ci.jswere already async methods. The lazy fallbacks insideformatAsMarkdown()anddisplay()now throw instead, since a pending promise there would fail silently.Tests
Four tests cover the exclusion, the untouched case, the fetch failure, and non-test failures. Removing the exclusion makes only the first fail, while the other three keep passing.
Fixes: #1164