Skip to content

test(remix): Add server-side instrumentation integration tests. #5538

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

Merged
merged 4 commits into from
Aug 9, 2022
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
1 change: 1 addition & 0 deletions packages/node-integration-tests/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"@types/mysql": "^2.15.21",
"@types/pg": "^8.6.5",
"apollo-server": "^3.6.7",
"axios": "^0.27.2",
"cors": "^2.8.5",
"express": "^4.17.3",
"graphql": "^16.3.0",
Expand Down
40 changes: 30 additions & 10 deletions packages/node-integration-tests/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
/* eslint-disable @typescript-eslint/no-unsafe-member-access */
import { parseSemver } from '@sentry/utils';
import { logger, parseSemver } from '@sentry/utils';
import axios from 'axios';
import { Express } from 'express';
import * as http from 'http';
import { RequestOptions } from 'https';
import nock from 'nock';
import * as path from 'path';
import { getPortPromise } from 'portfinder';

/**
* Returns`describe` or `describe.skip` depending on allowed major versions of Node.
*
Expand All @@ -33,7 +33,7 @@ export const conditionalTest = (allowedVersion: { min?: number; max?: number }):
export const assertSentryEvent = (actual: Record<string, unknown>, expected: Record<string, unknown>): void => {
expect(actual).toMatchObject({
event_id: expect.any(String),
timestamp: expect.any(Number),
timestamp: expect.anything(),
...expected,
});
};
Expand All @@ -47,8 +47,8 @@ export const assertSentryEvent = (actual: Record<string, unknown>, expected: Rec
export const assertSentryTransaction = (actual: Record<string, unknown>, expected: Record<string, unknown>): void => {
expect(actual).toMatchObject({
event_id: expect.any(String),
timestamp: expect.any(Number),
start_timestamp: expect.any(Number),
timestamp: expect.anything(),
start_timestamp: expect.anything(),
spans: expect.any(Array),
type: 'transaction',
...expected,
Expand All @@ -71,12 +71,18 @@ export const parseEnvelope = (body: string): Array<Record<string, unknown>> => {
* @param url The url the intercepted requests will be directed to.
* @param count The expected amount of requests to the envelope endpoint. If
* the amount of sentrequests is lower than`count`, this function will not resolve.
* @param method The method of the request. Defaults to `GET`.
* @returns The intercepted envelopes.
*/
export const getMultipleEnvelopeRequest = async (url: string, count: number): Promise<Record<string, unknown>[][]> => {
export const getMultipleEnvelopeRequest = async (
url: string,
count: number,
method: 'get' | 'post' = 'get',
): Promise<Record<string, unknown>[][]> => {
const envelopes: Record<string, unknown>[][] = [];

return new Promise(resolve => {
// eslint-disable-next-line no-async-promise-executor
return new Promise(async resolve => {
nock('https://dsn.ingest.sentry.io')
.post('/api/1337/envelope/', body => {
const envelope = parseEnvelope(body);
Expand All @@ -92,7 +98,17 @@ export const getMultipleEnvelopeRequest = async (url: string, count: number): Pr
.query(true) // accept any query params - used for sentry_key param
.reply(200);

http.get(url);
try {
if (method === 'get') {
await axios.get(url);
} else {
await axios.post(url);
}
} catch (e) {
// We sometimes expect the request to fail, but not the test.
// So, we do nothing.
logger.warn(e);
}
});
};

Expand Down Expand Up @@ -133,10 +149,14 @@ export const getAPIResponse = async (url: URL, headers?: Record<string, string>)
* Intercepts and extracts a single request containing a Sentry envelope
*
* @param url The url the intercepted request will be directed to.
* @param method The method of the request. Defaults to `GET`.
* @returns The extracted envelope.
*/
export const getEnvelopeRequest = async (url: string): Promise<Array<Record<string, unknown>>> => {
return (await getMultipleEnvelopeRequest(url, 1))[0];
export const getEnvelopeRequest = async (
url: string,
method: 'get' | 'post' = 'get',
): Promise<Array<Record<string, unknown>>> => {
return (await getMultipleEnvelopeRequest(url, 1, method))[0];
};

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/remix/test/integration/app/entry.server.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import * as Sentry from '@sentry/remix';
Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
tracesSampleRate: 1,
// Disabling to test series of envelopes deterministically.
autoSessionTracking: false,
});

export default function handleRequest(
Expand Down
2 changes: 1 addition & 1 deletion packages/remix/test/integration/app/root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export const meta: MetaFunction = ({ data }) => ({
baggage: data.sentryBaggage,
});

function App() {
export function App() {
return (
<html lang="en">
<head>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { ActionFunction, json, redirect, LoaderFunction } from '@remix-run/node';
import { useActionData } from '@remix-run/react';

export const loader: LoaderFunction = async ({ params: { id } }) => {
if (id === '-1') {
throw new Error('Unexpected Server Error from Loader');
}
};

export const action: ActionFunction = async ({ params: { id } }) => {
if (id === '-1') {
throw new Error('Unexpected Server Error');
}

if (id === '-2') {
// Note: This GET request triggers to the `Loader` of the URL, not the `Action`.
throw redirect('/action-json-response/-1');
}

return json({ test: 'test' });
};

export default function ActionJSONResponse() {
const data = useActionData();

return (
<div>
<h1>{data && data.test ? data.test : 'Not Found'}</h1>
</div>
);
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
import { json, LoaderFunction } from '@remix-run/node';
import { json, LoaderFunction, redirect } from '@remix-run/node';
import { useLoaderData } from '@remix-run/react';

type LoaderData = { id: string };

export const loader: LoaderFunction = async ({ params: { id } }) => {
if (id === '-2') {
throw new Error('Unexpected Server Error from Loader');
}

if (id === '-1') {
throw redirect('/loader-json-response/-2');
}

return json({
id,
});
Expand All @@ -14,7 +22,7 @@ export default function LoaderJSONResponse() {

return (
<div>
<h1>{data.id}</h1>
<h1>{data && data.id ? data.id : 'Not Found'}</h1>
</div>
);
}
131 changes: 131 additions & 0 deletions packages/remix/test/integration/test/server/action.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import {
assertSentryTransaction,
getEnvelopeRequest,
runServer,
getMultipleEnvelopeRequest,
assertSentryEvent,
} from './utils/helpers';

jest.spyOn(console, 'error').mockImplementation();

describe('Remix API Actions', () => {
it('correctly instruments a parameterized Remix API action', async () => {
const baseURL = await runServer();
const url = `${baseURL}/action-json-response/123123`;
const envelope = await getEnvelopeRequest(url, 'post');
const transaction = envelope[2];

assertSentryTransaction(transaction, {
transaction: 'routes/action-json-response/$id',
spans: [
{
description: 'routes/action-json-response/$id',
Copy link
Member

Choose a reason for hiding this comment

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

We should really think about if we can make this name better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe something like `${action | loader | documentRequest} of ${route}`?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I like that, even if the info is a bit redundant

op: 'remix.server.action',
},
{
description: 'routes/action-json-response/$id',
op: 'remix.server.loader',
},
{
description: 'routes/action-json-response/$id',
op: 'remix.server.documentRequest',
},
],
});
});

it('reports an error thrown from the action', async () => {
const baseURL = await runServer();
const url = `${baseURL}/action-json-response/-1`;

const [transaction, event] = await getMultipleEnvelopeRequest(url, 2, 'post');

assertSentryTransaction(transaction[2], {
contexts: {
trace: {
status: 'internal_error',
tags: {
'http.status_code': '500',
},
},
},
});

assertSentryEvent(event[2], {
exception: {
values: [
{
type: 'Error',
value: 'Unexpected Server Error',
stacktrace: expect.any(Object),
mechanism: {
data: {
function: 'action',
},
handled: true,
type: 'instrument',
},
},
],
},
});
});

it('handles a thrown 500 response', async () => {
const baseURL = await runServer();
const url = `${baseURL}/action-json-response/-2`;

const [transaction_1, event, transaction_2] = await getMultipleEnvelopeRequest(url, 3, 'post');

assertSentryTransaction(transaction_1[2], {
contexts: {
trace: {
op: 'http.server',
status: 'ok',
tags: {
method: 'POST',
'http.status_code': '302',
},
},
},
tags: {
transaction: 'routes/action-json-response/$id',
},
});

assertSentryTransaction(transaction_2[2], {
contexts: {
trace: {
op: 'http.server',
status: 'internal_error',
tags: {
method: 'GET',
'http.status_code': '500',
},
},
},
tags: {
transaction: 'routes/action-json-response/$id',
},
});

assertSentryEvent(event[2], {
exception: {
values: [
{
type: 'Error',
value: 'Unexpected Server Error from Loader',
stacktrace: expect.any(Object),
mechanism: {
data: {
function: 'loader',
},
handled: true,
type: 'instrument',
},
},
],
},
});
});
});
Loading