Skip to content

fix: stop blaming failing tests for a test step that reported none - #146

Closed
EliSchleifer wants to merge 1 commit into
mainfrom
eli/uploader-no-failures-message
Closed

EliSchleifer wants to merge 1 commit into
mainfrom
eli/uploader-no-failures-message

Conversation

@EliSchleifer

Copy link
Copy Markdown
Member

The problem

getFailureReason picks its annotation from previous-step-outcome alone, so
any non-zero exit from the step that ran the tests is reported as:

The test results you are uploading contain non quarantined test failures -- see above for details.

That is wrong whenever the test command fails without producing failures: no
test files matched its filter, a build or setup error, a crash before results
were written.

It matters more than the wording suggests, because the annotation is the only
part of a failure GitHub surfaces without opening the log — and in exactly those
runs the CLI has already printed the correct diagnosis two lines above it:

⚠️  No test failures found, but non zero exit code provided: 1
This may indicate that some tests were not run or that there were other issues during the test run.
##[error]The test results you are uploading contain non quarantined test failures -- see above for details.

So the CLI says "no failures, something else went wrong" and the annotation
immediately contradicts it. A reader who trusts the annotation goes hunting for
a failing test that does not exist.

Found while chasing a trunk2 merge-queue failure. The actual cause was a package
whose vitest run matched no test files — vitest exits 1 on "No test files
found" — so pnpm -r test failed a leg in which Fail: 0. The annotation sent
me looking for a broken test for a while before I read the raw log.

What this changes

The action knows the step's outcome and never why, so the message now states
only that, points at the log, and names the alternative rather than asserting
one cause:

The step that ran the tests failed -- see above for details. If no failing tests are listed, it failed without reporting any (for example: no test files matched, or it exited before writing results).

A cancelled step gets its own wording instead of being called a failure —
previousStepFailed already treats cancelled as failed, so that case was
also being reported as failing tests:

The step that ran the tests was cancelled -- the uploaded results are likely incomplete.

Rejected alternative

Having the CLI tell the action which case it is (an extra exit code or an
output) would let the annotation state the cause with certainty rather than
offering the reader two possibilities. That is the better end state, but it is a
two-repo change with a version-skew window, and it is not needed for the
annotation to stop being wrong. This PR is the half that can ship alone.

Testing

The three new assertions are on the exported message builder, not through
core.setFailed. jest.mock("@actions/core", …) does not intercept the ESM
import in this suite — the real @actions/core runs and the fixture records
nothing, which I confirmed by watching real ::error:: output during a run
whose mock.calls was empty. That is also why the existing tests in this file
only assert return values. Restructuring the suite onto
jest.unstable_mockModule would be the way to assert through the mock; it
seemed out of scope here, but happy to do it if you would rather.

pnpm test green (53 tests), and dist/ rebuilt with pnpm build since the
action runs the committed bundle.

`getFailureReason` chose its annotation purely from `previous-step-outcome`, so
any non-zero exit from the test step was reported as "The test results you are
uploading contain non quarantined test failures". That is wrong whenever the
command fails *without producing* failures — no test files matched its filter, a
build or setup error, a crash before results were written.

It matters because the annotation is the only part GitHub surfaces without
opening the log, and in exactly those runs the CLI has already printed the
correct diagnosis two lines above it:

    ⚠️  No test failures found, but non zero exit code provided: 1
    This may indicate that some tests were not run or that there were other
    issues during the test run.
    ##[error]The test results you are uploading contain non quarantined test failures

A reader trusting the annotation goes looking for a failing test that does not
exist. (Found chasing a trunk2 merge-queue failure that turned out to be a
package whose vitest run matched no test files.)

The action knows the step's outcome and never why, so the message now states
only that, points at the log, and names the alternative. A cancelled step gets
its own wording rather than being called a failure.

Tested on the message builder rather than through `core.setFailed`: `jest.mock`
does not intercept the ESM import in this suite, so the real `@actions/core`
runs and records nothing — which is why the existing tests only assert return
values.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@trunk-staging-io

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@trunk-io

trunk-io Bot commented Sep 15, 2026

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

dfrankland added a commit that referenced this pull request Oct 21, 2026
From #146. When the step that ran the tests failed, the annotation said
"The test results you are uploading contain non quarantined test
failures", which is wrong whenever the step failed without producing
any: no test files matched, a build or setup error, a crash before
results were written. The action knows the step's outcome and never
why, so the message now says only that, points at the log, and names
the alternative. A cancelled step gets its own wording rather than
being called a failure.

Exit code 70 keeps the generic message: that is the cli failing on its
own, not the test step.

Co-Authored-By: Eli Schleifer <eli@trunk.io>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@dfrankland

Copy link
Copy Markdown
Member

Thanks Eli. I've folded this into #152, with you as co-author on the commit, so I'm closing this one. The wording and the cancelled case are unchanged. Two changes around it:

  • Exit 70, where the CLI fails on its own (bad token, no results with allow-missing-junit-files: false), keeps the generic "A failure occurred while executing the command" message instead of blaming the test step. getFailureReason now reads the exit code from error.status; the old exit code 70 check never matched, because execSync doesn't put the code in the message.
  • errors.test.ts now uses jest.unstable_mockModule, which you suggested, so the tests check the message through setFailed.

Once the CLI's summary output (TRUNK-18881) lands, the hedge can become a certain "failed without running tests" message whenever zero tests were seen.

@dfrankland dfrankland closed this Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants