Merge pull request #21295 from overleaf/jpa-fix-filestore-tests

[filestore] fix conditional tests following sharing changes

GitOrigin-RevId: 7d804acb14942fb6d06ee38f782fc78796e9182e

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 () {
         })
       })
     })
-  })
+  }
 })

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) {