address comments

Author: baileympearson

src/sdam/server.ts

@@ -328,6 +328,11 @@ export class Server extends TypedEventEmitter<ServerEvents> {
     // NOTE: This is a hack! We can't retrieve the connections used for executing an operation
     //       (and prevent them from being checked back in) at the point of operation execution.
     //       This should be considered as part of the work for NODE-2882
+    // NOTE:
+    //       When incrementing operation count, it's important that we increment it before we
+    //       attempt to check out a connection from the pool.  This ensures that operations that
+    //       are waiting for a connection are included in the operation count.  Load balanced
+    //       mode will only ever have a single server, so the operation count doesn't matter.
     if (this.loadBalanced && session && conn == null && isPinnableCommand(cmd, session)) {
       this.s.pool.checkOut((err, checkedOut) => {
         if (err || checkedOut == null) {

test/integration/server-selection/server_selection_operation_count.prose.test.ts renamed to test/integration/server-selection/server_selection.prose.operation_count.test.ts

@@ -41,9 +41,11 @@ async function ensurePoolIsFull(client: MongoClient) {
     throw new Error('Connection pool did not fill up');
   }
 }
+
+// Step 1: Configure a sharded cluster with two mongoses. Use a 4.2.9 or newer server version.
 const TEST_METADATA: MongoDBMetadataUI = { requires: { mongodb: '>=4.2.9', topology: 'sharded' } };
 
-describe('Server Selection Operation Count Prose', function () {
+describe('operationCount-based Selection Within Latency Window - Prose Test', function () {
   let client: MongoClient;
   let seeds: Array<string>;
   let counts: Record<string, number> = {};

test/unit/assorted/server_selection_latency_window_utils.ts

@@ -51,29 +51,26 @@ export function loadLatencyWindowTests(directory: string) {
 
 function compareResultsToExpected(
   { tolerance, expected_frequencies }: Outcome,
-  observed_frequencies: FrequencyMap
+  observedFrequencies: FrequencyMap
 ) {
-  const expectedFrequencies = Object.entries(expected_frequencies);
   expect(
-    expectedFrequencies,
+    Object.entries(expected_frequencies),
     'Encountered an empty set of frequencies to assert on.  Is there something wrong with the test or the runner?'
   ).to.have.length.greaterThan(0);
   for (const [address, frequency] of Object.entries(expected_frequencies)) {
     if (frequency === 0) {
-      expect(observed_frequencies).not.to.haveOwnProperty(address);
+      expect(observedFrequencies).not.to.haveOwnProperty(address);
     } else {
-      expect(observed_frequencies).to.haveOwnProperty(address).to.exist;
-      const actual_frequency = observed_frequencies[address];
-      const is_too_low = actual_frequency < frequency - tolerance;
-      expect(
-        is_too_low,
-        `expected frequency of ${frequency}+/-${tolerance} but received ${actual_frequency}`
-      ).to.be.false;
-      const is_too_high = actual_frequency > frequency + tolerance;
-      expect(
-        is_too_high,
-        `expected frequency of ${frequency}+/-${tolerance} but received ${actual_frequency}`
-      ).to.be.false;
+      expect(observedFrequencies).to.haveOwnProperty(address).that.is.a('number');
+      const actualFrequency = observedFrequencies[address];
+      const isTooLow = actualFrequency < frequency - tolerance;
+      const isTooHigh = actualFrequency > frequency + tolerance;
+
+      if (isTooHigh || isTooLow) {
+        expect.fail(
+          `expected frequency of ${frequency}+/-${tolerance} but received ${actualFrequency}`
+        );
+      }
     }
   }
 }