Skip to content
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

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

Merged
merged 1 commit into from
Mar 19, 2025
Merged

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

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
8 changes: 2 additions & 6 deletions bson/bson_corpus_spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down Expand Up @@ -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)

Expand Down
71 changes: 3 additions & 68 deletions internal/integration/unified/unified_spec_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,71 +24,6 @@ import (
)

var (
skippedTests = map[string]string{
// GODRIVER-1773: This test runs a "find" with limit=4 and batchSize=3. It expects batchSize values of three for
// the "find" and one for the "getMore", but we send three for both.
"A successful find event with a getmore and the server kills the cursor (<= 4.4)": "See GODRIVER-1773",

// GODRIVER-2577: The following spec tests require canceling ops immediately, but the current logic clears pools
// and cancels in-progress ops after two the heartbeat failures.
"Connection pool clear uses interruptInUseConnections=true after monitor timeout": "Godriver clears after multiple timeout",
"Error returned from connection pool clear with interruptInUseConnections=true is retryable": "Godriver clears after multiple timeout",
"Error returned from connection pool clear with interruptInUseConnections=true is retryable for write": "Godriver clears after multiple timeout",

// TODO(GODRIVER-2843): Fix and unskip these test cases.
"Find operation with snapshot": "Test fails frequently. See GODRIVER-2843",
"Write commands with snapshot session do not affect snapshot reads": "Test fails frequently. See GODRIVER-2843",

// TODO(GODRIVER-3043): Avoid Appending Write/Read Concern in Atlas Search
// Index Helper Commands.
"dropSearchIndex ignores read and write concern": "Sync GODRIVER-3074, but skip testing bug GODRIVER-3043",
"listSearchIndexes ignores read and write concern": "Sync GODRIVER-3074, but skip testing bug GODRIVER-3043",
"updateSearchIndex ignores the read and write concern": "Sync GODRIVER-3074, but skip testing bug GODRIVER-3043",

// TODO(DRIVERS-2829): Create CSOT Legacy Timeout Analogues and Compatibility Field
"Reset server and pool after network timeout error during authentication": "Uses unsupported socketTimeoutMS",
"Ignore network timeout error on find": "Uses unsupported socketTimeoutMS",
"A successful find with options": "Uses unsupported maxTimeMS",
"estimatedDocumentCount with maxTimeMS": "Uses unsupported maxTimeMS",
"supports configuring getMore maxTimeMS": "Uses unsupported maxTimeMS",

// TODO(GODRIVER-3137): Implement Gossip cluster time"
"unpin after TransientTransactionError error on commit": "Implement GODRIVER-3137",

// TODO(GODRIVER-3034): Drivers should unpin connections when ending a session
"unpin on successful abort": "Implement GODRIVER-3034",
"unpin after non-transient error on abort": "Implement GODRIVER-3034",
"unpin after TransientTransactionError error on abort": "Implement GODRIVER-3034",
"unpin when a new transaction is started": "Implement GODRIVER-3034",
"unpin when a non-transaction write operation uses a session": "Implement GODRIVER-3034",
"unpin when a non-transaction read operation uses a session": "Implement GODRIVER-3034",

// DRIVERS-2722: Setting "maxTimeMS" on a command that creates a cursor
// also limits the lifetime of the cursor. That may be surprising to
// users, so omit "maxTimeMS" from operations that return user-managed
// cursors.
"timeoutMS can be overridden for a find": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"timeoutMS can be configured for an operation - find on collection": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"timeoutMS can be configured for an operation - aggregate on collection": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"timeoutMS can be configured for an operation - aggregate on database": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"timeoutMS can be configured on a MongoClient - find on collection": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"timeoutMS can be configured on a MongoClient - aggregate on collection": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"timeoutMS can be configured on a MongoClient - aggregate on database": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"operation is retried multiple times for non-zero timeoutMS - find on collection": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"operation is retried multiple times for non-zero timeoutMS - aggregate on collection": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"operation is retried multiple times for non-zero timeoutMS - aggregate on database": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",
"timeoutMS applied to find command": "maxTimeMS is disabled on find and aggregate. See DRIVERS-2722.",

// DRIVERS-2953: This test requires that the driver sends a "getMore"
// with "maxTimeMS" set. However, "getMore" can only include "maxTimeMS"
// for tailable awaitData cursors. Including "maxTimeMS" on "getMore"
// for any other cursor type results in a server error:
//
// (BadValue) cannot set maxTimeMS on getMore command for a non-awaitData cursor
//
"Non-tailable cursor lifetime remaining timeoutMS applied to getMore if timeoutMode is unset": "maxTimeMS can't be set on a getMore. See DRIVERS-2953",
}

logMessageValidatorTimeout = 10 * time.Millisecond
lowHeartbeatFrequency = 500 * time.Millisecond
)
Expand Down Expand Up @@ -171,6 +106,8 @@ func runTestFile(t *testing.T, filepath string, expectValidFail bool, opts ...*O
CreateClient(false)

mt.RunOpts(testCase.Description, mtOpts, func(mt *mtest.T) {
spectest.CheckSkip(mt.T)

// Skip CSOT spec tests when SKIP_CSOT_TESTS=true. In Evergreen, we
// typically set that environment variable on Windows and macOS
// because the CSOT spec tests are unreliable on those hosts.
Expand All @@ -186,6 +123,7 @@ func runTestFile(t *testing.T, filepath string, expectValidFail bool, opts ...*O
}
}
}()

err := testCase.Run(mt)
if expectValidFail {
if err != nil {
Expand Down Expand Up @@ -285,9 +223,6 @@ func (tc *TestCase) Run(ls LoggerSkipper) error {
if tc.SkipReason != nil {
ls.Skipf("skipping for reason: %q", *tc.SkipReason)
}
if skipReason, ok := skippedTests[tc.Description]; ok {
ls.Skipf("skipping due to known failure: %q", skipReason)
}

// Validate that we support the schema declared by the test file before attempting to use its contents.
if err := checkSchemaVersion(tc.schemaVersion); err != nil {
Expand Down
27 changes: 6 additions & 21 deletions internal/integration/unified_spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is "Network error on minPoolSize background creation" replaced?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That test was converted to a unified spec test in #1522 and now passes reliably (see here). Since the new unified test passes, I decided to not skip it.

}
)
var defaultHeartbeatInterval = 500 * time.Millisecond

type testFile struct {
RunOn []mtest.RunOnBlock `bson:"runOn"`
Expand Down Expand Up @@ -254,12 +240,11 @@ func runSpecTestCase(mt *mtest.T, test *testCase, testFile testFile) {

// Start the test without setting client options so the setup will be done with a default client.
mt.RunOpts(test.Description, opts, func(mt *mtest.T) {
spectest.CheckSkip(mt.T)

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)
}

// 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" {
Expand Down
6 changes: 1 addition & 5 deletions internal/serverselector/server_selector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down
Loading
Loading