Clean up TestDef API, clarify naming

Author: katcharov

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

@@ -216,7 +216,11 @@ public void setUp(
         ignoreExtraEvents = false;
         testDef = testDef(directoryName, fileDescription, testDescription, isReactive());
         UnifiedTestModifications.doSkips(testDef);
+
+        boolean skip = testDef.wasAssignedModifier(UnifiedTestModifications.Modifier.SKIP);
+        assumeFalse(skip, "Skipping test");
         skips(fileDescription, testDescription);
+
         assertTrue(
                 schemaVersion.equals("1.0")
                         || schemaVersion.equals("1.1")

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

@@ -36,19 +36,19 @@ final class UnifiedTestFailureValidator extends UnifiedSyncTest {
     @Override
     @BeforeEach
     public void setUp(
-            final String directoryName,
             @Nullable final String fileDescription,
             @Nullable final String testDescription,
+            final String directoryName,
             final String schemaVersion,
             @Nullable final BsonArray runOnRequirements,
             final BsonArray entitiesArray,
             final BsonArray initialData,
             final BsonDocument definition) {
         try {
             super.setUp(
-                    directoryName,
                     fileDescription,
                     testDescription,
+                    directoryName,
                     schemaVersion,
                     runOnRequirements,
                     entitiesArray,

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

@@ -18,7 +18,6 @@
 
 import com.mongodb.assertions.Assertions;
 import com.mongodb.lang.NonNull;
-import com.mongodb.lang.Nullable;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -34,7 +33,6 @@
 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;
-import static org.junit.jupiter.api.Assumptions.assumeFalse;
 
 public final class UnifiedTestModifications {
     public static void doSkips(final TestDef def) {
@@ -90,7 +88,7 @@ public static void doSkips(final TestDef def) {
         // added as part of https://jira.mongodb.org/browse/JAVA-4976 , but unknown Jira to complete
         // The implementation of the functionality related to clearing the connection pool before closing the connection
         // will be carried out once the specification is finalized and ready.
-        def.skipTodo("")
+        def.skipUnknownReason("")
                 .test("connection-monitoring-and-pooling/logging", "connection-logging", "Connection checkout fails due to error establishing connection");
 
         // load-balancers
@@ -129,7 +127,7 @@ public static void doSkips(final TestDef def) {
                 .test("crud", "count", "Deprecated count without a filter")
                 .test("crud", "count", "Deprecated count with a filter")
                 .test("crud", "count", "Deprecated count with skip and limit");
-        def.skipTodo("See downstream changes comment on https://jira.mongodb.org/browse/JAVA-4275")
+        def.skipUnknownReason("See downstream changes comment on https://jira.mongodb.org/browse/JAVA-4275")
                 .test("crud", "findOneAndReplace-hint-unacknowledged", "Unacknowledged findOneAndReplace with hint string on 4.4+ server")
                 .test("crud", "findOneAndReplace-hint-unacknowledged", "Unacknowledged findOneAndReplace with hint document on 4.4+ server")
                 .test("crud", "findOneAndUpdate-hint-unacknowledged", "Unacknowledged findOneAndUpdate with hint string on 4.4+ server")
@@ -292,54 +290,54 @@ private TestDef(final String dir, final String file, final String test, final bo
          * Test is skipped because it is pending implementation, and there is
          * a Jira ticket tracking this which has more information.
          *
-         * @param skip reason for skipping the test; must start with a Jira URL
+         * @param ticket reason for skipping the test; must start with a Jira URL
          */
-        public TestApplicator skipJira(final String skip) {
-            Assertions.assertTrue(skip.startsWith("https://jira.mongodb.org/browse/JAVA-"));
-            return new TestApplicator(this, skip);
+        public TestApplicator skipJira(final String ticket) {
+            Assertions.assertTrue(ticket.startsWith("https://jira.mongodb.org/browse/JAVA-"));
+            return new TestApplicator(this, ticket);
         }
 
         /**
          * Test is skipped because the feature under test was deprecated, and
          * was removed in the Java driver.
          *
-         * @param skip reason for skipping the test
+         * @param reason reason for skipping the test
          */
-        public TestApplicator skipDeprecated(final String skip) {
-            return new TestApplicator(this, skip);
+        public TestApplicator skipDeprecated(final String reason) {
+            return new TestApplicator(this, reason);
         }
 
         /**
          * Test is skipped because the Java driver cannot comply with the spec.
          *
-         * @param skip reason for skipping the test
+         * @param reason reason for skipping the test
          */
-        public TestApplicator skipNoncompliant(final String skip) {
-            return new TestApplicator(this, skip);
+        public TestApplicator skipNoncompliant(final String reason) {
+            return new TestApplicator(this, reason);
         }
 
         /**
          * Test is skipped because the Java Reactive driver cannot comply with the spec.
          *
-         * @param skip reason for skipping the test
+         * @param reason reason for skipping the test
          */
-        public TestApplicator skipNoncompliantReactive(final String skip) {
-            return new TestApplicator(this, skip);
+        public TestApplicator skipNoncompliantReactive(final String reason) {
+            return new TestApplicator(this, reason);
         }
 
         /**
          * The test is skipped, as specified. This should be paired with a
          * "when" clause.
          */
-        public TestApplicator skipAccordingToSpec(final String skip) {
-            return new TestApplicator(this, skip);
+        public TestApplicator skipAccordingToSpec(final String reason) {
+            return new TestApplicator(this, reason);
         }
 
         /**
          * The test is skipped for an unknown reason.
          */
-        public TestApplicator skipTodo(final String skip) {
-            return new TestApplicator(this, skip);
+        public TestApplicator skipUnknownReason(final String reason) {
+            return new TestApplicator(this, reason);
         }
 
         public TestApplicator modify(final Modifier... modifiers) {
@@ -360,10 +358,6 @@ public boolean wasAssignedModifier(final Modifier modifier) {
      */
     public static final class TestApplicator {
         private final TestDef testDef;
-
-        private final boolean shouldSkip;
-        @Nullable
-        private final String reasonToApply;
         private final List<Modifier> modifiersToApply;
         private Supplier<Boolean> precondition;
         private boolean matchWasPerformed = false;
@@ -372,32 +366,24 @@ private TestApplicator(
                 final TestDef testDef,
                 final List<Modifier> modifiersToApply) {
             this.testDef = testDef;
-            this.shouldSkip = false;
-            this.reasonToApply = null;
             this.modifiersToApply = modifiersToApply;
         }
 
         private TestApplicator(
                 final TestDef testDef,
                 @NonNull
-                final String reason) {
+                final String skipReason) {
             this.testDef = testDef;
-            this.shouldSkip = true;
-            this.reasonToApply = reason;
-            this.modifiersToApply = new ArrayList<>();
+            this.modifiersToApply = Arrays.asList(Modifier.SKIP);
         }
 
         private TestApplicator onMatch(final boolean match) {
             matchWasPerformed = true;
             if (precondition != null && !precondition.get()) {
                 return this;
             }
-            if (shouldSkip) {
-                assumeFalse(match, reasonToApply);
-            } else {
-                if (match) {
-                    this.testDef.modifiers.addAll(this.modifiersToApply);
-                }
+            if (match) {
+                this.testDef.modifiers.addAll(this.modifiersToApply);
             }
             return this;
         }
@@ -510,5 +496,9 @@ public enum Modifier {
          * Reactive only.
          */
         WAIT_FOR_BATCH_CURSOR_CREATION,
+        /**
+         * Skip the test.
+         */
+        SKIP,
     }
 }