From 1f2e5c48942e7b797a0c1c13c7f656ae3e34b9c9 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 8 Mar 2021 18:05:16 -0500 Subject: [PATCH 1/2] test: complete unified test runner Completes the unified runner by passing all the poc spec tests. NODE-2287 --- src/apm.ts | 2 +- src/mongo_client.ts | 51 ++++++++++++------- src/sdam/topology.ts | 4 +- src/sessions.ts | 30 +++++++---- .../unified-spec-runner/entities.ts | 4 +- .../unified-spec-runner/operations.ts | 4 +- test/functional/unified-spec-runner/runner.ts | 38 +++----------- .../unified-runner.test.ts | 21 ++++---- 8 files changed, 78 insertions(+), 76 deletions(-) diff --git a/src/apm.ts b/src/apm.ts index d3c7f9ec6da..80414202a0d 100644 --- a/src/apm.ts +++ b/src/apm.ts @@ -30,7 +30,7 @@ export class Instrumentation extends EventEmitter { const instrumentation = this; mongoClientClass.prototype.connect = function (this: MongoClient, callback: Callback) { // override monitorCommands to be switched on - this.s.options = { ...(this.s.options ?? {}), monitorCommands: true }; + this.monitorCommands = true; this.on(Connection.COMMAND_STARTED, event => instrumentation.emit(Instrumentation.STARTED, event) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 034e305ca26..c052f4399e5 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -230,14 +230,14 @@ export type WithSessionCallback = (session: ClientSession) => Promise | voi /** @internal */ export interface MongoClientPrivate { url: string; - options?: MongoClientOptions; sessions: Set; - readConcern?: ReadConcern; - writeConcern?: WriteConcern; - readPreference: ReadPreference; bsonOptions: BSONSerializeOptions; namespace: MongoDBNamespace; - logger: Logger; + readonly options?: MongoOptions; + readonly readConcern?: ReadConcern; + readonly writeConcern?: WriteConcern; + readonly readPreference: ReadPreference; + readonly logger: Logger; } const kOptions = Symbol('options'); @@ -293,29 +293,36 @@ export class MongoClient extends EventEmitter { */ [kOptions]: MongoOptions; - // debugging - originalUri; - originalOptions; - constructor(url: string, options?: MongoClientOptions) { super(); - this.originalUri = url; - this.originalOptions = options; - this[kOptions] = parseOptions(url, this, options); + // eslint-disable-next-line @typescript-eslint/no-this-alias + const client = this; + // The internal state this.s = { url, - options: this[kOptions], sessions: new Set(), - readConcern: this[kOptions].readConcern, - writeConcern: this[kOptions].writeConcern, - readPreference: this[kOptions].readPreference, bsonOptions: resolveBSONOptions(this[kOptions]), namespace: ns('admin'), - logger: this[kOptions].logger + + get options() { + return client[kOptions]; + }, + get readConcern() { + return client[kOptions].readConcern; + }, + get writeConcern() { + return client[kOptions].writeConcern; + }, + get readPreference() { + return client[kOptions].readPreference; + }, + get logger() { + return client[kOptions].logger; + } }; } @@ -326,6 +333,16 @@ export class MongoClient extends EventEmitter { get serverApi(): Readonly { return this[kOptions].serverApi && Object.freeze({ ...this[kOptions].serverApi }); } + /** + * Intended for APM use only + * @internal + */ + get monitorCommands(): boolean { + return this[kOptions].monitorCommands; + } + set monitorCommands(value: boolean) { + this[kOptions].monitorCommands = value; + } get autoEncrypter(): AutoEncrypter | undefined { return this[kOptions].autoEncrypter; diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 93b348aee1c..7fc33980d83 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -50,7 +50,7 @@ import type { MongoCredentials } from '../cmap/auth/mongo_credentials'; import type { Transaction } from '../transactions'; import type { CloseOptions } from '../cmap/connection_pool'; import { DestroyOptions, Connection } from '../cmap/connection'; -import type { MongoClientOptions, ServerApi } from '../mongo_client'; +import type { MongoOptions, ServerApi } from '../mongo_client'; import { DEFAULT_OPTIONS } from '../connection_string'; import { serialize, deserialize } from '../bson'; @@ -566,7 +566,7 @@ export class Topology extends EventEmitter { } /** Start a logical session */ - startSession(options: ClientSessionOptions, clientOptions?: MongoClientOptions): ClientSession { + startSession(options: ClientSessionOptions, clientOptions?: MongoOptions): ClientSession { const session = new ClientSession(this, this.s.sessionPool, options, clientOptions); session.once('ended', () => { this.s.sessions.delete(session); diff --git a/src/sessions.ts b/src/sessions.ts index 53900e7f3fb..fe6d708416b 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -22,7 +22,7 @@ import { maybePromise } from './utils'; import type { Topology } from './sdam/topology'; -import type { MongoClientOptions } from './mongo_client'; +import type { MongoOptions } from './mongo_client'; import { executeOperation } from './operations/execute_operation'; import { RunAdminCommandOperation } from './operations/run_command'; import type { AbstractCursor } from './cursor/abstract_cursor'; @@ -76,7 +76,7 @@ class ClientSession extends EventEmitter { /** @internal */ sessionPool: ServerSessionPool; hasEnded: boolean; - clientOptions?: MongoClientOptions; + clientOptions?: MongoOptions; supports: { causalConsistency: boolean }; clusterTime?: ClusterTime; operationTime?: Timestamp; @@ -98,7 +98,7 @@ class ClientSession extends EventEmitter { topology: Topology, sessionPool: ServerSessionPool, options: ClientSessionOptions, - clientOptions?: MongoClientOptions + clientOptions?: MongoOptions ) { super(); @@ -111,7 +111,6 @@ class ClientSession extends EventEmitter { } options = options ?? {}; - clientOptions = clientOptions || {}; this.topology = topology; this.sessionPool = sessionPool; @@ -263,11 +262,22 @@ class ClientSession extends EventEmitter { // increment txnNumber this.incrementTransactionNumber(); - // create transaction state - this.transaction = new Transaction( - Object.assign({}, this.clientOptions, options || this.defaultTransactionOptions) - ); + this.transaction = new Transaction({ + readConcern: + options?.readConcern ?? + this.defaultTransactionOptions.readConcern ?? + this.clientOptions?.readConcern, + writeConcern: + options?.writeConcern ?? + this.defaultTransactionOptions.writeConcern ?? + this.clientOptions?.writeConcern, + readPreference: + options?.readPreference ?? + this.defaultTransactionOptions.readPreference ?? + this.clientOptions?.readPreference, + maxCommitTimeMS: options?.maxCommitTimeMS ?? this.defaultTransactionOptions.maxCommitTimeMS + }); this.transaction.transition(TxnState.STARTING_TRANSACTION); } @@ -503,8 +513,8 @@ function endTransaction(session: ClientSession, commandName: string, callback: C let writeConcern; if (session.transaction.options.writeConcern) { writeConcern = Object.assign({}, session.transaction.options.writeConcern); - } else if (session.clientOptions && session.clientOptions.w) { - writeConcern = { w: session.clientOptions.w }; + } else if (session.clientOptions && session.clientOptions.writeConcern) { + writeConcern = { w: session.clientOptions.writeConcern.w }; } if (txnState === TxnState.TRANSACTION_COMMITTED) { diff --git a/test/functional/unified-spec-runner/entities.ts b/test/functional/unified-spec-runner/entities.ts index 79f42f2b31c..b27a6d38671 100644 --- a/test/functional/unified-spec-runner/entities.ts +++ b/test/functional/unified-spec-runner/entities.ts @@ -178,7 +178,9 @@ export class EntitiesMap extends Map { const map = new EntitiesMap(); for (const entity of entities ?? []) { if ('client' in entity) { - const uri = config.url({ useMultipleMongoses: entity.client.useMultipleMongoses }); + const useMultipleMongoses = + config.topologyType === 'Sharded' && entity.client.useMultipleMongoses; + const uri = config.url({ useMultipleMongoses }); const client = new UnifiedMongoClient(uri, entity.client); await client.connect(); map.set(entity.client.id, client); diff --git a/test/functional/unified-spec-runner/operations.ts b/test/functional/unified-spec-runner/operations.ts index c0608eebf52..18d19bfb73d 100644 --- a/test/functional/unified-spec-runner/operations.ts +++ b/test/functional/unified-spec-runner/operations.ts @@ -136,7 +136,7 @@ operations.set('assertSessionNotDirty', async ({ entities, operation }) => { operations.set('assertSessionPinned', async ({ entities, operation }) => { const session = entities.getEntity('session', operation.arguments.session); - expect(session.transaction.isPinned).to.be.false; + expect(session.transaction.isPinned).to.be.true; }); operations.set('assertSessionUnpinned', async ({ entities, operation }) => { @@ -310,7 +310,7 @@ operations.set('targetedFailPoint', async ({ entities, operation }) => { const session = entities.getEntity('session', operation.arguments.session); expect(session.transaction.isPinned, 'Session must be pinned for a targetedFailPoint').to.be.true; const client = session.client; - client.enableFailPoint(operation.arguments.failPoint); + return client.enableFailPoint(operation.arguments.failPoint); }); operations.set('delete', async ({ entities, operation }) => { diff --git a/test/functional/unified-spec-runner/runner.ts b/test/functional/unified-spec-runner/runner.ts index 1d3e5b9cbab..f8fe77a533c 100644 --- a/test/functional/unified-spec-runner/runner.ts +++ b/test/functional/unified-spec-runner/runner.ts @@ -15,30 +15,11 @@ interface MongoDBMochaTestContext extends Mocha.Context { configuration: TestConfiguration; } -const SKIPPED_TESTS = [ - // These were already skipped in our existing spec tests - 'unpin after transient error within a transaction and commit', - 'Dirty explicit session is discarded', - - // TODO Un-skip these to complete unified runner - 'withTransaction inherits transaction options from client', - 'withTransaction inherits transaction options from defaultTransactionOptions', - 'remain pinned after non-transient Interrupted error on insertOne', - 'unpin after transient error within a transaction', - 'Client side error in command starting transaction', - 'explicitly create collection using create command', - 'create index on a non-existing collection', - 'InsertMany succeeds after PrimarySteppedDown', - 'withTransaction and no transaction options set', - 'withTransaction explicit transaction options', - 'InsertOne fails after connection failure when retryWrites option is false', - 'InsertOne fails after multiple retryable writeConcernErrors' -]; - export async function runUnifiedTest( ctx: MongoDBMochaTestContext, unifiedSuite: uni.UnifiedSuite, - test: uni.Test + test: uni.Test, + testsToSkip?: string[] ): Promise { // Some basic expectations we can catch early expect(test).to.exist; @@ -56,7 +37,7 @@ export async function runUnifiedTest( ctx.skip(); } - if (SKIPPED_TESTS.includes(test.description)) { + if (testsToSkip?.includes(test.description)) { ctx.skip(); } @@ -76,20 +57,13 @@ export async function runUnifiedTest( ...(test.runOnRequirements ?? []) ]; - let doesNotMeetRunOnRequirement = false; - for (const requirement of allRequirements) { const met = await topologySatisfies(ctx.configuration, requirement, utilClient); if (!met) { - doesNotMeetRunOnRequirement = true; // it doesn't meet a run on requirement - break; + return ctx.skip(); } } - if (doesNotMeetRunOnRequirement) { - ctx.skip(); - } - // If initialData is specified, for each collectionData therein the test runner MUST drop the // collection and insert the specified documents (if any) using a "majority" write concern. If no // documents are specified, the test runner MUST create the collection with a "majority" write concern. @@ -188,14 +162,14 @@ export async function runUnifiedTest( } } -export function runUnifiedSuite(specTests: uni.UnifiedSuite[]): void { +export function runUnifiedSuite(specTests: uni.UnifiedSuite[], testsToSkip?: string[]): void { for (const unifiedSuite of specTests) { context(String(unifiedSuite.description), function () { for (const test of unifiedSuite.tests) { it(String(test.description), { metadata: { sessions: { skipLeakTests: true } }, test: async function () { - await runUnifiedTest(this, unifiedSuite, test); + await runUnifiedTest(this, unifiedSuite, test, testsToSkip); } }); } diff --git a/test/functional/unified-spec-runner/unified-runner.test.ts b/test/functional/unified-spec-runner/unified-runner.test.ts index da4206e0064..86eca4e1b2d 100644 --- a/test/functional/unified-spec-runner/unified-runner.test.ts +++ b/test/functional/unified-spec-runner/unified-runner.test.ts @@ -1,17 +1,16 @@ import { loadSpecTests } from '../../spec/index'; import { runUnifiedSuite } from './runner'; +const SKIPPED_TESTS = [ + // commitTransaction retry seems to be swallowed by mongos in this case + 'unpin after transient error within a transaction and commit', + // These two tests need to run against multiple mongoses + 'Dirty explicit session is discarded', + // Will be implemented as part of NODE-2034 + 'Client side error in command starting transaction' +]; + describe('Unified test format runner', function unifiedTestRunner() { // Valid tests that should pass - runUnifiedSuite(loadSpecTests('unified-test-format/valid-pass')); - - // Valid tests that should fail - // for (const unifiedSuite of loadSpecTests('unified-test-format/valid-fail')) { - // // TODO - // } - - // Tests that are invalid, would be good to gracefully fail on - // for (const unifiedSuite of loadSpecTests('unified-test-format/invalid')) { - // // TODO - // } + runUnifiedSuite(loadSpecTests('unified-test-format/valid-pass'), SKIPPED_TESTS); }); From dd4bc075af6f9fee9b66fff747f5e92a65c1a41d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 9 Mar 2021 17:36:08 -0500 Subject: [PATCH 2/2] fix: correct issue with targetedFailPoints --- .../unified-spec-runner/entities.ts | 81 +++++++++++++------ .../unified-spec-runner/operations.ts | 8 +- 2 files changed, 62 insertions(+), 27 deletions(-) diff --git a/test/functional/unified-spec-runner/entities.ts b/test/functional/unified-spec-runner/entities.ts index b27a6d38671..37743aab710 100644 --- a/test/functional/unified-spec-runner/entities.ts +++ b/test/functional/unified-spec-runner/entities.ts @@ -1,4 +1,11 @@ -import { MongoClient, Db, Collection, GridFSBucket, Document } from '../../../src/index'; +import { + MongoClient, + Db, + Collection, + GridFSBucket, + Document, + HostAddress +} from '../../../src/index'; import { ReadConcern } from '../../../src/read_concern'; import { WriteConcern } from '../../../src/write_concern'; import { ReadPreference } from '../../../src/read_preference'; @@ -18,10 +25,6 @@ interface UnifiedChangeStream extends ChangeStream { eventCollector: InstanceType; } -interface UnifiedClientSession extends ClientSession { - client: UnifiedMongoClient; -} - export type CommandEvent = CommandStartedEvent | CommandSucceededEvent | CommandFailedEvent; export class UnifiedMongoClient extends MongoClient { @@ -75,23 +78,49 @@ export class UnifiedMongoClient extends MongoClient { } return this.events; } +} - async enableFailPoint(failPoint: Document): Promise { - const admin = this.db().admin(); +export class FailPointMap extends Map { + async enableFailPoint( + addressOrClient: HostAddress | UnifiedMongoClient, + failPoint: Document + ): Promise { + let client: MongoClient; + let address: string; + if (addressOrClient instanceof MongoClient) { + client = addressOrClient; + address = client.topology.s.seedlist.join(','); + } else { + // create a new client + address = addressOrClient.toString(); + client = new MongoClient(`mongodb://${address}`); + await client.connect(); + } + + const admin = client.db('admin'); const result = await admin.command(failPoint); + + if (!(addressOrClient instanceof MongoClient)) { + // we created this client + await client.close(); + } + expect(result).to.have.property('ok', 1); - this.failPoints.push(failPoint.configureFailPoint); + this.set(address, failPoint.configureFailPoint); return result; } - async disableFailPoints(): Promise { - return Promise.all( - this.failPoints.map(configureFailPoint => - this.db().admin().command({ - configureFailPoint, - mode: 'off' - }) - ) + async disableFailPoints(): Promise { + const entries = Array.from(this.entries()); + await Promise.all( + entries.map(async ([hostAddress, configureFailPoint]) => { + const client = new MongoClient(`mongodb://${hostAddress}`); + await client.connect(); + const admin = client.db('admin'); + const result = await admin.command({ configureFailPoint, mode: 'off' }); + expect(result).to.have.property('ok', 1); + await client.close(); + }) ); } } @@ -100,7 +129,7 @@ export type Entity = | UnifiedMongoClient | Db | Collection - | UnifiedClientSession + | ClientSession | UnifiedChangeStream | GridFSBucket | Document; // Results from operations @@ -124,10 +153,17 @@ ENTITY_CTORS.set('bucket', GridFSBucket); ENTITY_CTORS.set('stream', ChangeStream); export class EntitiesMap extends Map { + failPoints: FailPointMap; + + constructor(entries?: readonly (readonly [string, E])[] | null) { + super(entries); + this.failPoints = new FailPointMap(); + } + mapOf(type: 'client'): EntitiesMap; mapOf(type: 'db'): EntitiesMap; mapOf(type: 'collection'): EntitiesMap; - mapOf(type: 'session'): EntitiesMap; + mapOf(type: 'session'): EntitiesMap; mapOf(type: 'bucket'): EntitiesMap; mapOf(type: 'stream'): EntitiesMap; mapOf(type: EntityTypeId): EntitiesMap { @@ -141,7 +177,7 @@ export class EntitiesMap extends Map { getEntity(type: 'client', key: string, assertExists?: boolean): UnifiedMongoClient; getEntity(type: 'db', key: string, assertExists?: boolean): Db; getEntity(type: 'collection', key: string, assertExists?: boolean): Collection; - getEntity(type: 'session', key: string, assertExists?: boolean): UnifiedClientSession; + getEntity(type: 'session', key: string, assertExists?: boolean): ClientSession; getEntity(type: 'bucket', key: string, assertExists?: boolean): GridFSBucket; getEntity(type: 'stream', key: string, assertExists?: boolean): UnifiedChangeStream; getEntity(type: EntityTypeId, key: string, assertExists = true): Entity { @@ -161,8 +197,8 @@ export class EntitiesMap extends Map { } async cleanup(): Promise { + await this.failPoints.disableFailPoints(); for (const [, client] of this.mapOf('client')) { - await client.disableFailPoints(); await client.close(); } for (const [, session] of this.mapOf('session')) { @@ -230,10 +266,7 @@ export class EntitiesMap extends Map { } } - const session = client.startSession(options) as UnifiedClientSession; - // targetedFailPoint operations need to access the client the session came from - session.client = client; - + const session = client.startSession(options); map.set(entity.session.id, session); } else if ('bucket' in entity) { const db = map.getEntity('db', entity.bucket.database); diff --git a/test/functional/unified-spec-runner/operations.ts b/test/functional/unified-spec-runner/operations.ts index 18d19bfb73d..65f152f563b 100644 --- a/test/functional/unified-spec-runner/operations.ts +++ b/test/functional/unified-spec-runner/operations.ts @@ -249,7 +249,7 @@ operations.set('findOneAndUpdate', async ({ entities, operation }) => { operations.set('failPoint', async ({ entities, operation }) => { const client = entities.getEntity('client', operation.arguments.client); - return client.enableFailPoint(operation.arguments.failPoint); + return entities.failPoints.enableFailPoint(client, operation.arguments.failPoint); }); operations.set('insertOne', async ({ entities, operation }) => { @@ -309,8 +309,10 @@ operations.set('startTransaction', async ({ entities, operation }) => { operations.set('targetedFailPoint', async ({ entities, operation }) => { const session = entities.getEntity('session', operation.arguments.session); expect(session.transaction.isPinned, 'Session must be pinned for a targetedFailPoint').to.be.true; - const client = session.client; - return client.enableFailPoint(operation.arguments.failPoint); + await entities.failPoints.enableFailPoint( + session.transaction._pinnedServer.s.description.hostAddress, + operation.arguments.failPoint + ); }); operations.set('delete', async ({ entities, operation }) => {