From 42fc65cb282a8d5b8bf853775a8eedc421d33524 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Fri, 21 Jul 2023 17:58:55 -0700 Subject: [PATCH] Test against Node 20, update tests for compatibility (#7636) Test against Node 20 and update tests as needed. This unblocks our integrations from testing against Node 20 as well since the testsuite is part of this changeset / has Node 20 test failures. --- .changeset/four-mangos-hammer.md | 6 ++ .circleci/config.yml | 2 +- .../src/httpServerTests.ts | 2 +- .../express4/expressSpecific.test.ts | 2 +- .../plugin/drainHttpServer/stoppable.test.ts | 81 +++++++++++++++---- .../drainHttpServer/stoppable/server.js | 17 ---- 6 files changed, 76 insertions(+), 34 deletions(-) create mode 100644 .changeset/four-mangos-hammer.md delete mode 100644 packages/server/src/__tests__/plugin/drainHttpServer/stoppable/server.js diff --git a/.changeset/four-mangos-hammer.md b/.changeset/four-mangos-hammer.md new file mode 100644 index 00000000000..09aabfd0680 --- /dev/null +++ b/.changeset/four-mangos-hammer.md @@ -0,0 +1,6 @@ +--- +'@apollo/server-integration-testsuite': patch +'@apollo/server': patch +--- + +Update test suite for compatibility with Node v20 diff --git a/.circleci/config.yml b/.circleci/config.yml index 1035978ddc8..fdc717008d9 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -132,7 +132,6 @@ jobs: command: "! git grep FIXM\x45" workflows: - version: 2 Build: jobs: - NodeJS: @@ -143,6 +142,7 @@ workflows: - "14" - "16" - "18" + - "20" - "Check for FIXM\x45" - Prettier - ESLint diff --git a/packages/integration-testsuite/src/httpServerTests.ts b/packages/integration-testsuite/src/httpServerTests.ts index 789bf54b429..a431901c6c2 100644 --- a/packages/integration-testsuite/src/httpServerTests.ts +++ b/packages/integration-testsuite/src/httpServerTests.ts @@ -382,7 +382,7 @@ export function defineIntegrationTestSuiteHttpServerTests( return req.then((res) => { expect(res.status).toEqual(400); expect( - ['Unexpected token f', 'Bad Request', 'Invalid JSON'].some( + ['in JSON at position 1', 'Bad Request', 'Invalid JSON'].some( (substring) => (res.error as HTTPError).text.includes(substring), ), ).toBe(true); diff --git a/packages/server/src/__tests__/express4/expressSpecific.test.ts b/packages/server/src/__tests__/express4/expressSpecific.test.ts index 4a4eba63c21..fc80e4c2284 100644 --- a/packages/server/src/__tests__/express4/expressSpecific.test.ts +++ b/packages/server/src/__tests__/express4/expressSpecific.test.ts @@ -193,7 +193,7 @@ it('supporting doubly-encoded variables example from migration guide', async () query: 'query Hello($s: String!){hello(s: $s)}', variables: '{malformed JSON}', }) - .expect(400, 'Unexpected token m in JSON at position 1'); + .expect(400, /in JSON at position 1/); await server.stop(); }); diff --git a/packages/server/src/__tests__/plugin/drainHttpServer/stoppable.test.ts b/packages/server/src/__tests__/plugin/drainHttpServer/stoppable.test.ts index b44c56af7a6..95259bb981b 100644 --- a/packages/server/src/__tests__/plugin/drainHttpServer/stoppable.test.ts +++ b/packages/server/src/__tests__/plugin/drainHttpServer/stoppable.test.ts @@ -32,7 +32,6 @@ const a: any = require('awaiting'); const request: any = require('requisition'); import fs from 'fs'; import { Stopper } from '../../../plugin/drainHttpServer/stoppable'; -import child from 'child_process'; import path from 'path'; import type { AddressInfo } from 'net'; import { describe, it, expect, afterEach, beforeEach } from '@jest/globals'; @@ -113,7 +112,7 @@ Object.keys(schemes).forEach((schemeName) => { const err = await a.failure( request(`${schemeName}://localhost:${p}`).agent(scheme.agent()), ); - expect(err.message).toMatch(/ECONNREFUSED/); + expect(err.code).toMatch(/ECONNREFUSED/); expect(closed).toBe(1); }); @@ -135,9 +134,54 @@ Object.keys(schemes).forEach((schemeName) => { scheme.agent({ keepAlive: true }), ), ); - expect(err.message).toMatch(/ECONNREFUSED/); - expect(closed).toBe(0); + expect(err.code).toMatch(/ECONNREFUSED/); + + // Node 19 (http) and 20.4+ (https) more aggressively close idle + // connections. `Stopper` is no longer needed for the purpose of closing + // idle connections in these versions. However, `Stopper` _is_ still + // useful for gracefully finishing in-flight requests within the timeout + // (and aborting requests beyond the timeout). + const isNode20 = !!process.version.match(/^v20\./); + expect(closed).toBe(isNode20 ? 1 : 0); }); + + // This test specifically added for Node 20 fails for Node 14. Just going + // to skip it since we're dropping Node 14 soon anyway. + const node14 = !!process.version.match(/^v14\./); + (node14 ? it.skip : it)( + 'with unfinished requests', + async () => { + const server = scheme.server(async (_req, res) => { + res.writeHead(200); + res.write('hi'); // note lack of end()! + }); + // The server will prevent itself from closing while the connection + // remains open (default no timeout). This will close the connection + // after 100ms so the test can finish. + server.setTimeout(100); + + server.listen(0); + const p = port(server); + + const response = await request( + `${schemeName}://localhost:${p}`, + ).agent(scheme.agent({ keepAlive: true })); + // ensure we got the headers, etc. + expect(response.status).toBe(200); + + server.close(); + await a.event(server, 'close'); + + try { + await response.text(); + } catch (e: any) { + expect(e.code).toMatch(/ECONNRESET/); + } + // ensure the expectation in the catch block is reached (+ the one above) + expect.assertions(2); + }, + 35000, + ); }); describe('Stopper', function () { @@ -159,7 +203,7 @@ Object.keys(schemes).forEach((schemeName) => { const err = await a.failure( request(`${schemeName}://localhost:${p}`).agent(scheme.agent()), ); - expect(err.message).toMatch(/ECONNREFUSED/); + expect(err.code).toMatch(/ECONNREFUSED/); expect(closed).toBe(1); expect(gracefully).toBe(true); @@ -185,7 +229,7 @@ Object.keys(schemes).forEach((schemeName) => { scheme.agent({ keepAlive: true }), ), ); - expect(err.message).toMatch(/ECONNREFUSED/); + expect(err.code).toMatch(/ECONNREFUSED/); expect(closed).toBe(1); expect(gracefully).toBe(true); @@ -301,12 +345,21 @@ Object.keys(schemes).forEach((schemeName) => { if (schemeName === 'http') { it('with in-flights finishing before grace period ends', async () => { - const file = path.join(__dirname, 'stoppable', 'server.js'); - const server = child.spawn('node', [file]); - const [data] = await a.event(server.stdout, 'data'); - const port = +data.toString(); - expect(typeof port).toBe('number'); - const res = await request(`${schemeName}://localhost:${port}/`).agent( + let stopper: Stopper; + const killServerBarrier = resolvable(); + const server = http.createServer(async (_, res) => { + res.writeHead(200); + res.write('hello'); + + await killServerBarrier; + res.end('world'); + await stopper.stop(); + }); + stopper = new Stopper(server); + server.listen(0); + const p = port(server); + + const res = await request(`${schemeName}://localhost:${p}/`).agent( scheme.agent({ keepAlive: true }), ); let gotBody = false; @@ -320,13 +373,13 @@ Object.keys(schemes).forEach((schemeName) => { expect(gotBody).toBe(false); // Tell the server that its request should finish. - server.kill('SIGUSR1'); + killServerBarrier.resolve(); const body = await bodyPromise; expect(gotBody).toBe(true); expect(body).toBe('helloworld'); - // Wait for subprocess to go away. + // Wait for server to close. await a.event(server, 'close'); }); } diff --git a/packages/server/src/__tests__/plugin/drainHttpServer/stoppable/server.js b/packages/server/src/__tests__/plugin/drainHttpServer/stoppable/server.js deleted file mode 100644 index 7943953dede..00000000000 --- a/packages/server/src/__tests__/plugin/drainHttpServer/stoppable/server.js +++ /dev/null @@ -1,17 +0,0 @@ -// This server is run by a test in stoppable.test.js. Its HTTP server should -// only ever get one request. It will respond with a 200 and start writing its -// body and then start the Stopper process with no hard-destroy grace period. It -// will finish the request on SIGUSR1. - -import http from 'http'; -import { Stopper } from '../../../../../dist/esm/plugin/drainHttpServer/stoppable.js'; - -let stopper; -const server = http.createServer((req, res) => { - res.writeHead(200); - res.write('hello'); - process.on('SIGUSR1', () => res.end('world')); - stopper.stop(); -}); -stopper = new Stopper(server); -server.listen(0, () => console.log(server.address().port));