ref(tests): Refactor Node integration tests further. (#5596)

It seems that whenever a manual `http` request is initiated from a test (or test server) without waiting for / asserting on events or transactions, there are also transactions as side effects which may or may not finish before the test ends. This specifically happens with tests that assert request headers for `trace` and `baggage`, because they don't need to wait for the request / scenario to finish.

So it looks like the leakage to the next test doesn't come from the `nock` interceptor or an unclosed server, it comes from an unflushed Sentry transaction. The other tests using `getEnvelopeRequest` or `getMultipleEnvelopeRequest` finish wait for the events to be flushed.

While debugging this, I also made some more refactors in utilities, which I think will make them more concise.

Summary:

- [x] Making sure the events are flushed before tests that use `getAPIResponse` or `runScenario` complete.
- [x] Converted `http` usages in helpers to `axios` calls for simplicity and debuggability.
- [x] Moved `nock` initialization out of `runServer` so we don't need to carry `nock.Scope` around anymore. The `nock` instance is either manually initialized, or inside the `getMultipleEnvelopeRequest` function. The tests that are using manual `http` calls don't normally need nock anyways.
- [x] Filtering envelopes by their types on `nock` level, so if there is a need to filter them, we don't lose the expected `count`. Also, that makes the envelope indices inside tests simpler.
- [x] Switched to `nock.cleanAll()` and `nock.removeInterceptor(mock)` to end `nock` instead of `.persist(false)`.

Co-authored-by: Abhijeet Prasad <[email protected]>

Author: onurtemizkan

packages/node-integration-tests/README.md

@@ -33,9 +33,12 @@ A custom server configuration can be used, supplying a script that exports a val
 
 `utils/` contains helpers and Sentry-specific assertions that can be used in (`test.ts`).
 
-`runServer` utility function returns an object containing `url`, [`http.Server`](https://nodejs.org/dist/latest-v16.x/docs/api/http.html#class-httpserver) and [`nock.Scope`](https://github.com/nock/nock#usage) instances for the created server. The `url` property is the base URL for the server. The `http.Server` instance is used to finish the server eventually, and the `nock.Scope` instance is used to bind / unbind interceptors for the test case.
+`runServer` utility function returns an object containing `url` and [`http.Server`](https://nodejs.org/dist/latest-v16.x/docs/api/http.html#class-httpserver)  instance for the created server. The `url` property is the base URL for the server. The `http.Server` instance is used to finish the server eventually.
+
+The responsibility of ending the server is delegated to the test case. Data collection helpers such as `getEnvelopeRequest` and `getAPIResponse` expect the server instance to be available and finish it before their resolution. Tests that do not use those helpers will need to end the server manually.
+
+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 flakiness.
 
-The responsibility of ending the server and interceptor is delegated to the test case. Data collection helpers such as `getEnvelopeRequest` and `getAPIResponse` expect those instances to be available and finish them before their resolution. Test that do not use those helpers will need to end the server and interceptors manually.
 ## Running Tests Locally
 
 Tests can be run locally with:

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

@@ -1,8 +1,8 @@
 import { assertSentryEvent, getEnvelopeRequest, runServer } from '../../../utils/index';
 
 test('should capture and send Express controller error.', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${__dirname}/server.ts`);
-  const event = await getEnvelopeRequest({ url: `${url}/express`, server, scope });
+  const { url, server } = await runServer(__dirname, `${__dirname}/server.ts`);
+  const event = await getEnvelopeRequest({ url: `${url}/express`, server });
 
   expect((event[2] as any).exception.values[0].stacktrace.frames.length).toBeGreaterThan(0);
 

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

@@ -4,10 +4,10 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
 test('Should not overwrite baggage if the incoming request already has Sentry baggage data.', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
   const response = (await getAPIResponse(
-    { url: `${url}/express`, server, scope },
+    { url: `${url}/express`, server },
     {
       baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
     },
@@ -23,10 +23,10 @@ test('Should not overwrite baggage if the incoming request already has Sentry ba
 });
 
 test('Should propagate sentry trace baggage data from an incoming to an outgoing request.', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
   const response = (await getAPIResponse(
-    { url: `${url}/express`, server, scope },
+    { url: `${url}/express`, server },
     {
       'sentry-trace': '',
       baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great',
@@ -43,10 +43,10 @@ test('Should propagate sentry trace baggage data from an incoming to an outgoing
 });
 
 test('Should propagate empty baggage if sentry-trace header is present in incoming request but no baggage header', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
   const response = (await getAPIResponse(
-    { url: `${url}/express`, server, scope },
+    { url: `${url}/express`, server },
     {
       'sentry-trace': '',
     },
@@ -62,10 +62,10 @@ test('Should propagate empty baggage if sentry-trace header is present in incomi
 });
 
 test('Should propagate empty sentry and ignore original 3rd party baggage entries if sentry-trace header is present', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
   const response = (await getAPIResponse(
-    { url: `${url}/express`, server, scope },
+    { url: `${url}/express`, server },
     {
       'sentry-trace': '',
       baggage: 'foo=bar',
@@ -82,9 +82,9 @@ test('Should propagate empty sentry and ignore original 3rd party baggage entrie
 });
 
 test('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
-  const response = (await getAPIResponse({ url: `${url}/express`, server, scope }, {})) as TestAPIResponse;
+  const response = (await getAPIResponse({ url: `${url}/express`, server }, {})) as TestAPIResponse;
 
   expect(response).toBeDefined();
   expect(response).toMatchObject({
@@ -99,10 +99,10 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
 });
 
 test('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
   const response = (await getAPIResponse(
-    { url: `${url}/express`, server, scope },
+    { url: `${url}/express`, server },
     {
       baggage: 'foo=bar,bar=baz',
     },

packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/test.ts

@@ -4,9 +4,9 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
 test('Does not include transaction name if transaction source is not set', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
-  const response = (await getAPIResponse({ url: `${url}/express`, server, scope })) as TestAPIResponse;
+  const response = (await getAPIResponse({ url: `${url}/express`, server })) as TestAPIResponse;
   const baggageString = response.test_data.baggage;
 
   expect(response).toBeDefined();

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

@@ -4,9 +4,9 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
 test('should attach a `baggage` header to an outgoing request.', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
-  const response = (await getAPIResponse({ url: `${url}/express`, server, scope })) as TestAPIResponse;
+  const response = (await getAPIResponse({ url: `${url}/express`, server })) as TestAPIResponse;
 
   expect(response).toBeDefined();
   expect(response).toMatchObject({

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

@@ -4,10 +4,10 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
 test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
   const response = (await getAPIResponse(
-    { url: `${url}/express`, server, scope },
+    { url: `${url}/express`, server },
     {
       'sentry-trace': '',
       baggage: 'sentry-release=2.1.0,sentry-environment=myEnv',
@@ -24,9 +24,9 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
 });
 
 test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
-  const response = (await getAPIResponse({ url: `${url}/express`, server, scope }, {})) as TestAPIResponse;
+  const response = (await getAPIResponse({ url: `${url}/express`, server }, {})) as TestAPIResponse;
 
   expect(response).toBeDefined();
   expect(response).toMatchObject({

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

@@ -4,10 +4,10 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
 test('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
   const response = (await getAPIResponse(
-    { url: `${url}/express`, server, scope },
+    { url: `${url}/express`, server },
     {
       'sentry-trace': '',
       baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',

packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts

@@ -4,9 +4,9 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
 test('Includes transaction in baggage if the transaction name is parameterized', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
-  const response = (await getAPIResponse({ url: `${url}/express`, server, scope })) as TestAPIResponse;
+  const response = (await getAPIResponse({ url: `${url}/express`, server })) as TestAPIResponse;
 
   expect(response).toBeDefined();
   expect(response).toMatchObject({

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

@@ -5,10 +5,10 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
 test('Should assign `sentry-trace` header which sets parent trace id of an outgoing request.', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
   const response = (await getAPIResponse(
-    { url: `${url}/express`, server, scope },
+    { url: `${url}/express`, server },
     {
       'sentry-trace': '12312012123120121231201212312012-1121201211212012-0',
     },

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

@@ -5,9 +5,9 @@ import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
 test('should attach a `sentry-trace` header to an outgoing request.', async () => {
-  const { url, server, scope } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
+  const { url, server } = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
-  const response = (await getAPIResponse({ url: `${url}/express`, server, scope })) as TestAPIResponse;
+  const response = (await getAPIResponse({ url: `${url}/express`, server })) as TestAPIResponse;
 
   expect(response).toBeDefined();
   expect(response).toMatchObject({