Retry flaky unified tests (#1565)

JAVA-5795

Author: katcharov

driver-core/src/main/com/mongodb/assertions/Assertions.java

@@ -179,6 +179,19 @@ public static boolean assertTrue(final boolean value) throws AssertionError {
         return true;
     }
 
+    /**
+     * @param value A value to check.
+     * @param message The message.
+     * @return {@code true}.
+     * @throws AssertionError If {@code value} is {@code false}.
+     */
+    public static boolean assertTrue(final boolean value, final String message) throws AssertionError {
+        if (!value) {
+            throw new AssertionError(message);
+        }
+        return true;
+    }
+
     /**
      * @param value A value to check.
      * @return {@code false}.

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<Arguments>? {
-            return getTestData("unified-test-format/crud")
+            return getTestData("unified-test-format/crud", true)
         }
     }
 }

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<Arguments>? {
-            return getTestData("unified-test-format/crud")
+            return getTestData("unified-test-format/crud", false)
         }
     }
 }

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,

driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/UnifiedReactiveStreamsTest.java

@@ -24,13 +24,19 @@
 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;
 import com.mongodb.reactivestreams.client.syncadapter.SyncClientEncryption;
 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<Arguments> getTestData(final String directory) throws URISyntaxException, IOException {
+        return getTestData(directory, true);
+    }
 }

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<Arguments> getTestData(final String directory) throws URISyntaxException, IOException {
+        return getTestData(directory, false);
+    }
 }

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

@@ -62,9 +62,11 @@
 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 +83,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.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;
@@ -91,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;
@@ -101,6 +106,10 @@ public abstract class UnifiedTest {
     private static final Set<String> PRESTART_POOL_ASYNC_WORK_MANAGER_FILE_DESCRIPTIONS = Collections.singleton(
             "wait queue timeout errors include details about checked out connections");
 
+    public static final int RETRY_ATTEMPTS = 3;
+    public static final int FORCE_FLAKY_ATTEMPTS = 10;
+    private static final Set<String> ATTEMPTED_TESTS_TO_HENCEFORTH_IGNORE = new HashSet<>();
+
     @Nullable
     private String fileDescription;
     private String schemaVersion;
@@ -156,32 +165,49 @@ public Entities getEntities() {
     }
 
     @NonNull
-    protected static Collection<Arguments> getTestData(final String directory) throws URISyntaxException, IOException {
+    protected static Collection<Arguments> getTestData(final String directory, final boolean isReactive)
+            throws URISyntaxException, IOException {
         List<Arguments> 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);
+                applyCustomizations(testDef);
+
+                boolean forceFlaky = testDef.wasAssignedModifier(Modifier.FORCE_FLAKY);
+                boolean retry = forceFlaky || testDef.wasAssignedModifier(Modifier.RETRY);
+
+                int attempts;
+                if (retry) {
+                    attempts = forceFlaky ? FORCE_FLAKY_ATTEMPTS : RETRY_ATTEMPTS;
+                } else {
+                    attempts = 1;
+                }
+
+                for (int attempt = 1; attempt <= attempts; attempt++) {
+                    String testName = MessageFormat.format("{0}: {1}", fileDescription, testDescription);
+                    data.add(Arguments.of(
+                            testName,
+                            fileDescription,
+                            testDescription,
+                            directory,
+                            attempt,
+                            attempts,
+                            fileDocument.getString("schemaVersion").getValue(),
+                            fileDocument.getArray("runOnRequirements", null),
+                            fileDocument.getArray("createEntities", new BsonArray()),
+                            fileDocument.getArray("initialData", new BsonArray()),
+                            testDocument.clone()));
+                }
             }
         }
         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;
     }
@@ -194,9 +220,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,
@@ -218,9 +247,9 @@ public void setUp(
         ignoreExtraEvents = false;
         if (directoryName != null && fileDescription != null && testDescription != null) {
             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);
@@ -295,8 +324,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) {
     }
@@ -305,40 +335,72 @@ 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 = testDef.wasAssignedModifier(Modifier.FORCE_FLAKY);
+        if (!forceFlaky) {
+            boolean ignoreThisTest = ATTEMPTED_TESTS_TO_HENCEFORTH_IGNORE.contains(testName);
+            assumeFalse(ignoreThisTest, "Skipping a retryable test that already succeeded");
+            // The attempt is what counts, since a test may fail with
+            // something like "ignored", and would not be retried.
+            // Only failures should trigger another attempt.
+            ATTEMPTED_TESTS_TO_HENCEFORTH_IGNORE.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<LogMatcher.Tweak> 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<LogMatcher.Tweak> 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 (TestAbortedException e) {
+            // if a test is ignored, we do not retry
+            throw e;
+        } catch (Throwable e) {
+            if (forceFlaky) {
+                throw e;
+            }
+            if (testDef != null && !testDef.matchesThrowable(e)) {
+                // if the throwable is not matched, test definitions were not intended to apply; rethrow it
+                throw e;
             }
-            compareLogMessages(rootContext, definition, tweaks);
+            boolean isLastAttempt = attemptNumber == totalAttempts;
+            if (isLastAttempt) {
+                throw e;
+            }
+
+            ATTEMPTED_TESTS_TO_HENCEFORTH_IGNORE.remove(testName);
+            abort("Ignoring failure and retrying attempt " + attemptNumber);
         }
     }