From 85f4544805d4552079fb9a8a9e8d0ea257a5a1b4 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 8 May 2024 17:04:46 +0200 Subject: [PATCH] Merge pull request #18252 from overleaf/jpa-refactor-for-flaky-test [web] refactor background job for setting root doc automatically GitOrigin-RevId: 719c010eb3e5b692908b7a6fea9d8522b9fc01b9 --- .../Features/Project/ProjectRootDocManager.js | 25 +++++++++++++++++-- .../ThirdPartyDataStore/TpdsUpdateHandler.js | 19 +------------- .../src/Project/ProjectRootDocManagerTests.js | 6 +++++ .../TpdsUpdateHandlerTests.js | 20 +++------------ 4 files changed, 33 insertions(+), 37 deletions(-) diff --git a/services/web/app/src/Features/Project/ProjectRootDocManager.js b/services/web/app/src/Features/Project/ProjectRootDocManager.js index 5e49cb6fcb..d822e86845 100644 --- a/services/web/app/src/Features/Project/ProjectRootDocManager.js +++ b/services/web/app/src/Features/Project/ProjectRootDocManager.js @@ -19,13 +19,34 @@ const ProjectGetter = require('./ProjectGetter') const DocumentHelper = require('../Documents/DocumentHelper') const Path = require('path') const fs = require('fs') -const { promisify } = require('util') const async = require('async') const globby = require('globby') const _ = require('lodash') const { promisifyAll } = require('@overleaf/promise-utils') +const logger = require('@overleaf/logger') +const { + BackgroundTaskTracker, +} = require('../../infrastructure/GracefulShutdown') + +const rootDocResets = new BackgroundTaskTracker('root doc resets') module.exports = ProjectRootDocManager = { + setRootDocAutomaticallyInBackground(projectId) { + rootDocResets.add() + setTimeout(async () => { + try { + await ProjectRootDocManager.promises.setRootDocAutomatically(projectId) + } catch (err) { + logger.warn( + { err }, + 'failed to set root doc automatically in background' + ) + } finally { + rootDocResets.done() + } + }, 30 * 1000) + }, + setRootDocAutomatically(projectId, callback) { if (callback == null) { callback = function () {} @@ -302,7 +323,7 @@ module.exports = ProjectRootDocManager = { module.exports = ProjectRootDocManager module.exports.promises = promisifyAll(module.exports, { - without: ['_rootDocSort'], + without: ['_rootDocSort', 'setRootDocAutomaticallyInBackground'], multiResult: { findRootDocFileFromDirectory: ['path', 'content'], }, diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js index 5d39a66c58..c303f81559 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js @@ -11,13 +11,6 @@ const FileTypeManager = require('../Uploads/FileTypeManager') const CooldownManager = require('../Cooldown/CooldownManager') const Errors = require('../Errors/Errors') const Modules = require('../../infrastructure/Modules') -const { - BackgroundTaskTracker, -} = require('../../infrastructure/GracefulShutdown') - -const ROOT_DOC_TIMEOUT_LENGTH = 30 * 1000 - -const rootDocResets = new BackgroundTaskTracker('root doc resets') async function newUpdate( userId, @@ -137,17 +130,7 @@ async function getOrCreateProjectByName(userId, projectName) { // have a crack at setting the root doc after a while, on creation // we won't have it yet, but should have been sent it it within 30 // seconds - rootDocResets.add() - setTimeout(() => { - ProjectRootDocManager.promises - .setRootDocAutomatically(project._id) - .then(() => { - rootDocResets.done() - }) - .catch(err => { - logger.warn({ err }, 'failed to set root doc after project creation') - }) - }, ROOT_DOC_TIMEOUT_LENGTH) + ProjectRootDocManager.setRootDocAutomaticallyInBackground(project._id) return project } diff --git a/services/web/test/unit/src/Project/ProjectRootDocManagerTests.js b/services/web/test/unit/src/Project/ProjectRootDocManagerTests.js index 57a97ae828..ab35f7c5bc 100644 --- a/services/web/test/unit/src/Project/ProjectRootDocManagerTests.js +++ b/services/web/test/unit/src/Project/ProjectRootDocManagerTests.js @@ -47,6 +47,12 @@ describe('ProjectRootDocManager', function () { './ProjectEntityHandler': (this.ProjectEntityHandler = {}), './ProjectEntityUpdateHandler': (this.ProjectEntityUpdateHandler = {}), './ProjectGetter': (this.ProjectGetter = {}), + '../../infrastructure/GracefulShutdown': { + BackgroundTaskTracker: class { + add() {} + done() {} + }, + }, globby: this.globby, fs: this.fs, }, diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js index 8527e8c1ab..a305e9095c 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js @@ -8,14 +8,6 @@ const MODULE_PATH = '../../../../app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js' describe('TpdsUpdateHandler', function () { - beforeEach(function () { - this.clock = sinon.useFakeTimers() - }) - - afterEach(function () { - this.clock.restore() - }) - beforeEach(function () { this.projectName = 'My recipes' this.projects = { @@ -93,9 +85,7 @@ describe('TpdsUpdateHandler', function () { .withArgs(this.projects.archived2, this.userId) .returns(true) this.RootDocManager = { - promises: { - setRootDocAutomatically: sinon.stub().resolves(), - }, + setRootDocAutomaticallyInBackground: sinon.stub(), } this.UpdateMerger = { promises: { @@ -503,10 +493,8 @@ function expectProjectCreated() { }) it('sets the root doc', function () { - // Fire pending timers - this.clock.next() expect( - this.RootDocManager.promises.setRootDocAutomatically + this.RootDocManager.setRootDocAutomaticallyInBackground ).to.have.been.calledWith(this.projects.active1._id) }) } @@ -518,9 +506,7 @@ function expectProjectNotCreated() { }) it('does not set the root doc', function () { - // Fire pending timers - this.clock.next() - expect(this.RootDocManager.promises.setRootDocAutomatically).not.to.have + expect(this.RootDocManager.setRootDocAutomaticallyInBackground).not.to.have .been.called }) }