From d5437e5e6491f83351512428f24b1aabb85facb8 Mon Sep 17 00:00:00 2001 From: Manan Bhatt Date: Wed, 5 Aug 2026 20:25:54 +0530 Subject: [PATCH 1/3] ci: gate e2e/integration execution, not compilation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both modules ended with `tasks.forEach(task -> task.onlyIf { ... })`, which gates *every* task — compileTestJava and spotlessCheck included. Since ci.yml runs `./gradlew clean build -x test`, neither module's test sources were ever type-checked or linted on push or PR: 138 e2e tests and the integration suites could rot to compile errors unnoticed. Gate Test and JacocoReport instead (JacocoReport alongside Test, because jacocoTestReport dependsOn test — left ungated it would try to report on a skipped test task during a plain `./gradlew build`). Ungating alone was not sufficient: `-x test` prunes compileTestJava along with the excluded task, so the build step now names `testClasses` explicitly. Verified earlier by injecting a type error into a suite — `:e2e:compileTestJava FAILED`. Ungating spotless surfaced import-order violations in files it had never checked: SuiteHttpApi404, EnvironmentClientTests, PromptClientTests. Fixed via spotlessApply; those diffs are imports and blank lines only. Verified: both compileTestJava tasks now run under `clean build testClasses -x test`, while :e2e:test and :tests:test stay SKIPPED without -Pe2e / -PIntegrationTests. --- .github/workflows/ci.yml | 5 ++++- e2e/build.gradle | 8 +++++++- e2e/src/test/java/SuiteHttpApi404.java | 2 +- tests/build.gradle | 5 ++++- .../client/http/EnvironmentClientTests.java | 15 ++++++++------- .../conductor/client/http/PromptClientTests.java | 2 +- 6 files changed, 25 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 69ee6efa4..6f711707d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -134,9 +134,12 @@ jobs: distribution: "zulu" java-version: "21" + # `testClasses` is requested explicitly: `-x test` prunes compileTestJava along + # with the excluded `test` task, so without it test sources are only type-checked + # in the continue-on-error "Run Tests" step below. - name: Build id: build - run: ./gradlew clean build -x test + run: ./gradlew clean build testClasses -x test - name: Run Tests id: tests diff --git a/e2e/build.gradle b/e2e/build.gradle index 61fd17e26..c2f29dad4 100644 --- a/e2e/build.gradle +++ b/e2e/build.gradle @@ -46,4 +46,10 @@ tasks.withType(Test) { jacocoTestReport { dependsOn test // tests are required to run before generating the report } -tasks.forEach(task -> task.onlyIf { project.hasProperty('e2e') }) +// Gate execution only. Gating *every* task (as this used to) also skips +// compileTestJava and spotlessCheck, so `./gradlew build` never type-checked these +// suites and they could rot to compile errors unnoticed. JacocoReport is gated +// alongside Test because jacocoTestReport dependsOn test — left ungated it would try +// to build a report from a skipped test task. +tasks.withType(Test).configureEach { onlyIf { project.hasProperty('e2e') } } +tasks.withType(JacocoReport).configureEach { onlyIf { project.hasProperty('e2e') } } diff --git a/e2e/src/test/java/SuiteHttpApi404.java b/e2e/src/test/java/SuiteHttpApi404.java index d71569537..8fc4350a7 100644 --- a/e2e/src/test/java/SuiteHttpApi404.java +++ b/e2e/src/test/java/SuiteHttpApi404.java @@ -18,8 +18,8 @@ import io.orkes.conductor.client.AgentClient; import io.orkes.conductor.client.ApiClient; import io.orkes.conductor.client.exceptions.AgentAPIException; -import io.orkes.conductor.client.exceptions.AgentNotFoundException; import io.orkes.conductor.client.exceptions.AgentException; +import io.orkes.conductor.client.exceptions.AgentNotFoundException; import io.orkes.conductor.client.http.OrkesAgentClient; import static org.junit.jupiter.api.Assertions.assertInstanceOf; diff --git a/tests/build.gradle b/tests/build.gradle index c50ea1786..44a9fdbd4 100644 --- a/tests/build.gradle +++ b/tests/build.gradle @@ -38,4 +38,7 @@ tasks.withType(Test) { jacocoTestReport { dependsOn test // tests are required to run before generating the report } -tasks.forEach(task -> task.onlyIf { project.hasProperty('IntegrationTests') }) \ No newline at end of file +// Gate execution only — see the equivalent note in e2e/build.gradle. Gating every +// task also skipped compileTestJava and spotlessCheck for this module. +tasks.withType(Test).configureEach { onlyIf { project.hasProperty('IntegrationTests') } } +tasks.withType(JacocoReport).configureEach { onlyIf { project.hasProperty('IntegrationTests') } } \ No newline at end of file diff --git a/tests/src/test/java/io/orkes/conductor/client/http/EnvironmentClientTests.java b/tests/src/test/java/io/orkes/conductor/client/http/EnvironmentClientTests.java index 6dd0e3359..8a2e41aad 100644 --- a/tests/src/test/java/io/orkes/conductor/client/http/EnvironmentClientTests.java +++ b/tests/src/test/java/io/orkes/conductor/client/http/EnvironmentClientTests.java @@ -12,17 +12,18 @@ */ package io.orkes.conductor.client.http; -import io.orkes.conductor.client.EnvironmentClient; -import io.orkes.conductor.client.model.Tag; -import io.orkes.conductor.client.model.environment.EnvironmentVariable; -import io.orkes.conductor.client.util.ClientTestUtil; +import java.util.List; +import java.util.Optional; +import java.util.UUID; + import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; -import java.util.List; -import java.util.Optional; -import java.util.UUID; +import io.orkes.conductor.client.EnvironmentClient; +import io.orkes.conductor.client.model.Tag; +import io.orkes.conductor.client.model.environment.EnvironmentVariable; +import io.orkes.conductor.client.util.ClientTestUtil; public class EnvironmentClientTests { diff --git a/tests/src/test/java/io/orkes/conductor/client/http/PromptClientTests.java b/tests/src/test/java/io/orkes/conductor/client/http/PromptClientTests.java index 53d427232..a1e972d6c 100644 --- a/tests/src/test/java/io/orkes/conductor/client/http/PromptClientTests.java +++ b/tests/src/test/java/io/orkes/conductor/client/http/PromptClientTests.java @@ -16,6 +16,7 @@ import java.util.List; import java.util.Map; +import org.conductoross.conductor.client.model.ai.PromptTemplate; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -29,7 +30,6 @@ import io.orkes.conductor.client.model.integration.Category; import io.orkes.conductor.client.model.integration.IntegrationApiUpdate; import io.orkes.conductor.client.model.integration.IntegrationUpdate; -import org.conductoross.conductor.client.model.ai.PromptTemplate; import io.orkes.conductor.client.util.ClientTestUtil; public class PromptClientTests { From 2eb99402d9a4a84edf2938b9bffe35672beba2cd Mon Sep 17 00:00:00 2001 From: Manan Bhatt Date: Wed, 5 Aug 2026 20:25:54 +0530 Subject: [PATCH 2/3] test(e2e): fail hard when the server is unreachable; fix /api stripping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkServerHealth used assumeTrue, and a skipped JUnit test is a passing JUnit test — so an unreachable server produced a green run that had verified nothing. Reaching these suites already requires opting in with -Pe2e, so by that point the caller has explicitly asked for e2e: being unable to reach a server is a failure of what they asked for, not a reason to quietly do nothing. Now throws, with a message naming the variable to set, the minimum server version, and the credential requirement. ConnectException usually carries a null message, so the exception type is reported rather than "(null)". Also replaces `SERVER_URL.replace("/api", "")` for deriving BASE_URL. That strips every occurrence rather than the suffix, and the pattern matches inside "//api..." too: "https://api.example.com/api" collapsed to "https:/.example.com". Harmless against localhost, broken against any hosted server. Verified: with -Pe2e against a dead server, "Suite1BasicValidation > initializationError FAILED" (it dies in @BeforeAll, so this costs no LLM spend); without -Pe2e, :e2e:test is still SKIPPED so `./gradlew build` is unaffected for developers without a server. --- e2e/src/test/java/BaseTest.java | 53 ++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/e2e/src/test/java/BaseTest.java b/e2e/src/test/java/BaseTest.java index 5224165ff..d0252f001 100644 --- a/e2e/src/test/java/BaseTest.java +++ b/e2e/src/test/java/BaseTest.java @@ -25,14 +25,13 @@ import com.fasterxml.jackson.databind.ObjectMapper; import static org.junit.jupiter.api.Assertions.fail; -import static org.junit.jupiter.api.Assumptions.assumeTrue; /** * Base class for all e2e tests. * *

