From cd64720abe3a592e41bdf99d527b83caf51766ff Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Fri, 15 Aug 2025 07:31:16 -0400 Subject: [PATCH] Merge pull request #27903 from overleaf/em-unit-tests-redis Make Redis available to unit tests GitOrigin-RevId: 7bd403d9ad4be504a87bc9108d60686e6c2a9fb1 --- .../app/src/infrastructure/RedisWrapper.js | 53 +++--- services/web/docker-compose.ci.yml | 7 +- services/web/docker-compose.yml | 7 +- .../unit/src/Cooldown/CooldownManagerTests.js | 160 ++++-------------- 4 files changed, 72 insertions(+), 155 deletions(-) diff --git a/services/web/app/src/infrastructure/RedisWrapper.js b/services/web/app/src/infrastructure/RedisWrapper.js index 3017d03e47..e08d3b8c4f 100644 --- a/services/web/app/src/infrastructure/RedisWrapper.js +++ b/services/web/app/src/infrastructure/RedisWrapper.js @@ -2,29 +2,36 @@ const Settings = require('@overleaf/settings') const redis = require('@overleaf/redis-wrapper') const { addConnectionDrainer } = require('./GracefulShutdown') -if ( - (typeof global.beforeEach === 'function' && - process.argv.join(' ').match(/unit/)) || - process.env.VITEST -) { - throw new Error( - 'It looks like unit tests are running, but you are connecting to Redis. Missing a stub?' - ) +/** + * A per-feature interface to Redis, looks up the feature in `settings.redis` + * and returns an appropriate client. Necessary because we don't want to + * migrate web over to redis-cluster all at once. + * + * @param feature - one of 'websessions' | 'ratelimiter' | ... + */ +function client(feature) { + const redisFeatureSettings = Settings.redis[feature] || Settings.redis.web + const client = redis.createClient(redisFeatureSettings) + addConnectionDrainer(`redis ${feature}`, async () => { + await client.disconnect() + }) + return client } -// A per-feature interface to Redis, -// looks up the feature in `settings.redis` -// and returns an appropriate client. -// Necessary because we don't want to migrate web over -// to redis-cluster all at once. -module.exports = { - // feature = 'websessions' | 'ratelimiter' | ... - client(feature) { - const redisFeatureSettings = Settings.redis[feature] || Settings.redis.web - const client = redis.createClient(redisFeatureSettings) - addConnectionDrainer(`redis ${feature}`, async () => { - await client.disconnect() - }) - return client - }, +async function cleanupTestRedis() { + const rclient = client() + ensureTestRedis(rclient) + await rclient.flushall() } + +function ensureTestRedis(rclient) { + const host = rclient.options.host + const env = process.env.NODE_ENV + if (host !== 'redis_test' || env !== 'test') { + throw new Error( + `Refusing to clear Redis instance '${host}' in environment '${env}'` + ) + } +} + +module.exports = { client, cleanupTestRedis } diff --git a/services/web/docker-compose.ci.yml b/services/web/docker-compose.ci.yml index e736b1823e..e859ab8222 100644 --- a/services/web/docker-compose.ci.yml +++ b/services/web/docker-compose.ci.yml @@ -22,8 +22,10 @@ services: OVERLEAF_CONFIG: NODE_ENV: test NODE_OPTIONS: "--unhandled-rejections=strict" + REDIS_HOST: redis_test VITEST_NO_CACHE: true depends_on: + - redis_test - mongo test_acceptance: @@ -38,6 +40,7 @@ services: environment: BASE_CONFIG: OVERLEAF_CONFIG: + REDIS_HOST: redis_test extra_hosts: - 'www.overleaf.test:127.0.0.1' volumes: @@ -46,7 +49,7 @@ services: command: npm run test:acceptance:app user: root depends_on: - - redis + - redis_test - mongo - saml - ldap @@ -89,7 +92,7 @@ services: command: tar -cf /tmp/build/build.tar public/ user: root - redis: + redis_test: image: redis:7.4.3 mongo: diff --git a/services/web/docker-compose.yml b/services/web/docker-compose.yml index 4acfa80926..4e4ee92da6 100644 --- a/services/web/docker-compose.yml +++ b/services/web/docker-compose.yml @@ -19,11 +19,13 @@ services: LOG_LEVEL: ${LOG_LEVEL:-} NODE_ENV: test NODE_OPTIONS: "--unhandled-rejections=strict" + REDIS_HOST: redis_test entrypoint: /overleaf/bin/shared/wait_for_it mongo:27017 --timeout=60 -- command: npm run --silent test:unit:app user: node depends_on: - mongo + - redis_test test_acceptance: image: node:22.17.0 @@ -42,12 +44,13 @@ services: LOG_LEVEL: ${LOG_LEVEL:-} MONGO_SERVER_SELECTION_TIMEOUT: 600000 MONGO_SOCKET_TIMEOUT: 300000 + REDIS_HOST: redis_test # OVERLEAF_ALLOW_ANONYMOUS_READ_AND_WRITE_SHARING: 'true' extra_hosts: - 'www.overleaf.test:127.0.0.1' depends_on: - - redis + - redis_test - mongo - saml - ldap @@ -85,7 +88,7 @@ services: - "run" - "cypress:run-ct" - redis: + redis_test: image: redis:7.4.3 mongo: diff --git a/services/web/test/unit/src/Cooldown/CooldownManagerTests.js b/services/web/test/unit/src/Cooldown/CooldownManagerTests.js index 7efefe1b04..5531c4b8ea 100644 --- a/services/web/test/unit/src/Cooldown/CooldownManagerTests.js +++ b/services/web/test/unit/src/Cooldown/CooldownManagerTests.js @@ -1,153 +1,57 @@ -const SandboxedModule = require('sandboxed-module') -const sinon = require('sinon') const { expect } = require('chai') -const modulePath = require('path').join( - __dirname, - '../../../../app/src/Features/Cooldown/CooldownManager' -) +const { ObjectId } = require('mongodb-legacy') +const CooldownManager = require('../../../../app/src/Features/Cooldown/CooldownManager') +const { + cleanupTestRedis, +} = require('../../../../app/src/infrastructure/RedisWrapper') describe('CooldownManager', function () { + beforeEach(cleanupTestRedis) + beforeEach(function () { - this.projectId = 'abcdefg' - this.rclient = { set: sinon.stub(), get: sinon.stub() } - this.RedisWrapper = { client: () => this.rclient } - this.CooldownManager = SandboxedModule.require(modulePath, { - requires: { - '../../infrastructure/RedisWrapper': this.RedisWrapper, - }, - }) + this.project1Id = new ObjectId().toString() + this.project2Id = new ObjectId().toString() }) describe('_buildKey', function () { it('should build a properly formatted redis key', function () { - expect(this.CooldownManager._buildKey('ABC')).to.equal('Cooldown:{ABC}') + expect(CooldownManager._buildKey('ABC')).to.equal('Cooldown:{ABC}') }) }) describe('isProjectOnCooldown', function () { - describe('when project is on cooldown', function () { - beforeEach(function () { - this.rclient.get = sinon.stub().callsArgWith(1, null, '1') - }) - - it('should fetch key from redis', async function () { - await this.CooldownManager.promises.isProjectOnCooldown(this.projectId) - this.rclient.get.callCount.should.equal(1) - this.rclient.get.calledWith('Cooldown:{abcdefg}').should.equal(true) - }) - - it('should produce a true result', async function () { - const result = await this.CooldownManager.promises.isProjectOnCooldown( - this.projectId + describe('when no project is on cooldown', function () { + it('returns false for project 1', async function () { + const result = await CooldownManager.promises.isProjectOnCooldown( + this.project1Id ) - expect(result).to.equal(true) - }) - }) - - describe('when project is not on cooldown', function () { - beforeEach(function () { - this.rclient.get = sinon.stub().callsArgWith(1, null, null) + expect(result).to.be.false }) - it('should fetch key from redis', async function () { - await this.CooldownManager.promises.isProjectOnCooldown(this.projectId) - this.rclient.get.callCount.should.equal(1) - this.rclient.get.calledWith('Cooldown:{abcdefg}').should.equal(true) - }) - - it('should produce a false result', async function () { - const result = await this.CooldownManager.promises.isProjectOnCooldown( - this.projectId + it('returns false for project 2', async function () { + const result = await CooldownManager.promises.isProjectOnCooldown( + this.project2Id ) - expect(result).to.equal(false) + expect(result).to.be.false }) }) - - describe('when rclient.get produces an error', function () { - beforeEach(function () { - this.rclient.get = sinon.stub().callsArgWith(1, new Error('woops')) + describe('when project 1 is on cooldown', function () { + beforeEach(async function () { + await CooldownManager.promises.putProjectOnCooldown(this.project1Id) }) - it('should fetch key from redis', async function () { - try { - await this.CooldownManager.promises.isProjectOnCooldown( - this.projectId - ) - } catch { - // ignore errors - expected - } - this.rclient.get.callCount.should.equal(1) - this.rclient.get.calledWith('Cooldown:{abcdefg}').should.equal(true) + it('returns true for project 1', async function () { + const result = await CooldownManager.promises.isProjectOnCooldown( + this.project1Id + ) + expect(result).to.be.true }) - it('should produce an error', async function () { - let error - - try { - await this.CooldownManager.promises.isProjectOnCooldown( - this.projectId - ) - } catch (err) { - error = err - } - - expect(error).to.be.instanceOf(Error) - }) - }) - }) - - describe('putProjectOnCooldown', function () { - describe('when rclient.set does not produce an error', function () { - beforeEach(function () { - this.rclient.set = sinon.stub().callsArgWith(4, null) - }) - - it('should set a key in redis', async function () { - await this.CooldownManager.promises.putProjectOnCooldown(this.projectId) - this.rclient.set.callCount.should.equal(1) - this.rclient.set.calledWith('Cooldown:{abcdefg}').should.equal(true) - }) - - it('should not produce an error', async function () { - let error - try { - await this.CooldownManager.promises.putProjectOnCooldown( - this.projectId - ) - } catch (err) { - error = err - } - expect(error).not.to.exist - }) - }) - - describe('when rclient.set produces an error', function () { - beforeEach(function () { - this.rclient.set = sinon.stub().callsArgWith(4, new Error('woops')) - }) - - it('should set a key in redis', async function () { - try { - await this.CooldownManager.promises.putProjectOnCooldown( - this.projectId - ) - } catch { - // ignore errors - expected - } - this.rclient.set.callCount.should.equal(1) - this.rclient.set.calledWith('Cooldown:{abcdefg}').should.equal(true) - }) - - it('produce an error', async function () { - let error - try { - await this.CooldownManager.promises.putProjectOnCooldown( - this.projectId - ) - } catch (err) { - error = err - } - expect(error).to.be.instanceOf(Error) + it('returns false for project 2', async function () { + const result = await CooldownManager.promises.isProjectOnCooldown( + this.project2Id + ) + expect(result).to.be.false }) }) })