chore(test): Test Remix and NextJS SDKs on Node 20. (#8577)

Made updates about test server termination. Integrated
[`http-terminator`](https://www.npmjs.com/package/http-terminator) as
closing all sockets / cancelling pending requests and such produced
plenty of maintenance code.

Also, found out that
[`portfinder`](https://www.npmjs.com/package/portfinder) does not work
well in Node 20, and causes flakiness. Removed it from Remix and NextJS
tests, as it can be replaced with `app.listen(0, ...)`.

Author: onurtemizkan

.github/workflows/build.yml

@@ -430,7 +430,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        node: [10, 12, 14, 16, 18]
+        node: [10, 12, 14, 16, 18, 20]
     steps:
       - name: Check out current commit (${{ needs.job_get_metadata.outputs.commit_label }})
         uses: actions/checkout@v3
@@ -706,7 +706,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        node: [14, 16, 18]
+        node: [14, 16, 18, 20]
         remix: [1, 2]
     steps:
       - name: Check out current commit (${{ needs.job_get_metadata.outputs.commit_label }})

packages/nextjs/test/integration/test/server/utils/helpers.ts

@@ -1,10 +1,10 @@
-import { getPortPromise } from 'portfinder';
 import { TestEnv } from '../../../../../../node-integration-tests/utils';
 import * as http from 'http';
 import * as path from 'path';
 import { createServer, Server } from 'http';
 import { parse } from 'url';
 import next from 'next';
+import { AddressInfo } from 'net';
 
 // Type not exported from NextJS
 // @ts-ignore
@@ -24,9 +24,10 @@ export const createNextServer = async config => {
   });
 };
 
-export const startServer = async (server: Server, port: string | number) => {
-  return new Promise(resolve => {
-    server.listen(port || 0, () => {
+export const startServer = async (server: Server) => {
+  return new Promise<{ server: http.Server; url: string }>(resolve => {
+    server.listen(0, () => {
+      const port = (server.address() as AddressInfo).port;
       const url = `http://localhost:${port}`;
       resolve({ server, url });
     });
@@ -39,7 +40,6 @@ export class NextTestEnv extends TestEnv {
   }
 
   public static async init(): Promise<NextTestEnv> {
-    const port = await getPortPromise();
     const server = await createNextServer({
       dev: false,
       dir: path.resolve(__dirname, '../../..'),
@@ -50,8 +50,8 @@ export class NextTestEnv extends TestEnv {
       conf: path.resolve(__dirname, '../../next.config.js'),
     });
 
-    await startServer(server, port);
+    const { url } = await startServer(server);
 
-    return new NextTestEnv(server, `http://localhost:${port}`);
+    return new NextTestEnv(server, url);
   }
 }

packages/node-integration-tests/package.json

@@ -31,6 +31,7 @@
     "cors": "^2.8.5",
     "express": "^4.17.3",
     "graphql": "^16.3.0",
+    "http-terminator": "^3.2.0",
     "mongodb": "^3.7.3",
     "mongodb-memory-server-global": "^7.6.3",
     "mysql": "^2.18.1",

packages/node-integration-tests/utils/index.ts

@@ -6,10 +6,14 @@ import type { AxiosRequestConfig } from 'axios';
 import axios from 'axios';
 import type { Express } from 'express';
 import type * as http from 'http';
+import type { HttpTerminator } from 'http-terminator';
+import { createHttpTerminator } from 'http-terminator';
 import type { AddressInfo } from 'net';
 import nock from 'nock';
 import * as path from 'path';
 
+const NODE_VERSION = parseSemver(process.versions.node).major;
+
 export type TestServerConfig = {
   url: string;
   server: http.Server;
@@ -40,7 +44,6 @@ export type DataCollectorOptions = {
  * @return {*}  {jest.Describe}
  */
 export const conditionalTest = (allowedVersion: { min?: number; max?: number }): jest.Describe => {
-  const NODE_VERSION = parseSemver(process.versions.node).major;
   if (!NODE_VERSION) {
     return describe.skip;
   }
@@ -123,10 +126,12 @@ async function makeRequest(
 
 export class TestEnv {
   private _axiosConfig: AxiosRequestConfig | undefined = undefined;
+  private _terminator: HttpTerminator;
 
   public constructor(public readonly server: http.Server, public readonly url: string) {
     this.server = server;
     this.url = url;
+    this._terminator = createHttpTerminator({ server: this.server, gracefulTerminationTimeout: 0 });
   }
 
   /**
@@ -244,12 +249,20 @@ export class TestEnv {
               // Ex: Remix scope bleed tests.
               nock.cleanAll();
 
-              this.server.close(() => {
-                resolve(envelopes);
-              });
+              // Abort all pending requests to nock to prevent hanging / flakes.
+              // See: https://github.com/nock/nock/issues/1118#issuecomment-544126948
+              nock.abortPendingRequests();
+
+              this._closeServer()
+                .catch(e => {
+                  logger.warn(e);
+                })
+                .finally(() => {
+                  resolve(envelopes);
+                });
+            } else {
+              resolve(envelopes);
             }
-
-            resolve(envelopes);
           }
 
           return true;
@@ -291,12 +304,14 @@ export class TestEnv {
 
         nock.cleanAll();
 
-        this.server.close(() => {
+        void this._closeServer().then(() => {
           resolve(reqCount);
         });
-
-        resolve(reqCount);
       }, options.timeout || 1000);
     });
   }
+
+  private _closeServer(): Promise<void> {
+    return this._terminator.terminate();
+  }
 }

packages/remix/package.json

@@ -39,8 +39,7 @@
   "devDependencies": {
     "@remix-run/node": "^1.4.3",
     "@remix-run/react": "^1.4.3",
-    "@types/express": "^4.17.14",
-    "portfinder": "^1.0.28"
+    "@types/express": "^4.17.14"
   },
   "peerDependencies": {
     "@remix-run/node": "1.x",

packages/remix/test/integration/test/server/action.test.ts

@@ -38,7 +38,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
         cookies: expect.any(Object),
         headers: {
           'user-agent': expect.any(String),
-          host: 'localhost:8000',
+          host: expect.stringContaining('localhost:'),
         },
       },
     });
@@ -114,7 +114,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
         cookies: expect.any(Object),
         headers: {
           'user-agent': expect.any(String),
-          host: 'localhost:8000',
+          host: expect.stringContaining('localhost:'),
         },
       },
     });
@@ -134,7 +134,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
         cookies: expect.any(Object),
         headers: {
           'user-agent': expect.any(String),
-          host: 'localhost:8000',
+          host: expect.stringContaining('localhost:'),
         },
       },
     });

packages/remix/test/integration/test/server/utils/helpers.ts

@@ -1,9 +1,9 @@
 import express from 'express';
 import { createRequestHandler } from '@remix-run/express';
-import { getPortPromise } from 'portfinder';
 import { wrapExpressCreateRequestHandler } from '@sentry/remix';
 import { TestEnv } from '../../../../../../node-integration-tests/utils';
 import * as http from 'http';
+import { AddressInfo } from 'net';
 
 export * from '../../../../../../node-integration-tests/utils';
 
@@ -16,18 +16,18 @@ export class RemixTestEnv extends TestEnv {
     const requestHandlerFactory =
       adapter === 'express' ? wrapExpressCreateRequestHandler(createRequestHandler) : createRequestHandler;
 
-    const port = await getPortPromise();
-
+    let serverPort;
     const server = await new Promise<http.Server>(resolve => {
       const app = express();
 
       app.all('*', requestHandlerFactory({ build: require('../../../build') }));
 
-      const server = app.listen(port, () => {
+      const server = app.listen(0, () => {
+        serverPort = (server.address() as AddressInfo).port;
         resolve(server);
       });
     });
 
-    return new RemixTestEnv(server, `http://localhost:${port}`);
+    return new RemixTestEnv(server, `http://localhost:${serverPort}`);
   }
 }