From 29381cf0543adb1606e602e07611fbc5ab091200 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 24 Oct 2024 13:43:10 +0200 Subject: [PATCH] Merge pull request #21321 from overleaf/jpa-reconfigure-filestore [filestore] refactor acceptance tests GitOrigin-RevId: a6bb1527220b1c062d980c79d2ccb62973b99d2c --- package-lock.json | 16 ---- services/filestore/app/js/FileHandler.js | 8 +- services/filestore/package.json | 2 - .../test/acceptance/js/FilestoreApp.js | 73 ++++++------------- .../test/acceptance/js/FilestoreTests.js | 21 +++--- .../test/acceptance/js/TestHelper.js | 21 ++++++ .../test/unit/js/FileHandlerTests.js | 2 +- 7 files changed, 61 insertions(+), 82 deletions(-) diff --git a/package-lock.json b/package-lock.json index 146de00e0e..b25d5d670d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17185,12 +17185,6 @@ "nan": "^2.14.0" } }, - "node_modules/disrequire": { - "version": "1.1.2", - "resolved": "https://registry.npmjs.org/disrequire/-/disrequire-1.1.2.tgz", - "integrity": "sha512-UaachK0eL7neLyL2emXptVGyggGJKowJd24rqCZi9N2CDxuCQTk7wdePTgS7py4HMh+qxUI6zzTVinVwCDTbIA==", - "dev": true - }, "node_modules/dnd-core": { "version": "16.0.1", "resolved": "https://registry.npmjs.org/dnd-core/-/dnd-core-16.0.1.tgz", @@ -38451,14 +38445,12 @@ "aws-sdk": "^2.718.0", "chai": "^4.3.6", "chai-as-promised": "^7.1.1", - "disrequire": "^1.1.0", "mocha": "^10.2.0", "mongodb": "^6.1.0", "sandboxed-module": "2.0.4", "sinon": "9.0.2", "sinon-chai": "^3.7.0", "streamifier": "^0.1.1", - "timekeeper": "^2.2.0", "typescript": "^5.0.4" } }, @@ -49235,7 +49227,6 @@ "bunyan": "^1.8.15", "chai": "^4.3.6", "chai-as-promised": "^7.1.1", - "disrequire": "^1.1.0", "express": "^4.21.0", "glob": "^7.1.6", "lodash.once": "^4.1.1", @@ -49247,7 +49238,6 @@ "sinon": "9.0.2", "sinon-chai": "^3.7.0", "streamifier": "^0.1.1", - "timekeeper": "^2.2.0", "tiny-async-pool": "^1.1.0", "typescript": "^5.0.4" }, @@ -59089,12 +59079,6 @@ "nan": "^2.14.0" } }, - "disrequire": { - "version": "1.1.2", - "resolved": "https://registry.npmjs.org/disrequire/-/disrequire-1.1.2.tgz", - "integrity": "sha512-UaachK0eL7neLyL2emXptVGyggGJKowJd24rqCZi9N2CDxuCQTk7wdePTgS7py4HMh+qxUI6zzTVinVwCDTbIA==", - "dev": true - }, "dnd-core": { "version": "16.0.1", "resolved": "https://registry.npmjs.org/dnd-core/-/dnd-core-16.0.1.tgz", diff --git a/services/filestore/app/js/FileHandler.js b/services/filestore/app/js/FileHandler.js index 27be42fe7d..63928414b5 100644 --- a/services/filestore/app/js/FileHandler.js +++ b/services/filestore/app/js/FileHandler.js @@ -1,7 +1,7 @@ const Settings = require('@overleaf/settings') const { callbackify } = require('util') const fs = require('fs') -const PersistorManager = require('./PersistorManager') +let PersistorManager = require('./PersistorManager') const LocalFileWriter = require('./LocalFileWriter') const FileConverter = require('./FileConverter') const KeyBuilder = require('./KeyBuilder') @@ -30,6 +30,12 @@ module.exports = { }, } +if (process.env.NODE_ENV === 'test') { + module.exports._TESTONLYSwapPersistorManager = _PersistorManager => { + PersistorManager = _PersistorManager + } +} + async function copyObject(bucket, sourceKey, destinationKey) { await PersistorManager.copyObject(bucket, sourceKey, destinationKey) } diff --git a/services/filestore/package.json b/services/filestore/package.json index e680f54b2b..53afdc65db 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -39,14 +39,12 @@ "aws-sdk": "^2.718.0", "chai": "^4.3.6", "chai-as-promised": "^7.1.1", - "disrequire": "^1.1.0", "mocha": "^10.2.0", "mongodb": "^6.1.0", "sandboxed-module": "2.0.4", "sinon": "9.0.2", "sinon-chai": "^3.7.0", "streamifier": "^0.1.1", - "timekeeper": "^2.2.0", "typescript": "^5.0.4" } } diff --git a/services/filestore/test/acceptance/js/FilestoreApp.js b/services/filestore/test/acceptance/js/FilestoreApp.js index 924f852eea..dfd0e0ab44 100644 --- a/services/filestore/test/acceptance/js/FilestoreApp.js +++ b/services/filestore/test/acceptance/js/FilestoreApp.js @@ -1,46 +1,31 @@ const logger = require('@overleaf/logger') +const ObjectPersistor = require('@overleaf/object-persistor') const Settings = require('@overleaf/settings') -const fs = require('fs') -const Path = require('path') const { promisify } = require('util') -const disrequire = require('disrequire') const AWS = require('aws-sdk') +const App = require('../../../app') +const FileHandler = require('../../../app/js/FileHandler') logger.logger.level('info') -const fsReaddir = promisify(fs.readdir) const sleep = promisify(setTimeout) class FilestoreApp { - constructor() { - this.running = false - this.initing = false - } - async runServer() { - if (this.running) { - return - } - - if (this.initing) { - return await this.waitForInit() - } - this.initing = true - - this.app = await FilestoreApp.requireApp() - - await new Promise((resolve, reject) => { - this.server = this.app.listen( - Settings.internal.filestore.port, - '127.0.0.1', - err => { - if (err) { - return reject(err) + if (!this.server) { + await new Promise((resolve, reject) => { + this.server = App.listen( + Settings.internal.filestore.port, + '127.0.0.1', + err => { + if (err) { + return reject(err) + } + resolve() } - resolve() - } - ) - }) + ) + }) + } if (Settings.filestore.backend === 's3') { try { @@ -51,14 +36,11 @@ class FilestoreApp { } } - this.initing = false - this.persistor = require('../../../app/js/PersistorManager') - } - - async waitForInit() { - while (this.initing) { - await sleep(1000) - } + this.persistor = ObjectPersistor({ + ...Settings.filestore, + paths: Settings.path, + }) + FileHandler._TESTONLYSwapPersistorManager(this.persistor) } async stop() { @@ -105,19 +87,6 @@ class FilestoreApp { } } } - - static async requireApp() { - // unload the app, as we may be doing this on multiple runs with - // different settings, which affect startup in some cases - const files = await fsReaddir(Path.resolve(__dirname, '../../../app/js')) - files.forEach(file => { - disrequire(Path.resolve(__dirname, '../../../app/js', file)) - }) - disrequire(Path.resolve(__dirname, '../../../app')) - disrequire('@overleaf/object-persistor') - - return require('../../../app') - } } module.exports = FilestoreApp diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index f363456d6d..19db3ec919 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/services/filestore/test/acceptance/js/FilestoreTests.js @@ -12,7 +12,6 @@ const { Storage } = require('@google-cloud/storage') const streamifier = require('streamifier') chai.use(require('chai-as-promised')) const { ObjectId } = require('mongodb') -const tk = require('timekeeper') const ChildProcess = require('child_process') const fsWriteFile = promisify(fs.writeFile) @@ -506,11 +505,9 @@ describe('Filestore', function () { if (backendSettings.backend === 'gcs') { describe('when deleting a file in GCS', function () { - let fileId, fileUrl, content, error, date + let fileId, fileUrl, content, error, dateBefore, dateAfter beforeEach(async function () { - date = new Date() - tk.freeze(date) fileId = new ObjectId() fileUrl = `${filestoreUrl}/project/${projectId}/file/${fileId}` @@ -519,23 +516,27 @@ describe('Filestore', function () { const readStream = streamifier.createReadStream(content) let res = await fetch(fileUrl, { method: 'POST', body: readStream }) if (!res.ok) throw new Error(res.statusText) + dateBefore = new Date() res = await fetch(fileUrl, { method: 'DELETE' }) + dateAfter = new Date() if (!res.ok) throw new Error(res.statusText) }) - afterEach(function () { - tk.reset() - }) - it('should not throw an error', function () { expect(error).not.to.exist }) it('should copy the file to the deleted-files bucket', async function () { - await TestHelper.expectPersistorToHaveFile( + let date = dateBefore + const keys = [] + while (date <= dateAfter) { + keys.push(`${projectId}/${fileId}-${date.toISOString()}`) + date = new Date(date.getTime() + 1) + } + await TestHelper.expectPersistorToHaveSomeFile( app.persistor, `${Settings.filestore.stores.user_files}-deleted`, - `${projectId}/${fileId}-${date.toISOString()}`, + keys, content ) }) diff --git a/services/filestore/test/acceptance/js/TestHelper.js b/services/filestore/test/acceptance/js/TestHelper.js index 2edeb80d27..384f8aab6f 100644 --- a/services/filestore/test/acceptance/js/TestHelper.js +++ b/services/filestore/test/acceptance/js/TestHelper.js @@ -1,5 +1,6 @@ const streamifier = require('streamifier') const fetch = require('node-fetch') +const ObjectPersistor = require('@overleaf/object-persistor') const { expect } = require('chai') @@ -7,6 +8,7 @@ module.exports = { uploadStringToPersistor, getStringFromPersistor, expectPersistorToHaveFile, + expectPersistorToHaveSomeFile, expectPersistorNotToHaveFile, streamToString, getMetric, @@ -50,6 +52,25 @@ async function expectPersistorToHaveFile(persistor, bucket, key, content) { expect(foundContent).to.equal(content) } +async function expectPersistorToHaveSomeFile(persistor, bucket, keys, content) { + let foundContent + for (const key of keys) { + try { + foundContent = await getStringFromPersistor(persistor, bucket, key) + break + } catch (err) { + if (err instanceof ObjectPersistor.Errors.NotFoundError) { + continue + } + throw err + } + } + if (foundContent === undefined) { + expect.fail(`Could not find any of the specified keys: ${keys}`) + } + expect(foundContent).to.equal(content) +} + async function expectPersistorNotToHaveFile(persistor, bucket, key) { await expect( getStringFromPersistor(persistor, bucket, key) diff --git a/services/filestore/test/unit/js/FileHandlerTests.js b/services/filestore/test/unit/js/FileHandlerTests.js index d4899dd20a..1285051293 100644 --- a/services/filestore/test/unit/js/FileHandlerTests.js +++ b/services/filestore/test/unit/js/FileHandlerTests.js @@ -89,7 +89,7 @@ describe('FileHandler', function () { }, fs, }, - globals: { console }, + globals: { console, process }, }) })