From 7920570dd887889c983219b9289a26318eda3b1d Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Sun, 23 Feb 2020 15:42:04 +0000 Subject: [PATCH 1/7] [misc] test/unit: add missing require stubs for metrics and settings --- services/filestore/test/unit/js/ImageOptimiserTests.js | 5 ++++- services/filestore/test/unit/js/KeybuilderTests.js | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/services/filestore/test/unit/js/ImageOptimiserTests.js b/services/filestore/test/unit/js/ImageOptimiserTests.js index e4bc967345..947400d0d8 100644 --- a/services/filestore/test/unit/js/ImageOptimiserTests.js +++ b/services/filestore/test/unit/js/ImageOptimiserTests.js @@ -19,7 +19,10 @@ describe('ImageOptimiser', function() { ImageOptimiser = SandboxedModule.require(modulePath, { requires: { './SafeExec': SafeExec, - 'logger-sharelatex': logger + 'logger-sharelatex': logger, + 'metrics-sharelatex': { + Timer: sinon.stub().returns({ done: sinon.stub() }) + } } }) }) diff --git a/services/filestore/test/unit/js/KeybuilderTests.js b/services/filestore/test/unit/js/KeybuilderTests.js index 9dcb38f74f..4364b668a3 100644 --- a/services/filestore/test/unit/js/KeybuilderTests.js +++ b/services/filestore/test/unit/js/KeybuilderTests.js @@ -7,7 +7,9 @@ describe('LocalFileWriter', function() { const key = 'wombat/potato' beforeEach(function() { - KeyBuilder = SandboxedModule.require(modulePath) + KeyBuilder = SandboxedModule.require(modulePath, { + requires: { 'settings-sharelatex': {} } + }) }) describe('cachedKey', function() { From 54e3b577728ac1334150a056498ee603da042a02 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Sun, 23 Feb 2020 15:43:26 +0000 Subject: [PATCH 2/7] [misc] test/unit: add missing globals that are lazy loaded --- services/filestore/test/unit/js/S3PersistorTests.js | 2 +- services/filestore/test/unit/js/SafeExecTests.js | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/services/filestore/test/unit/js/S3PersistorTests.js b/services/filestore/test/unit/js/S3PersistorTests.js index 07bda746bc..ac104e36f2 100644 --- a/services/filestore/test/unit/js/S3PersistorTests.js +++ b/services/filestore/test/unit/js/S3PersistorTests.js @@ -158,7 +158,7 @@ describe('S3PersistorTests', function() { 'metrics-sharelatex': Metrics, crypto }, - globals: { console } + globals: { console, Buffer } }) }) diff --git a/services/filestore/test/unit/js/SafeExecTests.js b/services/filestore/test/unit/js/SafeExecTests.js index 1092be00be..6b89c53c01 100644 --- a/services/filestore/test/unit/js/SafeExecTests.js +++ b/services/filestore/test/unit/js/SafeExecTests.js @@ -12,6 +12,7 @@ describe('SafeExec', function() { options = { timeout: 10 * 1000, killSignal: 'SIGTERM' } safeExec = SandboxedModule.require(modulePath, { + globals: { process }, requires: { 'settings-sharelatex': settings } From 94b8a7f89170b44c72a42d4a736cfb3d24968d7c Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Sun, 23 Feb 2020 15:44:16 +0000 Subject: [PATCH 3/7] [misc] test/unit: KeybuilderTests: use a unique suite label --- services/filestore/test/unit/js/KeybuilderTests.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/test/unit/js/KeybuilderTests.js b/services/filestore/test/unit/js/KeybuilderTests.js index 4364b668a3..774fc2f366 100644 --- a/services/filestore/test/unit/js/KeybuilderTests.js +++ b/services/filestore/test/unit/js/KeybuilderTests.js @@ -2,7 +2,7 @@ const SandboxedModule = require('sandboxed-module') const modulePath = '../../../app/js/KeyBuilder.js' -describe('LocalFileWriter', function() { +describe('KeybuilderTests', function() { let KeyBuilder const key = 'wombat/potato' From 6589aa6ae0d3ec9332fd8dcdd447eec9d34aca0a Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Sun, 23 Feb 2020 18:25:39 +0000 Subject: [PATCH 4/7] [misc] test/acceptance: harden the startup check for s3 Signed-off-by: Jakob Ackermann --- .../test/acceptance/js/FilestoreApp.js | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/services/filestore/test/acceptance/js/FilestoreApp.js b/services/filestore/test/acceptance/js/FilestoreApp.js index 20564e2d40..eb34ad8302 100644 --- a/services/filestore/test/acceptance/js/FilestoreApp.js +++ b/services/filestore/test/acceptance/js/FilestoreApp.js @@ -4,11 +4,7 @@ const fs = require('fs') const Path = require('path') const { promisify } = require('util') const disrequire = require('disrequire') -const rp = require('request-promise-native').defaults({ - resolveWithFullResponse: true -}) - -const S3_TRIES = 30 +const AWS = require('aws-sdk') logger.logger.level('info') @@ -80,21 +76,31 @@ class FilestoreApp { return } - let s3Available = false + const s3 = new AWS.S3({ + accessKeyId: Settings.filestore.s3.key, + secretAccessKey: Settings.filestore.s3.secret, + endpoint: Settings.filestore.s3.endpoint, + s3ForcePathStyle: true, + signatureVersion: 'v4' + }) - while (tries < S3_TRIES && !s3Available) { + while (true) { try { - const response = await rp.get(`${Settings.filestore.s3.endpoint}/`) - if ([200, 404].includes(response.statusCode)) { - s3Available = true - } + return await s3 + .putObject({ + Key: 'startup', + Body: '42', + Bucket: Settings.filestore.stores.user_files + }) + .promise() } catch (err) { // swallow errors, as we may experience them until fake-s3 is running - } finally { - tries++ - if (!s3Available) { - await sleep(1000) + if (tries === 9) { + // throw just before hitting the 10s test timeout + throw err } + tries++ + await sleep(1000) } } } From 847f124d7b0ad9afd719568c1913047d3418fd57 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Sun, 23 Feb 2020 18:26:30 +0000 Subject: [PATCH 5/7] [misc] test/acceptance: skip the shutdown in case we did not start yet Signed-off-by: Jakob Ackermann --- services/filestore/test/acceptance/js/FilestoreApp.js | 1 + 1 file changed, 1 insertion(+) diff --git a/services/filestore/test/acceptance/js/FilestoreApp.js b/services/filestore/test/acceptance/js/FilestoreApp.js index eb34ad8302..6bc4f32719 100644 --- a/services/filestore/test/acceptance/js/FilestoreApp.js +++ b/services/filestore/test/acceptance/js/FilestoreApp.js @@ -62,6 +62,7 @@ class FilestoreApp { } async stop() { + if (!this.server) return const closeServer = promisify(this.server.close).bind(this.server) try { await closeServer() From 516102e6fb691d6d65cc6c533b6749a0bf08feb0 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 28 Feb 2020 12:26:46 +0100 Subject: [PATCH 6/7] [misc] test/acceptance: do not hard code fake credentials --- services/filestore/test/acceptance/js/FilestoreTests.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index ccef80bc01..91fdc0f4c7 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/services/filestore/test/acceptance/js/FilestoreTests.js @@ -415,8 +415,8 @@ describe('Filestore', function() { const s3ClientSettings = { credentials: { - accessKeyId: 'fake', - secretAccessKey: 'fake' + accessKeyId: process.env.AWS_ACCESS_KEY_ID, + secretAccessKey: process.env.AWS_SECRET_ACCESS_KEY }, endpoint: process.env.AWS_S3_ENDPOINT, sslEnabled: false, From 2b9d0868c5211348fc533d88023496bc22b1d721 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 28 Feb 2020 12:27:06 +0100 Subject: [PATCH 7/7] [misc] test/acceptance: retrieve ingress metrics just before using it The upload request can bump the ingress metric. The content hash validation might require a full download in case the ETag field of the upload response is not a md5 sum. --- .../test/acceptance/js/FilestoreTests.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index 91fdc0f4c7..c6a1e08444 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/services/filestore/test/acceptance/js/FilestoreTests.js @@ -148,12 +148,8 @@ describe('Filestore', function() { }) beforeEach(async function() { - // retrieve previous metrics from the app if (Settings.filestore.backend === 's3') { - ;[previousEgress, previousIngress] = await Promise.all([ - getMetric(filestoreUrl, 's3_egress'), - getMetric(filestoreUrl, 's3_ingress') - ]) + previousEgress = await getMetric(filestoreUrl, 's3_egress') } projectId = `acceptance_tests_${Math.random()}` }) @@ -195,6 +191,15 @@ describe('Filestore', function() { await pipeline(readStream, writeStream, resultStream) }) + beforeEach(async function retrievePreviousIngressMetrics() { + // The upload request can bump the ingress metric. + // The content hash validation might require a full download + // in case the ETag field of the upload response is not a md5 sum. + if (Settings.filestore.backend === 's3') { + previousIngress = await getMetric(filestoreUrl, 's3_ingress') + } + }) + it('should return 404 for a non-existant id', async function() { const options = { uri: fileUrl + '___this_is_clearly_wrong___' } await expect(