Merge pull request #27903 from overleaf/em-unit-tests-redis

Make Redis available to unit tests

GitOrigin-RevId: 7bd403d9ad4be504a87bc9108d60686e6c2a9fb1

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 }

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:

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:

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)
-      })
-
-      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)
+        expect(result).to.be.false
       })
 
-      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('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('returns true for project 1', async function () {
+        const result = await CooldownManager.promises.isProjectOnCooldown(
+          this.project1Id
+        )
+        expect(result).to.be.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
       })
     })
   })