From e56e5442fdc4e6d91a03358d468a172a3ece4dc7 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 24 Oct 2024 13:42:20 +0200 Subject: [PATCH] Merge pull request #21295 from overleaf/jpa-fix-filestore-tests [filestore] fix conditional tests following sharing changes GitOrigin-RevId: 7d804acb14942fb6d06ee38f782fc78796e9182e --- .../test/acceptance/js/FilestoreTests.js | 59 +++++++++++++------ .../test/acceptance/js/TestHelper.js | 10 +++- 2 files changed, 49 insertions(+), 20 deletions(-) diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index 441a179c59..f363456d6d 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/services/filestore/test/acceptance/js/FilestoreTests.js @@ -77,8 +77,10 @@ describe('Filestore', function () { } // redefine the test suite for every available backend - Object.keys(BackendSettings).forEach(backend => { - describe(backend, function () { + for (const [backendVariantWithShardNumber, backendSettings] of Object.entries( + BackendSettings + )) { + describe(backendVariantWithShardNumber, function () { let app, previousEgress, previousIngress, @@ -95,12 +97,12 @@ describe('Filestore', function () { before(async function () { // create the app with the relevant filestore settings - Settings.filestore = BackendSettings[backend] + Settings.filestore = backendSettings app = new FilestoreApp() await app.runServer() }) - if (BackendSettings[backend].gcs) { + if (backendSettings.gcs) { before(async function () { // create test buckets for gcs const storage = new Storage(Settings.filestore.gcs.endpoint) @@ -164,7 +166,8 @@ describe('Filestore', function () { await fsWriteFile(localFileReadPath, constantFileContent) const readStream = fs.createReadStream(localFileReadPath) - await fetch(fileUrl, { method: 'POST', body: readStream }) + const res = await fetch(fileUrl, { method: 'POST', body: readStream }) + if (!res.ok) throw new Error(res.statusText) }) beforeEach(async function retrievePreviousIngressMetrics() { @@ -212,7 +215,9 @@ describe('Filestore', function () { }) it('should not leak a socket', async function () { - await fetch(fileUrl) + const res = await fetch(fileUrl) + if (!res.ok) throw new Error(res.statusText) + await res.text() await expectNoSockets() }) @@ -269,7 +274,7 @@ describe('Filestore', function () { expect(body).to.equal(newContent) }) - if (['S3Persistor', 'GcsPersistor'].includes(backend)) { + if (backendSettings.backend !== 'fs') { it('should record an egress metric for the upload', async function () { const metric = await TestHelper.getMetric( filestoreUrl, @@ -281,6 +286,7 @@ describe('Filestore', function () { it('should record an ingress metric when downloading the file', async function () { const response = await fetch(fileUrl) expect(response.ok).to.be.true + await response.text() const metric = await TestHelper.getMetric( filestoreUrl, `${metricPrefix}_ingress` @@ -295,6 +301,7 @@ describe('Filestore', function () { headers: { Range: 'bytes=0-8' }, }) expect(response.ok).to.be.true + await response.text() const metric = await TestHelper.getMetric( filestoreUrl, `${metricPrefix}_ingress` @@ -421,7 +428,8 @@ describe('Filestore', function () { largeFileContent += Math.random() const readStream = streamifier.createReadStream(largeFileContent) - await fetch(fileUrl, { method: 'POST', body: readStream }) + const res = await fetch(fileUrl, { method: 'POST', body: readStream }) + if (!res.ok) throw new Error(res.statusText) }) it('should be able to get the file back', async function () { @@ -449,7 +457,11 @@ describe('Filestore', function () { }) }) - if (backend === 'S3Persistor' || backend === 'FallbackGcsToS3Persistor') { + if ( + (backendSettings.backend === 's3' && !backendSettings.fallback) || + (backendSettings.backend === 'gcs' && + backendSettings.fallback?.backend === 's3') + ) { describe('with a file in a specific bucket', function () { let constantFileContent, fileId, fileUrl, bucketName @@ -492,7 +504,7 @@ describe('Filestore', function () { }) } - if (backend === 'GcsPersistor') { + if (backendSettings.backend === 'gcs') { describe('when deleting a file in GCS', function () { let fileId, fileUrl, content, error, date @@ -505,8 +517,10 @@ describe('Filestore', function () { content = '_wombat_' + Math.random() const readStream = streamifier.createReadStream(content) - await fetch(fileUrl, { method: 'POST', body: readStream }) - await fetch(fileUrl, { method: 'DELETE' }) + let res = await fetch(fileUrl, { method: 'POST', body: readStream }) + if (!res.ok) throw new Error(res.statusText) + res = await fetch(fileUrl, { method: 'DELETE' }) + if (!res.ok) throw new Error(res.statusText) }) afterEach(function () { @@ -536,7 +550,7 @@ describe('Filestore', function () { }) } - if (BackendSettings[backend].fallback) { + if (backendSettings.fallback) { describe('with a fallback', function () { let constantFileContent, fileId, @@ -596,6 +610,7 @@ describe('Filestore', function () { it('should not copy the file to the primary', async function () { const response = await fetch(fileUrl) expect(response.ok).to.be.true + await response.text() await TestHelper.expectPersistorNotToHaveFile( app.persistor.primaryPersistor, @@ -619,6 +634,7 @@ describe('Filestore', function () { it('copies the file to the primary', async function () { const response = await fetch(fileUrl) expect(response.ok).to.be.true + await response.text() // wait for the file to copy in the background await msleep(1000) @@ -753,7 +769,11 @@ describe('Filestore', function () { beforeEach(async function () { const readStream = streamifier.createReadStream(constantFileContent) - await fetch(fileUrl, { method: 'POST', body: readStream }) + const res = await fetch(fileUrl, { + method: 'POST', + body: readStream, + }) + if (!res.ok) throw new Error(res.statusText) }) it('should store the file on the primary', async function () { @@ -860,7 +880,8 @@ describe('Filestore', function () { const stat = await fsStat(localFileReadPath) localFileSize = stat.size const readStream = fs.createReadStream(localFileReadPath) - await fetch(fileUrl, { method: 'POST', body: readStream }) + const res = await fetch(fileUrl, { method: 'POST', body: readStream }) + if (!res.ok) throw new Error(res.statusText) }) it('should be able get the file back', async function () { @@ -869,7 +890,7 @@ describe('Filestore', function () { expect(body.substring(0, 8)).to.equal('%PDF-1.5') }) - if (['S3Persistor', 'GcsPersistor'].includes(backend)) { + if (backendSettings.backend !== 'fs') { it('should record an egress metric for the upload', async function () { const metric = await TestHelper.getMetric( filestoreUrl, @@ -890,6 +911,7 @@ describe('Filestore', function () { it('should not time out', async function () { const response = await fetch(previewFileUrl) expect(response.status).to.equal(200) + await response.arrayBuffer() }) it('should respond with image data', async function () { @@ -912,13 +934,16 @@ describe('Filestore', function () { it('should not time out', async function () { const response = await fetch(previewFileUrl) expect(response.status).to.equal(200) + await response.arrayBuffer() }) it('should not leak sockets', async function () { const response1 = await fetch(previewFileUrl) expect(response1.status).to.equal(200) + // do not read the response body, should be destroyed immediately const response2 = await fetch(previewFileUrl) expect(response2.status).to.equal(200) + // do not read the response body, should be destroyed immediately await expectNoSockets() }) @@ -931,5 +956,5 @@ describe('Filestore', function () { }) }) }) - }) + } }) diff --git a/services/filestore/test/acceptance/js/TestHelper.js b/services/filestore/test/acceptance/js/TestHelper.js index 1b7eae4d0a..2edeb80d27 100644 --- a/services/filestore/test/acceptance/js/TestHelper.js +++ b/services/filestore/test/acceptance/js/TestHelper.js @@ -15,10 +15,14 @@ module.exports = { async function getMetric(filestoreUrl, metric) { const res = await fetch(`${filestoreUrl}/metrics`) expect(res.status).to.equal(200) - const metricRegex = new RegExp(`^${metric}{[^}]+} ([0-9]+)$`, 'm') + const metricRegex = new RegExp(`^${metric}{[^}]+} ([0-9]+)$`, 'gm') const body = await res.text() - const found = metricRegex.exec(body) - return parseInt(found ? found[1] : 0) || 0 + let v = 0 + // Sum up size="lt-128KiB" and size="gte-128KiB" + for (const [, found] of body.matchAll(metricRegex)) { + v += parseInt(found, 10) || 0 + } + return v } function streamToString(stream) {