Skip to content

test(NODE-5265): fix flaky operation count test #3688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 30, 2023
Merged

test(NODE-5265): fix flaky operation count test #3688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/sdam/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,13 +343,13 @@ export class Server extends TypedEventEmitter<ServerEvents> {
return;
}

this.s.operationCount += 1;
this.incrementOperationCount();

this.pool.withConnection(
conn,
(err, conn, cb) => {
if (err || !conn) {
this.s.operationCount -= 1;
this.decrementOperationCount();
if (!err) {
return cb(new MongoRuntimeError('Failed to create connection without error'));
}
Expand All @@ -364,7 +364,7 @@ export class Server extends TypedEventEmitter<ServerEvents> {
cmd,
finalOptions,
makeOperationHandler(this, conn, cmd, finalOptions, (error, response) => {
this.s.operationCount -= 1;
this.decrementOperationCount();
cb(error, response);
})
);
Expand Down Expand Up @@ -420,6 +420,20 @@ export class Server extends TypedEventEmitter<ServerEvents> {
}
}
}

/**
* Decrement the operation count, returning the new count.
*/
private decrementOperationCount(): number {
return (this.s.operationCount -= 1);
}

/**
* Increment the operation count, returning the new count.
*/
private incrementOperationCount(): number {
return (this.s.operationCount += 1);
}
Comment on lines +427 to +436
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to return the new count, if we're not chaining it or using the returned value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No was just a general practice I like to use - if the method would normally be void try potentially returning something useful if possible.

}

function calculateRoundTripTime(oldRtt: number, duration: number): number {
Expand Down
17 changes: 9 additions & 8 deletions test/integration/server-selection/operation_count.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { expect } from 'chai';
import * as sinon from 'sinon';

import { AbstractCursor, Collection, ConnectionPool, MongoClient } from '../../mongodb';
import { FailPoint, sleep } from '../../tools/utils';
import { FailPoint } from '../../tools/utils';

const testMetadata: MongoDBMetadataUI = {
requires: {
Expand Down Expand Up @@ -118,20 +118,21 @@ describe('Server Operation Count Tests', function () {
const server = Array.from(client.topology.s.servers.values())[0];
expect(server.s.operationCount).to.equal(0);
const commandSpy = sinon.spy(server, 'command');
const incrementSpy = sinon.spy(server, 'incrementOperationCount');
const decrementSpy = sinon.spy(server, 'decrementOperationCount');

const operationPromises = Array.from({ length: 10 }, () =>
collection.insertOne({ count: 1 })
);

// operation count is incremented after connection checkout, which happens asynchronously (even though there are plenty of connections in the pool).
// we sleep to give the event loop a turn so that all the commands check out a connection before asserting the operation count
await sleep(1);

expect(server.s.operationCount).to.equal(10);

await Promise.all(operationPromises);
await Promise.allSettled(operationPromises);

expect(commandSpy.called).to.be.true;
// This test is flaky when sleeping and asserting the operation count after the sleep but before the
// promise execution, so we assert instead that the count was incremented 10 times and decremented 10
// times - the total number of operations.
expect(incrementSpy.callCount).to.equal(10);
expect(decrementSpy.callCount).to.equal(10);
expect(server.s.operationCount).to.equal(0);
});
});
Expand Down