Skip to content

test: Fix node-integration-test timeouts & cleanup #13280

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 16 commits into from
Aug 12, 2024
8 changes: 0 additions & 8 deletions dev-packages/node-integration-tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,6 @@ requests, and assertions. Test server, interceptors and assertions are all run o

`utils/` contains helpers and Sentry-specific assertions that can be used in (`test.ts`).

`TestEnv` class contains methods to create and execute requests on a test server instance. `TestEnv.init()` which starts
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not actually used anywhere anymore, so removed this.

Copy link
Member Author

Choose a reason for hiding this comment

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

or, actually moved it to the remix package, where it is used/needed.

a test server and returns a `TestEnv` instance must be called by each test. The test server is automatically shut down
after each test, if a data collection helper method such as `getEnvelopeRequest` and `getAPIResponse` is used. Tests
that do not use those helper methods will need to end the server manually.

`TestEnv` instance has two public properties: `url` and `server`. The `url` property is the base URL for the server. The
`http.Server` instance is used to finish the server eventually.

Nock interceptors are internally used to capture envelope requests by `getEnvelopeRequest` and
`getMultipleEnvelopeRequest` helpers. After capturing required requests, the interceptors are removed. Nock can manually
be used inside the test cases to intercept requests but should be removed before the test ends, as not to cause
Expand Down
4 changes: 2 additions & 2 deletions dev-packages/node-integration-tests/jest.setup.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const { cleanupChildProcesses } = require('./utils/runner');

// Increases test timeout from 5s to 45s
jest.setTimeout(45000);
// Default timeout: 15s
jest.setTimeout(15000);
Copy link
Member Author

Choose a reason for hiding this comment

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

Reducing this by default, in cases where we need more we can opt-in to a higher timeout in a case-by-case basis (which also leads to us being conscious about which tests take long)


afterEach(() => {
cleanupChildProcesses();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,19 @@ app.get('/test/error/:id', (req, res) => {

setTimeout(() => {
// We flush to ensure we are sending exceptions in a certain order
Sentry.flush(3000).then(
Sentry.flush(1000).then(
() => {
// We send this so we can wait for this, to know the test is ended & server can be closed
if (id === '3') {
Sentry.captureException(new Error('Final exception was captured'));
Copy link
Member Author

Choose a reason for hiding this comment

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

this lead to a "race condition" in tests, which did not fail the test, but showed errors in the test log:

we wait for the error event to be sent, then end the test. at this point the server is shut down, which leads to the http request being aborted.

now, we specifically wait for this event, so we know everything is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I think my mistake here was that I though the test was terminated when both the request ends and the error was received. Maybe that is actually what we wanna do in the test? idk

}
res.send({});
},
() => {
res.send({});
},
);
}, 1000);
}, 1);
});

Sentry.setupExpressErrorHandler(app);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,17 @@ test('allows to call init multiple times', done => {
},
},
})
.expect({
event: {
exception: {
values: [
{
value: 'Final exception was captured',
},
],
},
},
})
.start(done);

runner
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ import * as path from 'path';
import { conditionalTest } from '../../../utils';
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

// This test takes some time because it connects the debugger etc.
// So we increase the timeout here
jest.setTimeout(45_000);

