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

This PR streamlines and fixes some timing/cleanup issues we had in
integration tests, which sometimes lead to issues.

One problem was introduced here:
#13253

This change lead to the server being shut down (because `done` is
called) before the HTTP requests are finished, leading to error logs.

Another problem was that in some cases we had leaking processes, where
we did not properly close servers we started - this was everywhere we
used `createTestServer`.

I also moved some code from the node-integration-tests package to the
remix package, that was only used there (and not properly
depended/imported on).

For future debugging, this was shown by running tests with
`--detectOpenHandles`.

Author: mydea

dev-packages/node-integration-tests/README.md

@@ -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
-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

dev-packages/node-integration-tests/jest.setup.js

@@ -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);
 
 afterEach(() => {
   cleanupChildProcesses();

dev-packages/node-integration-tests/suites/express/multiple-init/server.ts

@@ -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'));
+        }
         res.send({});
       },
       () => {
         res.send({});
       },
     );
-  }, 1000);
+  }, 1);
 });
 
 Sentry.setupExpressErrorHandler(app);

dev-packages/node-integration-tests/suites/express/multiple-init/test.ts

@@ -48,6 +48,17 @@ test('allows to call init multiple times', done => {
         },
       },
     })
+    .expect({
+      event: {
+        exception: {
+          values: [
+            {
+              value: 'Final exception was captured',
+            },
+          ],
+        },
+      },
+    })
     .start(done);
 
   runner

dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts

@@ -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: [

dev-packages/node-integration-tests/suites/tracing/connect/scenario.js

@@ -13,7 +13,7 @@ Sentry.init({
 const connect = require('connect');
 const http = require('http');
 
-const init = async () => {
+const run = async () => {
   const app = connect();
 
   app.use('/', function (req, res, next) {
@@ -34,4 +34,4 @@ const init = async () => {
   sendPortToRunner(port);
 };
 
-init();
+run();

dev-packages/node-integration-tests/suites/tracing/connect/test.ts

@@ -1,7 +1,5 @@
 import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
 
-jest.setTimeout(20000);
-
 describe('connect auto-instrumentation', () => {
   afterAll(async () => {
     cleanupChildProcesses();

dev-packages/node-integration-tests/suites/tracing/hapi/scenario.js

@@ -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,
@@ -65,4 +65,4 @@ const init = async () => {
   sendPortToRunner(port);
 };
 
-init();
+run();

dev-packages/node-integration-tests/suites/tracing/hapi/test.ts

@@ -1,7 +1,5 @@
 import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
 
-jest.setTimeout(20000);
-
 describe('hapi auto-instrumentation', () => {
   afterAll(async () => {
     cleanupChildProcesses();

dev-packages/node-integration-tests/suites/tracing/mongodb/test.ts

@@ -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;
 

dev-packages/node-integration-tests/suites/tracing/mongoose/test.ts

@@ -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;
 

dev-packages/node-integration-tests/suites/tracing/mysql2/test.ts

@@ -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();

dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/scenario.ts

@@ -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));
@@ -56,4 +56,4 @@ async function init(): Promise<void> {
 }
 
 // eslint-disable-next-line @typescript-eslint/no-floating-promises
-init();
+run();

dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/test.ts

@@ -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.');
 

dev-packages/node-integration-tests/suites/tracing/nestjs-errors/scenario.ts

@@ -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));
@@ -54,4 +54,4 @@ async function init(): Promise<void> {
 }
 
 // eslint-disable-next-line @typescript-eslint/no-floating-promises
-init();
+run();

dev-packages/node-integration-tests/suites/tracing/nestjs-errors/test.ts

@@ -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.');
 

dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/scenario.ts

@@ -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));
@@ -56,4 +56,4 @@ async function init(): Promise<void> {
 }
 
 // eslint-disable-next-line @typescript-eslint/no-floating-promises
-init();
+run();

dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/test.ts

@@ -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.');
 

dev-packages/node-integration-tests/suites/tracing/nestjs/scenario.ts

@@ -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();

dev-packages/node-integration-tests/suites/tracing/nestjs/test.ts

@@ -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.');
 

dev-packages/node-integration-tests/suites/tracing/postgres/test.ts

@@ -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 = {

dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts

@@ -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();

dev-packages/node-integration-tests/suites/tracing/redis/test.ts

@@ -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();

dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/test.ts

@@ -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()
@@ -72,7 +72,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
               },
             },
           })
-          .start(done);
+          .start(closeTestServer);
       });
   });
 });

dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing/test.ts

@@ -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()
@@ -42,7 +42,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
               },
             },
           })
-          .start(done);
+          .start(closeTestServer);
       });
   });
 });

dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts

@@ -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({
@@ -41,7 +41,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
               },
             },
           })
-          .start(done);
+          .start(closeTestServer);
       });
   });
 });

dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts

@@ -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({
@@ -41,7 +41,7 @@ conditionalTest({ min: 18 })('outgoing fetch', () => {
               },
             },
           })
-          .start(done);
+          .start(closeTestServer);
       });
   });
 });

dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/test.ts

@@ -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()
@@ -70,6 +70,6 @@ test('outgoing http requests create breadcrumbs', done => {
             },
           },
         })
-        .start(done);
+        .start(closeTestServer);
     });
 });

dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing/test.ts

@@ -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()
@@ -40,6 +40,6 @@ test('outgoing http requests are correctly instrumented with tracing disabled',
             },
           },
         })
-        .start(done);
+        .start(closeTestServer);
     });
 });

dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/test.ts

@@ -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({
@@ -39,6 +39,6 @@ test('outgoing sampled http requests without active span are correctly instrumen
             },
           },
         })
-        .start(done);
+        .start(closeTestServer);
     });
 });