From 3b2c915029dd7ab8514454f52abd52b59beadd72 Mon Sep 17 00:00:00 2001 From: Maxim Katcharov Date: Thu, 5 Dec 2024 16:15:31 -0700 Subject: [PATCH] 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 2057c0e8d0..95cde6a1a3 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 58ce02dfa6..c472f62c21 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; }