Skip to content

CCOR-13193 - adding test run against oss server in ci - #497

Merged
chrishagglund-ship-it merged 14 commits into
mainfrom
e2e-against-conductor-with-local-script
Sep 21, 2026
Merged

chrishagglund-ship-it merged 14 commits into
mainfrom
e2e-against-conductor-with-local-script

Conversation

@chrishagglund-ship-it

@chrishagglund-ship-it chrishagglund-ship-it commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Adds a second integration-test CI job that runs the suite against plain Conductor
OSS, alongside the existing authenticated run against the shared Orkes dev server.

scripts/docker-compose-oss.yaml brings up Conductor OSS + Postgres + httpbin, and
its image: line is the single place the default tag is written;
scripts/run-integration-oss.sh runs the same stack locally.

Orkes-Enterprise-only surface (Authorization, Secrets, Schema, Service Registry,
metadata/scheduler tags) is gated behind CONDUCTOR_SERVER_TYPE=oss and skips on
OSS. The Signal API tests do run on OSS, using WAIT-based fixture variants
(complex_wf_signal_test_oss and friends) — YIELD is Orkes-only, so the existing
fixtures never reach a signalable state there. A few call sites need an OSS-specific
path rather than a skip (archiving on delete, the missing /workflow/{id}/status,
create-with-overwrite); each is gated on OSS on purpose, so the Orkes path still
fails if the real behaviour regresses.

Requires the E2E_TEST_OSS_CONDUCTOR_VERSION org variable, with a repository
access policy that includes this repo. The new job fails fast if it can't resolve
an image tag — except on a fork PR, where GitHub withholds the variable and the job
falls through to the compose file's default rather than failing.

Also fixes three latent bugs in the existing Orkes run

Running against a fresh, single-tenant server surfaced these. All three affect the
integration-test job too, not just the new one:

  1. test_async.py was clobbering another suite's task def — it registered a bare
    TaskDef under python_integration_test_task, the name
    test_workflow_execution.py runs real workflows against. Those two run as
    concurrent CI jobs against one server, so last writer won, and a bare def gets the
    server's default 3600s responseTimeoutSeconds — so a stalled worker held the task
    for an hour and the workflow sat IN_PROGRESS. It now registers its own def instead.
  2. run_workflow_execution_tests never registered its task def
    generate_tasks_defs() was dead code, so the def was whatever another suite left
    behind. Registering it sets response_timeout_seconds=2 (see 1) and makes the suite
    work on a fresh server.
  3. The Signal API tests signalled before the workflow was parked
    _wait_for_blocking_task matched any in-flight task, but the outer SUB_WORKFLOW
    flips to IN_PROGRESS before the nested workflow reaches its blocking task, so the
    signal raced the real block (intermittent "Expected BLOCKING_TASK, got None" / 404
    "Found no blocked task"). It now descends into running sub-workflows and matches
    only a non-terminal WAIT/YIELD task.

Also

  • Server-flavour checks route through one is_oss() helper instead of 9 open-coded
    env reads, 4 of them inverted.
  • test_task_metadata_service.py gains a tearDownClass — its defs used to be left
    behind, so re-runs hit "already exists".
  • Fixture owner emails use the test@conductoross.io placeholder csharp-sdk settled
    on for this ticket, not real employee addresses.
  • run-integration-oss.sh and the CI job both pull the image unconditionally so a
    cached mutable tag isn't reused; OSS health wait 120s → 180s; workflow_dispatch
    gains an oss_conductor_version input.

@chrishagglund-ship-it
chrishagglund-ship-it marked this pull request as ready for review August 19, 2026 17:56
@chrishagglund-ship-it chrishagglund-ship-it changed the title adding test run against oss server in ci CCOR-13193 - adding test run against oss server in ci Aug 26, 2026
Comment thread scripts/run-integration-oss.sh
Comment thread .github/workflows/pull_request.yml
@chrishagglund-ship-it
chrishagglund-ship-it force-pushed the e2e-against-conductor-with-local-script branch 2 times, most recently from 0e08ab5 to 99ad862 Compare September 21, 2026 15:25
chrishagglund-ship-it and others added 14 commits September 21, 2026 10:56
…tched instead of re-fetching it, halving poll requests and fixing its always-empty timeout log.
…or fork repo PRs which won't get access to the variable.
Ports javascript-sdk#176's resolution of the same review feedback, so the OSS
harness stays diffable across the SDKs. csharp-sdk#171, java-sdk#156 and
go-sdk#279 carry the identical change.

One tag, one home. The tag was written twice: FORK_PR_FALLBACK_VERSION in the
integration-tests-oss job, and `latest` as the local script's default. A local
run therefore could not reproduce a CI failure, which is most of why the script
exists. Rather than teach one of them to read the other, make the `image:` line
of docker-compose-oss.yaml the only place it is written and let both fall
through to it: the script applies no default of its own and exports
OSS_CONDUCTOR_VERSION only when --version actually supplied one, and the
fork-PR branch of the resolve step now leaves the variable unset instead of
pinning its own copy. Fork CI and a plain local run reach the identical image
by the identical path, with no YAML parsing on either side and one hardcode
removed rather than a mechanism added. The non-fork empty case still fails
loudly.

Worth stating plainly: E2E_TEST_OSS_CONDUCTOR_VERSION is currently set to
`latest` at the org level, so the pin is nominal for normal CI runs until
someone sets it to a real version. The compose default is what actually holds
the line today, on fork PRs and locally.

The script's "Using ..." and "Pulling ..." lines now come from `compose config
--images` instead of reconstructing the tag, so they stay honest whichever
source supplied it. Note the grep: `--images` lists every service's image and
does not reliably honour the service-name filter it accepts, so the server's
image is selected by name rather than by position. This stack has three
services and postgres:16 sorts first, so position would have been wrong here.

Pull in CI. The script pulls before `up`, CI did not. On a GitHub-hosted runner
the VM is ephemeral and starts with no cached copy, so `up` pulls anyway and
the step is redundant today — kept regardless, because it costs no extra
network pull (`up` then finds the image locally), it splits "couldn't pull the
image" from "the stack didn't come up" into two distinct red steps, and it is
what stops a mutable tag going stale the day this job moves to a self-hosted
runner with a warm Docker daemon. It also prints the tag in use, which matters
now that a fork PR's tag is not spelled out in the workflow.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The marker description said "excluded from CI", which stopped being true
once integration-tests-oss existed: that job runs --bucket=all, which
applies no -m filter, so the three marked cases (test_02 sync, test_02
async, test_03_no_performance_overhead) do run there. Reading the
description alone, deselecting them in the OSS job looks like the
consistent thing to do -- it isn't.

The carve-out is server-specific, not test-specific. It exists because
the shared sdkdev server doesn't reliably fire a server-side task timeout
on a CI-bounded timeline, so the long-sync/long-async buckets deselect
those cases. A dedicated local OSS stack doesn't have that problem: all
seven --bucket=all OSS runs on this branch are green, and the per-test
logs on five of them show all three marked cases passing every time.

It is also their only CI coverage -- core ignores both lease files and
the long-* buckets deselect the marker -- so filtering them out of the
OSS job would leave the server-side timeout path unexercised.

Restores the rationale comment on integration-tests-oss, which was in the
commit that introduced --bucket=all (458cd6b) and was trimmed by a later
cleanup pass (7bf2eec), reworded so "the carve-out does not apply" can't
be misread as "the test is skipped".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chrishagglund-ship-it
chrishagglund-ship-it force-pushed the e2e-against-conductor-with-local-script branch from 99ad862 to b23a246 Compare September 21, 2026 16:57
@chrishagglund-ship-it
chrishagglund-ship-it merged commit d62963f into main Sep 21, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants