chore: remove all array skips in favor of skip function

Author: baileympearson

test/integration/collection-management/collection_management.spec.test.js

@@ -7,5 +7,7 @@ const { runUnifiedSuite } = require('../../tools/unified-spec-runner/runner');
 const SKIPPED_TESTS = ['modifyCollection to changeStreamPreAndPostImages enabled'];
 
 describe('Collection management unified spec tests', function () {
-  runUnifiedSuite(loadSpecTests('collection-management'), SKIPPED_TESTS);
+  runUnifiedSuite(loadSpecTests('collection-management'), ({ description }) =>
+    SKIPPED_TESTS.includes(description) ? `the Node driver does not have a collMod helper.` : null
+  );
 });

test/integration/command-monitoring/command_monitoring.spec.test.ts

@@ -12,5 +12,7 @@ import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner';
 const SKIP = ['A successful unordered bulk write with an unacknowledged write concern'];
 
 describe('Command Monitoring Spec (unified)', () => {
-  runUnifiedSuite(loadSpecTests(path.join('command-monitoring', 'unified')), SKIP);
+  runUnifiedSuite(loadSpecTests(path.join('command-monitoring', 'unified')), ({ description }) =>
+    SKIP.includes(description) ? `TODO(NODE-4261): support skip reasons in unified tests` : null
+  );
 });

test/integration/load-balancers/load_balancers.spec.test.js

@@ -3,63 +3,44 @@ const path = require('path');
 const { loadSpecTests } = require('../../spec/index');
 const { runUnifiedSuite } = require('../../tools/unified-spec-runner/runner');
 
-const SKIP = [
-  // Verified they use the same connection but the Node implementation executes
-  // a getMore before the killCursors even though the stream is immediately
-  // closed.
-  // TODO(NODE-3970): implement and reference a node specific integration test for this
-  'change streams pin to a connection',
+const filter = ({ description }) => {
+  if (description === 'change streams pin to a connection') {
+    // Verified they use the same connection but the Node implementation executes
+    // a getMore before the killCursors even though the stream is immediately
+    // closed.
+    // TODO(NODE-3970): implement and reference a node specific integration test for this
+    return 'TODO(NODE-3970)';
+  }
 
-  // TODO(DRIVERS-1847): The following three tests are skipped pending a decision made on DRIVERS-1847,
-  // since pinning the connection on any getMore error is very awkward in node and likely results
-  // in sub-optimal pinning.
-  'pinned connections are not returned after an network error during getMore',
-  'pinned connections are not returned to the pool after a non-network error on getMore',
-  'stale errors are ignored',
+  if (
+    [
+      'pinned connections are not returned after an network error during getMore',
+      'pinned connections are not returned to the pool after a non-network error on getMore',
+      'stale errors are ignored'
+    ].includes(description)
+  ) {
+    // TODO(DRIVERS-1847): The following three tests are skipped pending a decision made on DRIVERS-1847,
+    // since pinning the connection on any getMore error is very awkward in node and likely results
+    // in sub-optimal pinning.
+    return 'TODO(DRIVERS-1847)';
+  }
 
-  // This test is skipped because it assumes drivers attempt connections on the first operation,
-  // but Node has a connect() method that is called before the first operation is ever run.
-  // TODO(NODE-2149): Refactor connect()
-  'errors during the initial connection hello are ignored',
+  if (
+    process.env.AUTH === 'auth' &&
+    [
+      'errors during authentication are processed',
+      'wait queue timeout errors include cursor statistics',
+      'wait queue timeout errors include transaction statistics',
+      'operations against non-load balanced clusters fail if URI contains loadBalanced=true',
+      'operations against non-load balanced clusters succeed if URI contains loadBalanced=false'
+    ].includes(description)
+  ) {
+    return 'TODO(NODE-3891): fix tests broken when AUTH enabled';
+  }
 
-  ...(process.env.SERVERLESS
-    ? [
-        // TODO(NODE-2471): Unskip these when there isn't a ping command sent when credentials are defined
-        'no connection is pinned if all documents are returned in the initial batch',
-        'pinned connections are returned when the cursor is drained',
-        'pinned connections are returned to the pool when the cursor is closed',
-        'pinned connections are returned after a network error during a killCursors request',
-        'aggregate pins the cursor to a connection',
-        'errors during the initial connection hello are ignored',
-        'all operations go to the same mongos',
-        'transaction can be committed multiple times',
-        'pinned connection is not released after a non-transient CRUD error',
-        'pinned connection is not released after a non-transient commit error',
-        'pinned connection is released after a non-transient abort error',
-        'pinned connection is released after a transient network commit error',
-        'pinned connection is released after a transient non-network abort error',
-        'pinned connection is released after a transient network abort error',
-        'pinned connection is released on successful abort',
-        'pinned connection is returned when a new transaction is started',
-        'pinned connection is returned when a non-transaction operation uses the session',
-        'a connection can be shared by a transaction and a cursor',
-        'wait queue timeout errors include cursor statistics',
-        'wait queue timeout errors include transaction statistics'
-      ]
-    : []),
-
-  // TODO: NODE-3891 - fix tests broken when AUTH enabled
-  ...(process.env.AUTH === 'auth'
-    ? [
-        'errors during authentication are processed',
-        'wait queue timeout errors include cursor statistics',
-        'wait queue timeout errors include transaction statistics',
-        'operations against non-load balanced clusters fail if URI contains loadBalanced=true',
-        'operations against non-load balanced clusters succeed if URI contains loadBalanced=false'
-      ]
-    : [])
-];
+  return null;
+};
 
 describe('Load Balancer Unified Tests', function () {
-  runUnifiedSuite(loadSpecTests(path.join('load-balancers')), SKIP);
+  runUnifiedSuite(loadSpecTests(path.join('load-balancers')), filter);
 });

