From b0b65c7c256e97b4453fd5c4ea9f31f255c88594 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 11:38:49 +0000 Subject: [PATCH 1/8] test: Run node integration tests in isolation --- packages/node-integration-tests/package.json | 2 +- .../node-integration-tests/utils/run-tests.ts | 61 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 packages/node-integration-tests/utils/run-tests.ts diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index b6fb1dba74d9..3c0aa09cfaf0 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -17,7 +17,7 @@ "fix:prettier": "prettier --write \"{suites,utils}/**/*.ts\"", "type-check": "tsc", "pretest": "run-s --silent prisma:init", - "test": "jest --forceExit", + "test": "ts-node ./utils/run-tests.ts", "test:watch": "yarn test --watch" }, "dependencies": { diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts new file mode 100644 index 000000000000..7bd8f290253a --- /dev/null +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -0,0 +1,61 @@ +/* eslint-disable no-console */ +import childProcess from 'child_process'; +import os from 'os'; + +const testPaths = childProcess.execSync('jest --listTests', { encoding: 'utf8' }).trim().split('\n'); + +let testsSucceeded = true; +const testAmount = testPaths.length; +const fails: string[] = []; + +const threads = os.cpus().map(async (_, i) => { + let testPath = testPaths.pop(); + while (testPath !== undefined) { + console.log(`(Worker ${i}) Running test "${testPath}"`); + await new Promise(resolve => { + const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); + + let output = ''; + + p.stdout.on('data', (data: Buffer) => { + output = output + data.toString(); + }); + + p.stderr.on('data', (data: Buffer) => { + output = output + data.toString(); + }); + + p.on('error', error => { + console.log(`(Worker ${i}) Error in test "${testPath}"`, error); + console.log(output); + resolve(); + }); + + p.on('exit', exitcode => { + console.log(`(Worker ${i}) Finished test "${testPath}"`); + console.log(output); + if (exitcode !== 0) { + fails.push(`FAILED: "${testPath}"`); + testsSucceeded = false; + } + resolve(); + }); + }); + testPath = testPaths.pop(); + } +}); + +void Promise.all(threads).then(() => { + console.log('-------------------'); + console.log(`Successfully ran ${testAmount} tests.`); + if (!testsSucceeded) { + console.log('Not all tests succeeded:\n'); + fails.forEach(fail => { + console.log(`● ${fail}`); + }); + process.exit(1); + } else { + console.log('All testcases returned the expected results.'); + process.exit(0); + } +}); From e4b763108b3d1caf9197009c01815fe1802a4a79 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 12:05:44 +0000 Subject: [PATCH 2/8] Avoid port race condition --- packages/node-integration-tests/utils/index.ts | 13 +++++++++++-- packages/node-integration-tests/utils/run-tests.ts | 6 +++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index ad1bb0d4b188..7baba995b03d 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -7,7 +7,7 @@ import { Express } from 'express'; import * as http from 'http'; import nock from 'nock'; import * as path from 'path'; -import { getPortPromise } from 'portfinder'; +import { getPorts } from 'portfinder'; export type TestServerConfig = { url: string; @@ -151,7 +151,16 @@ export class TestEnv { } }); - void getPortPromise().then(port => { + getPorts(50, {}, (err, ports) => { + if (err) { + throw err; + } + + const port = ports.find( + // Only allow ports that do not overlap with other workers - this is done to avoid race-conditions + p => p % Number(process.env.TEST_WORKERS_AMOUNT) === Number(process.env.TEST_PORT_MODULO), + ); + const url = `http://localhost:${port}/test`; const server = app.listen(port, () => { resolve([server, url]); diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index 7bd8f290253a..61c7c82a8c6c 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -13,7 +13,11 @@ const threads = os.cpus().map(async (_, i) => { while (testPath !== undefined) { console.log(`(Worker ${i}) Running test "${testPath}"`); await new Promise(resolve => { - const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); + const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit'], { + // These env vars are used to give workers a limited, non-overlapping set of ports to occupy. + // This is done to avoid race-conditions during port reservation. + env: { ...process.env, TEST_WORKERS_AMOUNT: String(os.cpus().length), TEST_PORT_MODULO: String(i) }, + }); let output = ''; From b86bdb73ab772bbb53e90279999eeed3877c9799 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 12:11:47 +0000 Subject: [PATCH 3/8] Better log message --- packages/node-integration-tests/utils/run-tests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index 61c7c82a8c6c..aa5eff61ec42 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -59,7 +59,7 @@ void Promise.all(threads).then(() => { }); process.exit(1); } else { - console.log('All testcases returned the expected results.'); + console.log('All tests succeeded.'); process.exit(0); } }); From dd5fe3fd8e9a817cdf2335b248dd66a73aa10030 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 12:57:11 +0000 Subject: [PATCH 4/8] Avoid port race condition (finally pls) --- packages/node-integration-tests/package.json | 3 +-- .../node-integration-tests/utils/index.ts | 19 ++++--------------- .../node-integration-tests/utils/run-tests.ts | 6 +----- packages/remix/package.json | 3 ++- 4 files changed, 8 insertions(+), 23 deletions(-) diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index 3c0aa09cfaf0..d966b9cbbdf0 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -34,8 +34,7 @@ "mongodb-memory-server-global": "^7.6.3", "mysql": "^2.18.1", "nock": "^13.1.0", - "pg": "^8.7.3", - "portfinder": "^1.0.28" + "pg": "^8.7.3" }, "config": { "mongodbMemoryServer": { diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts index 7baba995b03d..153cbcb794d2 100644 --- a/packages/node-integration-tests/utils/index.ts +++ b/packages/node-integration-tests/utils/index.ts @@ -5,9 +5,9 @@ import { logger, parseSemver } from '@sentry/utils'; import axios, { AxiosRequestConfig } from 'axios'; import { Express } from 'express'; import * as http from 'http'; +import { AddressInfo } from 'net'; import nock from 'nock'; import * as path from 'path'; -import { getPorts } from 'portfinder'; export type TestServerConfig = { url: string; @@ -151,20 +151,9 @@ export class TestEnv { } }); - getPorts(50, {}, (err, ports) => { - if (err) { - throw err; - } - - const port = ports.find( - // Only allow ports that do not overlap with other workers - this is done to avoid race-conditions - p => p % Number(process.env.TEST_WORKERS_AMOUNT) === Number(process.env.TEST_PORT_MODULO), - ); - - const url = `http://localhost:${port}/test`; - const server = app.listen(port, () => { - resolve([server, url]); - }); + const server = app.listen(0, () => { + const url = `http://localhost:${(server.address() as AddressInfo).port}/test`; + resolve([server, url]); }); }); diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index aa5eff61ec42..047dc59b6080 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -13,11 +13,7 @@ const threads = os.cpus().map(async (_, i) => { while (testPath !== undefined) { console.log(`(Worker ${i}) Running test "${testPath}"`); await new Promise(resolve => { - const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit'], { - // These env vars are used to give workers a limited, non-overlapping set of ports to occupy. - // This is done to avoid race-conditions during port reservation. - env: { ...process.env, TEST_WORKERS_AMOUNT: String(os.cpus().length), TEST_PORT_MODULO: String(i) }, - }); + const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); let output = ''; diff --git a/packages/remix/package.json b/packages/remix/package.json index eb92b97feeb7..85b3e3ed6ab7 100644 --- a/packages/remix/package.json +++ b/packages/remix/package.json @@ -34,7 +34,8 @@ }, "devDependencies": { "@remix-run/node": "^1.4.3", - "@remix-run/react": "^1.4.3" + "@remix-run/react": "^1.4.3", + "portfinder": "^1.0.28" }, "peerDependencies": { "@remix-run/node": "1.x", From 8028d9e280bcbf07f81d4ef0a5c6a6edcd3407f2 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 13:25:38 +0000 Subject: [PATCH 5/8] Small readability improvements --- .../node-integration-tests/utils/run-tests.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index 047dc59b6080..bb1f596f89b9 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -9,29 +9,29 @@ const testAmount = testPaths.length; const fails: string[] = []; const threads = os.cpus().map(async (_, i) => { - let testPath = testPaths.pop(); - while (testPath !== undefined) { + while (testPaths.length > 0) { + const testPath = testPaths.pop(); console.log(`(Worker ${i}) Running test "${testPath}"`); await new Promise(resolve => { - const p = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); + const jestProcess = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); let output = ''; - p.stdout.on('data', (data: Buffer) => { + jestProcess.stdout.on('data', (data: Buffer) => { output = output + data.toString(); }); - p.stderr.on('data', (data: Buffer) => { + jestProcess.stderr.on('data', (data: Buffer) => { output = output + data.toString(); }); - p.on('error', error => { + jestProcess.on('error', error => { console.log(`(Worker ${i}) Error in test "${testPath}"`, error); console.log(output); resolve(); }); - p.on('exit', exitcode => { + jestProcess.on('exit', exitcode => { console.log(`(Worker ${i}) Finished test "${testPath}"`); console.log(output); if (exitcode !== 0) { @@ -41,7 +41,6 @@ const threads = os.cpus().map(async (_, i) => { resolve(); }); }); - testPath = testPaths.pop(); } }); From d697937f1b6d6b2eb2ec507e245cb7910d48c05f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Sep 2022 14:27:20 +0000 Subject: [PATCH 6/8] s/testAmount/numTests/ --- packages/node-integration-tests/utils/run-tests.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index bb1f596f89b9..41264e654a31 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -5,7 +5,7 @@ import os from 'os'; const testPaths = childProcess.execSync('jest --listTests', { encoding: 'utf8' }).trim().split('\n'); let testsSucceeded = true; -const testAmount = testPaths.length; +const numTests = testPaths.length; const fails: string[] = []; const threads = os.cpus().map(async (_, i) => { @@ -46,7 +46,7 @@ const threads = os.cpus().map(async (_, i) => { void Promise.all(threads).then(() => { console.log('-------------------'); - console.log(`Successfully ran ${testAmount} tests.`); + console.log(`Successfully ran ${numTests} tests.`); if (!testsSucceeded) { console.log('Not all tests succeeded:\n'); fails.forEach(fail => { From cc3814591cf33b8f46c84590db519188e196aa89 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 07:51:12 +0000 Subject: [PATCH 7/8] Apply review suggestions --- .../node-integration-tests/utils/run-tests.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index 41264e654a31..fde4d155ee7a 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -2,19 +2,22 @@ import childProcess from 'child_process'; import os from 'os'; +// This variable will act as a job queue that is consumed by a number of worker threads. Each item represents a test to run. const testPaths = childProcess.execSync('jest --listTests', { encoding: 'utf8' }).trim().split('\n'); -let testsSucceeded = true; const numTests = testPaths.length; const fails: string[] = []; -const threads = os.cpus().map(async (_, i) => { +// We're creating a worker for each CPU core. +const workers = os.cpus().map(async (_, i) => { while (testPaths.length > 0) { const testPath = testPaths.pop(); console.log(`(Worker ${i}) Running test "${testPath}"`); await new Promise(resolve => { const jestProcess = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); + // We're collecting the output and logging it all at once instead of inheriting stdout and stderr, so that the + // test outputs aren't interwoven. let output = ''; jestProcess.stdout.on('data', (data: Buffer) => { @@ -26,8 +29,9 @@ const threads = os.cpus().map(async (_, i) => { }); jestProcess.on('error', error => { - console.log(`(Worker ${i}) Error in test "${testPath}"`, error); console.log(output); + console.log(`(Worker ${i}) Error in test "${testPath}"`, error); + fails.push(`FAILED: "${testPath}"`); resolve(); }); @@ -36,7 +40,6 @@ const threads = os.cpus().map(async (_, i) => { console.log(output); if (exitcode !== 0) { fails.push(`FAILED: "${testPath}"`); - testsSucceeded = false; } resolve(); }); @@ -44,10 +47,10 @@ const threads = os.cpus().map(async (_, i) => { } }); -void Promise.all(threads).then(() => { +void Promise.all(workers).then(() => { console.log('-------------------'); console.log(`Successfully ran ${numTests} tests.`); - if (!testsSucceeded) { + if (fails.length > 0) { console.log('Not all tests succeeded:\n'); fails.forEach(fail => { console.log(`● ${fail}`); From 4aee3fd53abc8138c4382fd302e8ce61230bac6c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Sep 2022 07:52:11 +0000 Subject: [PATCH 8/8] Clarify comment --- packages/node-integration-tests/utils/run-tests.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node-integration-tests/utils/run-tests.ts b/packages/node-integration-tests/utils/run-tests.ts index fde4d155ee7a..f29aa1a2399b 100644 --- a/packages/node-integration-tests/utils/run-tests.ts +++ b/packages/node-integration-tests/utils/run-tests.ts @@ -16,8 +16,8 @@ const workers = os.cpus().map(async (_, i) => { await new Promise(resolve => { const jestProcess = childProcess.spawn('jest', ['--runTestsByPath', testPath as string, '--forceExit']); - // We're collecting the output and logging it all at once instead of inheriting stdout and stderr, so that the - // test outputs aren't interwoven. + // We're collecting the output and logging it all at once instead of inheriting stdout and stderr, so that + // test outputs of the individual workers aren't interwoven, in case they print at the same time. let output = ''; jestProcess.stdout.on('data', (data: Buffer) => {