-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
test(remix): Add Remix v2 future flags integration tests. #8397
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../../common/routes/action-json-response.$id'; | ||
export { default } from '../../../common/routes/action-json-response.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/capture-exception'; | ||
export { default } from '../../common/routes/capture-exception'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/capture-message'; | ||
export { default } from '../../common/routes/capture-message'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../../common/routes/error-boundary-capture.$id'; | ||
export { default } from '../../../common/routes/error-boundary-capture.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/index'; | ||
export { default } from '../../common/routes/index'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../../common/routes/loader-defer-response'; | ||
export { default } from '../../../common/routes/loader-defer-response'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../../common/routes/loader-json-response.$id'; | ||
export { default } from '../../../common/routes/loader-json-response.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../../common/routes/manual-tracing.$id'; | ||
export { default } from '../../../common/routes/manual-tracing.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../../common/routes/scope-bleed.$id'; | ||
export { default } from '../../../common/routes/scope-bleed.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { RemixBrowser, useLocation, useMatches } from '@remix-run/react'; | ||
import { hydrate } from 'react-dom'; | ||
import * as Sentry from '@sentry/remix'; | ||
import { useEffect } from 'react'; | ||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
tracesSampleRate: 1, | ||
integrations: [ | ||
new Sentry.BrowserTracing({ | ||
routingInstrumentation: Sentry.remixRouterInstrumentation(useEffect, useLocation, useMatches), | ||
}), | ||
], | ||
}); | ||
|
||
hydrate(<RemixBrowser />, document); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import type { EntryContext } from '@remix-run/node'; | ||
import { RemixServer } from '@remix-run/react'; | ||
import { renderToString } from 'react-dom/server'; | ||
import * as Sentry from '@sentry/remix'; | ||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
tracesSampleRate: 1, | ||
// Disabling to test series of envelopes deterministically. | ||
autoSessionTracking: false, | ||
}); | ||
|
||
export default function handleRequest( | ||
request: Request, | ||
responseStatusCode: number, | ||
responseHeaders: Headers, | ||
remixContext: EntryContext, | ||
) { | ||
let markup = renderToString(<RemixServer context={remixContext} url={request.url} />); | ||
|
||
responseHeaders.set('Content-Type', 'text/html'); | ||
|
||
return new Response('<!DOCTYPE html>' + markup, { | ||
status: responseStatusCode, | ||
headers: responseHeaders, | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import { V2_MetaFunction, LoaderFunction, json, defer, redirect } from '@remix-run/node'; | ||
import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration } from '@remix-run/react'; | ||
import { withSentry } from '@sentry/remix'; | ||
|
||
export const meta: V2_MetaFunction = ({ data }) => [ | ||
{ charset: 'utf-8' }, | ||
{ title: 'New Remix App' }, | ||
{ name: 'viewport', content: 'width=device-width,initial-scale=1' }, | ||
{ name: 'sentry-trace', content: data.sentryTrace }, | ||
{ name: 'baggage', content: data.sentryBaggage }, | ||
]; | ||
|
||
export const loader: LoaderFunction = async ({ request }) => { | ||
const url = new URL(request.url); | ||
const type = url.searchParams.get('type'); | ||
|
||
switch (type) { | ||
case 'empty': | ||
return {}; | ||
case 'plain': | ||
return { | ||
data_one: [], | ||
data_two: 'a string', | ||
}; | ||
case 'json': | ||
return json({ data_one: [], data_two: 'a string' }, { headers: { 'Cache-Control': 'max-age=300' } }); | ||
case 'defer': | ||
return defer({ data_one: [], data_two: 'a string' }); | ||
case 'null': | ||
return null; | ||
case 'undefined': | ||
return undefined; | ||
case 'throwRedirect': | ||
throw redirect('/?type=plain'); | ||
case 'returnRedirect': | ||
return redirect('/?type=plain'); | ||
default: { | ||
return {}; | ||
} | ||
} | ||
}; | ||
|
||
function App() { | ||
return ( | ||
<html lang="en"> | ||
<head> | ||
<Meta /> | ||
<Links /> | ||
</head> | ||
<body> | ||
<Outlet /> | ||
<ScrollRestoration /> | ||
<Scripts /> | ||
<LiveReload /> | ||
</body> | ||
</html> | ||
); | ||
} | ||
|
||
export default withSentry(App); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/action-json-response.$id'; | ||
export { default } from '../../common/routes/action-json-response.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/capture-exception'; | ||
export { default } from '../../common/routes/capture-exception'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/capture-message'; | ||
export { default } from '../../common/routes/capture-message'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/error-boundary-capture.$id'; | ||
export { default } from '../../common/routes/error-boundary-capture.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/index'; | ||
export { default } from '../../common/routes/index'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/loader-defer-response'; | ||
export { default } from '../../common/routes/loader-defer-response'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/loader-json-response.$id'; | ||
export { default } from '../../common/routes/loader-json-response.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/manual-tracing.$id'; | ||
export { default } from '../../common/routes/manual-tracing.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from '../../common/routes/scope-bleed.$id'; | ||
export { default } from '../../common/routes/scope-bleed.$id'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,16 @@ | ||
/** @type {import('@remix-run/dev').AppConfig} */ | ||
const useV2 = process.env.REMIX_VERSION === '2'; | ||
|
||
module.exports = { | ||
appDirectory: 'app', | ||
appDirectory: useV2 ? 'app_v2' : 'app_v1', | ||
assetsBuildDirectory: 'public/build', | ||
serverBuildPath: 'build/index.js', | ||
publicPath: '/build/', | ||
future: { | ||
v2_errorBoundary: useV2, | ||
v2_headers: useV2, | ||
v2_meta: useV2, | ||
v2_normalizeFormMethod: useV2, | ||
v2_routeConvention: useV2, | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ import { getMultipleSentryEnvelopeRequests } from './utils/helpers'; | |
import { test, expect } from '@playwright/test'; | ||
import { Event } from '@sentry/types'; | ||
|
||
const useV2 = process.env.REMIX_VERSION === '2'; | ||
|
||
test('should capture React component errors.', async ({ page }) => { | ||
const envelopes = await getMultipleSentryEnvelopeRequests<Event>(page, 2, { | ||
url: '/error-boundary-capture/0', | ||
|
@@ -12,7 +14,9 @@ test('should capture React component errors.', async ({ page }) => { | |
expect(pageloadEnvelope.contexts?.trace.op).toBe('pageload'); | ||
expect(pageloadEnvelope.tags?.['routing.instrumentation']).toBe('remix-router'); | ||
expect(pageloadEnvelope.type).toBe('transaction'); | ||
expect(pageloadEnvelope.transaction).toBe('routes/error-boundary-capture/$id'); | ||
expect(pageloadEnvelope.transaction).toBe( | ||
useV2 ? 'routes/error-boundary-capture.$id' : 'routes/error-boundary-capture/$id', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a lot of context around React/Remix error boundaries so I'm wondering: Why is the transaction name different here? Shouldn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like it's caused by the new route convention. I think we should address this in our transaction name logic for consistency. I'll check if we have access to any information about the final URLs inside our build-time route objects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep the dot notation for transaction names if they are using the new route convention. I'd rather just match the schema that remix uses - this transaction name (with periods) also means it's much easier for users to go in and find the exact file that was generating this route. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good points, Abhi. Then let's keep it. |
||
); | ||
|
||
expect(errorEnvelope.level).toBe('error'); | ||
expect(errorEnvelope.sdk?.name).toBe('sentry.javascript.remix'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
const useV2 = process.env.REMIX_VERSION === '2'; | ||
|
||
import { getFirstSentryEnvelopeRequest } from './utils/helpers'; | ||
import { test, expect } from '@playwright/test'; | ||
import { Event } from '@sentry/types'; | ||
|
@@ -8,5 +10,6 @@ test('should add `pageload` transaction on load.', async ({ page }) => { | |
expect(envelope.contexts?.trace.op).toBe('pageload'); | ||
expect(envelope.tags?.['routing.instrumentation']).toBe('remix-router'); | ||
expect(envelope.type).toBe('transaction'); | ||
expect(envelope.transaction).toBe('routes/index'); | ||
|
||
expect(envelope.transaction).toBe(useV2 ? 'root' : 'routes/index'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise, I'm assuming this is also something our current instrumentation just doesn't cover correctly in v2? |
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No action required lol