Port transactions spec tests to unified format #1310

jyemin · 2024-02-14T19:43:28Z

jmikola · 2024-02-15T14:32:15Z

@jyemin: Looking at the failures related to transactions in the most recent patch build:

latest Sharded Cluster NoAuth NoSSL JDK8 Linux fails with MigrationConflict(272), which looks like DRIVERS-2816
4.2 Sharded Cluster NoAuth NoSSL JDK8 Linux and 4.2 Sharded Cluster Auth SSL JDK8 Linux are both failing with StaleDbVersion(249), which could be related to StaleDbVersion Errors on Sharded Clusters; however, I expect the Java UTR already has that logic as these shouldn't be the first unified tests utilizing distinct and transactions.

jyemin · 2024-02-15T21:28:26Z

I expect the Java UTR already has that logic as these shouldn't be the first unified tests utilizing distinct and transactions.

It does not have that logic because no tests had been failing. Since I'm only seeing the failures on 4.2 sharded clusters, I am currently planning to skip the two distinct tests in that variant

The other failures are coming from write concern errors on collection drop. @ShaneHarvey suggested that we add a retry of drop until it succeeds. I've done that and now 3.6 and 4.0 sharded clusters are looking good.

ShaneHarvey · 2024-02-15T21:30:36Z

I'll add that Python's UTR also does not run killAllSessions or distinct since we haven't seen test failures that need those workarounds yet.

jyemin · 2024-02-15T21:34:28Z

killAllSessions definitely needed for these.

JAVA-4171

jyemin · 2024-02-15T23:58:45Z

driver-core/src/test/functional/com/mongodb/client/test/CollectionHelper.java

