Assertion fixes, clarity

Author: katcharov

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<String> completed = new HashSet<>();
+    private static Set<String> 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);
         }
     }
 

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<Modifier> modifiers = new ArrayList<>();
-        private Function<AssertionFailedError, Boolean> matchesError;
+        private Function<Throwable, Boolean> 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<Modifier> modifiersToApply;
-        private Function<AssertionFailedError, Boolean> matchesError;
+        private Function<Throwable, Boolean> 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<Boolean> 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;
         }