Skip to content

Commit 9e2bd51

Browse files
lforstlforst
authored
ref(nextjs): Fix tests (#14058)
Adds finishing touches to the target branch, unskipping tests ensuring we are not breaking anything.
1 parent dd57025 commit 9e2bd51

File tree

9 files changed

+91
-54
lines changed

9 files changed

+91
-54
lines changed

‎dev-packages/e2e-tests/test-applications/nextjs-13/instrumentation.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ export function register() {
1212
// We are doing a lot of events at once in this test app
1313
bufferSize: 1000,
1414
},
15-
debug: false,
1615
});
1716
}
1817
}

‎dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts

Lines changed: 8 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
import { expect, test } from '@playwright/test';
22
import { waitForTransaction } from '@sentry-internal/test-utils';
33

4-
// TODO(lforst): This cannot make it into production - Make sure to fix this test
5-
// The problem is that if we are not applying build time instrumentation Next.js will still emit spans (which is fine, but we need to find a different way of testing that build time instrumentation is successfully disabled - maybe with an attribute or something if build-time instrumentation is applied)
6-
test.skip('should not automatically create transactions for routes that were excluded from auto wrapping (string)', async ({
4+
test('should not apply build-time instrumentation for routes that were excluded from auto wrapping (string)', async ({
75
request,
86
}) => {
97
const transactionPromise = waitForTransaction('nextjs-13', async transactionEvent => {
@@ -15,19 +13,13 @@ test.skip('should not automatically create transactions for routes that were exc
1513

1614
expect(await (await request.get(`/api/endpoint-excluded-with-string`)).text()).toBe('{"success":true}');
1715

18-
let transactionPromiseReceived = false;
19-
transactionPromise.then(() => {
20-
transactionPromiseReceived = true;
21-
});
22-
23-
await new Promise(resolve => setTimeout(resolve, 5_000));
16+
const transaction = await transactionPromise;
2417

25-
expect(transactionPromiseReceived).toBe(false);
18+
expect(transaction.contexts?.trace?.data?.['sentry.origin']).toBeDefined();
19+
expect(transaction.contexts?.trace?.data?.['sentry.origin']).not.toBe('auto.http.nextjs'); // This is the origin set by the build time instrumentation
2620
});
2721

28-
// TODO(lforst): This cannot make it into production - Make sure to fix this test
29-
// The problem is that if we are not applying build time instrumentation Next.js will still emit spans (which is fine, but we need to find a different way of testing that build time instrumentation is successfully disabled - maybe with an attribute or something if build-time instrumentation is applied)
30-
test.skip('should not automatically create transactions for routes that were excluded from auto wrapping (regex)', async ({
22+
test('should not apply build-time instrumentation for routes that were excluded from auto wrapping (regex)', async ({
3123
request,
3224
}) => {
3325
const transactionPromise = waitForTransaction('nextjs-13', async transactionEvent => {
@@ -39,12 +31,8 @@ test.skip('should not automatically create transactions for routes that were exc
3931

4032
expect(await (await request.get(`/api/endpoint-excluded-with-regex`)).text()).toBe('{"success":true}');
4133

42-
let transactionPromiseReceived = false;
43-
transactionPromise.then(() => {
44-
transactionPromiseReceived = true;
45-
});
46-
47-
await new Promise(resolve => setTimeout(resolve, 5_000));
34+
const transaction = await transactionPromise;
4835

49-
expect(transactionPromiseReceived).toBe(false);
36+
expect(transaction.contexts?.trace?.data?.['sentry.origin']).toBeDefined();
37+
expect(transaction.contexts?.trace?.data?.['sentry.origin']).not.toBe('auto.http.nextjs'); // This is the origin set by the build time instrumentation
5038
});

‎dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/getServerSideProps.test.ts

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,11 @@ test('Should report an error event for errors thrown in getServerSideProps', asy
6060
data: {
6161
'http.response.status_code': 500,
6262
'sentry.op': 'http.server',
63-
'sentry.origin': expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/),
63+
'sentry.origin': 'auto',
6464
'sentry.source': 'route',
6565
},
6666
op: 'http.server',
67-
origin: expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/),
67+
origin: 'auto',
6868
span_id: expect.any(String),
6969
status: 'internal_error',
7070
trace_id: expect.any(String),
@@ -81,8 +81,7 @@ test('Should report an error event for errors thrown in getServerSideProps', asy
8181
start_timestamp: expect.any(Number),
8282
timestamp: expect.any(Number),
8383
transaction: 'GET /[param]/error-getServerSideProps',
84-
// TODO: This test fails depending on the next version (next 13: 'custom', next >14: 'route')
85-
// transaction_info: { source: 'custom' },
84+
transaction_info: { source: 'route' },
8685
type: 'transaction',
8786
});
8887
});
@@ -148,11 +147,11 @@ test('Should report an error event for errors thrown in getServerSideProps in pa
148147
data: {
149148
'http.response.status_code': 500,
150149
'sentry.op': 'http.server',
151-
'sentry.origin': expect.stringMatching(/^auto(\.http\.otel\.http)?$/),
150+
'sentry.origin': 'auto',
152151
'sentry.source': 'route',
153152
},
154153
op: 'http.server',
155-
origin: expect.stringMatching(/^auto(\.http\.otel\.http)?$/),
154+
origin: 'auto',
156155
span_id: expect.any(String),
157156
status: 'internal_error',
158157
trace_id: expect.any(String),
@@ -169,8 +168,7 @@ test('Should report an error event for errors thrown in getServerSideProps in pa
169168
start_timestamp: expect.any(Number),
170169
timestamp: expect.any(Number),
171170
transaction: 'GET /[param]/customPageExtension',
172-
// TODO: This test fails depending on the next version (next 13: 'custom', next >14: 'route')
173-
// transaction_info: { source: 'custom' },
171+
transaction_info: { source: 'route' },
174172
type: 'transaction',
175173
});
176174
});

‎dev-packages/e2e-tests/test-applications/nextjs-app-dir/next-env.d.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@
33
/// <reference types="next/navigation-types/compat/navigation" />
44

55
// NOTE: This file should not be edited
6-
// see https://nextjs.org/docs/app/building-your-application/configuring/typescript for more information.
6+
// see https://nextjs.org/docs/basic-features/typescript for more information.

‎dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,17 @@ test('Should create a transaction for edge routes', async ({ request }) => {
2525

2626
test('Faulty edge routes', async ({ request }) => {
2727
const edgerouteTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
28-
return transactionEvent?.transaction === 'GET /api/error-edge-endpoint';
28+
return (
29+
transactionEvent?.transaction === 'GET /api/error-edge-endpoint' &&
30+
transactionEvent.contexts?.runtime?.name === 'vercel-edge'
31+
);
2932
});
3033

3134
const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => {
32-
return errorEvent?.exception?.values?.[0]?.value === 'Edge Route Error';
35+
return (
36+
errorEvent?.exception?.values?.[0]?.value === 'Edge Route Error' &&
37+
errorEvent.contexts?.runtime?.name === 'vercel-edge'
38+
);
3339
});
3440

3541
request.get('/api/error-edge-endpoint').catch(() => {
@@ -44,7 +50,6 @@ test('Faulty edge routes', async ({ request }) => {
4450
test.step('should create transactions with the right fields', () => {
4551
expect(edgerouteTransaction.contexts?.trace?.status).toBe('unknown_error');
4652
expect(edgerouteTransaction.contexts?.trace?.op).toBe('http.server');
47-
expect(edgerouteTransaction.contexts?.runtime?.name).toBe('vercel-edge');
4853
});
4954

5055
test.step('should have scope isolation', () => {

‎dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge.test.ts

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import { expect, test } from '@playwright/test';
22
import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';
33

4+
const packageJson = require('../package.json');
5+
46
test('Should record exceptions for faulty edge server components', async ({ page }) => {
57
const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => {
68
return errorEvent?.exception?.values?.[0]?.value === 'Edge Server Component Error';
@@ -19,8 +21,10 @@ test('Should record exceptions for faulty edge server components', async ({ page
1921
expect(errorEvent.transaction).toBe(`Page Server Component (/edge-server-components/error)`);
2022
});
2123

22-
// TODO(lforst): Fix this test
23-
test.skip('Should record transaction for edge server components', async ({ page }) => {
24+
test('Should record transaction for edge server components', async ({ page }) => {
25+
const nextjsVersion = packageJson.dependencies.next;
26+
const nextjsMajor = Number(nextjsVersion.split('.')[0]);
27+
2428
const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
2529
return (
2630
transactionEvent?.transaction === 'GET /edge-server-components' &&
@@ -33,9 +37,14 @@ test.skip('Should record transaction for edge server components', async ({ page
3337
const serverComponentTransaction = await serverComponentTransactionPromise;
3438

3539
expect(serverComponentTransaction).toBeDefined();
36-
expect(serverComponentTransaction.request?.headers).toBeDefined();
40+
expect(serverComponentTransaction.contexts?.trace?.op).toBe('http.server');
3741

38-
// Assert that isolation scope works properly
39-
expect(serverComponentTransaction.tags?.['my-isolated-tag']).toBe(true);
40-
expect(serverComponentTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
42+
// For some reason headers aren't picked up on Next.js 13 - also causing scope isolation to be broken
43+
if (nextjsMajor >= 14) {
44+
expect(serverComponentTransaction.request?.headers).toBeDefined();
45+
46+
// Assert that isolation scope works properly
47+
expect(serverComponentTransaction.tags?.['my-isolated-tag']).toBe(true);
48+
expect(serverComponentTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
49+
}
4150
});

‎dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -63,11 +63,13 @@ test('Should record exceptions and transactions for faulty route handlers', asyn
6363
expect(routehandlerError.transaction).toBe('PUT /route-handlers/[param]/error');
6464
});
6565

66-
// TODO(lforst): This cannot make it into production - Make sure to fix this test
67-
test.describe.skip('Edge runtime', () => {
66+
test.describe('Edge runtime', () => {
6867
test('should create a transaction for route handlers', async ({ request }) => {
6968
const routehandlerTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
70-
return transactionEvent?.transaction === 'PATCH /route-handlers/[param]/edge';
69+
return (
70+
transactionEvent?.transaction === 'PATCH /route-handlers/[param]/edge' &&
71+
transactionEvent.contexts?.runtime?.name === 'vercel-edge'
72+
);
7173
});
7274

7375
const response = await request.patch('/route-handlers/bar/edge');
@@ -81,11 +83,17 @@ test.describe.skip('Edge runtime', () => {
8183

8284
test('should record exceptions and transactions for faulty route handlers', async ({ request }) => {
8385
const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => {
84-
return errorEvent?.exception?.values?.[0]?.value === 'route-handler-edge-error';
86+
return (
87+
errorEvent?.exception?.values?.[0]?.value === 'route-handler-edge-error' &&
88+
errorEvent.contexts?.runtime?.name === 'vercel-edge'
89+
);
8590
});
8691

8792
const routehandlerTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
88-
return transactionEvent?.transaction === 'DELETE /route-handlers/[param]/edge';
93+
return (
94+
transactionEvent?.transaction === 'DELETE /route-handlers/[param]/edge' &&
95+
transactionEvent.contexts?.runtime?.name === 'vercel-edge'
96+
);
8997
});
9098

9199
await request.delete('/route-handlers/baz/edge').catch(() => {
@@ -101,12 +109,10 @@ test.describe.skip('Edge runtime', () => {
101109
expect(routehandlerError.tags?.['my-isolated-tag']).toBe(true);
102110
expect(routehandlerError.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
103111

104-
expect(routehandlerTransaction.contexts?.trace?.status).toBe('internal_error');
112+
expect(routehandlerTransaction.contexts?.trace?.status).toBe('unknown_error');
105113
expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server');
106-
expect(routehandlerTransaction.contexts?.runtime?.name).toBe('vercel-edge');
107114

108115
expect(routehandlerError.exception?.values?.[0].value).toBe('route-handler-edge-error');
109-
expect(routehandlerError.contexts?.runtime?.name).toBe('vercel-edge');
110116

111117
expect(routehandlerError.transaction).toBe('DELETE /route-handlers/[param]/edge');
112118
});

‎dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ test('Sends a transaction for a request to app router', async ({ page }) => {
1616
expect(transactionEvent.contexts?.trace).toEqual({
1717
data: expect.objectContaining({
1818
'sentry.op': 'http.server',
19-
'sentry.origin': expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/),
19+
'sentry.origin': 'auto',
2020
'sentry.sample_rate': 1,
2121
'sentry.source': 'route',
2222
'http.method': 'GET',
@@ -27,7 +27,7 @@ test('Sends a transaction for a request to app router', async ({ page }) => {
2727
'otel.kind': 'SERVER',
2828
}),
2929
op: 'http.server',
30-
origin: expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/),
30+
origin: 'auto',
3131
span_id: expect.any(String),
3232
status: 'ok',
3333
trace_id: expect.any(String),

‎packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
import {
2+
SEMANTIC_ATTRIBUTE_SENTRY_OP,
3+
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
24
Scope,
35
captureException,
46
getActiveSpan,
57
getCapturedScopesOnSpan,
68
getRootSpan,
79
handleCallbackErrors,
810
setCapturedScopesOnSpan,
11+
setHttpStatus,
912
withIsolationScope,
1013
withScope,
1114
} from '@sentry/core';
@@ -14,7 +17,7 @@ import type { RouteHandlerContext } from './types';
1417

1518
import { propagationContextFromHeaders, winterCGHeadersToDict } from '@sentry/utils';
1619

17-
import { isRedirectNavigationError } from './nextNavigationErrorUtils';
20+
import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils';
1821
import { commonObjectToIsolationScope, commonObjectToPropagationContext } from './utils/tracingUtils';
1922

2023
/**
@@ -49,22 +52,36 @@ export function wrapRouteHandlerWithSentry<F extends (...args: any[]) => any>(
4952
const propagationContext = commonObjectToPropagationContext(headers, incomingPropagationContext);
5053

5154
const activeSpan = getActiveSpan();
52-
if (activeSpan) {
53-
const rootSpan = getRootSpan(activeSpan);
55+
const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined;
56+
if (rootSpan) {
5457
const { scope } = getCapturedScopesOnSpan(rootSpan);
5558
setCapturedScopesOnSpan(rootSpan, scope ?? new Scope(), isolationScope);
59+
60+
if (process.env.NEXT_RUNTIME === 'edge') {
61+
rootSpan.updateName(`${method} ${parameterizedRoute}`);
62+
rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
63+
rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'http.server');
64+
}
5665
}
5766

5867
return withIsolationScope(isolationScope, () => {
59-
return withScope(scope => {
68+
return withScope(async scope => {
6069
scope.setTransactionName(`${method} ${parameterizedRoute}`);
6170
scope.setPropagationContext(propagationContext);
62-
return handleCallbackErrors(
71+
72+
const response: Response = await handleCallbackErrors(
6373
() => originalFunction.apply(thisArg, args),
6474
error => {
6575
// Next.js throws errors when calling `redirect()`. We don't wanna report these.
6676
if (isRedirectNavigationError(error)) {
6777
// Don't do anything
78+
} else if (isNotFoundNavigationError(error)) {
79+
if (activeSpan) {
80+
setHttpStatus(activeSpan, 404);
81+
}
82+
if (rootSpan) {
83+
setHttpStatus(rootSpan, 404);
84+
}
6885
} else {
6986
captureException(error, {
7087
mechanism: {
@@ -74,6 +91,21 @@ export function wrapRouteHandlerWithSentry<F extends (...args: any[]) => any>(
7491
}
7592
},
7693
);
94+
95+
try {
96+
if (response.status) {
97+
if (activeSpan) {
98+
setHttpStatus(activeSpan, response.status);
99+
}
100+
if (rootSpan) {
101+
setHttpStatus(rootSpan, response.status);
102+
}
103+
}
104+
} catch {
105+
// best effort - response may be undefined?
106+
}
107+
108+
return response;
77109
});
78110
});
79111
},

0 commit comments

Comments
 (0)