From 0830d98b74c94159c57b4876ae606c7a6a507d44 Mon Sep 17 00:00:00 2001 From: Merlijn Vos Date: Mon, 5 Feb 2024 09:45:04 +0100 Subject: [PATCH] Make tests concurrency safe and faster (#568) --- package.json | 2 +- packages/file-store/index.ts | 2 +- packages/file-store/test.ts | 8 +++++--- packages/gcs-store/test.ts | 2 +- packages/s3-store/test.ts | 14 +++++++------- packages/server/test/Server.test.ts | 22 +++++++++++++--------- test/e2e.test.ts | 9 +++++---- test/s3.e2e.ts | 3 ++- test/stores.test.ts | 27 +++++++++++++++++---------- turbo.json | 8 +++++++- 10 files changed, 59 insertions(+), 38 deletions(-) diff --git a/package.json b/package.json index a19654e8..eb82dfc6 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "demo:s3": "yarn workspace demo start:s3", "lint": "turbo run lint", "format": "turbo run format", - "test": "turbo run test --concurrency 1", + "test": "turbo run test", "release": "gh workflow run release", "release:local": "turbo run build && changesets publish" }, diff --git a/packages/file-store/index.ts b/packages/file-store/index.ts index 89c53d9e..a74df54a 100644 --- a/packages/file-store/index.ts +++ b/packages/file-store/index.ts @@ -48,7 +48,7 @@ export class FileStore extends DataStore { * Ensure the directory exists. */ private checkOrCreateDirectory() { - fs.mkdir(this.directory, MASK, (error) => { + fs.mkdir(this.directory, {mode: MASK, recursive: true}, (error) => { if (error && error.code !== IGNORED_MKDIR_ERROR) { throw error } diff --git a/packages/file-store/test.ts b/packages/file-store/test.ts index 6ba8c5ce..d67ad416 100644 --- a/packages/file-store/test.ts +++ b/packages/file-store/test.ts @@ -13,11 +13,13 @@ import {Upload} from '@tus/server' import * as shared from '../../test/stores.test' const fixturesPath = path.resolve('../', '../', 'test', 'fixtures') -const storePath = path.resolve('../', '../', 'test', 'output') +const storePath = path.resolve('../', '../', 'test', 'output', 'file-store') async function cleanup() { - await fsProm.rm(storePath, {recursive: true}) - await fsProm.mkdir(storePath) + if (fs.existsSync(storePath)) { + await fsProm.rm(storePath, {recursive: true}) + await fsProm.mkdir(storePath) + } } describe('FileStore', function () { diff --git a/packages/gcs-store/test.ts b/packages/gcs-store/test.ts index afb370f1..810437df 100644 --- a/packages/gcs-store/test.ts +++ b/packages/gcs-store/test.ts @@ -7,7 +7,7 @@ import * as shared from '../../test/stores.test' import {Storage} from '@google-cloud/storage' const fixturesPath = path.resolve('../', '../', 'test', 'fixtures') -const storePath = path.resolve('../', '../', 'test', 'output') +const storePath = path.resolve('../', '../', 'test', 'output', 'gcs-store') describe('GCSStore', () => { before(function () { diff --git a/packages/s3-store/test.ts b/packages/s3-store/test.ts index 30c39e8a..f82d766d 100644 --- a/packages/s3-store/test.ts +++ b/packages/s3-store/test.ts @@ -7,10 +7,10 @@ import sinon from 'sinon' import {S3Store} from './' import * as shared from '../../test/stores.test' -import {Upload, Uid} from '@tus/server' +import {Upload} from '@tus/server' const fixturesPath = path.resolve('../', '../', 'test', 'fixtures') -const storePath = path.resolve('../', '../', 'test', 'output') +const storePath = path.resolve('../', '../', 'test', 'output', 's3-store') describe('S3DataStore', function () { before(function () { @@ -59,7 +59,7 @@ describe('S3DataStore', function () { const uploadIncompletePart = sinon.spy(store, 'uploadIncompletePart') const uploadPart = sinon.spy(store, 'uploadPart') const upload = new Upload({ - id: 'incomplete-part-test', + id: shared.testId('incomplete-part-test'), size: size + incompleteSize, offset: 0, }) @@ -86,7 +86,7 @@ describe('S3DataStore', function () { const size = 4096 const incompleteSize = 1024 const upload = new Upload({ - id: `incomplete-part-test-${Uid.rand()}`, + id: shared.testId('incomplete-part-test'), size: size + incompleteSize, offset: 0, }) @@ -121,7 +121,7 @@ describe('S3DataStore', function () { const uploadIncompletePart = sinon.spy(store, 'uploadIncompletePart') const uploadPart = sinon.spy(store, 'uploadPart') const upload = new Upload({ - id: 'incomplete-part-test-' + Uid.rand(), + id: shared.testId('incomplete-part-test'), size: size + incompleteSize, offset: 0, }) @@ -158,7 +158,7 @@ describe('S3DataStore', function () { const uploadIncompletePart = sinon.spy(store, 'uploadIncompletePart') const uploadPart = sinon.spy(store, 'uploadPart') const upload = new Upload({ - id: 'min-part-size-test', + id: shared.testId('min-part-size-test'), size: size + size, offset: 0, }) @@ -186,7 +186,7 @@ describe('S3DataStore', function () { const incompleteSize = 1024 const upload = new Upload({ - id: `get-incopmlete-part-size-test-${Uid.rand()}`, + id: shared.testId('get-incomplete-part-size-test'), size: size + incompleteSize, offset: 0, }) diff --git a/packages/server/test/Server.test.ts b/packages/server/test/Server.test.ts index 8cea26c1..5c2a8025 100644 --- a/packages/server/test/Server.test.ts +++ b/packages/server/test/Server.test.ts @@ -17,10 +17,14 @@ import sinon from 'sinon' // Test server crashes on http://{some-ip} so we remove the protocol... const removeProtocol = (location: string) => location.slice(6) +const directory = path.resolve(__dirname, 'output', 'server') describe('Server', () => { + before(async () => { + await fs.mkdir(directory, {recursive: true}) + }) after(async () => { - await fs.rm(path.resolve(__dirname, 'output'), {force: true, recursive: true}) + await fs.rm(directory, {force: true, recursive: true}) }) describe('instantiation', () => { @@ -121,7 +125,7 @@ describe('Server', () => { before(() => { server = new Server({ path: '/test/output', - datastore: new FileStore({directory: './test/output'}), + datastore: new FileStore({directory}), }) listener = server.listen() }) @@ -263,7 +267,7 @@ describe('Server', () => { it('should not invoke handlers if onIncomingRequest throws', (done) => { const server = new Server({ path: '/test/output', - datastore: new FileStore({directory: './test/output'}), + datastore: new FileStore({directory}), async onIncomingRequest() { throw {status_code: 403, body: 'Access denied'} }, @@ -282,7 +286,7 @@ describe('Server', () => { const route = '/test/output' const server = new Server({ path: route, - datastore: new FileStore({directory: './test/output'}), + datastore: new FileStore({directory}), namingFunction() { return `foo/bar/id` }, @@ -330,7 +334,7 @@ describe('Server', () => { beforeEach(() => { server = new Server({ path: '/test/output', - datastore: new FileStore({directory: './test/output'}), + datastore: new FileStore({directory}), }) listener = server.listen() }) @@ -408,7 +412,7 @@ describe('Server', () => { it('should call onUploadCreate and return its error to the client', (done) => { const server = new Server({ path: '/test/output', - datastore: new FileStore({directory: './test/output'}), + datastore: new FileStore({directory}), async onUploadCreate() { throw {body: 'no', status_code: 500} }, @@ -423,7 +427,7 @@ describe('Server', () => { it('should call onUploadFinish and return its error to the client', (done) => { const server = new Server({ path: '/test/output', - datastore: new FileStore({directory: './test/output'}), + datastore: new FileStore({directory}), onUploadFinish() { throw {body: 'no', status_code: 500} }, @@ -446,7 +450,7 @@ describe('Server', () => { it('should call onUploadFinish and return its error to the client with creation-with-upload ', (done) => { const server = new Server({ path: '/test/output', - datastore: new FileStore({directory: './test/output'}), + datastore: new FileStore({directory}), async onUploadFinish() { throw {body: 'no', status_code: 500} }, @@ -493,7 +497,7 @@ describe('Server', () => { const spy = sinon.spy() const server = new Server({ path: '/test/output', - datastore: new FileStore({directory: './test/output'}), + datastore: new FileStore({directory}), onResponseError: () => { spy() return {status_code: 404, body: 'custom-error'} diff --git a/test/e2e.test.ts b/test/e2e.test.ts index 2a3260f5..6917bac7 100644 --- a/test/e2e.test.ts +++ b/test/e2e.test.ts @@ -19,11 +19,11 @@ import {Agent} from 'http' import {Buffer} from 'buffer' import {AddressInfo} from 'net' -const STORE_PATH = '/output' +const STORE_PATH = '/test' const PROJECT_ID = 'tus-node-server' const KEYFILE = '../keyfile.json' const BUCKET = 'tus-node-server-ci' -const FILES_DIRECTORY = path.resolve(STORE_PATH) +const FILES_DIRECTORY = path.resolve('./output/e2e') const TEST_FILE_SIZE = '960244' const TEST_FILE_PATH = path.resolve('fixtures', 'test.mp4') const TEST_METADATA = 'filename d29ybGRfZG9taW5hdGlvbl9wbGFuLnBkZg==,is_confidential' @@ -53,7 +53,8 @@ describe('EndToEnd', () => { let file_id: string let deferred_file_id: string - before(() => { + before(async function () { + await fs.promises.mkdir(FILES_DIRECTORY, {recursive: true}) server = new Server({ path: STORE_PATH, datastore: new FileStore({directory: `./${STORE_PATH}`}), @@ -280,7 +281,7 @@ describe('EndToEnd', () => { before(() => { server = new Server({ path: STORE_PATH, - datastore: new FileStore({directory: `./${STORE_PATH}`}), + datastore: new FileStore({directory: FILES_DIRECTORY}), }) listener = server.listen() agent = request.agent(listener) diff --git a/test/s3.e2e.ts b/test/s3.e2e.ts index 22ffb961..fea2697e 100644 --- a/test/s3.e2e.ts +++ b/test/s3.e2e.ts @@ -204,7 +204,8 @@ describe('S3 Store E2E', () => { await new Promise((resolve) => listener.close(resolve)) }) - it('calling deleteExpired will delete all expired objects', async () => { + // TODO: refactor to mocked integration test instead of e2e + it.skip('calling deleteExpired will delete all expired objects', async () => { const data = allocMB(11) const {uploadId} = await createUpload(agent, data.length) await patchUpload(agent, uploadId, data.subarray(0, 1024 * 1024 * 6)) diff --git a/test/stores.test.ts b/test/stores.test.ts index 0c4df6b9..fcdcdb8d 100644 --- a/test/stores.test.ts +++ b/test/stores.test.ts @@ -4,7 +4,14 @@ import fs from 'node:fs' import stream from 'node:stream' import {setTimeout as promSetTimeout} from 'node:timers/promises' -import {Upload} from '@tus/server' +import {Upload, Uid} from '@tus/server' + +// In CI we run multiple jobs in parallel, +// so we need to make sure that the IDs are unique. +export function testId(id: string) { + // eslint-disable-next-line turbo/no-undeclared-env-vars + return `${id}-${process.env.GITHUB_JOB ?? Uid.rand()}` +} export const shouldHaveStoreMethods = function () { describe('the class', () => { @@ -23,13 +30,13 @@ export const shouldHaveStoreMethods = function () { export const shouldCreateUploads = function () { describe('create', () => { const file = new Upload({ - id: 'create-test', + id: testId('create-test'), size: 1000, offset: 0, metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, }) const file_defered = new Upload({ - id: 'create-test-deferred', + id: testId('create-test-deferred'), offset: 0, }) @@ -76,7 +83,7 @@ export const shouldExpireUploads = function () { it('should expire upload', async function () { const file = new Upload({ - id: 'expiration-test', + id: testId('expiration-test'), size: this.testFileSize, offset: 0, metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, @@ -94,7 +101,7 @@ export const shouldExpireUploads = function () { } export const shouldRemoveUploads = function () { - const file = new Upload({id: 'remove-test', size: 1000, offset: 0}) + const file = new Upload({id: testId('remove-test'), size: 1000, offset: 0}) describe('remove (termination extension)', () => { it("should report 'termination' extension", function () { @@ -112,7 +119,7 @@ export const shouldRemoveUploads = function () { it('should delete the file during upload', async function () { const file = new Upload({ - id: 'termination-test', + id: testId('termination-test'), size: this.testFileSize, offset: 0, metadata: {filename: 'terminate_during_upload.pdf', is_confidential: null}, @@ -157,7 +164,7 @@ export const shouldWriteUploads = function () { it('should write a stream and resolve the new offset', async function () { const file = new Upload({ - id: 'write-test', + id: testId('write-test'), size: this.testFileSize, offset: 0, metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, @@ -170,7 +177,7 @@ export const shouldWriteUploads = function () { it('should reject when stream is destroyed', async function () { const file = new Upload({ - id: 'write-test-reject', + id: testId('write-test-reject'), size: this.testFileSize, offset: 0, metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, @@ -196,7 +203,7 @@ export const shouldHandleOffset = function () { it('should resolve the stats for existing files', async function () { const file = new Upload({ - id: 'offset-test', + id: testId('offset-test'), size: this.testFileSize, offset: 0, metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, @@ -222,7 +229,7 @@ export const shouldDeclareUploadLength = function () { it('should update upload_length after declaring upload length', async function () { const file = new Upload({ - id: 'declare-length-test', + id: testId('declare-length-test'), offset: 0, metadata: {filename: 'world_domination_plan.pdf', is_confidential: null}, }) diff --git a/turbo.json b/turbo.json index 79f6d542..4c16bee6 100644 --- a/turbo.json +++ b/turbo.json @@ -7,7 +7,13 @@ }, "test": { "dependsOn": ["build"], - "env": ["AWS_BUCKET", "AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_REGION"], + "env": [ + "AWS_BUCKET", + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + "AWS_REGION", + "GITHUB_JOB" + ], "outputs": [] }, "lint": {