From 0aa56fbe2c583311d290f907a2ec98a5ec7dd757 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 13 Jun 2025 15:40:15 +0200 Subject: [PATCH] [project-history] fix callback signature when processing no updates (#26420) * [project-history] fix tests and cover callback for processing updates The before setup was hiding that some tests were not doing what the assertions were expecting. * [project-history] fix callback signature when processing no updates GitOrigin-RevId: 4fa14d47b9a1afd998316b0c9024d49760785a47 --- .../app/js/UpdatesProcessor.js | 5 +- .../UpdatesManager/UpdatesProcessorTests.js | 140 ++++++++++++------ 2 files changed, 98 insertions(+), 47 deletions(-) diff --git a/services/project-history/app/js/UpdatesProcessor.js b/services/project-history/app/js/UpdatesProcessor.js index 112600ec6e..b4895c012d 100644 --- a/services/project-history/app/js/UpdatesProcessor.js +++ b/services/project-history/app/js/UpdatesProcessor.js @@ -546,7 +546,10 @@ export function _processUpdates( } if (filteredUpdates.length === 0) { // return early if there are no updates to apply - return SyncManager.setResyncState(projectId, newSyncState, callback) + return SyncManager.setResyncState(projectId, newSyncState, err => { + if (err) return callback(err) + callback(null, { resyncNeeded: false }) + }) } // only make request to history service if we have actual updates to process _getMostRecentVersionWithDebug( diff --git a/services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js b/services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js index c6e1811977..9715310f72 100644 --- a/services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js +++ b/services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js @@ -6,14 +6,14 @@ import * as Errors from '../../../../app/js/Errors.js' const MODULE_PATH = '../../../../app/js/UpdatesProcessor.js' describe('UpdatesProcessor', function () { - before(async function () { + beforeEach(async function () { this.extendLock = sinon.stub() this.BlobManager = { createBlobsForUpdates: sinon.stub(), } this.HistoryStoreManager = { getMostRecentVersion: sinon.stub(), - sendChanges: sinon.stub().yields(null, {}), + sendChanges: sinon.stub().yields(null, { resyncNeeded: true }), } this.LockManager = { runWithLock: sinon.spy((key, runner, callback) => @@ -318,8 +318,8 @@ describe('UpdatesProcessor', function () { this.ol_project_id, this.rawUpdates, this.extendLock, - err => { - this.callback(err) + (err, flushResponse) => { + this.callback(err, flushResponse) done() } ) @@ -385,8 +385,73 @@ describe('UpdatesProcessor', function () { ) }) - it('should call the callback with no error', function () { - this.callback.should.have.been.called + it('should call the callback with no error and flush response', function () { + this.callback.should.have.been.calledWith(null, { resyncNeeded: true }) + }) + }) + + describe('no updates', function () { + beforeEach(function (done) { + this.SyncManager.skipUpdatesDuringSync.yields( + null, + [], + this.newSyncState + ) + this.UpdatesProcessor._processUpdates( + this.project_id, + this.ol_project_id, + this.rawUpdates, + this.extendLock, + (err, flushResponse) => { + this.callback(err, flushResponse) + done() + } + ) + }) + + it('should not get the latest version id', function () { + this.HistoryStoreManager.getMostRecentVersion.should.not.have.been.calledWith( + this.project_id, + this.ol_project_id + ) + }) + + it('should skip updates when resyncing', function () { + this.SyncManager.skipUpdatesDuringSync.should.have.been.calledWith( + this.project_id, + this.rawUpdates + ) + }) + + it('should not expand sync updates', function () { + this.SyncManager.expandSyncUpdates.should.not.have.been.called + }) + + it('should not compress updates', function () { + this.UpdateCompressor.compressRawUpdates.should.not.have.been.called + }) + + it('should not create any blobs for the updates', function () { + this.BlobManager.createBlobsForUpdates.should.not.have.been.called + }) + + it('should not convert the updates into a change requests', function () { + this.UpdateTranslator.convertToChanges.should.not.have.been.called + }) + + it('should not send the change request to the history store', function () { + this.HistoryStoreManager.sendChanges.should.not.have.been.called + }) + + it('should set the sync state', function () { + this.SyncManager.setResyncState.should.have.been.calledWith( + this.project_id, + this.newSyncState + ) + }) + + it('should call the callback with fake flush response', function () { + this.callback.should.have.been.calledWith(null, { resyncNeeded: false }) }) }) @@ -415,7 +480,7 @@ describe('UpdatesProcessor', function () { }) describe('_skipAlreadyAppliedUpdates', function () { - before(function () { + beforeEach(function () { this.UpdateTranslator.isProjectStructureUpdate.callsFake( update => update.version != null ) @@ -423,7 +488,7 @@ describe('UpdatesProcessor', function () { }) describe('with all doc ops in order', function () { - before(function () { + beforeEach(function () { this.updates = [ { doc: 'id', v: 1 }, { doc: 'id', v: 2 }, @@ -443,7 +508,7 @@ describe('UpdatesProcessor', function () { }) describe('with all project ops in order', function () { - before(function () { + beforeEach(function () { this.updates = [ { version: 1 }, { version: 2 }, @@ -463,7 +528,7 @@ describe('UpdatesProcessor', function () { }) describe('with all multiple doc and ops in order', function () { - before(function () { + beforeEach(function () { this.updates = [ { doc: 'id1', v: 1 }, { doc: 'id1', v: 2 }, @@ -491,64 +556,47 @@ describe('UpdatesProcessor', function () { }) describe('with doc ops out of order', function () { - before(function () { + beforeEach(function () { this.updates = [ { doc: 'id', v: 1 }, { doc: 'id', v: 2 }, { doc: 'id', v: 4 }, { doc: 'id', v: 3 }, ] - this.skipFn = sinon.spy( - this.UpdatesProcessor._mocks, - '_skipAlreadyAppliedUpdates' - ) - try { - this.updatesToApply = - this.UpdatesProcessor._skipAlreadyAppliedUpdates( - this.project_id, - this.updates, - { docs: {} } - ) - } catch (error) {} - }) - - after(function () { - this.skipFn.restore() }) it('should throw an exception', function () { - this.skipFn.threw('OpsOutOfOrderError').should.equal(true) + expect(() => { + this.UpdatesProcessor._skipAlreadyAppliedUpdates( + this.project_id, + this.updates, + { docs: {} } + ) + }).to.throw(Errors.OpsOutOfOrderError) }) }) describe('with project ops out of order', function () { - before(function () { + beforeEach(function () { + this.UpdateTranslator.isProjectStructureUpdate.callsFake( + update => update.version != null + ) this.updates = [ { version: 1 }, { version: 2 }, { version: 4 }, { version: 3 }, ] - this.skipFn = sinon.spy( - this.UpdatesProcessor._mocks, - '_skipAlreadyAppliedUpdates' - ) - try { - this.updatesToApply = - this.UpdatesProcessor._skipAlreadyAppliedUpdates( - this.project_id, - this.updates, - { docs: {} } - ) - } catch (error) {} - }) - - after(function () { - this.skipFn.restore() }) it('should throw an exception', function () { - this.skipFn.threw('OpsOutOfOrderError').should.equal(true) + expect(() => { + this.UpdatesProcessor._skipAlreadyAppliedUpdates( + this.project_id, + this.updates, + { docs: {} } + ) + }).to.throw(Errors.OpsOutOfOrderError) }) }) })