From 51b88b37be8e959c85cef580b24df779db583ec1 Mon Sep 17 00:00:00 2001
From: Cristian Ferretti <37232625+jcferretti@users.noreply.github.com>
Date: Wed, 2 Aug 2023 16:11:11 -0400
Subject: [PATCH] For docker runs (tests) ensure we get results even when the
 run (test) fails. (#4254)

* Ensure the buildAndRun task runs the sync task even if it fails.

* Followup to review comment.

* Recover previous fix.

* Followup to review comments.
---
 buildSrc/src/main/groovy/Docker.groovy        | 26 +++++++++----------
 .../tools/docker/CombinedDockerRunTask.groovy |  1 +
 cpp-client/build.gradle                       |  2 +-
 go/build.gradle                               |  2 +-
 py/client/build.gradle                        |  8 +++---
 5 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/buildSrc/src/main/groovy/Docker.groovy b/buildSrc/src/main/groovy/Docker.groovy
index 3d1a2f77c7e..e9cabd94bed 100644
--- a/buildSrc/src/main/groovy/Docker.groovy
+++ b/buildSrc/src/main/groovy/Docker.groovy
@@ -302,8 +302,19 @@ class Docker {
             // Single task with explicit inputs and outputs, to let gradle detect if it is up to date, and let docker
             // cache what it can.
 
+            // Sync outputs to the desired location
+            def syncAfterBuildAndRun = project.tasks.register("${taskName}Sync", Sync) { sync ->
+                sync.with {
+                    // run the provided closure first
+                    cfg.copyOut.execute(sync)
+
+                    // then set the from location
+                    from dockerCopyLocation
+                }
+            }
+
             // Note that if "showLogsOnSuccess" is true, we don't run this way, since that would omit logs when cached.
-            def buildAndRun = project.tasks.register("${taskName}Run", CombinedDockerRunTask) { cacheableDockerTask ->
+            return project.tasks.register(taskName, CombinedDockerRunTask) { cacheableDockerTask ->
                 cacheableDockerTask.with {
                     // mark inputs, depend on dockerfile task and input sync task
                     inputs.files(makeImage.get().outputs.files)
@@ -334,18 +345,7 @@ class Docker {
 
                     remotePath.set(cfg.containerOutPath)
                     outputDir.set(project.file(dockerCopyLocation))
-                }
-            }
-            // Sync outputs to the desired location
-            return project.tasks.register(taskName, Sync) { sync ->
-                sync.with {
-                    dependsOn buildAndRun
-
-                    // run the provided closure first
-                    cfg.copyOut.execute(sync)
-
-                    // then set the from location
-                    from dockerCopyLocation
+                    finalizedBy syncAfterBuildAndRun
                 }
             }
         }
diff --git a/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy b/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy
index 5a812a0fc01..64d8250e8ac 100644
--- a/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy
+++ b/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy
@@ -111,6 +111,7 @@ class CombinedDockerRunTask extends AbstractDockerRemoteApiTask {
                     throw new GradleException("${failedMessage}, check logs for details")
                 }
             }
+
         } finally {
             if (containerId == null) {
                 return;
diff --git a/cpp-client/build.gradle b/cpp-client/build.gradle
index d1ee54b4ee8..ab6bf7cf670 100644
--- a/cpp-client/build.gradle
+++ b/cpp-client/build.gradle
@@ -124,5 +124,5 @@ def testCppClient = Docker.registerDockerTask(project, 'testCppClient') {
     parentContainers = [ Docker.registryTask(project, 'cpp-client-base') ]
     entrypoint = ['/cpp-client/install/bin/cpp-tests-to-junit.sh', '/out/cpp-test.xml', '/out/cpp-test.log']
 }
-deephavenDocker.shouldLogIfTaskFails tasks.named('testCppClientRun')
+deephavenDocker.shouldLogIfTaskFails testCppClient
 tasks.check.dependsOn(testCppClient)
diff --git a/go/build.gradle b/go/build.gradle
index 1231e46622b..a1baf10f6c2 100644
--- a/go/build.gradle
+++ b/go/build.gradle
@@ -86,5 +86,5 @@ def testGoClient = Docker.registerDockerTask(project, 'testGoClient') {
     parentContainers = [ Docker.registryTask(project, 'go') ]
     entrypoint = ['./go-test-to-junit.sh', '/out/go-test.xml', '/out/go.log']
 }
-deephavenDocker.shouldLogIfTaskFails tasks.named('testGoClientRun')
+deephavenDocker.shouldLogIfTaskFails testGoClient
 tasks.check.dependsOn(testGoClient)
diff --git a/py/client/build.gradle b/py/client/build.gradle
index 7c2ec933f85..589c006e8e2 100644
--- a/py/client/build.gradle
+++ b/py/client/build.gradle
@@ -81,7 +81,7 @@ deephavenDocker {
     networkName.set "pydeephaven-network-${randomSuffix}"
 }
 
-tasks.getByName('check').dependsOn(Docker.registerDockerTask(project, 'testPyClient') {
+def testPyClient = Docker.registerDockerTask(project, 'testPyClient') {
     copyIn {
         from('pydeephaven') {
             into 'project/pydeephaven'
@@ -112,5 +112,7 @@ tasks.getByName('check').dependsOn(Docker.registerDockerTask(project, 'testPyCli
     copyOut {
         into layout.buildDirectory.dir('test-results')
     }
-})
-deephavenDocker.shouldLogIfTaskFails tasks.named('testPyClientRun')
\ No newline at end of file
+}
+
+tasks.getByName('check').dependsOn(testPyClient)
+deephavenDocker.shouldLogIfTaskFails testPyClient