const EXPECTED_LOCAL_VARIABLES_EVENT = {
exception: {
values: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Sentry.init({
const connect = require('connect');
const http = require('http');

const init = async () => {
const run = async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

renaming these in a few places for consistency sake, init was a bit confusing IMHO.

const app = connect();

app.use('/', function (req, res, next) {
Expand All @@ -34,4 +34,4 @@ const init = async () => {
sendPortToRunner(port);
};

init();
run();
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

jest.setTimeout(20000);

describe('connect auto-instrumentation', () => {
afterAll(async () => {
cleanupChildProcesses();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const Boom = require('@hapi/boom');

const port = 5999;

const init = async () => {
const run = async () => {
const server = Hapi.server({
host: 'localhost',
port,
Expand Down Expand Up @@ -65,4 +65,4 @@ const init = async () => {
sendPortToRunner(port);
};

init();
run();
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

jest.setTimeout(20000);

describe('hapi auto-instrumentation', () => {
afterAll(async () => {
cleanupChildProcesses();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import { MongoMemoryServer } from 'mongodb-memory-server-global';

import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

jest.setTimeout(20000);

describe('MongoDB experimental Test', () => {
let mongoServer: MongoMemoryServer;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import { MongoMemoryServer } from 'mongodb-memory-server-global';

import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

jest.setTimeout(20000);

describe('Mongoose experimental Test', () => {
let mongoServer: MongoMemoryServer;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

// When running docker compose, we need a larger timeout, as this takes some time...
jest.setTimeout(75000);

describe('mysql2 auto instrumentation', () => {
afterAll(() => {
cleanupChildProcesses();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class AppController {
})
class AppModule {}

async function init(): Promise<void> {
async function run(): Promise<void> {
const app = await NestFactory.create(AppModule);
const { httpAdapter } = app.get(HttpAdapterHost);
Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter));
Expand All @@ -56,4 +56,4 @@ async function init(): Promise<void> {
}

// eslint-disable-next-line @typescript-eslint/no-floating-promises
init();
run();
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { conditionalTest } from '../../../utils';
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

jest.setTimeout(20000);

const { TS_VERSION } = process.env;
const isOldTS = TS_VERSION && TS_VERSION.startsWith('3.');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class AppController {
})
class AppModule {}

async function init(): Promise<void> {
async function run(): Promise<void> {
const app = await NestFactory.create(AppModule);
const { httpAdapter } = app.get(HttpAdapterHost);
Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter));
Expand All @@ -54,4 +54,4 @@ async function init(): Promise<void> {
}

// eslint-disable-next-line @typescript-eslint/no-floating-promises
init();
run();
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { conditionalTest } from '../../../utils';
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

jest.setTimeout(20000);

const { TS_VERSION } = process.env;
const isOldTS = TS_VERSION && TS_VERSION.startsWith('3.');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class AppController {
})
class AppModule {}

async function init(): Promise<void> {
async function run(): Promise<void> {
const app = await NestFactory.create(AppModule);
const { httpAdapter } = app.get(HttpAdapterHost);
Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter));
Expand All @@ -56,4 +56,4 @@ async function init(): Promise<void> {
}

// eslint-disable-next-line @typescript-eslint/no-floating-promises
init();
run();
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { conditionalTest } from '../../../utils';
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

jest.setTimeout(20000);

const { TS_VERSION } = process.env;
const isOldTS = TS_VERSION && TS_VERSION.startsWith('3.');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ class AppController {
})
class AppModule {}

async function init(): Promise<void> {
async function run(): Promise<void> {
const app = await NestFactory.create(AppModule);
await app.listen(port);
sendPortToRunner(port);
}

// eslint-disable-next-line @typescript-eslint/no-floating-promises
init();
run();
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { conditionalTest } from '../../../utils';
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

jest.setTimeout(20000);

const { TS_VERSION } = process.env;
const isOldTS = TS_VERSION && TS_VERSION.startsWith('3.');

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { createRunner } from '../../../utils/runner';

// When running docker compose, we need a larger timeout, as this takes some time...
jest.setTimeout(75000);

describe('postgres auto instrumentation', () => {
test('should auto-instrument `pg` package', done => {
const EXPECTED_TRANSACTION = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

// When running docker compose, we need a larger timeout, as this takes some time...
jest.setTimeout(75000);

describe('redis cache auto instrumentation', () => {
afterAll(() => {
cleanupChildProcesses();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

// When running docker compose, we need a larger timeout, as this takes some time...
jest.setTimeout(75000);

describe('redis auto instrumentation', () => {
afterAll(() => {
cleanupChildProcesses();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
test('outgoing fetch requests create breadcrumbs', done => {
createTestServer(done)
.start()
.then(SERVER_URL => {
.then(([SERVER_URL, closeTestServer]) => {
createRunner(__dirname, 'scenario.ts')
.withEnv({ SERVER_URL })
.ensureNoErrorOutput()
Expand Down Expand Up @@ -72,7 +72,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
},
},
})
.start(done);
.start(closeTestServer);
Copy link
Member Author

Choose a reason for hiding this comment

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

the main change, when we use createTestServer, make sure to use this instead of done (which under the hood also calls done), to make sure everything is closed properly.

});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
expect(headers['sentry-trace']).toBeUndefined();
})
.start()
.then(SERVER_URL => {
.then(([SERVER_URL, closeTestServer]) => {
createRunner(__dirname, 'scenario.ts')
.withEnv({ SERVER_URL })
.ensureNoErrorOutput()
Expand All @@ -42,7 +42,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
},
},
})
.start(done);
.start(closeTestServer);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
expect(headers['sentry-trace']).toBeUndefined();
})
.start()
.then(SERVER_URL => {
.then(([SERVER_URL, closeTestServer]) => {
createRunner(__dirname, 'scenario.ts')
.withEnv({ SERVER_URL })
.expect({
Expand All @@ -41,7 +41,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
},
},
})
.start(done);
.start(closeTestServer);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
expect(headers['sentry-trace']).toBeUndefined();
})
.start()
.then(SERVER_URL => {
.then(([SERVER_URL, closeTestServer]) => {
createRunner(__dirname, 'scenario.ts')
.withEnv({ SERVER_URL })
.expect({
Expand All @@ -41,7 +41,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
},
},
})
.start(done);
.start(closeTestServer);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { createTestServer } from '../../../../utils/server';
test('outgoing http requests create breadcrumbs', done => {
createTestServer(done)
.start()
.then(SERVER_URL => {
.then(([SERVER_URL, closeTestServer]) => {
createRunner(__dirname, 'scenario.ts')
.withEnv({ SERVER_URL })
.ensureNoErrorOutput()
Expand Down Expand Up @@ -70,6 +70,6 @@ test('outgoing http requests create breadcrumbs', done => {
},
},
})
.start(done);
.start(closeTestServer);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ test('outgoing http requests are correctly instrumented with tracing disabled',
expect(headers['sentry-trace']).toBeUndefined();
})
.start()
.then(SERVER_URL => {
.then(([SERVER_URL, closeTestServer]) => {
createRunner(__dirname, 'scenario.ts')
.withEnv({ SERVER_URL })
.ensureNoErrorOutput()
Expand All @@ -40,6 +40,6 @@ test('outgoing http requests are correctly instrumented with tracing disabled',
},
},
})
.start(done);
.start(closeTestServer);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ test('outgoing sampled http requests without active span are correctly instrumen
expect(headers['sentry-trace']).toBeUndefined();
})
.start()
.then(SERVER_URL => {
.then(([SERVER_URL, closeTestServer]) => {
createRunner(__dirname, 'scenario.ts')
.withEnv({ SERVER_URL })
.expect({
Expand All @@ -39,6 +39,6 @@ test('outgoing sampled http requests without active span are correctly instrumen
},
},
})
.start(done);
.start(closeTestServer);
});
});
Loading
Loading