chore(NODE-4348): enhance skip reason filtering in unified runner (#3303)

Co-authored-by: Neal Beeken <[email protected]>

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.` : false
+  );
 });

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` : false
+  );
 });

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

@@ -3,63 +3,50 @@ 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): implement and reference a node specific integration test for this';
+  }
 
-  // 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 above three tests are skipped pending a decision made in 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): Skipped pending a decision made on 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 (description === 'errors during the initial connection hello are ignored') {
+    // 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.
+    return 'TODO(NODE-2149): Refactor connect()';
+  }
 
-  ...(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'
-      ]
-    : []),
+  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';
+  }
 
-  // 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 false;
+};
 
 describe('Load Balancer Unified Tests', function () {
-  runUnifiedSuite(loadSpecTests(path.join('load-balancers')), SKIP);
+  runUnifiedSuite(loadSpecTests(path.join('load-balancers')), filter);
 });

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

@@ -1,32 +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): commitTransaction retry seems to be swallowed by mongos in this case`;
+  }
 
-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): Specify effect of client-side errors on in-progress transactions';
+  }
 
-  // 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): investigate why this is failing while the legacy version is passing';
+  }
 
-  // TODO(NODE-3308):
-  'A successful find event with a getmore and the server kills the cursor',
+  if (description === 'A successful find event with a getmore and the server kills the cursor') {
+    return 'TODO(NODE-3308): failures due unnecessary getMore and killCursors calls in 5.0';
+  }
 
-  // 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 : []);
+  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';
+  }
+
+  return false;
+};
 
 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,11 +36,15 @@ async function terminateOpenTransactions(client: MongoClient) {
   }
 }
 
+/*
+ * @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[]
+  skipFilter: uni.TestFilter = () => false
 ): Promise<void> {
   // Some basic expectations we can catch early
   expect(test).to.exist;
@@ -58,7 +62,13 @@ export async function runUnifiedTest(
     ctx.skip();
   }
 
-  if (testsToSkip?.includes(test.description)) {
+  const skipReason = skipFilter(test);
+
+  if (typeof skipReason === 'string') {
+    if (skipReason.length === 0) {
+      expect.fail(`Test was skipped with an empty skip reason: ${test.description}`);
+    }
+    ctx.skipReason = skipReason;
     ctx.skip();
   }
 
@@ -236,12 +246,20 @@ export async function runUnifiedTest(
   }
 }
 
-export function runUnifiedSuite(specTests: uni.UnifiedSuite[], testsToSkip?: string[]): void {
+/**
+ *
+ * @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 = () => false
+): void {
   for (const unifiedSuite of specTests) {
     context(String(unifiedSuite.description), function () {
       for (const test of unifiedSuite.tests) {
         it(String(test.description), async function () {
-          await runUnifiedTest(this, unifiedSuite, test, testsToSkip);
+          await runUnifiedTest(this, unifiedSuite, test, skipFilter);
         });
       }
     });

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

@@ -236,3 +236,8 @@ export interface ExpectedError {
   errorLabelsOmit?: string[];
   expectResult?: unknown;
 }
+
+/**
+ * A type that represents the test filter provided to the unifed runner.
+ */
+export type TestFilter = (test: Test) => string | false;