From aabe9c5c143d7ee4133027e70c8e6267257c56de Mon Sep 17 00:00:00 2001 From: Maxim Katcharov Date: Wed, 20 Nov 2024 13:48:30 -0700 Subject: [PATCH 1/5] Retry flaky unified tests JAVA-5393 --- .../client/coroutine/UnifiedCrudTest.kt | 2 +- .../mongodb/kotlin/client/UnifiedCrudTest.kt | 2 +- .../ClientSideOperationTimeoutTest.java | 9 +- .../unified/UnifiedReactiveStreamsTest.java | 11 ++ .../client/unified/UnifiedSyncTest.java | 11 ++ .../mongodb/client/unified/UnifiedTest.java | 132 ++++++++++++------ .../unified/UnifiedTestFailureValidator.java | 12 ++ .../unified/UnifiedTestModifications.java | 17 +++ .../mongodb/workload/WorkloadExecutor.java | 6 + 9 files changed, 157 insertions(+), 45 deletions(-) diff --git a/driver-kotlin-coroutine/src/integration/kotlin/com/mongodb/kotlin/client/coroutine/UnifiedCrudTest.kt b/driver-kotlin-coroutine/src/integration/kotlin/com/mongodb/kotlin/client/coroutine/UnifiedCrudTest.kt index 036ec5afcc4..5091058573e 100644 --- a/driver-kotlin-coroutine/src/integration/kotlin/com/mongodb/kotlin/client/coroutine/UnifiedCrudTest.kt +++ b/driver-kotlin-coroutine/src/integration/kotlin/com/mongodb/kotlin/client/coroutine/UnifiedCrudTest.kt @@ -24,7 +24,7 @@ internal class UnifiedCrudTest() : UnifiedTest() { @JvmStatic @Throws(URISyntaxException::class, IOException::class) fun data(): Collection? { - return getTestData("unified-test-format/crud") + return getTestData("unified-test-format/crud", true) } } } diff --git a/driver-kotlin-sync/src/integration/kotlin/com/mongodb/kotlin/client/UnifiedCrudTest.kt b/driver-kotlin-sync/src/integration/kotlin/com/mongodb/kotlin/client/UnifiedCrudTest.kt index eb06f5c1875..f030cb54645 100644 --- a/driver-kotlin-sync/src/integration/kotlin/com/mongodb/kotlin/client/UnifiedCrudTest.kt +++ b/driver-kotlin-sync/src/integration/kotlin/com/mongodb/kotlin/client/UnifiedCrudTest.kt @@ -24,7 +24,7 @@ internal class UnifiedCrudTest() : UnifiedTest() { @JvmStatic @Throws(URISyntaxException::class, IOException::class) fun data(): Collection? { - return getTestData("unified-test-format/crud") + return getTestData("unified-test-format/crud", false) } } } diff --git a/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/ClientSideOperationTimeoutTest.java b/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/ClientSideOperationTimeoutTest.java index 168ff4b8f81..a1063f05362 100644 --- a/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/ClientSideOperationTimeoutTest.java +++ b/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/ClientSideOperationTimeoutTest.java @@ -99,18 +99,25 @@ The Reactive Streams specification prevents us from allowing a subsequent next c @MethodSource("data") @Override public void shouldPassAllOutcomes( + final String testName, @Nullable final String fileDescription, @Nullable final String testDescription, @Nullable final String directoryName, + final int attemptNumber, + final int totalAttempts, final String schemaVersion, @Nullable final BsonArray runOnRequirements, final BsonArray entitiesArray, final BsonArray initialData, final BsonDocument definition) { try { - super.shouldPassAllOutcomes(fileDescription, + super.shouldPassAllOutcomes( + testName, + fileDescription, testDescription, directoryName, + attemptNumber, + totalAttempts, schemaVersion, runOnRequirements, entitiesArray, diff --git a/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/UnifiedReactiveStreamsTest.java b/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/UnifiedReactiveStreamsTest.java index 62c1315e240..28c8a27f8fa 100644 --- a/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/UnifiedReactiveStreamsTest.java +++ b/driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/UnifiedReactiveStreamsTest.java @@ -24,6 +24,7 @@ import com.mongodb.client.unified.UnifiedTest; import com.mongodb.client.unified.UnifiedTestModifications; import com.mongodb.client.vault.ClientEncryption; +import com.mongodb.lang.NonNull; import com.mongodb.reactivestreams.client.MongoClients; import com.mongodb.reactivestreams.client.gridfs.GridFSBuckets; import com.mongodb.reactivestreams.client.internal.vault.ClientEncryptionImpl; @@ -31,6 +32,11 @@ import com.mongodb.reactivestreams.client.syncadapter.SyncGridFSBucket; import com.mongodb.reactivestreams.client.syncadapter.SyncMongoClient; import com.mongodb.reactivestreams.client.syncadapter.SyncMongoDatabase; +import org.junit.jupiter.params.provider.Arguments; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.util.Collection; import static com.mongodb.client.unified.UnifiedTestModifications.Modifier; import static com.mongodb.client.unified.UnifiedTestModifications.TestDef; @@ -94,4 +100,9 @@ protected void postCleanUp(final TestDef testDef) { disableSleep(); } } + + @NonNull + protected static Collection getTestData(final String directory) throws URISyntaxException, IOException { + return getTestData(directory, true); + } } diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedSyncTest.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedSyncTest.java index 37db7cfe907..afcc8e4f1a3 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedSyncTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedSyncTest.java @@ -25,6 +25,12 @@ import com.mongodb.client.gridfs.GridFSBuckets; import com.mongodb.client.internal.ClientEncryptionImpl; import com.mongodb.client.vault.ClientEncryption; +import com.mongodb.lang.NonNull; +import org.junit.jupiter.params.provider.Arguments; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.util.Collection; public abstract class UnifiedSyncTest extends UnifiedTest { protected UnifiedSyncTest() { @@ -44,4 +50,9 @@ protected GridFSBucket createGridFSBucket(final MongoDatabase database) { protected ClientEncryption createClientEncryption(final MongoClient keyVaultClient, final ClientEncryptionSettings clientEncryptionSettings) { return new ClientEncryptionImpl(keyVaultClient, clientEncryptionSettings); } + + @NonNull + protected static Collection getTestData(final String directory) throws URISyntaxException, IOException { + return getTestData(directory, false); + } } diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java index d55feb55395..234844e5621 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java @@ -21,10 +21,6 @@ import com.mongodb.MongoNamespace; import com.mongodb.ReadPreference; import com.mongodb.UnixServerAddress; -import com.mongodb.client.unified.UnifiedTestModifications.TestDef; -import com.mongodb.event.TestServerMonitorListener; -import com.mongodb.internal.logging.LogMessage; -import com.mongodb.logging.TestLoggingInterceptor; import com.mongodb.WriteConcern; import com.mongodb.client.ClientSession; import com.mongodb.client.MongoClient; @@ -32,16 +28,20 @@ import com.mongodb.client.gridfs.GridFSBucket; import com.mongodb.client.model.Filters; import com.mongodb.client.test.CollectionHelper; +import com.mongodb.client.unified.UnifiedTestModifications.TestDef; import com.mongodb.client.vault.ClientEncryption; import com.mongodb.connection.ClusterDescription; import com.mongodb.connection.ClusterType; import com.mongodb.connection.ServerDescription; import com.mongodb.event.CommandEvent; import com.mongodb.event.CommandStartedEvent; +import com.mongodb.event.TestServerMonitorListener; import com.mongodb.internal.connection.TestCommandListener; import com.mongodb.internal.connection.TestConnectionPoolListener; +import com.mongodb.internal.logging.LogMessage; import com.mongodb.lang.NonNull; import com.mongodb.lang.Nullable; +import com.mongodb.logging.TestLoggingInterceptor; import com.mongodb.test.AfterBeforeParameterResolver; import org.bson.BsonArray; import org.bson.BsonBoolean; @@ -57,14 +57,17 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.opentest4j.AssertionFailedError; import org.opentest4j.TestAbortedException; import java.io.File; import java.io.IOException; import java.net.URISyntaxException; +import java.text.MessageFormat; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.concurrent.ExecutionException; @@ -81,6 +84,7 @@ import static com.mongodb.client.test.CollectionHelper.getCurrentClusterTime; import static com.mongodb.client.test.CollectionHelper.killAllSessions; import static com.mongodb.client.unified.RunOnRequirementsMatcher.runOnRequirementsMet; +import static com.mongodb.client.unified.UnifiedTestModifications.doSkips; import static com.mongodb.client.unified.UnifiedTestModifications.testDef; import static java.util.Collections.singletonList; import static java.util.stream.Collectors.toList; @@ -101,6 +105,9 @@ public abstract class UnifiedTest { private static final Set PRESTART_POOL_ASYNC_WORK_MANAGER_FILE_DESCRIPTIONS = Collections.singleton( "wait queue timeout errors include details about checked out connections"); + public static final int ATTEMPTS = 3; + private static Set completed = new HashSet<>(); + @Nullable private String fileDescription; private String schemaVersion; @@ -155,32 +162,51 @@ public Entities getEntities() { } @NonNull - protected static Collection getTestData(final String directory) throws URISyntaxException, IOException { + protected static Collection getTestData(final String directory, final boolean isReactive) + throws URISyntaxException, IOException { List data = new ArrayList<>(); for (File file : getTestFiles("/" + directory + "/")) { BsonDocument fileDocument = getTestDocument(file); - for (BsonValue cur : fileDocument.getArray("tests")) { - data.add(UnifiedTest.createTestData(directory, fileDocument, cur.asDocument())); + + final BsonDocument testDocument = cur.asDocument(); + String testDescription = testDocument.getString("description").getValue(); + String fileDescription = fileDocument.getString("description").getValue(); + TestDef testDef = testDef(directory, fileDescription, testDescription, isReactive); + doSkips(testDef); + + boolean forceFlaky = testDef.wasAssignedModifier(UnifiedTestModifications.Modifier.FORCE_FLAKY); + boolean retry = forceFlaky || testDef.wasAssignedModifier(UnifiedTestModifications.Modifier.RETRY); + + int attempts = retry ? ATTEMPTS : 1; + if (forceFlaky) { + attempts = 10; + } + + for (int attempt = 1; attempt <= attempts; attempt++) { + String testName = !retry + ? MessageFormat.format("{0}: {1}", fileDescription, testDescription) + : MessageFormat.format( + "{0}: {1} ({2} of {3})", + fileDescription, testDescription, attempt, attempts); + data.add(Arguments.of( + testName, + fileDescription, + testDescription, + directory, + attempt, + attempts * (forceFlaky ? -1 : 1), + fileDocument.getString("schemaVersion").getValue(), + fileDocument.getArray("runOnRequirements", null), + fileDocument.getArray("createEntities", new BsonArray()), + fileDocument.getArray("initialData", new BsonArray()), + testDocument)); + } } } return data; } - @NonNull - private static Arguments createTestData( - final String directory, final BsonDocument fileDocument, final BsonDocument testDocument) { - return Arguments.of( - fileDocument.getString("description").getValue(), - testDocument.getString("description").getValue(), - directory, - fileDocument.getString("schemaVersion").getValue(), - fileDocument.getArray("runOnRequirements", null), - fileDocument.getArray("createEntities", new BsonArray()), - fileDocument.getArray("initialData", new BsonArray()), - testDocument); - } - protected BsonDocument getDefinition() { return definition; } @@ -193,9 +219,12 @@ protected BsonDocument getDefinition() { @BeforeEach public void setUp( + final String testName, @Nullable final String fileDescription, @Nullable final String testDescription, @Nullable final String directoryName, + final int attemptNumber, + final int totalAttempts, final String schemaVersion, @Nullable final BsonArray runOnRequirements, final BsonArray entitiesArray, @@ -288,8 +317,9 @@ protected void postCleanUp(final TestDef testDef) { } /** - * This method is called once per {@link #setUp(String, String, String, String, org.bson.BsonArray, org.bson.BsonArray, org.bson.BsonArray, org.bson.BsonDocument)}, - * unless {@link #setUp(String, String, String, String, org.bson.BsonArray, org.bson.BsonArray, org.bson.BsonArray, org.bson.BsonDocument)} fails unexpectedly. + * This method is called once per + * {@link #setUp(String, String, String, String, int, int, String, org.bson.BsonArray, org.bson.BsonArray, org.bson.BsonArray, org.bson.BsonDocument)}, unless + * {@link #setUp(String, String, String, String, int, int, String, org.bson.BsonArray, org.bson.BsonArray, org.bson.BsonArray, org.bson.BsonDocument)} fails unexpectedly. */ protected void skips(final String fileDescription, final String testDescription) { } @@ -298,40 +328,58 @@ protected boolean isReactive() { return false; } - @ParameterizedTest(name = "{0}: {1}") + @ParameterizedTest(name = "{0}") @MethodSource("data") public void shouldPassAllOutcomes( + final String testName, @Nullable final String fileDescription, @Nullable final String testDescription, @Nullable final String directoryName, + final int attemptNumber, + final int totalAttempts, final String schemaVersion, @Nullable final BsonArray runOnRequirements, final BsonArray entitiesArray, final BsonArray initialData, final BsonDocument definition) { - BsonArray operations = definition.getArray("operations"); - for (int i = 0; i < operations.size(); i++) { - BsonValue cur = operations.get(i); - assertOperation(rootContext, cur.asDocument(), i); + boolean forceFlaky = totalAttempts < 0; + if (!forceFlaky) { + assumeFalse(completed.contains(testName), "Skipping retryable test that succeeded"); + completed.add(testName); } + try { + BsonArray operations = definition.getArray("operations"); + for (int i = 0; i < operations.size(); i++) { + BsonValue cur = operations.get(i); + assertOperation(rootContext, cur.asDocument(), i); + } - if (definition.containsKey("outcome")) { - assertOutcome(rootContext); - } + if (definition.containsKey("outcome")) { + assertOutcome(rootContext); + } - if (definition.containsKey("expectEvents")) { - compareEvents(rootContext, definition); - } + if (definition.containsKey("expectEvents")) { + compareEvents(rootContext, definition); + } - if (definition.containsKey("expectLogMessages")) { - ArrayList tweaks = new ArrayList<>(singletonList( - // `LogMessage.Entry.Name.OPERATION` is not supported, therefore we skip matching its value - LogMatcher.Tweak.skip(LogMessage.Entry.Name.OPERATION))); - if (getMongoClientSettings().getClusterSettings() - .getHosts().stream().anyMatch(serverAddress -> serverAddress instanceof UnixServerAddress)) { - tweaks.add(LogMatcher.Tweak.skip(LogMessage.Entry.Name.SERVER_PORT)); + if (definition.containsKey("expectLogMessages")) { + ArrayList tweaks = new ArrayList<>(singletonList( + // `LogMessage.Entry.Name.OPERATION` is not supported, therefore we skip matching its value + LogMatcher.Tweak.skip(LogMessage.Entry.Name.OPERATION))); + if (getMongoClientSettings().getClusterSettings() + .getHosts().stream().anyMatch(serverAddress -> serverAddress instanceof UnixServerAddress)) { + tweaks.add(LogMatcher.Tweak.skip(LogMessage.Entry.Name.SERVER_PORT)); + } + compareLogMessages(rootContext, definition, tweaks); + } + } catch (AssertionFailedError e) { + completed.remove(testName); + boolean lastAttempt = attemptNumber == Math.abs(totalAttempts); + if (forceFlaky || lastAttempt) { + throw e; + } else { + assumeFalse(completed.contains(testName), "Ignoring failure and retrying attempt " + attemptNumber); } - compareLogMessages(rootContext, definition, tweaks); } } diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestFailureValidator.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestFailureValidator.java index 88458f8af8e..0472ef8e6ce 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestFailureValidator.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestFailureValidator.java @@ -36,9 +36,12 @@ final class UnifiedTestFailureValidator extends UnifiedSyncTest { @Override @BeforeEach public void setUp( + final String testName, @Nullable final String fileDescription, @Nullable final String testDescription, final String directoryName, + final int attemptNumber, + final int totalAttempts, final String schemaVersion, @Nullable final BsonArray runOnRequirements, final BsonArray entitiesArray, @@ -46,9 +49,12 @@ public void setUp( final BsonDocument definition) { try { super.setUp( + testName, fileDescription, testDescription, directoryName, + attemptNumber, + totalAttempts, schemaVersion, runOnRequirements, entitiesArray, @@ -63,9 +69,12 @@ public void setUp( @ParameterizedTest @MethodSource("data") public void shouldPassAllOutcomes( + final String testName, @Nullable final String fileDescription, @Nullable final String testDescription, @Nullable final String directoryName, + final int attemptNumber, + final int totalAttempts, final String schemaVersion, @Nullable final BsonArray runOnRequirements, final BsonArray entitiesArray, @@ -74,9 +83,12 @@ public void shouldPassAllOutcomes( if (exception == null) { try { super.shouldPassAllOutcomes( + testName, fileDescription, testDescription, directoryName, + attemptNumber, + totalAttempts, schemaVersion, runOnRequirements, entitiesArray, diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java index 5b11218518f..4da6f9308bf 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java @@ -28,6 +28,7 @@ import static com.mongodb.ClusterFixture.isServerlessTest; import static com.mongodb.ClusterFixture.isSharded; import static com.mongodb.ClusterFixture.serverVersionLessThan; +import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.FORCE_FLAKY; import static com.mongodb.assertions.Assertions.assertNotNull; import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.IGNORE_EXTRA_EVENTS; import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.SKIP; @@ -38,6 +39,14 @@ public final class UnifiedTestModifications { public static void doSkips(final TestDef def) { + // TODO reasons for retry + def.modify(FORCE_FLAKY) + // Exception in encryption library: ChangeCipherSpec message sequence violation + .test("client-side-encryption", "namedKMS-createDataKey", "create datakey with named KMIP KMS provider") + // Number of checked out connections must match expected + .test("load-balancers", "cursors are correctly pinned to connections for load-balanced clusters", "pinned connections are returned after a network error during a killCursors request") + .test("client-side-encryption", "namedKMS-rewrapManyDataKey", "rewrap to kmip:name1"); + // atlas-data-lake def.skipAccordingToSpec("Data lake tests should only run on data lake") @@ -497,5 +506,13 @@ public enum Modifier { * Skip the test. */ SKIP, + /** + * Retry the test on failure. + */ + RETRY, + /** + * Retry the test multiple times, without ignoring failures. + */ + FORCE_FLAKY, } } diff --git a/driver-workload-executor/src/main/com/mongodb/workload/WorkloadExecutor.java b/driver-workload-executor/src/main/com/mongodb/workload/WorkloadExecutor.java index 0e995cb34fd..7aba736aebc 100644 --- a/driver-workload-executor/src/main/com/mongodb/workload/WorkloadExecutor.java +++ b/driver-workload-executor/src/main/com/mongodb/workload/WorkloadExecutor.java @@ -98,18 +98,24 @@ protected boolean terminateLoop() { BsonArray createEntities = fileDocument.getArray("createEntities", new BsonArray()); BsonArray initialData = fileDocument.getArray("initialData", new BsonArray()); unifiedTest.setUp( + "", null, null, null, + 1, + 1, schemaVersion, runOnRequirements, createEntities, initialData, testDocument); unifiedTest.shouldPassAllOutcomes( + "", null, null, null, + 1, + 1, schemaVersion, runOnRequirements, createEntities, From 6b534274a924eb8c0127ae07d948f11593be015e Mon Sep 17 00:00:00 2001 From: Maxim Katcharov Date: Mon, 2 Dec 2024 15:24:11 -0700 Subject: [PATCH 2/5] Remove parenthetical number from test name --- .../functional/com/mongodb/client/unified/UnifiedTest.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java index 234844e5621..ff703b7bdca 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java @@ -184,11 +184,7 @@ protected static Collection getTestData(final String directory, final } for (int attempt = 1; attempt <= attempts; attempt++) { - String testName = !retry - ? MessageFormat.format("{0}: {1}", fileDescription, testDescription) - : MessageFormat.format( - "{0}: {1} ({2} of {3})", - fileDescription, testDescription, attempt, attempts); + String testName = MessageFormat.format("{0}: {1}", fileDescription, testDescription); data.add(Arguments.of( testName, fileDescription, From 52d5a1764cf6554cb896ed8346ec4bbcb44324ed Mon Sep 17 00:00:00 2001 From: Maxim Katcharov Date: Wed, 4 Dec 2024 17:01:51 -0700 Subject: [PATCH 3/5] Add whenExceptionContains, fixes --- .../mongodb/client/unified/UnifiedTest.java | 19 ++++--- .../unified/UnifiedTestModifications.java | 54 +++++++++++++++---- 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java index ff703b7bdca..2057c0e8d00 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java @@ -84,7 +84,8 @@ import static com.mongodb.client.test.CollectionHelper.getCurrentClusterTime; import static com.mongodb.client.test.CollectionHelper.killAllSessions; import static com.mongodb.client.unified.RunOnRequirementsMatcher.runOnRequirementsMet; -import static com.mongodb.client.unified.UnifiedTestModifications.doSkips; +import static com.mongodb.client.unified.UnifiedTestModifications.Modifier; +import static com.mongodb.client.unified.UnifiedTestModifications.applyCustomizations; import static com.mongodb.client.unified.UnifiedTestModifications.testDef; import static java.util.Collections.singletonList; import static java.util.stream.Collectors.toList; @@ -173,10 +174,10 @@ protected static Collection getTestData(final String directory, final String testDescription = testDocument.getString("description").getValue(); String fileDescription = fileDocument.getString("description").getValue(); TestDef testDef = testDef(directory, fileDescription, testDescription, isReactive); - doSkips(testDef); + applyCustomizations(testDef); - boolean forceFlaky = testDef.wasAssignedModifier(UnifiedTestModifications.Modifier.FORCE_FLAKY); - boolean retry = forceFlaky || testDef.wasAssignedModifier(UnifiedTestModifications.Modifier.RETRY); + boolean forceFlaky = testDef.wasAssignedModifier(Modifier.FORCE_FLAKY); + boolean retry = forceFlaky || testDef.wasAssignedModifier(Modifier.RETRY); int attempts = retry ? ATTEMPTS : 1; if (forceFlaky) { @@ -241,9 +242,9 @@ public void setUp( rootContext.getAssertionContext().push(ContextElement.ofTest(definition)); ignoreExtraEvents = false; testDef = testDef(directoryName, fileDescription, testDescription, isReactive()); - UnifiedTestModifications.doSkips(testDef); + applyCustomizations(testDef); - boolean skip = testDef.wasAssignedModifier(UnifiedTestModifications.Modifier.SKIP); + boolean skip = testDef.wasAssignedModifier(Modifier.SKIP); assumeFalse(skip, "Skipping test"); skips(fileDescription, testDescription); @@ -369,6 +370,12 @@ public void shouldPassAllOutcomes( compareLogMessages(rootContext, definition, tweaks); } } catch (AssertionFailedError e) { + assertTrue(testDef.wasAssignedModifier(Modifier.RETRY)); + if (!testDef.matchesError(e)) { + // if the error is not matched, test definitions were not intended to apply; throw it + throw e; + } + completed.remove(testName); boolean lastAttempt = attemptNumber == Math.abs(totalAttempts); if (forceFlaky || lastAttempt) { diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java index 4da6f9308bf..58ce02dfa61 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java @@ -17,10 +17,12 @@ package com.mongodb.client.unified; import com.mongodb.assertions.Assertions; +import org.opentest4j.AssertionFailedError; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.function.Function; import java.util.function.Supplier; import static com.mongodb.ClusterFixture.isDataLakeTest; @@ -28,23 +30,28 @@ import static com.mongodb.ClusterFixture.isServerlessTest; import static com.mongodb.ClusterFixture.isSharded; import static com.mongodb.ClusterFixture.serverVersionLessThan; -import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.FORCE_FLAKY; import static com.mongodb.assertions.Assertions.assertNotNull; import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.IGNORE_EXTRA_EVENTS; +import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.RETRY; import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.SKIP; import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.SLEEP_AFTER_CURSOR_CLOSE; import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.SLEEP_AFTER_CURSOR_OPEN; import static com.mongodb.client.unified.UnifiedTestModifications.Modifier.WAIT_FOR_BATCH_CURSOR_CREATION; public final class UnifiedTestModifications { - public static void doSkips(final TestDef def) { + public static void applyCustomizations(final TestDef def) { // TODO reasons for retry - def.modify(FORCE_FLAKY) - // Exception in encryption library: ChangeCipherSpec message sequence violation - .test("client-side-encryption", "namedKMS-createDataKey", "create datakey with named KMIP KMS provider") - // Number of checked out connections must match expected - .test("load-balancers", "cursors are correctly pinned to connections for load-balanced clusters", "pinned connections are returned after a network error during a killCursors request") + // Exception in encryption library: ChangeCipherSpec message sequence violation + def.retry("TODO reason") + .whenExceptionContains("ChangeCipherSpec message sequence violation") + .test("client-side-encryption", "namedKMS-createDataKey", "create datakey with named KMIP KMS provider"); + + def.retry("TODO reason") + .whenExceptionContains("Number of checked out connections must match expected") + .test("load-balancers", "cursors are correctly pinned to connections for load-balanced clusters", "pinned connections are returned after a network error during a killCursors request"); + + def.retry("TODO reason") .test("client-side-encryption", "namedKMS-rewrapManyDataKey", "rewrap to kmip:name1"); // atlas-data-lake @@ -288,6 +295,7 @@ public static final class TestDef { private final boolean reactive; private final List modifiers = new ArrayList<>(); + private Function matchesError; private TestDef(final String dir, final String file, final String test, final boolean reactive) { this.dir = dir; @@ -332,7 +340,8 @@ public TestApplicator skipNoncompliant(final String reason) { * @param reason reason for skipping the test */ public TestApplicator skipNoncompliantReactive(final String reason) { - return new TestApplicator(this, reason, SKIP); + return new TestApplicator(this, reason, SKIP) + .when(() -> isReactive()); } /** @@ -350,6 +359,14 @@ public TestApplicator skipUnknownReason(final String reason) { return new TestApplicator(this, reason, SKIP); } + + /** + * The test will be retried, for the reason provided + */ + public TestApplicator retry(final String reason) { + return new TestApplicator(this, reason, RETRY); + } + public TestApplicator modify(final Modifier... modifiers) { return new TestApplicator(this, null, modifiers); } @@ -361,6 +378,13 @@ public boolean isReactive() { public boolean wasAssignedModifier(final Modifier modifier) { return this.modifiers.contains(modifier); } + + public boolean matchesError(final AssertionFailedError e) { + if (matchesError != null) { + return matchesError.apply(e); + } + return false; + } } /** @@ -368,17 +392,19 @@ public boolean wasAssignedModifier(final Modifier modifier) { */ public static final class TestApplicator { private final TestDef testDef; - private final List modifiersToApply; private Supplier precondition; private boolean matchWasPerformed = false; + private final List modifiersToApply; + private Function matchesError; + private TestApplicator( final TestDef testDef, final String reason, final Modifier... modifiersToApply) { this.testDef = testDef; this.modifiersToApply = Arrays.asList(modifiersToApply); - if (this.modifiersToApply.contains(SKIP)) { + if (this.modifiersToApply.contains(SKIP) || this.modifiersToApply.contains(RETRY)) { assertNotNull(reason); } } @@ -390,6 +416,7 @@ private TestApplicator onMatch(final boolean match) { } if (match) { this.testDef.modifiers.addAll(this.modifiersToApply); + this.testDef.matchesError = this.matchesError; } return this; } @@ -481,6 +508,13 @@ public TestApplicator when(final Supplier precondition) { this.precondition = precondition; return this; } + + public TestApplicator whenExceptionContains(final String fragment) { + this.matchesError = (final AssertionFailedError e) -> { + return e.getCause().getMessage().contains(fragment); + }; + return this; + } } public enum Modifier { From 3b2c915029dd7ab8514454f52abd52b59beadd72 Mon Sep 17 00:00:00 2001 From: Maxim Katcharov Date: Thu, 5 Dec 2024 16:15:31 -0700 Subject: [PATCH 4/5] Assertion fixes, clarity --- .../mongodb/client/unified/UnifiedTest.java | 30 ++++++++++--------- .../unified/UnifiedTestModifications.java | 27 ++++++++++------- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java index 2057c0e8d00..95cde6a1a35 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java @@ -57,7 +57,6 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; -import org.opentest4j.AssertionFailedError; import org.opentest4j.TestAbortedException; import java.io.File; @@ -96,6 +95,7 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assumptions.abort; import static org.junit.jupiter.api.Assumptions.assumeFalse; import static org.junit.jupiter.api.Assumptions.assumeTrue; import static util.JsonPoweredTestHelper.getTestDocument; @@ -107,7 +107,7 @@ public abstract class UnifiedTest { "wait queue timeout errors include details about checked out connections"); public static final int ATTEMPTS = 3; - private static Set completed = new HashSet<>(); + private static Set ignoreRemaining = new HashSet<>(); @Nullable private String fileDescription; @@ -341,8 +341,9 @@ public void shouldPassAllOutcomes( final BsonDocument definition) { boolean forceFlaky = totalAttempts < 0; if (!forceFlaky) { - assumeFalse(completed.contains(testName), "Skipping retryable test that succeeded"); - completed.add(testName); + boolean ignoreThisTest = ignoreRemaining.contains(testName); + assumeFalse(ignoreThisTest, "Skipping a retryable test that already succeeded"); + ignoreRemaining.add(testName); } try { BsonArray operations = definition.getArray("operations"); @@ -369,20 +370,21 @@ public void shouldPassAllOutcomes( } compareLogMessages(rootContext, definition, tweaks); } - } catch (AssertionFailedError e) { - assertTrue(testDef.wasAssignedModifier(Modifier.RETRY)); - if (!testDef.matchesError(e)) { - // if the error is not matched, test definitions were not intended to apply; throw it + } catch (Throwable e) { + if (forceFlaky) { throw e; } - - completed.remove(testName); - boolean lastAttempt = attemptNumber == Math.abs(totalAttempts); - if (forceFlaky || lastAttempt) { + if (!testDef.matchesThrowable(e)) { + // if the throwable is not matched, test definitions were not intended to apply; rethrow it + throw e; + } + boolean isLastAttempt = attemptNumber == Math.abs(totalAttempts); + if (isLastAttempt) { throw e; - } else { - assumeFalse(completed.contains(testName), "Ignoring failure and retrying attempt " + attemptNumber); } + + ignoreRemaining.remove(testName); + abort("Ignoring failure and retrying attempt " + attemptNumber); } } diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java index 58ce02dfa61..c472f62c217 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java @@ -44,11 +44,11 @@ public static void applyCustomizations(final TestDef def) { // TODO reasons for retry // Exception in encryption library: ChangeCipherSpec message sequence violation def.retry("TODO reason") - .whenExceptionContains("ChangeCipherSpec message sequence violation") + .whenFailureContains("ChangeCipherSpec message sequence violation") .test("client-side-encryption", "namedKMS-createDataKey", "create datakey with named KMIP KMS provider"); def.retry("TODO reason") - .whenExceptionContains("Number of checked out connections must match expected") + .whenFailureContains("Number of checked out connections must match expected") .test("load-balancers", "cursors are correctly pinned to connections for load-balanced clusters", "pinned connections are returned after a network error during a killCursors request"); def.retry("TODO reason") @@ -295,7 +295,7 @@ public static final class TestDef { private final boolean reactive; private final List modifiers = new ArrayList<>(); - private Function matchesError; + private Function matchesThrowable; private TestDef(final String dir, final String file, final String test, final boolean reactive) { this.dir = dir; @@ -379,9 +379,9 @@ public boolean wasAssignedModifier(final Modifier modifier) { return this.modifiers.contains(modifier); } - public boolean matchesError(final AssertionFailedError e) { - if (matchesError != null) { - return matchesError.apply(e); + public boolean matchesThrowable(final Throwable e) { + if (matchesThrowable != null) { + return matchesThrowable.apply(e); } return false; } @@ -396,7 +396,7 @@ public static final class TestApplicator { private boolean matchWasPerformed = false; private final List modifiersToApply; - private Function matchesError; + private Function matchesThrowable; private TestApplicator( final TestDef testDef, @@ -416,7 +416,7 @@ private TestApplicator onMatch(final boolean match) { } if (match) { this.testDef.modifiers.addAll(this.modifiersToApply); - this.testDef.matchesError = this.matchesError; + this.testDef.matchesThrowable = this.matchesThrowable; } return this; } @@ -509,9 +509,14 @@ public TestApplicator when(final Supplier precondition) { return this; } - public TestApplicator whenExceptionContains(final String fragment) { - this.matchesError = (final AssertionFailedError e) -> { - return e.getCause().getMessage().contains(fragment); + public TestApplicator whenFailureContains(final String messageFragment) { + this.matchesThrowable = (final Throwable e) -> { + // inspect the cause for failed assertions with a cause + if (e instanceof AssertionFailedError && e.getCause() != null) { + return e.getCause().getMessage().contains(messageFragment); + } else { + return e.getMessage().contains(messageFragment); + } }; return this; } From 784dc5135438bd0cca6cc209c2f44a820406250c Mon Sep 17 00:00:00 2001 From: Maxim Katcharov Date: Thu, 5 Dec 2024 16:57:16 -0700 Subject: [PATCH 5/5] Add docs --- .../com/mongodb/client/unified/UnifiedTest.java | 2 +- .../client/unified/UnifiedTestModifications.java | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java index 95cde6a1a35..3610d77a91a 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTest.java @@ -382,7 +382,7 @@ public void shouldPassAllOutcomes( if (isLastAttempt) { throw e; } - + ignoreRemaining.remove(testName); abort("Ignoring failure and retrying attempt " + attemptNumber); } diff --git a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java index c472f62c217..4ad7dc69b51 100644 --- a/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java +++ b/driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java @@ -509,6 +509,12 @@ public TestApplicator when(final Supplier precondition) { return this; } + /** + * The modification, if it is a RETRY, will only be applied when the + * failure message contains the provided message fragment. If an + * {@code AssertionFailedError} occurs, and has a cause, the cause's + * message will be checked. Otherwise, the throwable will be checked. + */ public TestApplicator whenFailureContains(final String messageFragment) { this.matchesThrowable = (final Throwable e) -> { // inspect the cause for failed assertions with a cause @@ -546,11 +552,15 @@ public enum Modifier { */ SKIP, /** - * Retry the test on failure. + * Ignore results and retry the test on failure. Will not repeat the + * test if the test succeeds. Multiple copies of the test are used to + * facilitate retries. */ RETRY, /** - * Retry the test multiple times, without ignoring failures. + * The test will be retried multiple times, without the results being + * ignored. This is a helper that can be used, in patches, to check + * if certain tests are (still) flaky. */ FORCE_FLAKY, }