@@ -89,7 +93,16 @@ public static void drop(final MongoNamespace namespace) {
    }

    public static void drop(final MongoNamespace namespace, final WriteConcern writeConcern) {
-        new DropCollectionOperation(namespace, writeConcern).execute(getBinding());
+        boolean success = false;


On sharded clusters < 4.2, drop sometimes fails on the first try with a write concern error. Trying again works around it. Same for create below, except with a different exception. :(

I'm curious whether other drivers will run into this. I imagine they will and if so a DRIVERS ticket is probably in order. What do you think @jmikola ?

I don't recall encountering a PHPLIB build failure for drop or create in this context. If you want to open a DRIVERS ticket, I'd suggest including the server errors in the description and immediately opening a specs PR with the desired clarification while this is still fresh on your mind.

jyemin · 2024-02-16T00:09:28Z

.../src/test/functional/com/mongodb/reactivestreams/client/unified/UnifiedTransactionsTest.java

 public class UnifiedTransactionsTest extends UnifiedReactiveStreamsTest {
    public UnifiedTransactionsTest(@SuppressWarnings("unused") final String fileDescription,
                                   @SuppressWarnings("unused") final String testDescription,
                                   final String schemaVersion, final BsonArray runOnRequirements, final BsonArray entitiesArray,
                                   final BsonArray initialData, final BsonDocument definition) {
        super(schemaVersion, runOnRequirements, entitiesArray, initialData, definition);
+        assumeFalse(fileDescription.equals("count"));
+        if (serverVersionLessThan(4, 4) && isSharded()) {


There's a way to work around this but since it only affects 4.2, which is EOL, just skip them.

jyemin · 2024-02-16T00:59:52Z

driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java

@@ -217,6 +218,9 @@ public void setUp() {
                this::createMongoClient,
                this::createGridFSBucket,
                this::createClientEncryption);
+
+        killAllSessions();


This is part of the unified test spec but until now wasn't needed.

jyemin · 2024-02-16T01:00:32Z

driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java

@@ -838,6 +856,7 @@ private OperationResult executeAssertSessionTransactionState(final BsonDocument
        //noinspection SwitchStatementWithTooFewBranches
        switch (state) {
            case "starting":
+            case "in_progress":


This is the same as is done in the legacy test runner. There doesn't appear to be any difference in observable state between the two.

jyemin · 2024-02-16T01:04:12Z

driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java

@@ -449,7 +467,7 @@ private OperationResult executeOperation(final UnifiedTestContext context, final
                case "abortTransaction":
                    return crudHelper.executeAbortTransaction(operation);
                case "withTransaction":
-                    return crudHelper.executeWithTransaction(operation, (op, idx) -> assertOperation(context, op, idx));
+                    return crudHelper.executeWithTransaction(operation, (op, idx) -> assertOperationAndThrow(context, op, idx));


This is necessary because, unlike a normal operation where we just assert the result (exceptional or not) and then move on to the next one, when an operation is executed in the body of a withTransaction callback, any exception thrown by that operation has to propagate out of the callback, so that it can trigger an abortTransaction instead of a commitTransaction.

...nc/src/test/functional/com/mongodb/client/unified/WithTransactionHelperTransactionsTest.java

driver-core/src/test/functional/com/mongodb/client/test/CollectionHelper.java

driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java

driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTransactionsTest.java

driver-core/src/test/functional/com/mongodb/client/test/CollectionHelper.java

driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTransactionsTest.java

jyemin · 2024-02-28T18:27:49Z

After merging the fix to JAVA-5334/DRIVER-2816 into this PR, sharded transaction tests started timing out (the drop command would take 90 seconds to complete). I have a fix for that in #1320. Once that is merged into master, I'll merge master into this PR and ensure the tests all pass again.

jyemin · 2024-02-28T20:07:59Z

Turns out that other PR is not necessary. Just moving killAllSessions to right before initial data creation did the trick.

jyemin force-pushed the j4171 branch from c8eec79 to d7d1ab3 Compare February 14, 2024 19:44

jyemin mentioned this pull request Feb 14, 2024

DRIVERS-1709: Convert withTransaction tests to unified format mongodb/specifications#1505

Merged

4 tasks

jyemin force-pushed the j4171 branch from b0fcb3a to aab9f89 Compare February 15, 2024 21:23

Port legacy transactions specification tests to the unified format

05fc373

JAVA-4171

jyemin force-pushed the j4171 branch from aab9f89 to 05fc373 Compare February 15, 2024 23:54

jyemin self-assigned this Feb 15, 2024

jyemin marked this pull request as ready for review February 15, 2024 23:55

jyemin commented Feb 16, 2024

View reviewed changes

Also retry create if interrupted

7fe8499

jyemin marked this pull request as draft February 16, 2024 12:47

jyemin added 2 commits February 16, 2024 15:01

Enhance error matcher to handle other exception types

5fe2bd6

checkstyle

f735e2f

jyemin marked this pull request as ready for review February 16, 2024 21:57

jyemin requested a review from stIncMale February 21, 2024 02:39

stIncMale requested changes Feb 26, 2024

View reviewed changes

jyemin added 2 commits February 26, 2024 13:00

Add comments

a9b538b

Remove inspection suppression

9c36545

jyemin requested a review from stIncMale February 26, 2024 18:07

checkstyle

5773770

stIncMale reviewed Feb 27, 2024

View reviewed changes

driver-core/src/test/functional/com/mongodb/client/test/CollectionHelper.java Outdated Show resolved Hide resolved

driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTransactionsTest.java Show resolved Hide resolved

jyemin added 2 commits February 27, 2024 19:10

Apply suggestion

3cc70b7

Merge branch 'master' into j4171

f3a6844

stIncMale self-requested a review February 28, 2024 00:32

stIncMale approved these changes Feb 28, 2024

View reviewed changes

Move killAllSessions earlier in setup

829b0aa

jyemin requested a review from stIncMale February 28, 2024 20:07

jyemin merged commit d85982d into mongodb:master Feb 28, 2024
54 of 56 checks passed

jyemin deleted the j4171 branch February 28, 2024 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port transactions spec tests to unified format #1310

Port transactions spec tests to unified format #1310

jyemin commented Feb 14, 2024

jmikola commented Feb 15, 2024

jyemin commented Feb 15, 2024

ShaneHarvey commented Feb 15, 2024 •

edited

jyemin commented Feb 15, 2024

jyemin Feb 15, 2024 •

edited

jmikola Feb 17, 2024

jyemin Feb 16, 2024

jyemin Feb 16, 2024

jyemin Feb 16, 2024 •

edited

jyemin Feb 16, 2024

jyemin commented Feb 28, 2024 •

edited

jyemin commented Feb 28, 2024

Port transactions spec tests to unified format #1310

Port transactions spec tests to unified format #1310

Conversation

jyemin commented Feb 14, 2024

jmikola commented Feb 15, 2024

jyemin commented Feb 15, 2024

ShaneHarvey commented Feb 15, 2024 • edited

jyemin commented Feb 15, 2024

jyemin Feb 15, 2024 • edited

Choose a reason for hiding this comment

jmikola Feb 17, 2024

Choose a reason for hiding this comment

jyemin Feb 16, 2024

Choose a reason for hiding this comment

jyemin Feb 16, 2024

Choose a reason for hiding this comment

jyemin Feb 16, 2024 • edited

Choose a reason for hiding this comment

jyemin Feb 16, 2024

Choose a reason for hiding this comment

jyemin commented Feb 28, 2024 • edited

jyemin commented Feb 28, 2024

ShaneHarvey commented Feb 15, 2024 •

edited

jyemin Feb 15, 2024 •

edited

jyemin Feb 16, 2024 •

edited

jyemin commented Feb 28, 2024 •

edited