Merge pull request #6332 from overleaf/ab-split-test-param-overrides

Split tests query param overrides

GitOrigin-RevId: 8112710d057ddc22cebf37a619dfc969be57b6cc

services/web/test/unit/src/Project/ProjectControllerTests.js

@@ -129,28 +129,12 @@ describe('ProjectController', function () {
         .returns({ newLogsUI: false, subvariant: null }),
     }
     this.SplitTestHandler = {
-      promises: {
-        getTestSegmentation: sinon.stub().resolves({ enabled: false }),
-      },
-      getTestSegmentation: sinon.stub().yields(null, { enabled: false }),
-    }
-    this.SplitTestV2Handler = {
       promises: {
         getAssignment: sinon.stub().resolves({ variant: 'default' }),
-        getAssignmentForSession: sinon.stub().resolves({ variant: 'default' }),
         assignInLocalsContext: sinon.stub().resolves({ variant: 'default' }),
-        assignInLocalsContextForSession: sinon
-          .stub()
-          .resolves({ variant: 'default' }),
       },
       getAssignment: sinon.stub().yields(null, { variant: 'default' }),
-      getAssignmentForSession: sinon
-        .stub()
-        .yields(null, { variant: 'default' }),
       assignInLocalsContext: sinon.stub().yields(null, { variant: 'default' }),
-      assignInLocalsContextForSession: sinon
-        .stub()
-        .yields(null, { variant: 'default' }),
     }
 
     this.ProjectController = SandboxedModule.require(MODULE_PATH, {
@@ -159,7 +143,6 @@ describe('ProjectController', function () {
         '@overleaf/settings': this.settings,
         '@overleaf/metrics': this.Metrics,
         '../SplitTests/SplitTestHandler': this.SplitTestHandler,
-        '../SplitTests/SplitTestV2Handler': this.SplitTestV2Handler,
         './ProjectDeleter': this.ProjectDeleter,
         './ProjectDuplicator': this.ProjectDuplicator,
         './ProjectCreationHandler': this.ProjectCreationHandler,
@@ -1126,21 +1109,6 @@ describe('ProjectController', function () {
     })
 
     describe('pdf caching feature flags', function () {
-      /* eslint-disable mocha/no-identical-title */
-      function showNoVariant() {
-        beforeEach(function () {
-          this.SplitTestHandler.getTestSegmentation = sinon
-            .stub()
-            .yields(null, { enabled: false })
-        })
-      }
-      function showVariant(variant) {
-        beforeEach(function () {
-          this.SplitTestHandler.getTestSegmentation = sinon
-            .stub()
-            .yields(null, { enabled: true, variant })
-        })
-      }
       function expectBandwidthTrackingEnabled() {
         it('should track pdf bandwidth', function (done) {
           this.res.render = (pageName, opts) => {
@@ -1177,160 +1145,11 @@ describe('ProjectController', function () {
           this.ProjectController.loadEditor(this.req, this.res)
         })
       }
-      function expectToCollectMetricsAndCachePDF() {
-        describe('with no query', function () {
-          expectBandwidthTrackingEnabled()
-          expectPDFCachingEnabled()
-        })
-
-        describe('with enable_pdf_caching=false', function () {
-          beforeEach(function () {
-            this.req.query.enable_pdf_caching = 'false'
-          })
-          expectBandwidthTrackingDisabled()
-          expectPDFCachingDisabled()
-        })
-
-        describe('with enable_pdf_caching=true', function () {
-          beforeEach(function () {
-            this.req.query.enable_pdf_caching = 'true'
-          })
-          expectBandwidthTrackingEnabled()
-          expectPDFCachingEnabled()
-        })
-      }
-      function expectToCollectMetricsOnly() {
-        describe('with no query', function () {
-          expectBandwidthTrackingEnabled()
-          expectPDFCachingDisabled()
-        })
-
-        describe('with enable_pdf_caching=false', function () {
-          beforeEach(function () {
-            this.req.query.enable_pdf_caching = 'false'
-          })
-          expectBandwidthTrackingDisabled()
-          expectPDFCachingDisabled()
-        })
-
-        describe('with enable_pdf_caching=true', function () {
-          beforeEach(function () {
-            this.req.query.enable_pdf_caching = 'true'
-          })
-          expectBandwidthTrackingEnabled()
-          expectPDFCachingDisabled()
-        })
-      }
-
-      function expectToCachePDFOnly() {
-        describe('with no query', function () {
-          expectBandwidthTrackingDisabled()
-          expectPDFCachingEnabled()
-        })
-
-        describe('with enable_pdf_caching=false', function () {
-          beforeEach(function () {
-            this.req.query.enable_pdf_caching = 'false'
-          })
-          expectBandwidthTrackingDisabled()
-          expectPDFCachingDisabled()
-        })
-
-        describe('with enable_pdf_caching=true', function () {
-          beforeEach(function () {
-            this.req.query.enable_pdf_caching = 'true'
-          })
-          expectBandwidthTrackingDisabled()
-          expectPDFCachingEnabled()
-        })
-      }
-
-      function expectToNotBeEnrolledAtAll() {
-        describe('with no query', function () {
-          expectBandwidthTrackingDisabled()
-          expectPDFCachingDisabled()
-        })
-
-        describe('with enable_pdf_caching=false', function () {
-          beforeEach(function () {
-            this.req.query.enable_pdf_caching = 'false'
-          })
-          expectBandwidthTrackingDisabled()
-          expectPDFCachingDisabled()
-        })
-
-        describe('with enable_pdf_caching=true', function () {
-          beforeEach(function () {
-            this.req.query.enable_pdf_caching = 'true'
-          })
-          expectBandwidthTrackingDisabled()
-          expectPDFCachingDisabled()
-        })
-      }
-
-      function tagAnonymous() {
-        beforeEach(function () {
-          this.SessionManager.isUserLoggedIn = sinon.stub().returns(false)
-        })
-      }
 
       beforeEach(function () {
         this.settings.enablePdfCaching = true
       })
 
-      describe('during regular roll-out', function () {
-        before(function () {
-          this.skip()
-        })
-        describe('disabled', function () {
-          showNoVariant()
-
-          describe('regular user', function () {
-            expectToNotBeEnrolledAtAll()
-          })
-          describe('anonymous user', function () {
-            tagAnonymous()
-            expectToCachePDFOnly()
-          })
-        })
-
-        describe('variant=collect-metrics', function () {
-          showVariant('collect-metrics')
-
-          describe('regular user', function () {
-            expectToCollectMetricsOnly()
-          })
-          describe('anonymous user', function () {
-            tagAnonymous()
-            expectToCachePDFOnly()
-          })
-        })
-
-        describe('variant=collect-metrics-and-enable-caching', function () {
-          showVariant('collect-metrics-and-enable-caching')
-
-          describe('regular user', function () {
-            expectToCollectMetricsAndCachePDF()
-          })
-          describe('anonymous user', function () {
-            tagAnonymous()
-            expectToCachePDFOnly()
-          })
-        })
-
-        describe('variant=enable-caching-only', function () {
-          showVariant('enable-caching-only')
-
-          describe('regular user', function () {
-            expectToCachePDFOnly()
-          })
-          describe('anonymous user', function () {
-            tagAnonymous()
-            expectToCachePDFOnly()
-          })
-        })
-      })
-
       describe('during opt-in only', function () {
         describe('with no query', function () {
           expectBandwidthTrackingDisabled()

services/web/test/unit/src/SplitTests/SplitTestMiddlewareTests.js

@@ -5,7 +5,6 @@ const modulePath = path.join(
   '../../../../app/src/Features/SplitTests/SplitTestMiddleware'
 )
 const sinon = require('sinon')
-const { assert } = require('chai')
 const MockResponse = require('../helpers/MockResponse')
 const MockRequest = require('../helpers/MockRequest')
 
@@ -13,75 +12,27 @@ describe('SplitTestMiddleware', function () {
   beforeEach(function () {
     this.SplitTestMiddleware = SandboxedModule.require(modulePath, {
       requires: {
-        './SplitTestV2Handler': (this.SplitTestV2Handler = {
+        './SplitTestHandler': (this.SplitTestHandler = {
           promises: {
-            getAssignmentForSession: sinon.stub().resolves(),
+            assignInLocalsContext: sinon.stub().resolves(),
           },
         }),
-        './SplitTestCache': (this.SplitTestCache = {
-          get: sinon.stub().resolves(),
-        }),
       },
     })
 
     this.req = new MockRequest()
-    this.req.session = {}
     this.res = new MockResponse()
     this.next = sinon.stub()
   })
 
-  it('assign split test variant in locals', async function () {
-    this.SplitTestCache.get.withArgs('ui-overhaul').resolves({
-      name: 'ui-overhaul',
-      getCurrentVersion: () => ({
-        versionNumber: 1,
-        active: true,
-      }),
-    })
-    this.SplitTestV2Handler.promises.getAssignmentForSession
-      .withArgs(this.req.session, 'ui-overhaul')
-      .resolves({
-        variant: 'new',
-      })
-
-    const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([
-      'ui-overhaul',
-    ])
-    await middleware(this.req, this.res, this.next)
-
-    assert.equal(this.res.locals.splitTestVariants['ui-overhaul'], 'new')
-    assert.deepEqual(this.req.session.cachedSplitTestAssignments, {
-      'ui-overhaul-1': 'new',
-    })
-    sinon.assert.calledOnce(this.next)
-  })
-
-  it('assign multiple split test variant in locals', async function () {
-    this.SplitTestCache.get
-      .withArgs('ui-overhaul')
-      .resolves({
-        name: 'ui-overhaul',
-        getCurrentVersion: () => ({
-          versionNumber: 1,
-          active: true,
-        }),
-      })
-      .withArgs('other-test')
-      .resolves({
-        name: 'other-test',
-        getCurrentVersion: () => ({
-          versionNumber: 1,
-          active: true,
-        }),
-      })
-
-    this.SplitTestV2Handler.promises.getAssignmentForSession
-      .withArgs(this.req.session, 'ui-overhaul')
+  it('assign multiple split test variants in locals', async function () {
+    this.SplitTestHandler.promises.assignInLocalsContext
+      .withArgs(this.req, 'ui-overhaul')
       .resolves({
         variant: 'default',
       })
-    this.SplitTestV2Handler.promises.getAssignmentForSession
-      .withArgs(this.req.session, 'other-test')
+    this.SplitTestHandler.promises.assignInLocalsContext
+      .withArgs(this.req, 'other-test')
       .resolves({
         variant: 'foobar',
       })
@@ -92,135 +43,47 @@ describe('SplitTestMiddleware', function () {
     ])
     await middleware(this.req, this.res, this.next)
 
-    assert.equal(this.res.locals.splitTestVariants['ui-overhaul'], 'default')
-    assert.equal(this.res.locals.splitTestVariants['other-test'], 'foobar')
-    assert.deepEqual(this.req.session.cachedSplitTestAssignments, {
-      'ui-overhaul-1': 'default',
-      'other-test-1': 'foobar',
-    })
-    sinon.assert.calledOnce(this.next)
-  })
-
-  it('variants are overridden in locals with query parameters', async function () {
-    this.SplitTestCache.get.withArgs('active-split-test').resolves({
-      name: 'active-split-test',
-      getCurrentVersion: () => ({
-        versionNumber: 1,
-        active: true,
-      }),
-    })
-
-    this.SplitTestV2Handler.promises.getAssignmentForSession
-      .withArgs(this.req.session, 'active-split-test')
-      .resolves({
-        variant: 'default',
-      })
-
-    const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([
-      'active-split-test',
-    ])
-
-    this.req.query['active-split-test'] = 'variant'
-
-    await middleware(this.req, this.res, this.next)
-
-    assert.equal(
-      this.res.locals.splitTestVariants['active-split-test'],
-      'variant'
+    sinon.assert.calledWith(
+      this.SplitTestHandler.promises.assignInLocalsContext,
+      this.req,
+      this.res,
+      'ui-overhaul'
     )
-    assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) // variants overriden using req.query are not cached
-    sinon.assert.calledOnce(this.next)
-  })
-
-  it('non-active split tests can be set in locals with query parameters', async function () {
-    const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([
-      'non-active-split-test',
-    ])
-
-    this.req.query['non-active-split-test'] = 'variant'
-
-    await middleware(this.req, this.res, this.next)
-
-    assert.equal(
-      this.res.locals.splitTestVariants['non-active-split-test'],
-      'variant'
+    sinon.assert.calledWith(
+      this.SplitTestHandler.promises.assignInLocalsContext,
+      this.req,
+      this.res,
+      'other-test'
     )
-    assert.deepEqual(this.req.session.cachedSplitTestAssignments, {}) // variants overriden using req.query are not cached
     sinon.assert.calledOnce(this.next)
   })
 
-  it('cached assignment in session is used', async function () {
-    this.req.session.cachedSplitTestAssignments = {
-      'ui-overhaul-1': 'cached-variant',
-    }
-    this.SplitTestCache.get.withArgs('ui-overhaul').resolves({
-      name: 'ui-overhaul',
-      getCurrentVersion: () => ({
-        versionNumber: 1,
-        active: true,
-      }),
-    })
+  it('assign no split test variant in locals', async function () {
+    const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([])
 
-    const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([
-      'ui-overhaul',
-    ])
     await middleware(this.req, this.res, this.next)
 
-    sinon.assert.notCalled(
-      this.SplitTestV2Handler.promises.getAssignmentForSession
-    )
-    assert.equal(
-      this.res.locals.splitTestVariants['ui-overhaul'],
-      'cached-variant'
-    )
-    assert.deepEqual(this.req.session.cachedSplitTestAssignments, {
-      'ui-overhaul-1': 'cached-variant',
-    })
+    sinon.assert.notCalled(this.SplitTestHandler.promises.assignInLocalsContext)
     sinon.assert.calledOnce(this.next)
   })
 
-  it('inactive split test is not assigned in locals', async function () {
-    this.SplitTestCache.get.withArgs('ui-overhaul').resolves({
-      name: 'ui-overhaul',
-      getCurrentVersion: () => ({
-        versionNumber: 1,
-        active: false,
-      }),
-    })
-
-    const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([
-      'ui-overhaul',
-    ])
-    await middleware(this.req, this.res, this.next)
-
-    assert.equal(this.res.locals.splitTestVariants, undefined)
-    assert.deepEqual(this.req.session.cachedSplitTestAssignments, {})
-    sinon.assert.calledOnce(this.next)
-  })
-
-  it('not existing split test is not assigned in locals', async function () {
-    this.SplitTestCache.get.withArgs('not-found').resolves(undefined)
-
-    const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([
-      'not-found',
-    ])
-    await middleware(this.req, this.res, this.next)
-
-    assert.equal(this.res.locals.splitTestVariants, undefined)
-    assert.deepEqual(this.req.session.cachedSplitTestAssignments, {})
-    sinon.assert.calledOnce(this.next)
-  })
-
-  it('next middleware is called even if there is an error', async function () {
-    this.SplitTestCache.get.throws('some error')
+  it('exception thrown by assignment does not fail the request', async function () {
+    this.SplitTestHandler.promises.assignInLocalsContext
+      .withArgs(this.req, this.res, 'some-test')
+      .throws(new Error('failure'))
 
     const middleware = this.SplitTestMiddleware.loadAssignmentsInLocals([
       'some-test',
     ])
+
     await middleware(this.req, this.res, this.next)
 
-    assert.equal(this.res.locals.splitTestVariants, undefined)
-    assert.deepEqual(this.req.session.cachedSplitTestAssignments, {})
+    sinon.assert.calledWith(
+      this.SplitTestHandler.promises.assignInLocalsContext,
+      this.req,
+      this.res,
+      'some-test'
+    )
     sinon.assert.calledOnce(this.next)
   })
 })

services/web/test/unit/src/Subscription/RecurlyEventHandlerTests.js

@@ -29,7 +29,7 @@ describe('RecurlyEventHandler', function () {
         './SubscriptionEmailHandler': (this.SubscriptionEmailHandler = {
           sendTrialOnboardingEmail: sinon.stub(),
         }),
-        '../SplitTests/SplitTestV2Handler': (this.SplitTestV2Handler = {
+        '../SplitTests/SplitTestHandler': (this.SplitTestHandler = {
           promises: {
             getAssignment: sinon.stub().resolves({ active: false }),
           },
@@ -76,14 +76,14 @@ describe('RecurlyEventHandler', function () {
       true
     )
     sinon.assert.calledWith(
-      this.SplitTestV2Handler.promises.getAssignment,
+      this.SplitTestHandler.promises.getAssignment,
       this.userId,
       'trial-onboarding-email'
     )
   })
 
   it('sends free trial onboarding email if user in ab group', async function () {
-    this.SplitTestV2Handler.promises.getAssignment = sinon
+    this.SplitTestHandler.promises.getAssignment = sinon
       .stub()
       .resolves({ active: true, variant: 'send-email' })
     this.userId = '123456789trial'
@@ -94,7 +94,7 @@ describe('RecurlyEventHandler', function () {
     await this.RecurlyEventHandler.sendSubscriptionStartedEvent(this.eventData)
 
     sinon.assert.calledWith(
-      this.SplitTestV2Handler.promises.getAssignment,
+      this.SplitTestHandler.promises.getAssignment,
       this.userId,
       'trial-onboarding-email'
     )

services/web/test/unit/src/Subscription/SubscriptionControllerTests.js

@@ -133,12 +133,12 @@ describe('SubscriptionController', function () {
     }
     this.SplitTestV2Hander = {
       promises: {
-        getAssignmentForSession: sinon.stub().resolves({ variant: 'default' }),
+        getAssignment: sinon.stub().resolves({ variant: 'default' }),
       },
     }
     this.SubscriptionController = SandboxedModule.require(modulePath, {
       requires: {
-        '../SplitTests/SplitTestV2Handler': this.SplitTestV2Hander,
+        '../SplitTests/SplitTestHandler': this.SplitTestV2Hander,
         '../Authentication/SessionManager': this.SessionManager,
         './SubscriptionHandler': this.SubscriptionHandler,
         './PlansLocator': this.PlansLocator,
@@ -163,9 +163,6 @@ describe('SubscriptionController', function () {
           recordEventForSession: sinon.stub(),
           setUserPropertyForUser: sinon.stub(),
         }),
-        '../SplitTests/SplitTestHandler': (this.SplitTestHandler = {
-          getTestSegmentation: () => {},
-        }),
       },
     })