test/integration/sessions/sessions.spec.test.ts

@@ -2,9 +2,13 @@ import { loadSpecTests } from '../../spec';
 import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner';
 
 describe('Sessions spec tests', function () {
-  runUnifiedSuite(loadSpecTests('sessions'), [
-    // TODO(NODE-3951): fix broken dirty sessions spec tests
-    'Dirty explicit session is discarded (insert)',
-    'Dirty explicit session is discarded (findAndModify)'
-  ]);
+  runUnifiedSuite(loadSpecTests('sessions'), ({ description }) =>
+    [
+      // TODO(NODE-3951): fix broken dirty sessions spec tests
+      'Dirty explicit session is discarded (insert)',
+      'Dirty explicit session is discarded (findAndModify)'
+    ].includes(description)
+      ? 'TODO(NODE-3951): fix broken dirty sessions spec tests'
+      : null
+  );
 });

test/integration/unified-test-format/unified_test_format.spec.test.ts

@@ -1,35 +1,46 @@
 import { loadSpecTests } from '../../spec/index';
 import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner';
+import { TestFilter } from '../../tools/unified-spec-runner/schema';
 
-// TODO: NODE-3891 - fix tests broken when AUTH enabled
-const FAILING_TESTS_AUTH_ENABLED = [
-  'FindOneAndUpdate is committed on first attempt',
-  'FindOneAndUpdate is not committed on first attempt',
-  'FindOneAndUpdate is never committed',
-  'eventType defaults to command if unset',
-  'events are captured during an operation',
-  'eventType can be set to command and cmap'
-];
+const filter: TestFilter = ({ description }) => {
+  if (description === 'unpin after transient error within a transaction and commit') {
+    // OLD COMMENT: commitTransaction retry seems to be swallowed by mongos in this case
+    // TODO(NODE-3943):
+    return `TODO(NODE-3943)`;
+  }
 
-const SKIPPED_TESTS = [
-  // TODO(NODE-3943):
-  // OLD COMMENT: commitTransaction retry seems to be swallowed by mongos in this case
-  'unpin after transient error within a transaction and commit',
+  if (description === 'Client side error in command starting transaction') {
+    // TODO(NODE-2034): Will be implemented as part of NODE-2034
+    return 'TODO(NODE-2034)';
+  }
 
-  // TODO(NODE-2034): Will be implemented as part of NODE-2034
-  'Client side error in command starting transaction',
+  if (description === 'Dirty explicit session is discarded') {
+    // TODO(NODE-3951): investigate why this is failing while the legacy version is passing
+    return 'TODO(NODE-3951)';
+  }
 
-  // TODO(NODE-3951): investigate why this is failing while the legacy version is passing
-  'Dirty explicit session is discarded',
+  if (description === 'A successful find event with a getmore and the server kills the cursor') {
+    return 'TODO(NODE-3308)';
+  }
 
-  // TODO(NODE-3308):
-  'A successful find event with a getmore and the server kills the cursor',
+  if (
+    process.env.AUTH === 'auth' &&
+    [
+      'FindOneAndUpdate is committed on first attempt',
+      'FindOneAndUpdate is not committed on first attempt',
+      'FindOneAndUpdate is never committed',
+      'eventType defaults to command if unset',
+      'events are captured during an operation',
+      'eventType can be set to command and cmap'
+    ].includes(description)
+  ) {
+    return 'TODO(NODE-3891): fix tests broken when AUTH enabled';
+  }
 
-  // TODO(NODE-4125): Fix change streams resume logic when used in iterator mode
-  'Test consecutive resume'
-].concat(process.env.AUTH === 'auth' ? FAILING_TESTS_AUTH_ENABLED : []);
+  return null;
+};
 
 describe('Unified test format runner', function unifiedTestRunner() {
   // Valid tests that should pass
-  runUnifiedSuite(loadSpecTests('unified-test-format/valid-pass'), SKIPPED_TESTS);
+  runUnifiedSuite(loadSpecTests('unified-test-format/valid-pass'), filter);
 });

