[OPIK-8197] [BE] fix: unhang PythonEvaluatorServiceTest and bound every test with a timeout (#8108)

Author: JetoPistola

.github/scripts/discover-backend-tests.sh

@@ -4,6 +4,12 @@ set -euo pipefail
 NUM_GROUPS=16
 UNIT_TIMEOUT=10
 INTEGRATION_TIMEOUT=20
+# Per-test timeout handed to JUnit, overriding the default in junit-platform.properties.
+# CI reruns failures 3 times, so a deterministic hang costs 4x these values: 8m against the 10m
+# unit wall, 16m against the 20m integration wall. Both must stay under their job timeout above,
+# or the job is cancelled before Surefire can name the offending test.
+UNIT_TEST_TIMEOUT=2m
+INTEGRATION_TEST_TIMEOUT=4m
 TEST_DIR="src/test/java"
 PATTERN="DropwizardAppExtensionProvider\|MySQLContainer\|ClickHouseContainer\|RedisContainer\|MinIOContainer"
 
@@ -62,9 +68,9 @@ done
 
 # Build JSON matrix: unit tests + N integration groups
 matrix="{\"include\":["
-matrix+="{\"name\":\"Unit Tests\",\"tests\":\"$unit_list\",\"timeout\":$UNIT_TIMEOUT}"
+matrix+="{\"name\":\"Unit Tests\",\"tests\":\"$unit_list\",\"timeout\":$UNIT_TIMEOUT,\"testTimeout\":\"$UNIT_TEST_TIMEOUT\"}"
 for ((i=1; i<=NUM_GROUPS; i++)); do
-  matrix+=",{\"name\":\"Integration Group $i\",\"tests\":\"${group_list[$i]}\",\"timeout\":$INTEGRATION_TIMEOUT}"
+  matrix+=",{\"name\":\"Integration Group $i\",\"tests\":\"${group_list[$i]}\",\"timeout\":$INTEGRATION_TIMEOUT,\"testTimeout\":\"$INTEGRATION_TEST_TIMEOUT\"}"
 done
 matrix+="]}"
 echo "matrix=$matrix" >> "$GITHUB_OUTPUT"

.github/workflows/backend_tests.yml

@@ -84,6 +84,7 @@ jobs:
                 -Dtest="${{ matrix.tests }}"
                 -Dmaven.test.failure.ignore=true
                 -Dsurefire.rerunFailingTestsCount=3
+                -Djunit.jupiter.execution.timeout.testable.method.default="${{ matrix.testTimeout }}"
 
             - name: Publish Test Report
               uses: EnricoMi/publish-unit-test-result-action/linux@v2

apps/opik-backend/src/test/java/com/comet/opik/TestTimeoutGuardTest.java

@@ -0,0 +1,88 @@
+package com.comet.opik;
+
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.time.Duration;
+import java.util.Properties;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Asserts that the shipped per-test timeout still fits CI's retry budget.
+ * <p>
+ * CI runs with {@code -Dsurefire.rerunFailingTestsCount=3}, and a timeout is an ordinary failure to
+ * Surefire, so a deterministic hang is attempted 4 times. If 4x the bound exceeds the job wall, the
+ * job is cancelled before Surefire can publish the named failure report -- losing exactly the
+ * diagnostic the timeout was added to provide. A 5m bound was caught by review for that reason.
+ * <p>
+ * Deliberately narrow: it reads the shipped value and checks the arithmetic. It does not
+ * re-implement JUnit's timeout grammar (JUnit validates that itself, throwing
+ * {@code DateTimeParseException} at engine startup) and does not scrape the workflow or discovery
+ * script for literal substrings, which would break on unrelated formatting changes there.
+ */
+@DisplayName("Test Timeout Guard")
+class TestTimeoutGuardTest {
+
+    private static final String TIMEOUT_PROPERTY = "junit.jupiter.execution.timeout.testable.method.default";
+
+    /** The wall this file's value has to fit: INTEGRATION_TIMEOUT in discover-backend-tests.sh. */
+    private static final Duration INTEGRATION_JOB_WALL = Duration.ofMinutes(20);
+
+    /** CI runs -Dsurefire.rerunFailingTestsCount=3, and a timeout is an ordinary failure. */
+    private static final int ATTEMPTS_PER_HANG = 4;
+
+    @Test
+    @DisplayName("the shipped default survives CI's retry budget")
+    void shippedDefaultFitsRetryBudget() {
+        // Read the file, never System.getProperty: CI always passes a -D override, so consulting it
+        // would mean the shipped default -- the value local runs actually inherit -- went unchecked.
+        var shipped = loadShippedTimeout();
+
+        var budget = shipped.multipliedBy(ATTEMPTS_PER_HANG);
+
+        assertThat(budget)
+                .as("%d attempts at the shipped '%s' must fit inside the %s job wall",
+                        ATTEMPTS_PER_HANG, shipped, INTEGRATION_JOB_WALL)
+                .isPositive()
+                .isLessThan(INTEGRATION_JOB_WALL);
+    }
+
+    @Test
+    @DisplayName("the timeout is scoped to testable methods so container startup stays exempt")
+    void timeoutIsScopedToTestableMethods() {
+        // A blanket junit.jupiter.execution.timeout.default would also bound @BeforeAll/@BeforeEach,
+        // making slow, variable Testcontainers startup the thing that fails instead of the test.
+        assertThat(loadProperties().stringPropertyNames())
+                .contains(TIMEOUT_PROPERTY)
+                .doesNotContain("junit.jupiter.execution.timeout.default");
+    }
+
+    private static Duration loadShippedTimeout() {
+        var value = loadProperties().getProperty(TIMEOUT_PROPERTY);
+        assertThat(value).as("%s must be configured", TIMEOUT_PROPERTY).isNotBlank();
+
+        // Only the units the shipped value is ever expected to use. Anything else is a deliberate
+        // failure rather than a guess: silently coercing an unrecognised value to a small number is
+        // how an oversized bound would slip past the assertion above.
+        var trimmed = value.trim();
+        assertThat(trimmed).as("shipped timeout should be expressed in whole minutes or seconds")
+                .matches("[1-9]\\d* ?[ms]");
+
+        var amount = Long.parseLong(trimmed.replaceAll("[^0-9]", ""));
+        return trimmed.endsWith("m") ? Duration.ofMinutes(amount) : Duration.ofSeconds(amount);
+    }
+
+    private static Properties loadProperties() {
+        var properties = new Properties();
+        try (var in = TestTimeoutGuardTest.class.getResourceAsStream("/junit-platform.properties")) {
+            assertThat(in).as("junit-platform.properties must be on the test classpath").isNotNull();
+            properties.load(in);
+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }
+        return properties;
+    }
+}