CSHARP-3678: Convert transactions spec tests to unified test format #1586

sanych-sun · 2025-01-06T20:40:27Z

This PR will close the following Jira tickets:

CSHARP-3678: Convert transactions spec tests to unified test format - PARTIAL! There were changes to transactions-convenient-api specs, which is not syncing in this PR, will do as a separate PR to reduce it's size.
CSHARP-4966: Revise runOnRequirements for transaction tests incompatible with load balancers

NOTICE: Some spec tests contains changes done locally, that has to be processed to spec repository at first. I'm discussing the changes with spec owners and will create PR to spec repo shortly.

sanych-sun · 2025-01-06T20:44:26Z

tests/MongoDB.Driver.TestHelpers/Core/FailPoint.cs

-                SpinWait.SpinUntil(() => _server.Description.State == ServerState.Connected, 1000);
+                if (!SpinWait.SpinUntil(() => _server.Description.State == ServerState.Connected, 1000))
+                {
+                    throw new InvalidOperationException("Server is not connected.");


It's better to throw here, to avoid getting cryptic exception ("pool is in paused state").

sanych-sun · 2025-01-06T20:48:24Z

tests/MongoDB.Driver.Tests/Specifications/UnifiedTestSpecRunner.cs

+                throw new SkipException("Skipped because CSharp Driver has an issue with handling read timeout for sync code-path.");
+            }
+
+            if (CoreTestConfiguration.Cluster.Description.Type == ClusterType.Sharded &&


Have to skip these tests for MongoDB sharded cluster 4.2, as they are not compatible with it. Confirmed with Java team, they have to skip them as well: https://github.com/mongodb/mongo-java-driver/blob/08880c810b76f8c8d165f6ce66c83060c7bc93c5/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java#L244

Add a comment stating that these tests are not compatible with MongoDB 4.2. Otherwise it is not clear why we only run them for 4.4+.

sanych-sun · 2025-01-06T20:52:28Z

tests/MongoDB.Driver.Tests/Specifications/UnifiedTestSpecRunner.cs

+        {
+            if (testCase.Name.Contains("add RetryableWriteError and UnknownTransactionCommitResult labels to connection errors"))
+            {
+                throw new SkipException("Skipped because CSharp Driver has an issue with handling read timeout for sync code-path.");


There is test that is failing in sync path because authors of the test trying to emulate network error, by setting fail point that blocks connection for longer then read timeout - so driver should abort the connection and get network error. That test works as expected for ASYNC path because we manually apply read timeout, but not for SYNC path. It worth to investigate and do the same for sync path as well, but as discussed with Boris - we probably will postpone it and will do in scope of CSOT implementation.

Reasonable. I would file a CSHARP ticket and note its number here so that we don't lose it.

Probably related to https://jira.mongodb.org/browse/CSHARP-3662

sanych-sun · 2025-01-06T20:55:12Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/IUnifiedFailPointOperation.cs

@@ -1,24 +0,0 @@
-/* Copyright 2020-present MongoDB Inc.


This interface was used to have a special case for failpoint operations to store them in a specialized collection for later disposal. This PR introducing a new method in EntityMap that let register any IDisposable for disposal after test execution.

sanych-sun · 2025-01-06T21:00:20Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedEntityMap.cs

-            var creator = new UnifiedEntityMapCreator(eventFormatters, loggingSettings);
-            return new UnifiedEntityMap(creator, async);
-        }
+            => new(eventFormatters, loggingSettings, async);


We use to have EntityMap kind of immutable (we created a new EntityMap for each CreateEntity operation, and in case of nested create entities operations - we merge them together), but in this PR I've run into a situation when CreateEntity operation inside of test references entities created by another CreateEntity operation. So it should have access to entity map created earlier... I've decided to let CreateEntity operation instead of creating and merging to simply modify single entity map.

sanych-sun · 2025-01-06T21:01:48Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedFailPointOperation.cs

        {
            var cluster = _client.GetClusterInternal();
            var session = NoCoreSession.NewHandle();

-            failPoint = FailPoint.Configure(cluster, session, _failPointCommand, _async);
+            var failPoint = FailPoint.Configure(cluster, session, _failPointCommand, _async);
+            _entityMap.RegisterForDispose(failPoint);


We register created failpoint for disposal together with entityMap, instead of returning it and letting test runner dispose them.

sanych-sun · 2025-01-06T21:03:45Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTargetedFailPointOperation.cs

        {
-            var pinnedServer = _session?.WrappedCoreSession?.CurrentTransaction?.PinnedServer;
+            var pinnedServer = _session?.WrappedCoreSession?.CurrentTransaction?.PinnedServer.EndPoint;


We cannot use the same cluster as provided session, because in several test cases we setup failPoint for hello command, which will turn cluster to be disconnected so disposal of the failPoint will throw (connection pool is paused state error). So have to use another client to configure failPoints.

sanych-sun · 2025-01-06T21:05:44Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTargetedFailPointOperation.cs

            if (pinnedServer == null)
            {
                throw new InvalidOperationException("UnifiedTargetedFailPointOperation requires a pinned server.");
            }
+
+            var client = DriverTestConfiguration.CreateMongoClient(useMultipleShardRouters: true);
+            _entityMap.RegisterForDispose(client);


Have to register for disposal both: created client and failPoint itself.

sanych-sun · 2025-01-06T21:06:18Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTestRunner.cs

@@ -161,13 +159,6 @@ public void Dispose()
        {
            _logger.LogDebug("Disposing");

-            if (_failPoints != null)


Disposed together with entityMap.

JamesKovacs

LGTM. Minor comments for your consideration but nothing blocking.

JamesKovacs · 2025-01-07T00:33:27Z

tests/MongoDB.Driver.Tests/Specifications/UnifiedTestSpecRunner.cs

+        {
+            if (testCase.Name.Contains("add RetryableWriteError and UnknownTransactionCommitResult labels to connection errors"))
+            {
+                throw new SkipException("Skipped because CSharp Driver has an issue with handling read timeout for sync code-path.");


Reasonable. I would file a CSHARP ticket and note its number here so that we don't lose it.

JamesKovacs · 2025-01-07T00:37:00Z

tests/MongoDB.Driver.Tests/Specifications/UnifiedTestSpecRunner.cs

+                throw new SkipException("Skipped because CSharp Driver has an issue with handling read timeout for sync code-path.");
+            }
+
+            if (CoreTestConfiguration.Cluster.Description.Type == ClusterType.Sharded &&


Add a comment stating that these tests are not compatible with MongoDB 4.2. Otherwise it is not clear why we only run them for 4.4+.

JamesKovacs · 2025-01-07T00:46:56Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedCountOperation.cs

+            {
+                var result = _session == null
+                    ? _collection.Count(_filter, _options, cancellationToken)
+                    : _collection.Count(_session, _filter, _options, cancellationToken);


IMongoCollection<T>.Count is obsolete. We should be using IMongoCollection<T>.CountDocuments instead.

JamesKovacs · 2025-01-07T00:47:23Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedCountOperation.cs

+            {
+                var result = _session == null
+                    ? await _collection.CountAsync(_filter, _options, cancellationToken)
+                    : await _collection.CountAsync(_session, _filter, _options, cancellationToken);


Same as above except CountDocumentsAsync.

CSHARP-3678: Convert transactions spec tests to unified test format

61b98fe

sanych-sun requested a review from a team as a code owner January 6, 2025 20:40

sanych-sun requested review from JamesKovacs and removed request for a team January 6, 2025 20:40

sanych-sun force-pushed the csharp3678 branch from 5a75488 to 61b98fe Compare January 6, 2025 20:41

sanych-sun commented Jan 6, 2025

View reviewed changes

JamesKovacs approved these changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSHARP-3678: Convert transactions spec tests to unified test format #1586

CSHARP-3678: Convert transactions spec tests to unified test format #1586

sanych-sun commented Jan 6, 2025 •

edited

Loading

sanych-sun Jan 6, 2025

sanych-sun Jan 6, 2025

JamesKovacs Jan 7, 2025

sanych-sun Jan 6, 2025

JamesKovacs Jan 7, 2025

sanych-sun Jan 8, 2025

sanych-sun Jan 6, 2025

sanych-sun Jan 6, 2025

sanych-sun Jan 6, 2025

sanych-sun Jan 6, 2025

sanych-sun Jan 6, 2025

sanych-sun Jan 6, 2025

JamesKovacs left a comment

JamesKovacs Jan 7, 2025

JamesKovacs Jan 7, 2025

JamesKovacs Jan 7, 2025

JamesKovacs Jan 7, 2025

CSHARP-3678: Convert transactions spec tests to unified test format #1586

Are you sure you want to change the base?

CSHARP-3678: Convert transactions spec tests to unified test format #1586

Conversation

sanych-sun commented Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKovacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun commented Jan 6, 2025 •

edited

Loading