Skip to content

test(express): Error handler tests #23724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
isaacs merged 1 commit into from
Aug 28, 2026
Merged

test(express): Error handler tests #23724

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 171 additions & 1 deletion packages/server-utils/test/integrations/express-error-handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,28 @@
import * as SentryCore from '@sentry/core';
import { afterEach, beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest';
import { isExpressErrorHandled } from '../../src/integrations/express/error-handled';
// oxlint-disable-next-line typescript/no-deprecated
import { expressErrorHandler } from '../../src/integrations/express/error-handler';
import { captureLayerError } from '../../src/integrations/express/instrumentation';
import type { HandleChannelContext } from '../../src/integrations/express/types';
import type { ExpressRequest, ExpressResponse, HandleChannelContext } from '../../src/integrations/express/types';

function makeErrorData(error: unknown, span?: unknown): HandleChannelContext {
return { error, _sentrySpan: span } as unknown as HandleChannelContext;
}

/** Express hands every layer `[req, res, next]`. Sentry reads the request from there and marks it as handled. */
function makeLayerErrorData(error: unknown, request: ExpressRequest, span?: unknown): HandleChannelContext {
return { error, _sentrySpan: span, arguments: [request] } as unknown as HandleChannelContext;
}

function makeRequest(): ExpressRequest {
return {
method: 'GET',
originalUrl: '/users/42?include=profile',
headers: { host: 'api.example.com', 'user-agent': 'vitest' },
} as unknown as ExpressRequest;
}

describe('captureLayerError', () => {
let captureExceptionSpy: MockInstance;

Expand Down Expand Up @@ -110,4 +126,158 @@ describe('captureLayerError', () => {
expect(withActiveSpanSpy).not.toHaveBeenCalled();
expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
});

describe('per-request dedup marker', () => {
it('captures once when the same error bubbles through several layers', () => {
const request = makeRequest();
const error = Object.assign(new Error('boom'), { statusCode: 500 });

captureLayerError(makeLayerErrorData(error, request), undefined);
captureLayerError(makeLayerErrorData(error, request), undefined);
captureLayerError(makeLayerErrorData(error, request), undefined);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
});

it('marks the request when the error is skipped, so the deprecated middleware defers', () => {
const request = makeRequest();
const error = Object.assign(new Error('bad request'), { statusCode: 400 });

captureLayerError(makeLayerErrorData(error, request), undefined);

expect(captureExceptionSpy).not.toHaveBeenCalled();
expect(isExpressErrorHandled(request)).toBe(true);
});

it('marks the request when shouldHandleError is false', () => {
const request = makeRequest();
const error = Object.assign(new Error('boom'), { statusCode: 500 });

captureLayerError(makeLayerErrorData(error, request), false);

expect(captureExceptionSpy).not.toHaveBeenCalled();
expect(isExpressErrorHandled(request)).toBe(true);
});

it('leaves the request unmarked when there is no error', () => {
const request = makeRequest();

captureLayerError(makeLayerErrorData(undefined, request), undefined);

expect(isExpressErrorHandled(request)).toBe(false);
});

// No request means nothing to mark, so every layer captures again. Express always passes one, so this cannot happen in practice.
it('captures on every layer when Express passes no request', () => {
const error = Object.assign(new Error('boom'), { statusCode: 500 });

captureLayerError(makeErrorData(error), undefined);
captureLayerError(makeErrorData(error), undefined);

expect(captureExceptionSpy).toHaveBeenCalledTimes(2);
});

// TODO: Sentry marks the request, not the error, so only the first error per request is captured. Later errors on the same request are lost.
it.fails('captures a second, distinct error raised on the same request', () => {
const request = makeRequest();
const firstError = Object.assign(new Error('first failure'), { statusCode: 400 });
const secondError = Object.assign(new Error('second failure'), { statusCode: 500 });

captureLayerError(makeLayerErrorData(firstError, request), undefined);
captureLayerError(makeLayerErrorData(secondError, request), undefined);

expect(captureExceptionSpy).toHaveBeenCalledWith(secondError, {
mechanism: { type: 'auto.http.express', handled: false },
});
});
});
});

describe('expressErrorHandler', () => {
let captureExceptionSpy: MockInstance;

beforeEach(() => {
captureExceptionSpy = vi.spyOn(SentryCore, 'captureException').mockImplementation(() => 'event-id');
});

afterEach(() => {
vi.restoreAllMocks();
});

function makeResponse(): ExpressResponse & { sentry?: string } {
return { once: () => undefined, removeListener: () => undefined } as unknown as ExpressResponse & {
sentry?: string;
};
}

// A request whose error the integration already captured, before this middleware runs.
function makeHandledRequest(): ExpressRequest {
const request = makeRequest();
captureLayerError(makeLayerErrorData(new Error('captured by the integration'), request), undefined);
return request;
}

it('captures a 5xx error and exposes the event id on the response', () => {
const res = makeResponse();
const error = Object.assign(new Error('boom'), { statusCode: 500 });

expressErrorHandler()(error, makeRequest(), res, vi.fn());

expect(captureExceptionSpy).toHaveBeenCalledWith(error, {
mechanism: { type: 'auto.middleware.express', handled: false },
});
expect(res.sentry).toBe('event-id');
});

it('does not capture a 4xx error', () => {
const res = makeResponse();
const error = Object.assign(new Error('bad request'), { statusCode: 400 });

expressErrorHandler()(error, makeRequest(), res, vi.fn());

expect(captureExceptionSpy).not.toHaveBeenCalled();
expect(res.sentry).toBeUndefined();
});

it.each([
['captured', 500],
['skipped', 400],
])('forwards the error to next when %s', (_case, statusCode) => {
const next = vi.fn();
const error = Object.assign(new Error('boom'), { statusCode });

expressErrorHandler()(error, makeRequest(), makeResponse(), next);

expect(next).toHaveBeenCalledExactlyOnceWith(error);
});

it('defers to the integration once the request is marked', () => {
const request = makeHandledRequest();
captureExceptionSpy.mockClear();
const error = Object.assign(new Error('boom'), { statusCode: 500 });

expressErrorHandler()(error, request, makeResponse(), vi.fn());

expect(captureExceptionSpy).not.toHaveBeenCalled();
});
Comment on lines +254 to +262

@sentry sentry Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: A request is marked as handled before checking if an error should be captured, causing subsequent, distinct errors on the same request to be silently dropped.
Severity: MEDIUM

Suggested Fix

Move the markExpressErrorHandled(request) call to after the shouldHandleError check. This ensures a request is only marked as handled if the error is actually captured and processed.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
packages/server-utils/test/integrations/express-error-handler.test.ts#L254-L262

Potential issue: The request is marked as handled by calling
`markExpressErrorHandled(request)` before the `shouldHandleError` check is performed. If
an initial error is skipped (e.g., a 400 error that is configured to be ignored), the
request is still marked. Consequently, if a second, distinct error occurs on the same
request (e.g., a 500 error), the `isExpressErrorHandled(request)` check will return
`true`, causing this subsequent error to be silently dropped instead of being captured.

Did we get this right? 👍 / 👎 to inform future reviews.


it('forwards the error to next even when it defers', () => {
const next = vi.fn();
const error = Object.assign(new Error('boom'), { statusCode: 500 });

expressErrorHandler()(error, makeHandledRequest(), makeResponse(), next);

expect(next).toHaveBeenCalledExactlyOnceWith(error);
});

// TODO: `res.sentry` carries the captured event id, but only this middleware sets it.
// Once the integration captures first, apps reading `res.sentry` get undefined instead of the id.
it.fails('exposes the event id on the response when the integration captured the error', () => {
const res = makeResponse();
const error = Object.assign(new Error('boom'), { statusCode: 500 });

expressErrorHandler()(error, makeHandledRequest(), res, vi.fn());

expect(res.sentry).toBe('event-id');
});
Comment on lines +273 to +282

@sentry sentry Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The res.sentry property is not set on the response when an error is captured before the expressErrorHandler middleware runs, as the handler exits early.
Severity: LOW

Suggested Fix

Ensure that res.sentry is set on the response object whenever an error is captured by the Express integration, regardless of whether it's captured in the main error handler or another part of the instrumentation.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
packages/server-utils/test/integrations/express-error-handler.test.ts#L273-L282

Potential issue: When an error is captured by the integration before the main
`expressErrorHandler` middleware runs, the request is marked as handled. The
`expressErrorHandler` then sees this mark via `isExpressErrorHandled(request)` and exits
early by calling `next(error)`. As a result, the code that sets the Sentry event ID on
the response, `(res as { sentry?: string }).sentry = eventId;`, is never reached. This
leaves `res.sentry` as `undefined` for any downstream middleware that might rely on it.

Did we get this right? 👍 / 👎 to inform future reviews.

});
Loading