From 6bd50387917fc07f9b6df68bb496e5f4b5b79c2e Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 26 Feb 2024 13:44:01 +0000 Subject: [PATCH] Merge pull request #17282 from overleaf/bg-object-id-unit-tests-ii add unit tests for ObjectId comparison with chai and sinon GitOrigin-RevId: e23156f6fd95f37d447f7569a01916c71bf04ede --- services/web/test/acceptance/bootstrap.js | 3 + services/web/test/unit/bootstrap.js | 4 + .../Project/FolderStructureBuilderTests.js | 30 ++-- .../unit/src/infrastructure/MongodbTests.js | 152 ++++++++++++++++++ 4 files changed, 169 insertions(+), 20 deletions(-) create mode 100644 services/web/test/unit/src/infrastructure/MongodbTests.js diff --git a/services/web/test/acceptance/bootstrap.js b/services/web/test/acceptance/bootstrap.js index 5efecd625b..e65200eafe 100644 --- a/services/web/test/acceptance/bootstrap.js +++ b/services/web/test/acceptance/bootstrap.js @@ -6,3 +6,6 @@ chai.use(require('chai-exclude')) // Do not truncate assertion errors chai.config.truncateThreshold = 0 + +// ensure every ObjectId has the id string as a property for correct comparisons +require('mongodb').ObjectId.cacheHexString = true diff --git a/services/web/test/unit/bootstrap.js b/services/web/test/unit/bootstrap.js index f502bda305..65fb238eb8 100644 --- a/services/web/test/unit/bootstrap.js +++ b/services/web/test/unit/bootstrap.js @@ -22,6 +22,9 @@ chai.config.truncateThreshold = 0 // add support for mongoose in sinon require('sinon-mongoose') +// ensure every ObjectId has the id string as a property for correct comparisons +require('mongodb').ObjectId.cacheHexString = true + /* * Global stubs */ @@ -78,6 +81,7 @@ function getSandboxedModuleRequires() { 'sanitize-html', 'sshpk', 'xml2js', + 'mongodb', ] for (const modulePath of internalModules) { requires[Path.resolve(__dirname, modulePath)] = require(modulePath) diff --git a/services/web/test/unit/src/Project/FolderStructureBuilderTests.js b/services/web/test/unit/src/Project/FolderStructureBuilderTests.js index 324ce7b2d1..fc09bfb056 100644 --- a/services/web/test/unit/src/Project/FolderStructureBuilderTests.js +++ b/services/web/test/unit/src/Project/FolderStructureBuilderTests.js @@ -1,24 +1,14 @@ const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') const { ObjectId } = require('mongodb') +const sinon = require('sinon') const MODULE_PATH = '../../../../app/src/Features/Project/FolderStructureBuilder' -const MOCK_OBJECT_ID = new ObjectId('657306930a1cf28031c358da') describe('FolderStructureBuilder', function () { beforeEach(function () { - this.FolderStructureBuilder = SandboxedModule.require(MODULE_PATH, { - requires: { - mongodb: { - ObjectId: class { - constructor() { - return MOCK_OBJECT_ID - } - }, - }, - }, - }) + this.FolderStructureBuilder = SandboxedModule.require(MODULE_PATH, {}) }) describe('buildFolderStructure', function () { @@ -28,8 +18,8 @@ describe('FolderStructureBuilder', function () { }) it('returns an empty root folder', function () { - expect(this.result).to.deep.equal({ - _id: MOCK_OBJECT_ID, + sinon.assert.match(this.result, { + _id: sinon.match.instanceOf(ObjectId), name: 'rootFolder', folders: [], docs: [], @@ -61,14 +51,14 @@ describe('FolderStructureBuilder', function () { }) it('returns a full folder structure', function () { - expect(this.result).to.deep.equal({ - _id: MOCK_OBJECT_ID, + sinon.assert.match(this.result, { + _id: sinon.match.instanceOf(ObjectId), name: 'rootFolder', docs: [{ _id: 'doc-1', name: 'main.tex' }], fileRefs: [{ _id: 'file-1', name: 'aaa.jpg' }], folders: [ { - _id: MOCK_OBJECT_ID, + _id: sinon.match.instanceOf(ObjectId), name: 'foo', docs: [ { _id: 'doc-2', name: 'other.tex' }, @@ -77,13 +67,13 @@ describe('FolderStructureBuilder', function () { fileRefs: [{ _id: 'file-2', name: 'bbb.jpg' }], folders: [ { - _id: MOCK_OBJECT_ID, + _id: sinon.match.instanceOf(ObjectId), name: 'foo1', docs: [], fileRefs: [], folders: [ { - _id: MOCK_OBJECT_ID, + _id: sinon.match.instanceOf(ObjectId), name: 'foo2', docs: [{ _id: 'doc-4', name: 'another.tex' }], fileRefs: [], @@ -94,7 +84,7 @@ describe('FolderStructureBuilder', function () { ], }, { - _id: MOCK_OBJECT_ID, + _id: sinon.match.instanceOf(ObjectId), name: 'bar', docs: [], fileRefs: [{ _id: 'file-3', name: 'ccc.jpg' }], diff --git a/services/web/test/unit/src/infrastructure/MongodbTests.js b/services/web/test/unit/src/infrastructure/MongodbTests.js new file mode 100644 index 0000000000..9c87c6ce8a --- /dev/null +++ b/services/web/test/unit/src/infrastructure/MongodbTests.js @@ -0,0 +1,152 @@ +const { ObjectId } = require('mongodb') +const { expect } = require('chai') +const sinon = require('sinon') + +describe('Mongo ObjectId comparison', function () { + const ObjectId1 = new ObjectId('111111111111111111111111') + const ObjectId2 = new ObjectId('65d8607441ef95170e89bd2a') + const ObjectId1a = new ObjectId('111111111111111111111111') + const ObjectId1b = new ObjectId('111111111111111111111111') + + const string1 = ObjectId1.toString() + const string2 = ObjectId2.toString() + const number1 = Number(ObjectId1) + const repr1 = 'new ObjectId("111111111111111111111111")' + + describe('using chai', function () { + describe('equal (===)', function () { + it('object ids should equal themselves', function () { + expect(ObjectId1).to.equal(ObjectId1) + expect(ObjectId2).to.equal(ObjectId2) + }) + it('different objects with the same id should not be equal', function () { + expect(ObjectId1a).to.not.equal(ObjectId1b) + }) + it('different object ids should not be equal', function () { + expect(ObjectId1).to.not.equal(ObjectId2) + }) + it('object id should not equal a string with the same id (left operand)', function () { + expect(ObjectId1).to.not.equal(string1) + }) + it('object id should not equal a string with the same id (right operand)', function () { + expect(string1).to.not.equal(ObjectId1) + }) + it('object id should not equal a string with a different id', function () { + expect(ObjectId1).to.not.equal(string2) + }) + it('object id should not equal a number with the same id (left operand)', function () { + expect(ObjectId1).to.not.equal(number1) + }) + it('object id should not equal a number with the same id (right operand)', function () { + expect(number1).to.not.equal(ObjectId1) + }) + it('object id should not equal the string representation with the same id (left operand)', function () { + expect(ObjectId1).to.not.equal(repr1) + }) + it('object id should not equal the string representation with the same id (right operand)', function () { + expect(repr1).to.not.equal(ObjectId1) + }) + }) + + describe('deep equal', function () { + it('object ids should deep equal themselves', function () { + expect(ObjectId1).to.deep.equal(ObjectId1) + expect(ObjectId2).to.deep.equal(ObjectId2) + }) + it('different objects with the same id should be deep equal', function () { + expect(ObjectId1a).to.deep.equal(ObjectId1b) + }) + it('different object ids should not be deep equal', function () { + expect(ObjectId1).to.not.deep.equal(ObjectId2) + }) + it('object id should not deep equal a string with the same id (left operand)', function () { + expect(ObjectId1).to.not.deep.equal(string1) + }) + it('object id should not deep equal a string with the same id (right operand)', function () { + expect(string1).to.not.deep.equal(ObjectId1) + }) + it('object id should not deep equal a string with a different id', function () { + expect(ObjectId1).to.not.deep.equal(string2) + }) + it('object id should not deep equal a number with the same id (left operand)', function () { + expect(ObjectId1).to.not.deep.equal(number1) + }) + it('object id should not deep equal a number with the same id (right operand)', function () { + expect(number1).to.not.deep.equal(ObjectId1) + }) + it('object id should not deep equal the string representation with the same id (left operand)', function () { + expect(ObjectId1).to.not.deep.equal(repr1) + }) + it('object id should not deep equal the string representation with the same id (right operand)', function () { + expect(repr1).to.not.deep.equal(ObjectId1) + }) + }) + }) + describe('using sinon', function () { + describe('match', function () { + it('object ids should match themselves', function () { + sinon.assert.match(ObjectId1, ObjectId1) + sinon.assert.match(ObjectId2, ObjectId2) + }) + it('different objects with the same id should match', function () { + sinon.assert.match(ObjectId1a, ObjectId1b) + }) + it('different object ids should not match', function () { + expect(() => { + sinon.assert.match(ObjectId1, ObjectId2) + }).to.throw() + }) + it('object id should not match a string with the same id (left operand)', function () { + expect(() => { + sinon.assert.match(ObjectId1, string1) + }).to.throw() + }) + it('object id should not match a string with the same id (right operand)', function () { + expect(() => { + sinon.assert.match(string1, ObjectId1) + }).to.throw() + }) + it('object id should not match a string with a different id', function () { + expect(() => { + sinon.assert.match(ObjectId1, string2) + }).to.throw() + }) + it('object id should not match a number with the same id (left operand)', function () { + // This assertion fails because ObjectId2 becomes NaN when coerced to a number. + expect(() => { + sinon.assert.match(ObjectId2, 123) + }).to.throw() + }) + it('object id should not match a number with the same id (left operand) but does match when the ObjectId has decimal digits only', function () { + // We would want this assertion to fail, but ObjectId1 becomes 1.1111...e+23 + // when coerced to a number, and this can match a number with the same value. + // + // For an(object, number) comparison sinon coerces the object to a number using == + // which takes the string representation of the object id and converts it to a number + // via .valueOf. Most of the time this gives NaN because the object ids + // are hexadecimal but if the ObjectId is a valid **decimal** number then it will + // be coerced to that number. This behaviour is by design in sinon but I think we + // can live with it because it is unlikely that we will compare an ObjectId to a + // number and get a false positive. + expect(() => { + sinon.assert.match(ObjectId1, number1) + }).to.not.throw() + }) + it('object id should not match a number with the same id (right operand)', function () { + expect(() => { + sinon.assert.match(number1, ObjectId1) + }).to.throw() + }) + it('object id should not match the string representation with the same id (left operand)', function () { + expect(() => { + sinon.assert.match(ObjectId1, repr1) + }).to.throw() + }) + it('object id should not match the string representation with the same id (right operand)', function () { + expect(() => { + sinon.assert.match(repr1, ObjectId1) + }).to.throw() + }) + }) + }) +})