From a85149e631bae8360781405a6462623fd1953fb8 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Mon, 12 Sep 2022 17:32:18 -0400 Subject: [PATCH 01/15] fix: pool clear event ordering --- src/cmap/connection_pool.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cmap/connection_pool.ts b/src/cmap/connection_pool.ts index 723b4c8f67..65bbe5c7ea 100644 --- a/src/cmap/connection_pool.ts +++ b/src/cmap/connection_pool.ts @@ -423,11 +423,10 @@ export class ConnectionPool extends TypedEventEmitter { this[kPoolState] = PoolState.paused; this.clearMinPoolSizeTimer(); - this.processWaitQueue(); - if (!alreadyPaused) { this.emit(ConnectionPool.CONNECTION_POOL_CLEARED, new ConnectionPoolClearedEvent(this)); } + this.processWaitQueue(); } /** Close the pool */ From 27986a4febe4f51d762a5ca3646457abf0dce8fa Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Mon, 12 Sep 2022 17:32:40 -0400 Subject: [PATCH 02/15] refactor: remove todo --- src/cmap/errors.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cmap/errors.ts b/src/cmap/errors.ts index c8355ec562..b2892715ad 100644 --- a/src/cmap/errors.ts +++ b/src/cmap/errors.ts @@ -24,7 +24,6 @@ export class PoolClosedError extends MongoDriverError { * @category Error */ export class PoolClearedError extends MongoNetworkError { - // TODO(NODE-3144): needs to extend RetryableError or be marked retryable in some other way per spec /** The address of the connection pool */ address: string; From dd44dc6858d986a355c60a590fa61457a8d641eb Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Mon, 12 Sep 2022 17:33:26 -0400 Subject: [PATCH 03/15] test: add retryable write pool clear prose test --- .../retryable_writes.spec.prose.test.ts | 239 ++++++++++++++---- 1 file changed, 189 insertions(+), 50 deletions(-) diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 2171885f85..b58abbcf28 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -1,64 +1,203 @@ import { expect } from 'chai'; -import { MongoError, MongoServerError, TopologyType } from '../../../src'; +import { Collection, MongoClient, MongoError, MongoServerError, TopologyType } from '../../../src'; describe('Retryable Writes Spec Prose', () => { - /** - * 1 Test that retryable writes raise an exception when using the MMAPv1 storage engine. - * For this test, execute a write operation, such as insertOne, which should generate an exception and the error code is 20. - * Assert that the error message is the replacement error message: - * - * ``` - * This MongoDB deployment does not support retryable writes. Please add - * retryWrites=false to your connection string. - * ``` - * Note: Drivers that rely on serverStatus to determine the storage engine in use MAY skip this test for sharded clusters, since mongos does not report this information in its serverStatus response. - */ - let client; - - beforeEach(async function () { - if ( - this.configuration.buildInfo.versionArray[0] < 4 || - this.configuration.topologyType !== TopologyType.ReplicaSetWithPrimary - ) { - this.currentTest.skipReason = - 'configureFailPoint only works on server versions greater than 4'; - this.skip(); + let client: MongoClient, failPointName; + + afterEach(async () => { + try { + if (failPointName) { + await client.db('admin').command({ configureFailPoint: failPointName, mode: 'off' }); + } + } finally { + failPointName = undefined; + await client?.close(); } - client = this.configuration.newClient(); - await client.connect(); }); - afterEach(async () => { - await client?.close(); + describe('1. Test that retryable writes raise an exception when using the MMAPv1 storage engine.', () => { + /** + * For this test, execute a write operation, such as insertOne, which should generate an exception and the error code is 20. + * Assert that the error message is the replacement error message: + * + * ``` + * This MongoDB deployment does not support retryable writes. Please add + * retryWrites=false to your connection string. + * ``` + * Note: Drivers that rely on serverStatus to determine the storage engine in use MAY skip this test for sharded clusters, since mongos does not report this information in its serverStatus response. + */ + beforeEach(async function () { + if ( + this.configuration.buildInfo.versionArray[0] < 4 || + this.configuration.topologyType !== TopologyType.ReplicaSetWithPrimary + ) { + this.currentTest.skipReason = + 'configureFailPoint only works on server versions greater than 4'; + this.skip(); + } + client = this.configuration.newClient(); + await client.connect(); + + failPointName = 'failCommand'; + const failPoint = await client.db('admin').command({ + configureFailPoint: failPointName, + mode: { times: 1 }, + data: { + failCommands: ['insert'], + errorCode: 20, // MMAP Error code, + closeConnection: false + } + }); + + expect(failPoint).to.have.property('ok', 1); + }); + + it('should error with the correct error message', async () => { + const error = await client + .db('test') + .collection('test') + .insertOne({ a: 1 }) + .catch(error => error); + + expect(error).to.exist; + expect(error).that.is.instanceOf(MongoServerError); + expect(error).to.have.property('originalError').that.instanceOf(MongoError); + expect(error.originalError).to.have.property('code', 20); + expect(error).to.have.property( + 'message', + 'This MongoDB deployment does not support retryable writes. Please add retryWrites=false to your connection string.' + ); + }); }); - it('retryable writes raise an exception when using the MMAPv1 storage engine', async () => { - const failPoint = await client.db('admin').command({ - configureFailPoint: 'failCommand', - mode: { times: 1 }, - data: { - failCommands: ['insert'], - errorCode: 20, // MMAP Error code, - closeConnection: false + describe('2. Test that drivers properly retry after encountering PoolClearedErrors.', () => { + // This test MUST be implemented by any driver that implements the CMAP specification. + // This test requires MongoDB 4.2.9+ for blockConnection support in the failpoint. + + let observedEvents: Array<{ name: string; event: Record }>; + let testCollection: Collection; + beforeEach(async function () { + // i. Create a client with maxPoolSize=1 and retryWrites=true. + // If testing against a sharded deployment, be sure to connect to only a single mongos. <-- TODO: what does that look like? + client = this.configuration.newClient({ + maxPoolSize: 1, + retryWrites: true, + monitorCommands: true + }); + console.log(client.options.retryReads, client.options.retryWrites); + await client.connect(); + + testCollection = client.db('retryable-writes-prose').collection('pool-clear-retry'); + await testCollection.drop().catch(() => null); + + // ii. Enable the following failpoint: + // NOTE: "ix. Disable the failpoint" is done in afterEach + failPointName = 'failCommand'; + const failPoint = await client.db('admin').command({ + configureFailPoint: failPointName, + mode: { times: 1 }, + data: { + failCommands: ['insert'], + errorCode: 91, + blockConnection: true, + blockTimeMS: 1000, + errorLabels: ['RetryableWriteError'] + } + }); + + expect(failPoint).to.have.property('ok', 1); + + observedEvents = []; + for (const observedEvent of [ + 'connectionCheckOutStarted', + 'connectionCheckedOut', + 'connectionCheckOutFailed', + 'connectionPoolCleared', + 'commandStarted' + ]) { + client.on(observedEvent, ev => { + observedEvents.push({ name: observedEvent, event: ev }); + }); } }); - expect(failPoint).to.have.property('ok', 1); - - const error = await client - .db('test') - .collection('test') - .insertOne({ a: 1 }) - .catch(error => error); - - expect(error).to.exist; - expect(error).that.is.instanceOf(MongoServerError); - expect(error).to.have.property('originalError').that.instanceOf(MongoError); - expect(error.originalError).to.have.property('code', 20); - expect(error).to.have.property( - 'message', - 'This MongoDB deployment does not support retryable writes. Please add retryWrites=false to your connection string.' - ); + it('should emit events in the expected sequence', { + metadata: { requires: { mongodb: '>=4.2.9' } }, + test: async function () { + // iii. Start two threads and attempt to perform an insertOne simultaneously on both. + await Promise.all([ + testCollection.insertOne({ test: 1 }), + testCollection.insertOne({ test: 2 }) + ]); + + client.removeAllListeners(); + // iv. Verify that both insertOne attempts succeed. + const result = await testCollection.find().toArray(); + expect(result).to.have.lengthOf(2); + const mappedAndSortedResult = result.map(item => item.test).sort(); + expect(mappedAndSortedResult).to.deep.equal([1, 2]); + + // v. Via CMAP monitoring, assert that the first check out succeeds. + const indexOfFirstCheckoutAttempt = observedEvents.findIndex( + ev => ev.name === 'connectionCheckOutStarted' + ); + expect(indexOfFirstCheckoutAttempt).to.be.greaterThan( + -1, + 'expected a checkout started event to exist' + ); + const indexOfFirstCheckoutSuccess = observedEvents.findIndex( + ev => ev.name === 'connectionCheckedOut' + ); + expect(indexOfFirstCheckoutSuccess).to.be.greaterThan( + -1, + 'expected at least one checkout success' + ); + const indexOfFirstCheckoutFailure = observedEvents.findIndex( + ev => ev.name === 'connectionCheckOutFailed' + ); + expect(indexOfFirstCheckoutFailure).to.be.greaterThan( + -1, + 'expected at least one checkout failure' + ); + + expect(indexOfFirstCheckoutSuccess).to.be.greaterThan( + indexOfFirstCheckoutAttempt, + 'expected checkout started before checkout success' + ); + expect(indexOfFirstCheckoutSuccess).to.be.lessThan( + indexOfFirstCheckoutFailure, + 'expected first connection checkout to succeed but it failed' + ); + + // vi. Via CMAP monitoring, assert that a PoolClearedEvent is then emitted. + const indexOfPoolClear = observedEvents.findIndex( + ev => ev.name === 'connectionPoolCleared' + ); + expect(indexOfPoolClear).to.be.greaterThan( + indexOfFirstCheckoutSuccess, + 'expected a pool cleared event to follow checkout success' + ); + + // vii. Via CMAP monitoring, assert that the second check out then fails due to a connection error. + expect(indexOfFirstCheckoutFailure).to.be.greaterThan( + indexOfPoolClear, + 'expected checkout failure after pool clear' + ); + expect(observedEvents[indexOfFirstCheckoutFailure].event).to.have.property( + 'reason', + 'connectionError' + ); + + // viii. Via Command Monitoring, assert that exactly three insert CommandStartedEvents were observed in total. + const observedInsertCommandStartedEvents = observedEvents.filter(({ name, event }) => { + return name === 'commandStarted' && event.commandName === 'insert'; + }); + expect(observedInsertCommandStartedEvents).to.have.lengthOf( + 3, + 'expected 3 insert command started events' + ); + } + }); }); }); From 3551eeeb7253245680187595e55621a192244fef Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Mon, 12 Sep 2022 17:53:13 -0400 Subject: [PATCH 04/15] test: add retryable reads prose test --- .../retryable_reads.spec.prose.test.ts | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 test/integration/retryable-reads/retryable_reads.spec.prose.test.ts diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts new file mode 100644 index 0000000000..a918b7efe4 --- /dev/null +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -0,0 +1,146 @@ +import { expect } from 'chai'; + +import { Collection, MongoClient } from '../../../src'; + +describe('Retryable Reads Spec Prose', () => { + let client: MongoClient, failPointName; + + afterEach(async () => { + try { + if (failPointName) { + await client.db('admin').command({ configureFailPoint: failPointName, mode: 'off' }); + } + } finally { + failPointName = undefined; + await client?.close(); + } + }); + + describe('PoolClearedError Retryability Test', () => { + // This test will be used to ensure drivers properly retry after encountering PoolClearedErrors. + // It MUST be implemented by any driver that implements the CMAP specification. + // This test requires MongoDB 4.2.9+ for blockConnection support in the failpoint. + + let observedEvents: Array<{ name: string; event: Record }>; + let testCollection: Collection; + beforeEach(async function () { + // 1. Create a client with maxPoolSize=1 and retryReads=true. + // If testing against a sharded deployment, be sure to connect to only a single mongos. <-- TODO: what does that look like? + client = this.configuration.newClient({ + maxPoolSize: 1, + retryReads: true, + monitorCommands: true + }); + await client.connect(); + + testCollection = client.db('retryable-reads-prose').collection('pool-clear-retry'); + await testCollection.drop().catch(() => null); + await testCollection.insertMany([{ test: 1 }, { test: 2 }]); + + // 2. Enable the following failpoint: + // NOTE: "9. Disable the failpoint" is done in afterEach + failPointName = 'failCommand'; + const failPoint = await client.db('admin').command({ + configureFailPoint: failPointName, + mode: { times: 1 }, + data: { + failCommands: ['find'], + errorCode: 91, + blockConnection: true, + blockTimeMS: 1000 + } + }); + + expect(failPoint).to.have.property('ok', 1); + + observedEvents = []; + for (const observedEvent of [ + 'connectionCheckOutStarted', + 'connectionCheckedOut', + 'connectionCheckOutFailed', + 'connectionPoolCleared', + 'commandStarted' + ]) { + client.on(observedEvent, ev => { + observedEvents.push({ name: observedEvent, event: ev }); + }); + } + }); + + it('should emit events in the expected sequence', { + metadata: { requires: { mongodb: '>=4.2.9' } }, + test: async function () { + // 3. Start two threads and attempt to perform a findOne simultaneously on both. + const results = await Promise.all([ + testCollection.findOne({ test: 1 }), + testCollection.findOne({ test: 2 }) + ]); + + client.removeAllListeners(); + // 4. Verify that both findOne attempts succeed. + expect(results[0]).to.have.property('test', 1); + expect(results[1]).to.have.property('test', 2); + + // 5. Via CMAP monitoring, assert that the first check out succeeds. + const indexOfFirstCheckoutAttempt = observedEvents.findIndex( + ev => ev.name === 'connectionCheckOutStarted' + ); + expect(indexOfFirstCheckoutAttempt).to.be.greaterThan( + -1, + 'expected a checkout started event to exist' + ); + const indexOfFirstCheckoutSuccess = observedEvents.findIndex( + ev => ev.name === 'connectionCheckedOut' + ); + expect(indexOfFirstCheckoutSuccess).to.be.greaterThan( + -1, + 'expected at least one checkout success' + ); + const indexOfFirstCheckoutFailure = observedEvents.findIndex( + ev => ev.name === 'connectionCheckOutFailed' + ); + expect(indexOfFirstCheckoutFailure).to.be.greaterThan( + -1, + 'expected at least one checkout failure' + ); + + expect(indexOfFirstCheckoutSuccess).to.be.greaterThan( + indexOfFirstCheckoutAttempt, + 'expected checkout started before checkout success' + ); + expect(indexOfFirstCheckoutSuccess).to.be.lessThan( + indexOfFirstCheckoutFailure, + 'expected first connection checkout to succeed but it failed' + ); + + // 6. Via CMAP monitoring, assert that a PoolClearedEvent is then emitted. + const indexOfPoolClear = observedEvents.findIndex( + ev => ev.name === 'connectionPoolCleared' + ); + expect(indexOfPoolClear).to.be.greaterThan( + indexOfFirstCheckoutSuccess, + 'expected a pool cleared event to follow checkout success' + ); + + // 7. Via CMAP monitoring, assert that the second check out then fails due to a connection error. + expect(indexOfFirstCheckoutFailure).to.be.greaterThan( + indexOfPoolClear, + 'expected checkout failure after pool clear' + ); + expect(observedEvents[indexOfFirstCheckoutFailure].event).to.have.property( + 'reason', + 'connectionError' + ); + + // 8. Via Command Monitoring, assert that exactly three find CommandStartedEvents were observed in total. + const observedInsertCommandStartedEvents = observedEvents.filter(({ name, event }) => { + return name === 'commandStarted' && event.commandName === 'find'; + }); + expect(observedInsertCommandStartedEvents).to.have.lengthOf( + 3, + 'expected 3 find command started events' + ); + } + }); + }); +}); From bea645ae1178854dfaff05bb68b790651bb027b5 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Tue, 13 Sep 2022 11:47:51 -0400 Subject: [PATCH 05/15] test: update topology requirements --- .../retryable-writes/retryable_writes.spec.prose.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index b58abbcf28..63b3808f03 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -85,7 +85,6 @@ describe('Retryable Writes Spec Prose', () => { retryWrites: true, monitorCommands: true }); - console.log(client.options.retryReads, client.options.retryWrites); await client.connect(); testCollection = client.db('retryable-writes-prose').collection('pool-clear-retry'); @@ -123,7 +122,7 @@ describe('Retryable Writes Spec Prose', () => { }); it('should emit events in the expected sequence', { - metadata: { requires: { mongodb: '>=4.2.9' } }, + metadata: { requires: { mongodb: '>=4.2.9', topology: ['replicaset', 'sharded'] } }, test: async function () { // iii. Start two threads and attempt to perform an insertOne simultaneously on both. await Promise.all([ From efea215f975fb4c2934cde8ca402c30efe6333f4 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Tue, 13 Sep 2022 11:48:15 -0400 Subject: [PATCH 06/15] test: standardize retryable writes prose tests --- .../retryable_writes.spec.prose.test.ts | 41 ++++++++----------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 63b3808f03..419927544a 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -28,14 +28,6 @@ describe('Retryable Writes Spec Prose', () => { * Note: Drivers that rely on serverStatus to determine the storage engine in use MAY skip this test for sharded clusters, since mongos does not report this information in its serverStatus response. */ beforeEach(async function () { - if ( - this.configuration.buildInfo.versionArray[0] < 4 || - this.configuration.topologyType !== TopologyType.ReplicaSetWithPrimary - ) { - this.currentTest.skipReason = - 'configureFailPoint only works on server versions greater than 4'; - this.skip(); - } client = this.configuration.newClient(); await client.connect(); @@ -53,21 +45,24 @@ describe('Retryable Writes Spec Prose', () => { expect(failPoint).to.have.property('ok', 1); }); - it('should error with the correct error message', async () => { - const error = await client - .db('test') - .collection('test') - .insertOne({ a: 1 }) - .catch(error => error); - - expect(error).to.exist; - expect(error).that.is.instanceOf(MongoServerError); - expect(error).to.have.property('originalError').that.instanceOf(MongoError); - expect(error.originalError).to.have.property('code', 20); - expect(error).to.have.property( - 'message', - 'This MongoDB deployment does not support retryable writes. Please add retryWrites=false to your connection string.' - ); + it('should error with the correct error message', { + metadata: { requires: { mongodb: '>=4.0.0', topology: ['replicaset', 'sharded'] } }, + test: async function () { + const error = await client + .db('test') + .collection('test') + .insertOne({ a: 1 }) + .catch(error => error); + + expect(error).to.exist; + expect(error).that.is.instanceOf(MongoServerError); + expect(error).to.have.property('originalError').that.instanceOf(MongoError); + expect(error.originalError).to.have.property('code', 20); + expect(error).to.have.property( + 'message', + 'This MongoDB deployment does not support retryable writes. Please add retryWrites=false to your connection string.' + ); + } }); }); From 25110c5a23f219cd33cb692d052f7b4a87956924 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Tue, 13 Sep 2022 12:06:55 -0400 Subject: [PATCH 07/15] test: loadbalanced topology not applicable to retryable reads pool clear test --- .../retryable-reads/retryable_reads.spec.prose.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts index a918b7efe4..3edc3698cd 100644 --- a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -68,7 +68,7 @@ describe('Retryable Reads Spec Prose', () => { }); it('should emit events in the expected sequence', { - metadata: { requires: { mongodb: '>=4.2.9' } }, + metadata: { requires: { mongodb: '>=4.2.9', topology: '!load-balanced' } }, test: async function () { // 3. Start two threads and attempt to perform a findOne simultaneously on both. const results = await Promise.all([ From a79a156e9ace0cc7205a5b17511419c751111fdc Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Tue, 13 Sep 2022 13:26:05 -0400 Subject: [PATCH 08/15] test: correct failpoint min server version --- .../retryable_writes.spec.prose.test.ts | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 419927544a..9f93af5edc 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -1,6 +1,6 @@ import { expect } from 'chai'; -import { Collection, MongoClient, MongoError, MongoServerError, TopologyType } from '../../../src'; +import { Collection, MongoClient, MongoError, MongoServerError } from '../../../src'; describe('Retryable Writes Spec Prose', () => { let client: MongoClient, failPointName; @@ -45,25 +45,28 @@ describe('Retryable Writes Spec Prose', () => { expect(failPoint).to.have.property('ok', 1); }); - it('should error with the correct error message', { - metadata: { requires: { mongodb: '>=4.0.0', topology: ['replicaset', 'sharded'] } }, - test: async function () { - const error = await client - .db('test') - .collection('test') - .insertOne({ a: 1 }) - .catch(error => error); - - expect(error).to.exist; - expect(error).that.is.instanceOf(MongoServerError); - expect(error).to.have.property('originalError').that.instanceOf(MongoError); - expect(error.originalError).to.have.property('code', 20); - expect(error).to.have.property( - 'message', - 'This MongoDB deployment does not support retryable writes. Please add retryWrites=false to your connection string.' - ); - } - }); + for (const testTopology of ['replicaset', 'sharded']) { + const minFailPointVersion = testTopology === 'replicaset' ? '>=4.0.0' : '>=4.1.5'; + it(`should error with the correct error message when topology is ${testTopology}`, { + metadata: { requires: { mongodb: minFailPointVersion, topology: [testTopology as any] } }, + test: async function () { + const error = await client + .db('test') + .collection('test') + .insertOne({ a: 1 }) + .catch(error => error); + + expect(error).to.exist; + expect(error).that.is.instanceOf(MongoServerError); + expect(error).to.have.property('originalError').that.instanceOf(MongoError); + expect(error.originalError).to.have.property('code', 20); + expect(error).to.have.property( + 'message', + 'This MongoDB deployment does not support retryable writes. Please add retryWrites=false to your connection string.' + ); + } + }); + } }); describe('2. Test that drivers properly retry after encountering PoolClearedErrors.', () => { From 0aee9e2d958b1f4ba97731b8a21b786c8d6a3c7b Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Wed, 14 Sep 2022 10:11:28 -0400 Subject: [PATCH 09/15] test: clean up before logic --- .../retryable-reads/retryable_reads.spec.prose.test.ts | 1 - .../retryable-writes/retryable_writes.spec.prose.test.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts index 3edc3698cd..98cdc47427 100644 --- a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -31,7 +31,6 @@ describe('Retryable Reads Spec Prose', () => { retryReads: true, monitorCommands: true }); - await client.connect(); testCollection = client.db('retryable-reads-prose').collection('pool-clear-retry'); await testCollection.drop().catch(() => null); diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 9f93af5edc..402a1a7246 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -83,7 +83,6 @@ describe('Retryable Writes Spec Prose', () => { retryWrites: true, monitorCommands: true }); - await client.connect(); testCollection = client.db('retryable-writes-prose').collection('pool-clear-retry'); await testCollection.drop().catch(() => null); From 310f87d8d4eaced8bb74122c4a9411af5a58567d Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Wed, 14 Sep 2022 10:54:24 -0400 Subject: [PATCH 10/15] test: clean up assertions --- .../retryable_reads.spec.prose.test.ts | 89 +++++++++---------- .../retryable_writes.spec.prose.test.ts | 86 ++++++++---------- 2 files changed, 78 insertions(+), 97 deletions(-) diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts index 98cdc47427..862267462e 100644 --- a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -21,7 +21,8 @@ describe('Retryable Reads Spec Prose', () => { // It MUST be implemented by any driver that implements the CMAP specification. // This test requires MongoDB 4.2.9+ for blockConnection support in the failpoint. - let observedEvents: Array<{ name: string; event: Record }>; + let cmapEvents: Array<{ name: string; event: Record }>; + let commandStartedEvents: Array>; let testCollection: Collection; beforeEach(async function () { // 1. Create a client with maxPoolSize=1 and retryReads=true. @@ -52,18 +53,21 @@ describe('Retryable Reads Spec Prose', () => { expect(failPoint).to.have.property('ok', 1); - observedEvents = []; + cmapEvents = []; for (const observedEvent of [ 'connectionCheckOutStarted', 'connectionCheckedOut', 'connectionCheckOutFailed', - 'connectionPoolCleared', - 'commandStarted' + 'connectionPoolCleared' ]) { client.on(observedEvent, ev => { - observedEvents.push({ name: observedEvent, event: ev }); + cmapEvents.push({ name: observedEvent, event: ev }); }); } + + client.on('commandStarted', ev => { + commandStartedEvents.push(ev); + }); }); it('should emit events in the expected sequence', { @@ -80,62 +84,49 @@ describe('Retryable Reads Spec Prose', () => { expect(results[0]).to.have.property('test', 1); expect(results[1]).to.have.property('test', 2); + // NOTE: For the subsequent checks, we rely on the exact sequence of ALL events + // for ease of readability; however, only the relative order matters for + // the purposes of this test, so if this ever becomes an issue, the test + // can be refactored to assert on relative index values instead + // 5. Via CMAP monitoring, assert that the first check out succeeds. - const indexOfFirstCheckoutAttempt = observedEvents.findIndex( - ev => ev.name === 'connectionCheckOutStarted' - ); - expect(indexOfFirstCheckoutAttempt).to.be.greaterThan( - -1, - 'expected a checkout started event to exist' + expect(cmapEvents.shift()).to.have.property( + 'name', + 'connectionCheckOutStarted', + 'expected 1) checkout 1 to start' ); - const indexOfFirstCheckoutSuccess = observedEvents.findIndex( - ev => ev.name === 'connectionCheckedOut' + expect(cmapEvents.shift()).to.have.property( + 'name', + 'connectionCheckOutStarted', + 'expected 2) checkout 2 to start' ); - expect(indexOfFirstCheckoutSuccess).to.be.greaterThan( - -1, - 'expected at least one checkout success' - ); - const indexOfFirstCheckoutFailure = observedEvents.findIndex( - ev => ev.name === 'connectionCheckOutFailed' - ); - expect(indexOfFirstCheckoutFailure).to.be.greaterThan( - -1, - 'expected at least one checkout failure' - ); - - expect(indexOfFirstCheckoutSuccess).to.be.greaterThan( - indexOfFirstCheckoutAttempt, - 'expected checkout started before checkout success' - ); - expect(indexOfFirstCheckoutSuccess).to.be.lessThan( - indexOfFirstCheckoutFailure, - 'expected first connection checkout to succeed but it failed' + expect(cmapEvents.shift()).to.have.property( + 'name', + 'connectionCheckOutStarted', + 'expected 3) first checkout to succeed' ); // 6. Via CMAP monitoring, assert that a PoolClearedEvent is then emitted. - const indexOfPoolClear = observedEvents.findIndex( - ev => ev.name === 'connectionPoolCleared' - ); - expect(indexOfPoolClear).to.be.greaterThan( - indexOfFirstCheckoutSuccess, - 'expected a pool cleared event to follow checkout success' + expect(cmapEvents.shift()).to.have.property( + 'name', + 'connectionPoolCleared', + 'expected 4) pool to clear' ); // 7. Via CMAP monitoring, assert that the second check out then fails due to a connection error. - expect(indexOfFirstCheckoutFailure).to.be.greaterThan( - indexOfPoolClear, - 'expected checkout failure after pool clear' - ); - expect(observedEvents[indexOfFirstCheckoutFailure].event).to.have.property( - 'reason', - 'connectionError' + const nextEvent = cmapEvents.shift(); + expect(nextEvent).to.have.property( + 'name', + 'connectionCheckOutFailed', + 'expected 5) checkout 2 to fail' ); + expect(nextEvent).to.have.deep.property('event.reason', 'connectionError'); // 8. Via Command Monitoring, assert that exactly three find CommandStartedEvents were observed in total. - const observedInsertCommandStartedEvents = observedEvents.filter(({ name, event }) => { - return name === 'commandStarted' && event.commandName === 'find'; - }); - expect(observedInsertCommandStartedEvents).to.have.lengthOf( + const observedFindCommandStartedEvents = commandStartedEvents.filter( + ({ commandName }) => commandName === 'find' + ); + expect(observedFindCommandStartedEvents).to.have.lengthOf( 3, 'expected 3 find command started events' ); diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 402a1a7246..3478d9744d 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -73,7 +73,8 @@ describe('Retryable Writes Spec Prose', () => { // This test MUST be implemented by any driver that implements the CMAP specification. // This test requires MongoDB 4.2.9+ for blockConnection support in the failpoint. - let observedEvents: Array<{ name: string; event: Record }>; + let cmapEvents: Array<{ name: string; event: Record }>; + let commandStartedEvents: Array>; let testCollection: Collection; beforeEach(async function () { // i. Create a client with maxPoolSize=1 and retryWrites=true. @@ -104,18 +105,20 @@ describe('Retryable Writes Spec Prose', () => { expect(failPoint).to.have.property('ok', 1); - observedEvents = []; + cmapEvents = []; for (const observedEvent of [ 'connectionCheckOutStarted', 'connectionCheckedOut', 'connectionCheckOutFailed', - 'connectionPoolCleared', - 'commandStarted' + 'connectionPoolCleared' ]) { client.on(observedEvent, ev => { - observedEvents.push({ name: observedEvent, event: ev }); + cmapEvents.push({ name: observedEvent, event: ev }); }); } + client.on('commandStarted', ev => { + commandStartedEvents.push(ev); + }); }); it('should emit events in the expected sequence', { @@ -134,61 +137,48 @@ describe('Retryable Writes Spec Prose', () => { const mappedAndSortedResult = result.map(item => item.test).sort(); expect(mappedAndSortedResult).to.deep.equal([1, 2]); + // NOTE: For the subsequent checks, we rely on the exact sequence of ALL events + // for ease of readability; however, only the relative order matters for + // the purposes of this test, so if this ever becomes an issue, the test + // can be refactored to assert on relative index values instead + // v. Via CMAP monitoring, assert that the first check out succeeds. - const indexOfFirstCheckoutAttempt = observedEvents.findIndex( - ev => ev.name === 'connectionCheckOutStarted' - ); - expect(indexOfFirstCheckoutAttempt).to.be.greaterThan( - -1, - 'expected a checkout started event to exist' - ); - const indexOfFirstCheckoutSuccess = observedEvents.findIndex( - ev => ev.name === 'connectionCheckedOut' - ); - expect(indexOfFirstCheckoutSuccess).to.be.greaterThan( - -1, - 'expected at least one checkout success' - ); - const indexOfFirstCheckoutFailure = observedEvents.findIndex( - ev => ev.name === 'connectionCheckOutFailed' + expect(cmapEvents.shift()).to.have.property( + 'name', + 'connectionCheckOutStarted', + 'expected 1) checkout 1 to start' ); - expect(indexOfFirstCheckoutFailure).to.be.greaterThan( - -1, - 'expected at least one checkout failure' + expect(cmapEvents.shift()).to.have.property( + 'name', + 'connectionCheckOutStarted', + 'expected 2) checkout 2 to start' ); - - expect(indexOfFirstCheckoutSuccess).to.be.greaterThan( - indexOfFirstCheckoutAttempt, - 'expected checkout started before checkout success' - ); - expect(indexOfFirstCheckoutSuccess).to.be.lessThan( - indexOfFirstCheckoutFailure, - 'expected first connection checkout to succeed but it failed' + expect(cmapEvents.shift()).to.have.property( + 'name', + 'connectionCheckOutStarted', + 'expected 3) first checkout to succeed' ); // vi. Via CMAP monitoring, assert that a PoolClearedEvent is then emitted. - const indexOfPoolClear = observedEvents.findIndex( - ev => ev.name === 'connectionPoolCleared' - ); - expect(indexOfPoolClear).to.be.greaterThan( - indexOfFirstCheckoutSuccess, - 'expected a pool cleared event to follow checkout success' + expect(cmapEvents.shift()).to.have.property( + 'name', + 'connectionPoolCleared', + 'expected 4) pool to clear' ); // vii. Via CMAP monitoring, assert that the second check out then fails due to a connection error. - expect(indexOfFirstCheckoutFailure).to.be.greaterThan( - indexOfPoolClear, - 'expected checkout failure after pool clear' - ); - expect(observedEvents[indexOfFirstCheckoutFailure].event).to.have.property( - 'reason', - 'connectionError' + const nextEvent = cmapEvents.shift(); + expect(nextEvent).to.have.property( + 'name', + 'connectionCheckOutFailed', + 'expected 5) checkout 2 to fail' ); + expect(nextEvent).to.have.deep.property('event.reason', 'connectionError'); // viii. Via Command Monitoring, assert that exactly three insert CommandStartedEvents were observed in total. - const observedInsertCommandStartedEvents = observedEvents.filter(({ name, event }) => { - return name === 'commandStarted' && event.commandName === 'insert'; - }); + const observedInsertCommandStartedEvents = commandStartedEvents.filter( + ({ commandName }) => commandName === 'insert' + ); expect(observedInsertCommandStartedEvents).to.have.lengthOf( 3, 'expected 3 insert command started events' From a0434d43506267455d443481c0ec0d9329e570d6 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Wed, 14 Sep 2022 12:02:23 -0400 Subject: [PATCH 11/15] test: fix array initialization --- .../retryable-reads/retryable_reads.spec.prose.test.ts | 1 + .../retryable-writes/retryable_writes.spec.prose.test.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts index 862267462e..1a96f4fb52 100644 --- a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -54,6 +54,7 @@ describe('Retryable Reads Spec Prose', () => { expect(failPoint).to.have.property('ok', 1); cmapEvents = []; + commandStartedEvents = []; for (const observedEvent of [ 'connectionCheckOutStarted', 'connectionCheckedOut', diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 3478d9744d..c5d85d8633 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -106,6 +106,7 @@ describe('Retryable Writes Spec Prose', () => { expect(failPoint).to.have.property('ok', 1); cmapEvents = []; + commandStartedEvents = []; for (const observedEvent of [ 'connectionCheckOutStarted', 'connectionCheckedOut', From bdf3cd704ee7bb4439330a15be28467cd2574ba8 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Wed, 14 Sep 2022 12:16:40 -0400 Subject: [PATCH 12/15] test: fix expected event name --- .../retryable-reads/retryable_reads.spec.prose.test.ts | 2 +- .../retryable-writes/retryable_writes.spec.prose.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts index 1a96f4fb52..4451c3eab2 100644 --- a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -103,7 +103,7 @@ describe('Retryable Reads Spec Prose', () => { ); expect(cmapEvents.shift()).to.have.property( 'name', - 'connectionCheckOutStarted', + 'connectionCheckedOut', 'expected 3) first checkout to succeed' ); diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index c5d85d8633..201555a21b 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -156,7 +156,7 @@ describe('Retryable Writes Spec Prose', () => { ); expect(cmapEvents.shift()).to.have.property( 'name', - 'connectionCheckOutStarted', + 'connectionCheckedOut', 'expected 3) first checkout to succeed' ); From 52d94477d09ab08998bafd495f3fa31cf5542702 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Wed, 14 Sep 2022 14:41:22 -0400 Subject: [PATCH 13/15] test: fix property access --- .../retryable-reads/retryable_reads.spec.prose.test.ts | 3 ++- .../retryable-writes/retryable_writes.spec.prose.test.ts | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts index 4451c3eab2..b908bb6666 100644 --- a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/no-non-null-assertion */ import { expect } from 'chai'; import { Collection, MongoClient } from '../../../src'; @@ -121,7 +122,7 @@ describe('Retryable Reads Spec Prose', () => { 'connectionCheckOutFailed', 'expected 5) checkout 2 to fail' ); - expect(nextEvent).to.have.deep.property('event.reason', 'connectionError'); + expect(nextEvent!.event).to.have.property('reason', 'connectionError'); // 8. Via Command Monitoring, assert that exactly three find CommandStartedEvents were observed in total. const observedFindCommandStartedEvents = commandStartedEvents.filter( diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 201555a21b..1de444a3f0 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/no-non-null-assertion */ import { expect } from 'chai'; import { Collection, MongoClient, MongoError, MongoServerError } from '../../../src'; @@ -174,7 +175,7 @@ describe('Retryable Writes Spec Prose', () => { 'connectionCheckOutFailed', 'expected 5) checkout 2 to fail' ); - expect(nextEvent).to.have.deep.property('event.reason', 'connectionError'); + expect(nextEvent!.event).to.have.property('reason', 'connectionError'); // viii. Via Command Monitoring, assert that exactly three insert CommandStartedEvents were observed in total. const observedInsertCommandStartedEvents = commandStartedEvents.filter( From 746adfa59c200da84a6f0a9f35229d2e6983d8d0 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Wed, 14 Sep 2022 17:24:52 -0400 Subject: [PATCH 14/15] test: ensure single mongos --- .../retryable-reads/retryable_reads.spec.prose.test.ts | 4 ++-- .../retryable-writes/retryable_writes.spec.prose.test.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts index b908bb6666..afe095676d 100644 --- a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -27,11 +27,11 @@ describe('Retryable Reads Spec Prose', () => { let testCollection: Collection; beforeEach(async function () { // 1. Create a client with maxPoolSize=1 and retryReads=true. - // If testing against a sharded deployment, be sure to connect to only a single mongos. <-- TODO: what does that look like? client = this.configuration.newClient({ maxPoolSize: 1, retryReads: true, - monitorCommands: true + monitorCommands: true, + useMultipleMongoses: false // If testing against a sharded deployment, be sure to connect to only a single mongos. }); testCollection = client.db('retryable-reads-prose').collection('pool-clear-retry'); diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 1de444a3f0..688b9d47c6 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -79,11 +79,11 @@ describe('Retryable Writes Spec Prose', () => { let testCollection: Collection; beforeEach(async function () { // i. Create a client with maxPoolSize=1 and retryWrites=true. - // If testing against a sharded deployment, be sure to connect to only a single mongos. <-- TODO: what does that look like? client = this.configuration.newClient({ maxPoolSize: 1, retryWrites: true, - monitorCommands: true + monitorCommands: true, + useMultipleMongoses: false // If testing against a sharded deployment, be sure to connect to only a single mongos. }); testCollection = client.db('retryable-writes-prose').collection('pool-clear-retry'); From 10f42932095e1a5313190ae2dea368e7b96cbb7c Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Wed, 14 Sep 2022 17:53:42 -0400 Subject: [PATCH 15/15] test: correctly wrap options --- .../retryable_reads.spec.prose.test.ts | 14 ++++++++------ .../retryable_writes.spec.prose.test.ts | 12 ++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts index afe095676d..af6f9cbed4 100644 --- a/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts +++ b/test/integration/retryable-reads/retryable_reads.spec.prose.test.ts @@ -27,12 +27,14 @@ describe('Retryable Reads Spec Prose', () => { let testCollection: Collection; beforeEach(async function () { // 1. Create a client with maxPoolSize=1 and retryReads=true. - client = this.configuration.newClient({ - maxPoolSize: 1, - retryReads: true, - monitorCommands: true, - useMultipleMongoses: false // If testing against a sharded deployment, be sure to connect to only a single mongos. - }); + client = this.configuration.newClient( + this.configuration.url({ + useMultipleMongoses: false // If testing against a sharded deployment, be sure to connect to only a single mongos. + }), + { maxPoolSize: 1, retryReads: true, monitorCommands: true } + ); + + console.log(client.options); testCollection = client.db('retryable-reads-prose').collection('pool-clear-retry'); await testCollection.drop().catch(() => null); diff --git a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts index 688b9d47c6..e0fa383fb1 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.prose.test.ts @@ -79,12 +79,12 @@ describe('Retryable Writes Spec Prose', () => { let testCollection: Collection; beforeEach(async function () { // i. Create a client with maxPoolSize=1 and retryWrites=true. - client = this.configuration.newClient({ - maxPoolSize: 1, - retryWrites: true, - monitorCommands: true, - useMultipleMongoses: false // If testing against a sharded deployment, be sure to connect to only a single mongos. - }); + client = this.configuration.newClient( + this.configuration.url({ + useMultipleMongoses: false // If testing against a sharded deployment, be sure to connect to only a single mongos. + }), + { maxPoolSize: 1, retryWrites: true, monitorCommands: true } + ); testCollection = client.db('retryable-writes-prose').collection('pool-clear-retry'); await testCollection.drop().catch(() => null);