Skip to content

test: expect(fn).toThrow(Class) passes when fn RETURNS an error — zero instances today, and nothing stops the next one #150

Closed

Description

@sebyx07

The hazard

In Bun 1.3.14, a function that returns an error satisfies .toThrow(). Verified directly:

expect(() => new TypeError('x')).toThrow(TypeError);  // PASSES — nothing threw
expect(() => ({ a: 1 })).toThrow();                   // correctly fails

So the matcher does not distinguish "threw an Error" from "returned an Error". Any test of the shape expect(() => f()).toThrow(SomeError) where f returns rather than throws is vacuously green.

Why this repo is unusually exposed to it

packages/*/src/errors.ts is full of error factories that return — loginFailed(), poolMaxInvalid(), rollbackStepsInvalid(), driverUnknown(), mfaRequiredUnenforceable(), and so on. The house pattern is throw someError(...). Writing expect(() => someError(x)).toThrow(...) — dropping the throw — is a natural slip, reads correctly, and passes.

That is exactly the "a test that cannot fail is not a test" rule the conventions already state, in a form no reviewer will spot: the test is green and the assertion looks right.

Current exposure: none

Swept all 731 .toThrow call sites across packages/ and scripts/. Exactly one matches the risky shape, and it is expect(() => reportError(hostile, …)).not.toThrow() (packages/core/src/error-reporter.test.ts:171) — a negated assertion on a void function, unaffected.

So this is a latent trap, not a live defect. Filing it because the sweep that found it was looking at something else, and the next person to write an error-factory test will not know.

How it was found

An agent on PR #148 wrote a test in this shape, noticed it passed against code that could not have satisfied it, and rewrote it with an explicit try/catch plus an identity assertion (packages/action/src/json-schema.test.ts). It self-reported the near-miss rather than banking the green.

Fix direction

Enforcement beats documentation (axiom 3). Options, cheapest first:

  1. A Biome lint rule, or a scripts/ check in the errors verify step, refusing expect(() => <ident>(…)).toThrow where the callee resolves to a known error factory (the errors.ts export sets are already enumerable — collectDeclaredCodes() walks them).
  2. A repo convention that error-throw tests use try/catch + isUltimateError(caught) + a code assertion, which is what the strongest existing tests already do, plus a check that .toThrow( never appears with a bare factory call.

Option 1 is a real gate step; option 2 alone is a convention, and a convention that is not a build error does not exist.

Found by the multi-agent bug sweep, 2026-08-18.

Activity

  1. developerz-ai commented on Aug 19, 2026

    @developerz-ai
    Contributor

    I couldn't reliably classify or scope this report right now, so I haven't guessed or handed it off. A maintainer will review it.

    🤖 Posted by developerz.ai — the maintainer agent, not a human.

  2. sebyx07 commented on Aug 20, 2026

    @sebyx07
    ContributorAuthor

    Fixed in #219 — scripts/to-throw-returns.ts.

    Premise confirmed by executing it, not assuming it: expect(() => new Boom('x')).toThrow(Boom) passes. So do the bare and string-matcher forms. Returning a non-Error is correctly reported.

    The check is itself a test, so if bun ever fixes toThrow, the rule goes red and can be deleted.

    rejects.toThrow is NOT vulnerable — a resolved promise is caught — so the rule excludes it rather than flagging correct assertions.

    Census: 370 expect(() => …).toThrow* sites and 196 exported functions declared to return an error (sendFailed, routeNotFound, fixtureUnknown, …). That is the live surface, and nothing guarded it. Zero are broken today — which is what makes this preventive rather than a cleanup, exactly as the issue asked.

    Reports only what is certain: the callback's body constructs an error, or calls one of those 196 (derived from source, never listed). Does not catch expect(returnsAnError).toThrow() — a named reference needs type information — and that hole is pinned by a test so it cannot be assumed away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions