Clean up TestDef API, clarify naming (#1566)

* Clean up TestDef API, clarify naming

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

Co-authored-by: Nabil Hachicha <[email protected]>

* PR fixes

---------

Co-authored-by: Nabil Hachicha <[email protected]>

Author: katcharov

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

@@ -91,6 +91,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.assumeFalse;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 import static util.JsonPoweredTestHelper.getTestDocument;
 import static util.JsonPoweredTestHelper.getTestFiles;
@@ -216,7 +217,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

@@ -17,8 +17,6 @@
 package com.mongodb.client.unified;
 
 import com.mongodb.assertions.Assertions;
-import com.mongodb.lang.NonNull;
-import com.mongodb.lang.Nullable;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -30,11 +28,12 @@
 import static com.mongodb.ClusterFixture.isServerlessTest;
 import static com.mongodb.ClusterFixture.isSharded;
 import static com.mongodb.ClusterFixture.serverVersionLessThan;
+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;
 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 +89,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 +128,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,58 +291,58 @@ 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, SKIP);
         }
 
         /**
          * 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, SKIP);
         }
 
         /**
          * 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, SKIP);
         }
 
         /**
          * 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, SKIP);
         }
 
         /**
          * 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, SKIP);
         }
 
         /**
          * 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, SKIP);
         }
 
         public TestApplicator modify(final Modifier... modifiers) {
-            return new TestApplicator(this, Arrays.asList(modifiers));
+            return new TestApplicator(this, null, modifiers);
         }
 
         public boolean isReactive() {
@@ -360,44 +359,28 @@ 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;
 
         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 reason,
+                final Modifier... modifiersToApply) {
             this.testDef = testDef;
-            this.shouldSkip = true;
-            this.reasonToApply = reason;
-            this.modifiersToApply = new ArrayList<>();
+            this.modifiersToApply = Arrays.asList(modifiersToApply);
+            if (this.modifiersToApply.contains(SKIP)) {
+                assertNotNull(reason);
+            }
         }
 
         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 +493,9 @@ public enum Modifier {
          * Reactive only.
          */
         WAIT_FOR_BATCH_CURSOR_CREATION,
+        /**
+         * Skip the test.
+         */
+        SKIP,
     }
 }