GODRIVER-3445 Move all logic for skipping spec tests by description into one place.

Author: matthewdale

bson/bson_corpus_spec_test.go

@@ -275,11 +275,7 @@ func runTest(t *testing.T, file string) {
 	content, err := os.ReadFile(filepath)
 	require.NoError(t, err)
 
-	// Remove ".json" from filename.
-	file = file[:len(file)-5]
-	testName := "bson_corpus--" + file
-
-	t.Run(testName, func(t *testing.T) {
+	t.Run(file, func(t *testing.T) {
 		var test testCase
 		require.NoError(t, json.Unmarshal(content, &test))
 
@@ -429,7 +425,7 @@ func runTest(t *testing.T, file string) {
 	})
 }
 
-func Test_BsonCorpus(t *testing.T) {
+func TestBSONCorpus(t *testing.T) {
 	jsonFiles, err := findJSONFilesInDir(dataDir)
 	require.NoErrorf(t, err, "error finding JSON files in %s: %v", dataDir, err)
 

internal/integration/unified_spec_test.go

@@ -27,6 +27,7 @@ import (
 	"go.mongodb.org/mongo-driver/v2/internal/failpoint"
 	"go.mongodb.org/mongo-driver/v2/internal/integration/mtest"
 	"go.mongodb.org/mongo-driver/v2/internal/integtest"
+	"go.mongodb.org/mongo-driver/v2/internal/spectest"
 	"go.mongodb.org/mongo-driver/v2/mongo"
 	"go.mongodb.org/mongo-driver/v2/mongo/address"
 	"go.mongodb.org/mongo-driver/v2/mongo/options"
@@ -37,26 +38,11 @@ import (
 )
 
 const (
-	gridFSFiles            = "fs.files"
-	gridFSChunks           = "fs.chunks"
-	spec1403SkipReason     = "servers less than 4.2 do not have mongocryptd; see SPEC-1403"
-	godriver2123SkipReason = "failpoints and timeouts together cause failures; see GODRIVER-2123"
+	gridFSFiles  = "fs.files"
+	gridFSChunks = "fs.chunks"
 )
 
-var (
-	defaultHeartbeatInterval = 500 * time.Millisecond
-	skippedTestDescriptions  = map[string]string{
-		// SPEC-1403: This test checks to see if the correct error is thrown when auto encrypting with a server < 4.2.
-		// Currently, the test will fail because a server < 4.2 wouldn't have mongocryptd, so Client construction
-		// would fail with a mongocryptd spawn error.
-		"operation fails with maxWireVersion < 8": spec1403SkipReason,
-		// GODRIVER-2123: The two tests below use a failpoint and a socket or server selection timeout.
-		// The timeout causes the eventual clearing of the failpoint in the test runner to fail with an
-		// i/o timeout.
-		"Ignore network timeout error on find":             godriver2123SkipReason,
-		"Network error on minPoolSize background creation": godriver2123SkipReason,
-	}
-)
+var defaultHeartbeatInterval = 500 * time.Millisecond
 
 type testFile struct {
 	RunOn           []mtest.RunOnBlock `bson:"runOn"`
@@ -257,9 +243,7 @@ func runSpecTestCase(mt *mtest.T, test *testCase, testFile testFile) {
 		if len(test.SkipReason) > 0 {
 			mt.Skip(test.SkipReason)
 		}
-		if skipReason, ok := skippedTestDescriptions[test.Description]; ok {
-			mt.Skipf("skipping due to known failure: %v", skipReason)
-		}
+		spectest.CheckSkip(mt.T)
 
 		// work around for SERVER-39704: run a non-transactional distinct against each shard in a sharded cluster
 		if mtest.ClusterTopologyKind() == mtest.Sharded && test.Description == "distinct" {

internal/serverselector/server_selector_test.go

@@ -296,11 +296,7 @@ func runTest(t *testing.T, testsDir string, directory string, filename string) {
 	content, err := ioutil.ReadFile(filepath)
 	require.NoError(t, err)
 
-	// Remove ".json" from filename.
-	filename = filename[:len(filename)-5]
-	testName := directory + "/" + filename + ":"
-
-	t.Run(testName, func(t *testing.T) {
+	t.Run(directory+"/"+filename, func(t *testing.T) {
 		var test testCase
 		require.NoError(t, bson.UnmarshalExtJSON(content, true, &test))
 

internal/spectest/skip.go

@@ -0,0 +1,79 @@
+package spectest
+
+import "testing"
+
+// skipTests is a map of fully-qualified test name to the reason for skipping
+// the test.
+var skipTests = map[string]string{
+	"TestURIOptionsSpec/single-threaded-options.json/Valid_options_specific_to_single-threaded_drivers_are_parsed_correctly": "the Go Driver is not single-threaded.",
+	// GODRIVER-2348: The wtimeoutMS write concern option is not supported.
+	"TestURIOptionsSpec/concern-options.json/Valid_read_and_write_concern_are_parsed_correctly": "the wtimeoutMS write concern option is not supported",
+
+	// SPEC-1403: This test checks to see if the correct error is thrown when
+	// auto encrypting with a server < 4.2. Currently, the test will fail
+	// because a server < 4.2 wouldn't have mongocryptd, so Client construction
+	// would fail with a mongocryptd spawn error.
+	"TestUnifiedSpecs/client-side-encryption/legacy/maxWireVersion.json/operation_fails_with_maxWireVersion_<_8": "servers less than 4.2 do not have mongocryptd; see SPEC-1403",
+
+	// GODRIVER-2123: The two tests below use a failpoint and a socket or server
+	// selection timeout. The timeout causes the eventual clearing of the
+	// failpoint in the test runner to fail with an i/o timeout.
+	"TestUnifiedSpecs/server-discovery-and-monitoring/unified/find-network-timeout-error.json/Ignore_network_timeout_error_on_find":    "failpoints and timeouts together cause failures; see GODRIVER-2123",
+	"TestUnifiedSpecs/server-discovery-and-monitoring/unified/minPoolSize-error.json/Network_error_on_minPoolSize_background_creation": "failpoints and timeouts together cause failures; see GODRIVER-2123",
+
+	// GODRIVER-1827: These 2 tests assert that in-use connections are not
+	// closed until checked back into a closed pool, but the Go connection pool
+	// aggressively closes in-use connections. That behavior is currently
+	// required by the "Client.Disconnect" API, so skip the tests.
+	"TestCMAPSpec/pool-close-destroy-conns.json/When_a_pool_is_closed,_it_MUST_first_destroy_all_available_connections_in_that_pool": "test requires that close does not aggressively close used connections",
+	"TestCMAPSpec/pool-close-destroy-conns.json/must_destroy_checked_in_connection_if_pool_has_been_closed":                          "test requires that close does not aggressively close used connections",
+
+	// GODRIVER-1826: The load-balancer SDAM error handling test "errors during
+	// authentication are processed" currently asserts that handshake errors
+	// trigger events "pool cleared" then "connection closed". However, the
+	// "error during minPoolSize population clears pool" test asserts that
+	// handshake errors trigger events "connection closed" then "pool cleared".
+	// The Go driver uses the same code path for creating all application
+	// connections, so those opposing event orders cannot be satisfied
+	// simultaneously.
+	//
+	// TODO(DRIVERS-1785): Re-enable this test once the spec test is updated to
+	// use the same event order as the "errors during authentication are
+	// processed" load-balancer SDAM spec test.
+	"TestCMAPSpec/pool-create-min-size-error.json/error_during_minPoolSize_population_clears_pool": "event ordering is incompatible with load-balancer SDAM spec test (DRIVERS-1785)",
+
+	// GODRIVER-1826: The Go connection pool does not currently always deliver
+	// connections created by maintain() to waiting check-outs. There is a race
+	// condition between the goroutine started by maintain() to check-in a
+	// requested connection and createConnections() picking up the next wantConn
+	// created by the waiting check-outs. Most of the time, createConnections()
+	// wins and starts creating new connections. That is not a problem for
+	// general use cases, but it prevents the "threads blocked by maxConnecting
+	// check out minPoolSize connections" test from passing.
+	//
+	// TODO(DRIVERS-2225): Re-enable this test once the spec test is updated to
+	// support the Go pool minPoolSize maintain() behavior.
+	"TestCMAPSpec/pool-checkout-minPoolSize-connection-maxConnecting.json/threads_blocked_by_maxConnecting_check_out_minPoolSize_connections": "test requires that connections established by minPoolSize are immediately used to satisfy check-out requests (DRIVERS-2225)",
+
+	// GODRIVER-1826: The Go connection pool currently delivers any available
+	// connection to the earliest waiting check-out request, independent of if
+	// that check-out request already requested a new connection. That behavior
+	// is currently incompatible with the "threads blocked by maxConnecting
+	// check out returned connections" test, which expects that check-out
+	// requests that request a new connection cannot be satisfied by a check-in.
+	//
+	// TODO(DRIVERS-2223): Re-enable this test once the spec test is updated to
+	// support the Go pool check-in behavior.
+	"TestCMAPSpec/pool-checkout-returned-connection-maxConnecting.json/threads_blocked_by_maxConnecting_check_out_returned_connections": "test requires a checked-in connections cannot satisfy a check-out waiting on a new connection (DRIVERS-2223)",
+
+	// TODO(GODRIVER-2129): Re-enable this test once GODRIVER-2129 is done.
+	"TestAuthSpec/connection-string.json/must_raise_an_error_when_the_hostname_canonicalization_is_invalid": "support will be added with GODRIVER-2129.",
+}
+
+// CheckSkip checks if the fully-qualified test name matches a skipped test
+// name. If the test name matches, the reason is logged and the test is skipped.
+func CheckSkip(t *testing.T) {
+	if reason := skipTests[t.Name()]; reason != "" {
+		t.Skipf("Skipping due to known failure: %q", reason)
+	}
+}

x/mongo/driver/auth/auth_spec_test.go

@@ -18,11 +18,6 @@ import (
 	"go.mongodb.org/mongo-driver/v2/mongo/options"
 )
 
-var skippedTests = map[string]string{
-	// GODRIVER-3175: Support will be added with GODRIVER-2129.
-	"must raise an error when the hostname canonicalization is invalid": "Support will be added with GODRIVER-2129.",
-}
-
 type credential struct {
 	Username  string
 	Password  *string
@@ -53,53 +48,53 @@ func runTestsInFile(t *testing.T, dirname string, filename string) {
 	var container testContainer
 	require.NoError(t, json.Unmarshal(content, &container))
 
-	for _, testCase := range container.Tests {
-		t.Run(filename, func(t *testing.T) {
-			runTest(t, testCase)
-		})
-	}
+	t.Run(filename, func(t *testing.T) {
+		for _, testCase := range container.Tests {
+			testCase := testCase // Capture range variable.
+
+			t.Run(testCase.Description, func(t *testing.T) {
+				spectest.CheckSkip(t)
+
+				runTest(t, testCase)
+			})
+		}
+	})
 }
 
 func runTest(t *testing.T, test testCase) {
-	if skipReason, ok := skippedTests[test.Description]; ok {
-		t.Skipf("skipping due to known failure: %q", skipReason)
-	}
-
-	t.Run(test.Description, func(t *testing.T) {
-		opts := options.Client().ApplyURI(test.URI)
+	opts := options.Client().ApplyURI(test.URI)
 
-		if test.Valid {
-			require.NoError(t, opts.Validate())
-		} else {
-			require.Error(t, opts.Validate())
+	if test.Valid {
+		require.NoError(t, opts.Validate())
+	} else {
+		require.Error(t, opts.Validate())
 
-			return
-		}
+		return
+	}
 
-		if test.Credential == nil {
-			require.Nil(t, opts.Auth)
-			return
-		}
-		require.NotNil(t, opts.Auth)
-		require.Equal(t, test.Credential.Username, opts.Auth.Username)
-
-		if test.Credential.Password == nil {
-			require.False(t, opts.Auth.PasswordSet)
-		} else {
-			require.True(t, opts.Auth.PasswordSet)
-			require.Equal(t, *test.Credential.Password, opts.Auth.Password)
-		}
+	if test.Credential == nil {
+		require.Nil(t, opts.Auth)
+		return
+	}
+	require.NotNil(t, opts.Auth)
+	require.Equal(t, test.Credential.Username, opts.Auth.Username)
+
+	if test.Credential.Password == nil {
+		require.False(t, opts.Auth.PasswordSet)
+	} else {
+		require.True(t, opts.Auth.PasswordSet)
+		require.Equal(t, *test.Credential.Password, opts.Auth.Password)
+	}
 
-		require.Equal(t, test.Credential.Source, opts.Auth.AuthSource)
+	require.Equal(t, test.Credential.Source, opts.Auth.AuthSource)
 
-		require.Equal(t, test.Credential.Mechanism, opts.Auth.AuthMechanism)
+	require.Equal(t, test.Credential.Mechanism, opts.Auth.AuthMechanism)
 
-		if len(test.Credential.MechProps) > 0 {
-			require.Equal(t, mapInterfaceToString(test.Credential.MechProps), opts.Auth.AuthMechanismProperties)
-		} else {
-			require.Equal(t, 0, len(opts.Auth.AuthMechanismProperties))
-		}
-	})
+	if len(test.Credential.MechProps) > 0 {
+		require.Equal(t, mapInterfaceToString(test.Credential.MechProps), opts.Auth.AuthMechanismProperties)
+	} else {
+		require.Equal(t, 0, len(opts.Auth.AuthMechanismProperties))
+	}
 }
 
 // Convert each interface{} value in the map to a string.