Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,17 @@
"dev": "next dev",
"build": "next build > .tmp_build_stdout 2> .tmp_build_stderr || (cat .tmp_build_stdout && cat .tmp_build_stderr && exit 1)",
"clean": "npx rimraf node_modules pnpm-lock.yaml .tmp_dev_server_logs",
"dev:webpack": "next dev --webpack",
"build-webpack": "next build --webpack > .tmp_build_stdout 2> .tmp_build_stderr || (cat .tmp_build_stdout && cat .tmp_build_stderr && exit 1)",
"start": "next start",
"lint": "eslint",
"test:prod": "TEST_ENV=production playwright test",
"test:dev": "TEST_ENV=development playwright test",
"test:dev-webpack": "TEST_ENV=development-webpack playwright test",
"test:build": "pnpm install && pnpm build",
"test:assert": "pnpm test:prod && pnpm test:dev"
"test:build-webpack": "pnpm install && pnpm build-webpack",
"test:assert": "pnpm test:prod && pnpm test:dev",
"test:assert-webpack": "pnpm test:prod && pnpm test:dev-webpack"
},
"dependencies": {
"@sentry/nextjs": "file:../../packed/sentry-nextjs-packed.tgz",
Expand All @@ -37,5 +42,14 @@
},
"volta": {
"extends": "../../package.json"
},
"sentryTest": {
"variants": [
{
"build-command": "pnpm test:build-webpack",
"label": "nextjs-16-streaming (webpack)",
"assert-command": "pnpm test:assert-webpack"
}
]
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
import { expect, test } from '@playwright/test';
import { waitForStreamedSpan, waitForStreamedSpans, getSpanOp } from '@sentry-internal/test-utils';
import { collectStreamedSpans, waitForStreamedSpan, getSpanOp } from '@sentry-internal/test-utils';
import { isDevMode } from './isDevMode';

// Streamed spans are flushed across multiple envelopes as they end, so the server-component child spans
// can arrive in a different (earlier) envelope than the `is_segment` root span. Accumulate spans across
// envelopes until the root span (which ends last) is seen.
function collectSpanNamesUntilSegment(segmentName: string): Promise<string[]> {
return collectStreamedSpans('nextjs-16-streaming', spans =>
spans.some(span => span.name === segmentName && span.is_segment),
).then(spans => spans.map(span => span.name));
}

test('Sends a streamed span for a request to app router with URL', async ({ page }) => {
test.skip(isDevMode, 'Turbopack intermittently returns 404 for nested dynamic routes in dev mode');

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 isDevMode check is too broad, causing tests to be skipped for the new development-webpack environment, which prevents the intended test coverage from being added.
Severity: MEDIUM

Suggested Fix

Create a more specific isTurbopackDevMode constant in nextjs-16-streaming/utils/isDevMode.ts that uses strict equality: process.env.TEST_ENV === 'development'. Update the test.skip() calls in server-components.test.ts to use this new constant. This ensures tests are only skipped for Turbopack and not for the new webpack dev environment.

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:
dev-packages/e2e-tests/test-applications/nextjs-16-streaming/tests/server-components.test.ts#L15

Potential issue: The `isDevMode` constant in `nextjs-16-streaming` is defined using
`process.env.TEST_ENV.includes('development')`. The pull request introduces a new test
environment, `development-webpack`, to add test coverage for webpack. However, because
`'development-webpack'` includes the substring `'development'`, the `isDevMode` check
evaluates to `true`. This causes all server-component tests that use
`test.skip(isDevMode, ...)` to be skipped when running in the new webpack dev mode. As a
result, the intended webpack test coverage is not added, leaving these components
untested in that environment.

Also affects:

  • dev-packages/e2e-tests/test-applications/nextjs-16-streaming/tests/server-components.test.ts:32
  • dev-packages/e2e-tests/test-applications/nextjs-16-streaming/tests/server-components.test.ts:53

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


Expand All @@ -22,14 +31,11 @@ test('Will create streamed spans for every server component and metadata generat
}) => {
test.skip(isDevMode, 'Turbopack intermittently returns 404 for nested dynamic routes in dev mode');

const spansPromise = waitForStreamedSpans('nextjs-16-streaming', spans => {
return spans.some(span => span.name === 'GET /nested-layout' && span.is_segment);
});
const spanNamesPromise = collectSpanNamesUntilSegment('GET /nested-layout');

await page.goto('/nested-layout');

const spans = await spansPromise;
const spanNames = spans.map(span => span.name);
const spanNames = await spanNamesPromise;

expect(spanNames).toContainEqual('render route (app) /nested-layout');
expect(spanNames).toContainEqual('build component tree');
Expand All @@ -46,14 +52,11 @@ test('Will create streamed spans for every server component and metadata generat
}) => {
test.skip(isDevMode, 'Turbopack intermittently returns 404 for nested dynamic routes in dev mode');

const spansPromise = waitForStreamedSpans('nextjs-16-streaming', spans => {
return spans.some(span => span.name === 'GET /nested-layout/[dynamic]' && span.is_segment);
});
const spanNamesPromise = collectSpanNamesUntilSegment('GET /nested-layout/[dynamic]');

await page.goto('/nested-layout/123');

const spans = await spansPromise;
const spanNames = spans.map(span => span.name);
const spanNames = await spanNamesPromise;

expect(spanNames).toContainEqual('resolve page components');
expect(spanNames).toContainEqual('render route (app) /nested-layout/[dynamic]');
Expand Down
31 changes: 31 additions & 0 deletions dev-packages/test-utils/src/event-proxy-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,37 @@ export function waitForStreamedSpans(
});
}

/**
* Accumulate streamed Span V2 spans across multiple envelopes until `isDone` returns true.
*
* Unlike {@link waitForStreamedSpans}, which resolves with the spans of a single envelope, this
* collects spans from every Span V2 envelope as they arrive and resolves with the full set once
* `isDone` is satisfied. Streamed spans are flushed in multiple envelopes as they end (a child span
* can be sent before its root segment span), so any assertion that needs the whole trace must
* accumulate rather than snapshot a single envelope.
*
* `isDone` receives all spans collected so far. A common predicate is "the segment/root span has
* arrived", since the root ends last and therefore flushes after its children:
*
* @example
* ```ts
* const spans = await collectStreamedSpans(PROXY_SERVER_NAME, allSpans =>
* allSpans.some(span => span.name === 'GET /nested-layout' && span.is_segment),
* );
* expect(spans.map(span => span.name)).toContainEqual('build component tree');
* ```
*/
export function collectStreamedSpans(
proxyServerName: string,
isDone: (spans: SerializedStreamedSpan[]) => boolean,
): Promise<SerializedStreamedSpan[]> {
const collected: SerializedStreamedSpan[] = [];
return waitForStreamedSpans(proxyServerName, spans => {
collected.push(...spans);
return isDone(collected);
}).then(() => collected);
}

/**
* Helper to get the span operation from a Span V2 JSON object.
*
Expand Down
1 change: 1 addition & 0 deletions dev-packages/test-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export {
waitForStreamedSpan,
waitForStreamedSpans,
waitForStreamedSpanEnvelope,
collectStreamedSpans,
getSpanOp,
} from './event-proxy-server';

Expand Down
Loading