diff --git a/backend/tests/integration/uploadValidation.integration.test.ts b/backend/tests/integration/uploadValidation.integration.test.ts index e54af36..5f57a80 100644 --- a/backend/tests/integration/uploadValidation.integration.test.ts +++ b/backend/tests/integration/uploadValidation.integration.test.ts @@ -35,6 +35,51 @@ async function storedFiles(): Promise { return fs.readdir(UPLOADS_DIR); } +const tick = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Waits for the uploads directory to reach the expected state, then returns it. + * + * `discardUnlessAccepted` unlinks from a `res.on('close')` handler, so nothing + * awaits it and nothing can — it hangs off a response event. `await request()` + * resolves when the response completes, which is when `close` fires, so a read + * taken straight afterwards races the unlink it is meant to observe. The + * property is an eventual one, so asserting it has to be eventual too. See #228. + * + * The final read is returned rather than a boolean, so the caller still asserts + * on real contents and a failure names the files that were actually there. + */ +async function filesSettlingTo(matches: (files: string[]) => boolean): Promise { + const deadline = Date.now() + 3000; + let files = await storedFiles(); + + while (!matches(files) && Date.now() < deadline) { + await tick(25); + files = await storedFiles(); + } + return files; +} + +/** + * Empties the uploads directory before each test in this file. + * + * The race runs in both directions, and this is the half that is easy to miss: + * a deletion still pending from the *previous* test corrupts the next test's + * baseline before its request is even sent. Polling cannot fix that — the + * baseline is already wrong — and waiting for the directory to look quiet only + * works while the unlink is faster than the wait, which is precisely the + * assumption #228 is about. + * + * Starting from empty removes the baseline as a variable entirely. Any orphan + * left by an earlier suite goes with it, which is correct: this directory is a + * temporary one and nothing outside these tests owns its contents. + */ +beforeEach(async () => { + for (const name of await storedFiles()) { + await fs.unlink(path.join(UPLOADS_DIR, name)).catch(() => undefined); + } +}); + function createItem() { return request(app) .post('/api/admin/items') @@ -106,27 +151,27 @@ describe('what it refuses', () => { }); it('leaves nothing on the volume when it refuses the content', async () => { - const before = await storedFiles(); + const before: string[] = []; await createItem().attach('images', HTML_BYTES, { filename: 'photo.jpg', contentType: 'image/jpeg' }); - expect(await storedFiles()).toHaveLength(before.length); + expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]); }); // A rejection must take the whole request with it. Accepting the good file // from a refused upload would leave a file behind that nothing references. it('discards the valid files from a request that also carried an invalid one', async () => { - const before = await storedFiles(); + const before: string[] = []; const res = await createItem() .attach('images', REAL_PNG, { filename: 'good.png', contentType: 'image/png' }) .attach('images', HTML_BYTES, { filename: 'bad.jpg', contentType: 'image/jpeg' }); expect(res.status).toBe(400); - expect(await storedFiles()).toHaveLength(before.length); + expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]); }); it('does not create the item when the upload is refused', async () => { @@ -164,7 +209,7 @@ describe('the uploads directory', () => { */ describe('what it leaves on disk', () => { it('removes the upload when the request is refused for its other fields', async () => { - const before = await storedFiles(); + const before: string[] = []; const res = await request(app) .post('/api/admin/items') @@ -177,11 +222,11 @@ describe('what it leaves on disk', () => { .attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' }); expect(res.status).toBe(400); - expect(await storedFiles()).toEqual(before); + expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]); }); it('removes the upload when the tags field is refused', async () => { - const before = await storedFiles(); + const before: string[] = []; const res = await request(app) .post('/api/admin/items') @@ -192,13 +237,13 @@ describe('what it leaves on disk', () => { .attach('images', REAL_PNG, { filename: 'photo.png', contentType: 'image/png' }); expect(res.status).toBe(400); - expect(await storedFiles()).toEqual(before); + expect(await filesSettlingTo((files) => files.length === 0)).toEqual([]); }); // The accepted case must not be swept up by the same cleanup: these files are // the ones the item now points at. it('keeps the upload when the request succeeds', async () => { - const before = await storedFiles(); + const before: string[] = []; const res = await createItem().attach('images', REAL_PNG, { filename: 'photo.png', @@ -206,6 +251,7 @@ describe('what it leaves on disk', () => { }); expect(res.status).toBe(200); - expect((await storedFiles()).length).toBe(before.length + 1); + // No unlink is scheduled for an accepted request, so this settles at once. + expect(await filesSettlingTo((files) => files.length === 1)).toHaveLength(1); }); });