test/tools/unified-spec-runner/runner.ts

@@ -36,51 +36,22 @@ async function terminateOpenTransactions(client: MongoClient) {
   }
 }
 
-export async function runUnifiedTest(
-  ctx: Mocha.Context,
-  unifiedSuite: uni.UnifiedSuite,
-  test: uni.Test
-): Promise<void>;
-
-/**
- * @deprecated use the overload that provides a function filter instead
+/*
+ * @param skipFilter - a function that returns null if the test should be run,
+ *                     or a skip reason if the test should be skipped
  */
 export async function runUnifiedTest(
   ctx: Mocha.Context,
   unifiedSuite: uni.UnifiedSuite,
   test: uni.Test,
-  testsToSkip?: string[]
-): Promise<void>;
-
-/**
- *
- * @param skipFilter - a function that returns true if the test should be skipped
- */
-export async function runUnifiedTest(
-  ctx: Mocha.Context,
-  unifiedSuite: uni.UnifiedSuite,
-  test: uni.Test,
-  skipFilter?: uni.TestFilter
-): Promise<void>;
-
-export async function runUnifiedTest(
-  ctx: Mocha.Context,
-  unifiedSuite: uni.UnifiedSuite,
-  test: uni.Test,
-  testsToSkipOrFilter?: string[] | uni.TestFilter
+  skipFilter: uni.TestFilter = () => null
 ): Promise<void> {
   // Some basic expectations we can catch early
   expect(test).to.exist;
   expect(unifiedSuite).to.exist;
   expect(ctx).to.exist;
   expect(ctx.configuration).to.exist;
 
-  if (Array.isArray(testsToSkipOrFilter)) {
-    const testsToSkip = testsToSkipOrFilter;
-    testsToSkipOrFilter = ({ description }) =>
-      testsToSkip.includes(description) ? 'Test was filtered without a skip reason' : null;
-  }
-
   const schemaVersion = patchVersion(unifiedSuite.schemaVersion);
   expect(semverSatisfies(schemaVersion, uni.SupportedVersion)).to.be.true;
 
@@ -91,7 +62,7 @@ export async function runUnifiedTest(
     ctx.skip();
   }
 
-  const skipReason = testsToSkipOrFilter?.(test);
+  const skipReason = skipFilter(test);
 
   if (typeof skipReason === 'string') {
     if (skipReason.length === 0) {
@@ -101,10 +72,6 @@ export async function runUnifiedTest(
     ctx.skip();
   }
 
-  if (testsToSkipOrFilter?.(test)) {
-    ctx.skip();
-  }
-
   let utilClient;
   if (ctx.configuration.isLoadBalanced) {
     // The util client can always point at the single mongos LB frontend.
@@ -279,32 +246,20 @@ export async function runUnifiedTest(
   }
 }
 
-export function runUnifiedSuite(specTests: uni.UnifiedSuite[]): void;
-
-/**
- * @deprecated use the overload that provides a function filter instead
- */
-export function runUnifiedSuite(specTests: uni.UnifiedSuite[], testsToSkip?: string[]): void;
-
 /**
  *
- * @param skipFilter - a function that returns true if the test should be skipped
+ * @param skipFilter - a function that returns null if the test should be run,
+ *                     or a skip reason if the test should be skipped
  */
-export function runUnifiedSuite(specTests: uni.UnifiedSuite[], skipFilter?: uni.TestFilter): void;
-
 export function runUnifiedSuite(
   specTests: uni.UnifiedSuite[],
-  testsToSkipOrFilter?: string[] | uni.TestFilter
+  skipFilter: uni.TestFilter = () => null
 ): void {
   for (const unifiedSuite of specTests) {
     context(String(unifiedSuite.description), function () {
       for (const test of unifiedSuite.tests) {
         it(String(test.description), async function () {
-          if (Array.isArray(testsToSkipOrFilter)) {
-            await runUnifiedTest(this, unifiedSuite, test, testsToSkipOrFilter);
-          } else {
-            await runUnifiedTest(this, unifiedSuite, test, testsToSkipOrFilter);
-          }
+          await runUnifiedTest(this, unifiedSuite, test, skipFilter);
         });
       }
     });