test: Make node integration test runner more resilient (#14245)

Noticed while debugging some test problems, that if axios throws an
error (e.g. you get a 500 error), jest cannot serialize the error
because it contains recursive data, leading to super hard to debug error
messages.

This makes our node integration test runner more resilient by
normalizing errors we get there to ensure circular references are
resolved.

## Update

While working on this, I found some further, actually more serious
problems - some tests were simply not running at all. Especially all the
session aggregrate tests were simply not being run and just "passed"
accidentally.

The core problem there was that the path to the scenario was incorrect,
so nothing was even started, plus some things where we seemed to catch
errors and still pass (?? I do not think I fixed all of the issues
there, but at least some of them...)

Now, we assert that the scenario file actually exists. Plus, instead of
setting `.expectError()` on the whole runner, you now have to pass this
to a specific `makeRequest` call as an optional option, and it will also
assert if we expect an error _but do not get one_. This way, the tests
can be more explicit and clear in what they do.

Author: mydea

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

@@ -14,20 +14,11 @@ suites/
             |---- scenario_2.ts [optional extra test scenario]
             |---- server_with_mongo.ts [optional custom server]
             |---- server_with_postgres.ts [optional custom server]
-utils/
-|---- defaults/
-      |---- server.ts [default Express server configuration]
 ```
 
 The tests are grouped by their scopes, such as `public-api` or `tracing`. In every group of tests, there are multiple
 folders containing test scenarios and assertions.
 
-Tests run on Express servers (a server instance per test). By default, a simple server template inside
-`utils/defaults/server.ts` is used. Every server instance runs on a different port.
-
-A custom server configuration can be used, supplying a script that exports a valid express server instance as default.
-`runServer` utility function accepts an optional `serverPath` argument for this purpose.
-
 `scenario.ts` contains the initialization logic and the test subject. By default, `{TEST_DIR}/scenario.ts` is used, but
 `runServer` also accepts an optional `scenarioPath` argument for non-standard usage.
 

dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts

@@ -14,7 +14,7 @@ afterAll(() => {
  * This test nevertheless covers the behavior so that we're aware.
  */
 test('withScope scope is NOT applied to thrown error caught by global handler', done => {
-  const runner = createRunner(__dirname, 'server.ts')
+  createRunner(__dirname, 'server.ts')
     .expect({
       event: {
         exception: {
@@ -42,16 +42,15 @@ test('withScope scope is NOT applied to thrown error caught by global handler',
         tags: expect.not.objectContaining({ local: expect.anything() }),
       },
     })
-    .start(done);
-
-  expect(() => runner.makeRequest('get', '/test/withScope')).rejects.toThrow();
+    .start(done)
+    .makeRequest('get', '/test/withScope', { expectError: true });
 });
 
 /**
  * This test shows that the isolation scope set tags are applied correctly to the error.
  */
 test('isolation scope is applied to thrown error caught by global handler', done => {
-  const runner = createRunner(__dirname, 'server.ts')
+  createRunner(__dirname, 'server.ts')
     .expect({
       event: {
         exception: {
@@ -81,7 +80,6 @@ test('isolation scope is applied to thrown error caught by global handler', done
         },
       },
     })
-    .start(done);
-
-  expect(() => runner.makeRequest('get', '/test/isolationScope')).rejects.toThrow();
+    .start(done)
+    .makeRequest('get', '/test/isolationScope', { expectError: true });
 });

dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts

@@ -5,7 +5,7 @@ afterAll(() => {
 });
 
 test('should capture and send Express controller error with txn name if tracesSampleRate is 0', done => {
-  const runner = createRunner(__dirname, 'server.ts')
+  createRunner(__dirname, 'server.ts')
     .ignore('transaction')
     .expect({
       event: {
@@ -33,7 +33,6 @@ test('should capture and send Express controller error with txn name if tracesSa
         transaction: 'GET /test/express/:id',
       },
     })
-    .start(done);
-
-  expect(() => runner.makeRequest('get', '/test/express/123')).rejects.toThrow();
+    .start(done)
+    .makeRequest('get', '/test/express/123', { expectError: true });
 });

dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/test.ts

@@ -5,7 +5,7 @@ afterAll(() => {
 });
 
 test('should capture and send Express controller error if tracesSampleRate is not set.', done => {
-  const runner = createRunner(__dirname, 'server.ts')
+  createRunner(__dirname, 'server.ts')
     .ignore('transaction')
     .expect({
       event: {
@@ -32,7 +32,6 @@ test('should capture and send Express controller error if tracesSampleRate is no
         },
       },
     })
-    .start(done);
-
-  expect(() => runner.makeRequest('get', '/test/express/123')).rejects.toThrow();
+    .start(done)
+    .makeRequest('get', '/test/express/123', { expectError: true });
 });

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts

@@ -9,7 +9,9 @@ test('Should overwrite baggage if the incoming request already has Sentry baggag
   const runner = createRunner(__dirname, '..', 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
+    headers: {
+      baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
+    },
   });
 
   expect(response).toBeDefined();
@@ -25,8 +27,10 @@ test('Should propagate sentry trace baggage data from an incoming to an outgoing
   const runner = createRunner(__dirname, '..', 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
-    baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great',
+    headers: {
+      'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
+      baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great',
+    },
   });
 
   expect(response).toBeDefined();
@@ -42,8 +46,10 @@ test('Should not propagate baggage data from an incoming to an outgoing request
   const runner = createRunner(__dirname, '..', 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    'sentry-trace': '',
-    baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great',
+    headers: {
+      'sentry-trace': '',
+      baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great',
+    },
   });
 
   expect(response).toBeDefined();
@@ -59,7 +65,9 @@ test('Should not propagate baggage if sentry-trace header is present in incoming
   const runner = createRunner(__dirname, '..', 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
+    headers: {
+      'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
+    },
   });
 
   expect(response).toBeDefined();
@@ -74,8 +82,10 @@ test('Should not propagate baggage and ignore original 3rd party baggage entries
   const runner = createRunner(__dirname, '..', 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
-    baggage: 'foo=bar',
+    headers: {
+      'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
+      baggage: 'foo=bar',
+    },
   });
 
   expect(response).toBeDefined();
@@ -107,7 +117,9 @@ test('Should populate Sentry and ignore 3rd party content if sentry-trace header
   const runner = createRunner(__dirname, '..', 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    baggage: 'foo=bar,bar=baz',
+    headers: {
+      baggage: 'foo=bar,bar=baz',
+    },
   });
 
   expect(response).toBeDefined();

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts

@@ -9,8 +9,10 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
   const runner = createRunner(__dirname, 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
-    baggage: 'sentry-release=2.1.0,sentry-environment=myEnv',
+    headers: {
+      'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
+      baggage: 'sentry-release=2.1.0,sentry-environment=myEnv',
+    },
   });
 
   expect(response).toBeDefined();

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts

@@ -9,8 +9,10 @@ test('should merge `baggage` header of a third party vendor with the Sentry DSC
   const runner = createRunner(__dirname, 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
-    baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
+    headers: {
+      'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
+      baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
+    },
   });
 
   expect(response).toBeDefined();

dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/test.ts

@@ -10,7 +10,9 @@ test('Should assign `sentry-trace` header which sets parent trace id of an outgo
   const runner = createRunner(__dirname, 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
-    'sentry-trace': '12312012123120121231201212312012-1121201211212012-0',
+    headers: {
+      'sentry-trace': '12312012123120121231201212312012-1121201211212012-0',
+    },
   });
 
   expect(response).toBeDefined();

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

@@ -22,9 +22,9 @@ describe('express setupExpressErrorHandler', () => {
         .start(done);
 
       // this error is filtered & ignored
-      expect(() => runner.makeRequest('get', '/test1')).rejects.toThrow();
+      runner.makeRequest('get', '/test1', { expectError: true });
       // this error is actually captured
-      expect(() => runner.makeRequest('get', '/test2')).rejects.toThrow();
+      runner.makeRequest('get', '/test2', { expectError: true });
     });
   });
 });

dev-packages/node-integration-tests/suites/public-api/startSpan/with-nested-spans/test.ts

@@ -1,5 +1,6 @@
 import type { SpanJSON } from '@sentry/types';
-import { assertSentryTransaction, cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+import { assertSentryTransaction } from '../../../../utils/assertions';
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
 
 afterAll(() => {
   cleanupChildProcesses();

dev-packages/node-integration-tests/suites/sessions/crashed-session-aggregate/test.ts

@@ -4,16 +4,10 @@ afterEach(() => {
   cleanupChildProcesses();
 });
 
-test('should aggregate successful and crashed sessions', async () => {
-  let _done: undefined | (() => void);
-  const promise = new Promise<void>(resolve => {
-    _done = resolve;
-  });
-
-  const runner = createRunner(__dirname, 'server.ts')
+test('should aggregate successful and crashed sessions', done => {
+  const runner = createRunner(__dirname, '..', 'server.ts')
     .ignore('transaction', 'event')
     .unignore('sessions')
-    .expectError()
     .expect({
       sessions: {
         aggregates: [
@@ -25,11 +19,9 @@ test('should aggregate successful and crashed sessions', async () => {
         ],
       },
     })
-    .start(_done);
-
-  runner.makeRequest('get', '/success');
-  runner.makeRequest('get', '/error_unhandled');
-  runner.makeRequest('get', '/success_next');
+    .start(done);
 
-  await promise;
+  runner.makeRequest('get', '/test/success');
+  runner.makeRequest('get', '/test/error_unhandled', { expectError: true });
+  runner.makeRequest('get', '/test/success_next');
 });

dev-packages/node-integration-tests/suites/sessions/errored-session-aggregate/test.ts

@@ -4,16 +4,10 @@ afterEach(() => {
   cleanupChildProcesses();
 });
 
-test('should aggregate successful, crashed and erroneous sessions', async () => {
-  let _done: undefined | (() => void);
-  const promise = new Promise<void>(resolve => {
-    _done = resolve;
-  });
-
-  const runner = createRunner(__dirname, 'server.ts')
+test('should aggregate successful, crashed and erroneous sessions', done => {
+  const runner = createRunner(__dirname, '..', 'server.ts')
     .ignore('transaction', 'event')
     .unignore('sessions')
-    .expectError()
     .expect({
       sessions: {
         aggregates: [
@@ -26,11 +20,9 @@ test('should aggregate successful, crashed and erroneous sessions', async () =>
         ],
       },
     })
-    .start(_done);
-
-  runner.makeRequest('get', '/success');
-  runner.makeRequest('get', '/error_handled');
-  runner.makeRequest('get', '/error_unhandled');
+    .start(done);
 
-  await promise;
+  runner.makeRequest('get', '/test/success');
+  runner.makeRequest('get', '/test/error_handled');
+  runner.makeRequest('get', '/test/error_unhandled', { expectError: true });
 });

dev-packages/node-integration-tests/suites/sessions/exited-session-aggregate/test.ts

@@ -4,16 +4,10 @@ afterEach(() => {
   cleanupChildProcesses();
 });
 
-test('should aggregate successful sessions', async () => {
-  let _done: undefined | (() => void);
-  const promise = new Promise<void>(resolve => {
-    _done = resolve;
-  });
-
-  const runner = createRunner(__dirname, 'server.ts')
+test('should aggregate successful sessions', done => {
+  const runner = createRunner(__dirname, '..', 'server.ts')
     .ignore('transaction', 'event')
     .unignore('sessions')
-    .expectError()
     .expect({
       sessions: {
         aggregates: [
@@ -24,11 +18,9 @@ test('should aggregate successful sessions', async () => {
         ],
       },
     })
-    .start(_done);
-
-  runner.makeRequest('get', '/success');
-  runner.makeRequest('get', '/success_next');
-  runner.makeRequest('get', '/success_slow');
+    .start(done);
 
-  await promise;
+  runner.makeRequest('get', '/test/success');
+  runner.makeRequest('get', '/test/success_next');
+  runner.makeRequest('get', '/test/success_slow');
 });

dev-packages/node-integration-tests/suites/sessions/server.ts

@@ -1,26 +1,24 @@
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
 import type { SessionFlusher } from '@sentry/core';
 import * as Sentry from '@sentry/node';
 
 Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
   release: '1.0',
+  transport: loggingTransport,
 });
 
+import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
 import express from 'express';
 
 const app = express();
 
-// ### Taken from manual tests ###
-// Hack that resets the 60s default flush interval, and replaces it with just a one second interval
 const flusher = (Sentry.getClient() as Sentry.NodeClient)['_sessionFlusher'] as SessionFlusher;
 
-let flusherIntervalId = flusher && flusher['_intervalId'];
-
-clearInterval(flusherIntervalId);
-
-flusherIntervalId = flusher['_intervalId'] = setInterval(() => flusher?.flush(), 2000);
-
-setTimeout(() => clearInterval(flusherIntervalId), 4000);
+// Flush after 2 seconds (to avoid waiting for the default 60s)
+setTimeout(() => {
+  flusher?.flush();
+}, 2000);
 
 app.get('/test/success', (_req, res) => {
   res.send('Success!');
@@ -52,4 +50,4 @@ app.get('/test/error_handled', (_req, res) => {
 
 Sentry.setupExpressErrorHandler(app);
 
-export default app;
+startExpressServerAndSendPortToRunner(app);

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

@@ -45,7 +45,6 @@ describe('connect auto-instrumentation', () => {
   test('CJS - should capture errors in `connect` middleware.', done => {
     createRunner(__dirname, 'scenario.js')
       .ignore('transaction')
-      .expectError()
       .expect({ event: EXPECTED_EVENT })
       .start(done)
       .makeRequest('get', '/error');
@@ -55,7 +54,6 @@ describe('connect auto-instrumentation', () => {
     createRunner(__dirname, 'scenario.js')
       .ignore('event')
       .expect({ transaction: { transaction: 'GET /error' } })
-      .expectError()
       .start(done)
       .makeRequest('get', '/error');
   });