From 69be7974cd1c794aca9c5a86c194798c407f0553 Mon Sep 17 00:00:00 2001
From: Chris Norman <cnorman@broadinstitute.org>
Date: Tue, 26 Apr 2022 12:28:18 -0400
Subject: [PATCH] Test for presence of ack result message and simplify
 ProcessControllerAckResult API.

---
 Dockerfile                                    |  2 +
 .../runtime/ProcessControllerAckResult.java   | 18 ++-----
 .../ProcessControllerAckResultUnitTest.java   | 51 +++++++++++++++++++
 3 files changed, 57 insertions(+), 14 deletions(-)
 create mode 100644 src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerAckResultUnitTest.java

diff --git a/Dockerfile b/Dockerfile
index 5163641d6d3..d11479fa29b 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -45,6 +45,8 @@ WORKDIR /gatk
 
 # Create a simple unit test runner
 ENV CI true
+# "export GATK_DOCKER_CONTAINER=true" is used to allow tests to determine when the're running on the docker
+# (some negative python tests use this to throw skip exceptions). See GATKBaseTest::isGATKDockerContainer.
 RUN echo "source activate gatk" > /root/run_unit_tests.sh && \
     echo "export GATK_DOCKER_CONTAINER=true" >> /root/run_unit_tests.sh && \
     echo "export TEST_JAR=\$( find /jars -name \"gatk*test.jar\" )" >> /root/run_unit_tests.sh && \
diff --git a/src/main/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerAckResult.java b/src/main/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerAckResult.java
index ee5b05f7f7c..9adfc311c68 100644
--- a/src/main/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerAckResult.java
+++ b/src/main/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerAckResult.java
@@ -4,7 +4,8 @@
 
 /**
  * Command acknowledgements that are returned from a process managed by StreamingProcessController.
- * Ack results can be positive, negative, or negative with a message.
+ * Ack results can be positive, negative, or negative with a message. Positive acks never have a messsage,
+ * negative acks may optionally have a message.
  */
 public class ProcessControllerAckResult {
 
@@ -46,18 +47,7 @@ public boolean isPositiveAck() {
      * @return true if this ack is negative and includes a message
      */
     public boolean hasMessage() {
-        return !isPositiveAck() && !message.isEmpty();
-    }
-
-    /**
-     * @return A (possibly empty) String with any message sent from the remote process.
-     * Only defined for negative acknowledgements {@link #hasMessage()}.
-     */
-    public String getNegativeACKMessage() {
-        if (isPositiveAck()) {
-            throw new GATKException("Can only retrieve messages for negative acknowledgements");
-        }
-        return message;
+        return !isPositiveAck() && message != null && !message.isEmpty();
     }
 
     /**
@@ -67,7 +57,7 @@ public String getDisplayMessage() {
         if (isPositiveAck()) {
             return ACK_LOG_MESSAGE;
         } else if (hasMessage()) {
-            return String.format("%s: %s", NCK_WITH_MESSAGE_LOG_MESSAGE, getNegativeACKMessage());
+            return String.format("%s: %s", NCK_WITH_MESSAGE_LOG_MESSAGE, message);
         } else {
             return NCK_LOG_MESSAGE;
         }
diff --git a/src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerAckResultUnitTest.java b/src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerAckResultUnitTest.java
new file mode 100644
index 00000000000..4fcc04e1010
--- /dev/null
+++ b/src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerAckResultUnitTest.java
@@ -0,0 +1,51 @@
+package org.broadinstitute.hellbender.utils.runtime;
+
+import org.broadinstitute.hellbender.GATKBaseTest;
+import org.testng.Assert;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class ProcessControllerAckResultUnitTest extends GATKBaseTest {
+
+    @DataProvider(name="ackResultTests")
+    private Object[][] getAckResultTests() {
+        return new Object[][] {
+                // ack result,
+                //      isPositiveAck, hasMessage, getDisplayMessage prefix
+                { new ProcessControllerAckResult(true),
+                        true, false, StreamingToolConstants.STREAMING_ACK_MESSAGE },
+                { new ProcessControllerAckResult(false),
+                        false, false, StreamingToolConstants.STREAMING_NCK_MESSAGE },
+                { new ProcessControllerAckResult("some message"),
+                        false, true, StreamingToolConstants.STREAMING_NCK_WITH_MESSAGE_MESSAGE },
+        };
+    }
+
+    @Test(dataProvider = "ackResultTests")
+    private void testHasMessage(
+            final ProcessControllerAckResult ackResult,
+            final boolean unusedIsPositiveAck,
+            final boolean hasMessage,
+            final String unusedGetDisplayMessagePrefix) {
+        Assert.assertEquals(ackResult.hasMessage(), hasMessage);
+    }
+
+    @Test(dataProvider = "ackResultTests")
+    private void testIsPositiveAck(
+            final ProcessControllerAckResult ackResult,
+            final boolean isPositiveAck,
+            final boolean unusedHasMessage,
+            final String unusedGetDisplayMessagePrefix) {
+        Assert.assertEquals(ackResult.isPositiveAck(), isPositiveAck);
+    }
+
+    @Test(dataProvider = "ackResultTests")
+    private void testGetDisplayMessage(
+            final ProcessControllerAckResult ackResult,
+            final boolean unusedIsPositiveAck,
+            final boolean unusedHasMessage,
+            final String getDisplayMessagePrefix) {
+        Assert.assertTrue(ackResult.getDisplayMessage().startsWith(getDisplayMessagePrefix));
+    }
+
+}