Provides: *

@@ -43,8 +42,24 @@ public abstract class BaseTest { protected static final String SERVER_URL = System.getenv().getOrDefault("CONDUCTOR_SERVER_URL", "http://localhost:8080/api"); - /** Base URL (without /api) for health checks and workflow fetches. */ - protected static final String BASE_URL = SERVER_URL.replace("/api", ""); + /** Base URL (without the {@code /api} suffix) for health checks and workflow fetches. */ + protected static final String BASE_URL = stripApiSuffix(SERVER_URL); + + /** + * Strip a single trailing {@code /api}, tolerating a trailing slash. + * + *

Deliberately not {@code replace("/api", "")}, which strips *every* occurrence: + * {@code https://api.example.com/api} collapsed to {@code https:/.example.com}, because + * the pattern also matches inside {@code //api...}. Harmless against localhost, broken + * against any hosted server. + */ + private static String stripApiSuffix(String url) { + String resolved = url; + while (resolved.endsWith("/")) { + resolved = resolved.substring(0, resolved.length() - 1); + } + return resolved.endsWith("/api") ? resolved.substring(0, resolved.length() - "/api".length()) : resolved; + } /** LLM model to use in e2e tests. */ protected static final String MODEL = System.getenv().getOrDefault("CONDUCTOR_AGENT_LLM_MODEL", "openai/gpt-4o-mini"); @@ -56,7 +71,14 @@ public abstract class BaseTest { /** * Check that the server is available before any tests in the class run. - * If the server is not reachable or unhealthy, all tests in the class are skipped. + * If the server is not reachable or unhealthy, all tests in the class fail. + * + *

Deliberately a hard failure, not {@code assumeTrue}. A skipped JUnit test is a + * passing JUnit test, so skipping here meant an unreachable server produced a + * green run that had verified nothing. Reaching these suites at all requires opting in + * with {@code -Pe2e} (see build.gradle), so by this point the caller has explicitly + * asked for e2e: being unable to reach a server is a failure of what they asked for, + * not a reason to quietly do nothing. */ @BeforeAll static void checkServerHealth() { @@ -75,12 +97,29 @@ static void checkServerHealth() { Object h = body.get("healthy"); healthy = Boolean.TRUE.equals(h); } - assumeTrue(healthy, "Server at " + BASE_URL + " is not healthy — skipping e2e tests"); + if (!healthy) { + throw new IllegalStateException(unreachable("server reported unhealthy")); + } + } catch (IllegalStateException e) { + throw e; } catch (Exception e) { - assumeTrue(false, "Server not available at " + BASE_URL + ": " + e.getMessage()); + // ConnectException and friends often carry a null message — naming the type is + // more useful than printing "(null)". + String detail = e.getMessage() != null && !e.getMessage().isBlank() + ? e.getClass().getSimpleName() + ": " + e.getMessage() + : e.getClass().getSimpleName(); + throw new IllegalStateException(unreachable(detail), e); } } + private static String unreachable(String detail) { + return "e2e suites need a reachable Conductor server, but " + BASE_URL + " is not usable (" + + detail + ").\n" + + " Set CONDUCTOR_SERVER_URL, or start a server with the agent runtime enabled\n" + + " (conductor-oss >= 3.32.0-rc.8). These suites also need server-side LLM\n" + + " credentials: OPENAI_API_KEY for the default openai/gpt-4o-mini model."; + } + /** * Fetch a full workflow execution from the server. * From 5e462ddb5b4833489c671a19052e24978d3c6132 Mon Sep 17 00:00:00 2001 From: Manan Bhatt Date: Wed, 5 Aug 2026 20:25:54 +0530 Subject: [PATCH 3/3] ci(e2e): run on push to main; drop the dead broker, Python, and run guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Runs on `push: [main]` in addition to PRs. The lane was PR-only, so a regression on main went unnoticed until someone opened the next PR. This matches the sibling SDKs, whose e2e jobs run on push+PR; none of them use a schedule, so cost tracks merge frequency rather than a clock, and the existing cancel-in-progress concurrency group collapses rapid pushes. - Pins CONDUCTOR_AGENT_LLM_MODEL=openai/gpt-4o-mini so CI spend cannot shift silently if BaseTest's default model changes. - Removes the mcp-testkit install/start steps and the Python setup that existed only for them. Suite4McpTools is plan()-only and asserts against a fabricated http://localhost:9999/mcp, so nothing ever connected to the broker on port 3001; `grep -rn '3001' e2e/` has no hits. This does not close the MCP gap — execution had no coverage before and still has none — it stops the job implying otherwise. - Removes the silently-empty run guard, which is now unreachable dead weight: with BaseTest failing hard instead of skipping, a green run that verified nothing cannot happen. - Corrects two false claims in the header. It said the suites never read the keys "(asserted by Suite2ToolCallingCredentials)" — that suite references neither key, and Suite7MediaTools does read OPENAI_API_KEY to gate a skip. And it said fork PRs "fail at the silently-empty guard"; they do not — /health ignores provider credentials, so the server boots, the suites run, and they fail on their LLM tasks at "Run e2e suites". --- .github/workflows/agent-e2e.yml | 78 ++++++++++++++------------------- 1 file changed, 33 insertions(+), 45 deletions(-) diff --git a/.github/workflows/agent-e2e.yml b/.github/workflows/agent-e2e.yml index 538bb49a0..687ea648a 100644 --- a/.github/workflows/agent-e2e.yml +++ b/.github/workflows/agent-e2e.yml @@ -5,13 +5,32 @@ name: Agent E2E # the agent runtime on by default from 3.32.0-rc.8 onward. A separate agent # server JAR is no longer used here. # -# These tests call real LLMs via the OPENAI_API_KEY / ANTHROPIC_API_KEY -# repo secrets. The suites themselves never read the keys (asserted by -# Suite2ToolCallingCredentials) — only the server process gets them. -# Fork PRs cannot see repo secrets, so for them the run fails at the -# silently-empty guard rather than passing vacuously. - -on: [pull_request, workflow_dispatch] +# These tests call real LLMs via the OPENAI_API_KEY / ANTHROPIC_API_KEY repo +# secrets. LLM and tool execution happens server-side, so the keys are consumed by +# the server process. Only one suite reads a key at all: Suite7MediaTools gates its +# live image-generation test on OPENAI_API_KEY. +# +# BaseTest.MODEL defaults to openai/gpt-4o-mini, so OPENAI_API_KEY is the one that +# must be present; ANTHROPIC_API_KEY matters only when the model is overridden. +# +# Fork PRs cannot see repo secrets. The server still boots healthy — /health does not +# check provider credentials — so the suites run and fail on their LLM tasks, and the +# job fails at "Run e2e suites". +# +# There is no separate "did anything actually run?" guard: BaseTest fails hard rather +# than assumeTrue-skipping when the server is unreachable, so a green run that +# verified nothing is not reachable. + +# Runs on push-to-main as well as PRs, so a regression on main is caught at merge +# rather than waiting for someone to open the next PR. Matches the sibling SDKs, +# whose e2e jobs run on push+PR; no workflow there uses a schedule, so cost tracks +# merge frequency rather than a clock, and the concurrency group above collapses +# rapid pushes. +on: + push: + branches: [main] + pull_request: + workflow_dispatch: permissions: contents: read @@ -31,6 +50,9 @@ jobs: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} CONDUCTOR_SERVER_URL: http://localhost:8080/api + # Pinned rather than left to BaseTest's default, so CI cost cannot shift silently + # if that default changes. + CONDUCTOR_AGENT_LLM_MODEL: openai/gpt-4o-mini steps: - name: Checkout code uses: actions/checkout@v4 @@ -41,13 +63,6 @@ jobs: distribution: temurin java-version: '21' - # Python is only needed for mcp-testkit and the XML guard. - # No `cache: pip` — it hard-fails without a requirements file. - - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: '3.12' - - name: Cache server JAR id: jar_cache uses: actions/cache@v4 @@ -61,15 +76,10 @@ jobs: curl -fL --retry 3 -o conductor-server.jar \ "https://repo1.maven.org/maven2/org/conductoross/conductor-server/${CONDUCTOR_SERVER_VERSION}/conductor-server-${CONDUCTOR_SERVER_VERSION}-boot.jar" - - name: Install mcp-testkit - run: | - python -m pip install --upgrade pip - pip install mcp-testkit - - - name: Start mcp-testkit - run: | - mcp-testkit --transport http --port 3001 & - sleep 2 + # No MCP broker is started: Suite4McpTools is plan()-only and asserts against a + # fabricated http://localhost:9999/mcp, so nothing ever connected to the broker + # this job used to run on port 3001. Wiring live MCP execution means adding one + # back and pointing the suite at it. - name: Start server run: | @@ -90,28 +100,6 @@ jobs: - name: Run e2e suites run: ./gradlew :e2e:test -Pe2e - # BaseTest assumeTrue-skips every suite when the server is unreachable — - # without this guard a boot failure after the health gate (or a future - # gate regression) would yield a green job that ran nothing. - - name: Guard against silently-empty runs - if: always() - run: | - python - <<'EOF' - import glob - import sys - import xml.etree.ElementTree as ET - - total = executed = 0 - for path in glob.glob("e2e/build/test-results/test/TEST-*.xml"): - root = ET.parse(path).getroot() - t = int(root.get("tests", 0)) - sk = int(root.get("skipped", 0)) - total += t - executed += t - sk - print(f"executed {executed}/{total} tests") - sys.exit(0 if executed > 0 else 1) - EOF - - name: Upload results if: always() uses: actions/upload-artifact@v4