✨ Concurrent E2E Test Execution#2675
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR updates the E2E Godog test runner to take advantage of scenario isolation (from #2651) by running most scenarios in parallel while reserving tagged scenarios for a serial pass, and then printing a combined failure summary.
Changes:
- Split E2E execution into two Godog runs: parallel (excluding
@Serial) followed by serial (@Serialonly), with aggregated failure output. - Tag TLS E2E feature(s) with
@Serialso they run only in the serial phase. - Reduce
test-experimental-e2etimeout from 20m to 10m.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/e2e/features_test.go | Runs E2E suite in parallel + serial phases and prints an aggregated failure summary. |
| test/e2e/features/tls.feature | Marks TLS feature as @Serial to exclude it from the parallel phase. |
| Makefile | Adjusts experimental E2E timeout setting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2675 +/- ##
==========================================
- Coverage 68.10% 68.08% -0.02%
==========================================
Files 145 145
Lines 10700 10700
==========================================
- Hits 7287 7285 -2
- Misses 2885 2886 +1
- Partials 528 529 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Create buffers to capture output for final summary | ||
| var parallelBuf, serialBuf bytes.Buffer | ||
|
|
||
| parallelOpts := cliOpts | ||
| if parallelOpts.Concurrency == 1 { | ||
| // Override default concurrency value with 100; otherwise use whatever was provided by CLI | ||
| parallelOpts.Concurrency = 100 | ||
| } | ||
| parallelOpts.Tags = "~@Serial" | ||
| // Write to both specified output (live to stdout, by default) and buffer (for summary) | ||
| parallelOpts.Output = io.MultiWriter(parallelOpts.Output, ¶llelBuf) | ||
| // run tests concurrently |
There was a problem hiding this comment.
This duplicates the entire godog console output into in-memory buffers (parallelBuf/serialBuf) via io.MultiWriter. With many scenarios (and especially at high concurrency), this can consume a lot of memory and slow down CI due to extra copying. Consider limiting what’s captured (e.g., only buffering after the "--- Failed steps:" marker, keeping only the last N lines, or writing captured output to a temp file and only reading it back on failure).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| go test test/e2e/features_test.go --godog.tags="@WebhookProviderCertManager" | ||
| ``` | ||
|
|
||
| Note that setting the tags in this way will disable the automatic test parallelization. If running in parallel with custom tags is desired, set `--godog.concurrency=100` for instance to re-enable. If this is done adding `&& ~@Serial` to the tags as well is highly recommended: |
There was a problem hiding this comment.
The sentence starting with “If this is done…” is grammatically awkward and a bit hard to parse. Consider rephrasing (e.g., “If you do this, adding && ~@Serial to the tag expression is highly recommended…”) to make the guidance clearer.
| Note that setting the tags in this way will disable the automatic test parallelization. If running in parallel with custom tags is desired, set `--godog.concurrency=100` for instance to re-enable. If this is done adding `&& ~@Serial` to the tags as well is highly recommended: | |
| Note that setting the tags in this way will disable the automatic test parallelization. If running in parallel with custom tags is desired, set `--godog.concurrency=100` for instance to re-enable. If you do this, adding `&& ~@Serial` to the tags is also highly recommended: |
|
There are also tests in |
Good callout; I haven't seen that fail yet, so it might be fine to leave it, or we could just get ahead of it. I'm fine either way. |
|
Let's get ahead of it, since the proxy tests may be disruptive. |
Done! |
|
/approve |
openshift-ci
Bot
added
the
approved
|
Been seeing test failures; if they continue I may need to run that test as |
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
Great PR — splitting e2e execution into parallel and serial phases cuts wall-clock time nearly in half. The @Serial tagging is well-chosen and the README updates are clear. One critical bug and one nit noted inline.
| set +e; \ | ||
| go test -count=1 -v ./test/e2e/features_test.go -timeout=${E2E_TIMEOUT} -args --godog.tags="~@Serial" --godog.concurrency=100; \ | ||
| parallelExit=$$?; \ | ||
| go test -count=1 -v ./test/e2e/features_test.go -timeout=${E2E_TIMEOUT} -args --godog.tags="@Serial" --godog.concurrency=1; \ | ||
| serialExit=$$?; \ | ||
| if [[ $$parallelExit -ne 0 ]] || [[ $$serialExit -ne 0 ]]; then \ | ||
| echo "e2e tests failed: parallel=$$parallelExit serial=$$serialExit"; \ | ||
| exit 1; \ | ||
| fi |
There was a problem hiding this comment.
This was an intentional decision to avoid complexity, as doing a timeout removes the ability for developers to command-line interrupt their tests.
| test-experimental-e2e: export INSTALL_DEFAULT_CATALOGS := false | ||
| test-experimental-e2e: PROMETHEUS_VALUES := helm/prom_experimental.yaml | ||
| test-experimental-e2e: E2E_TIMEOUT := 25m | ||
| test-experimental-e2e: E2E_TIMEOUT ?= 25m |
There was a problem hiding this comment.
My intent was to allow overriding here - not realizing that the way the timeout is define above in e2e sets it initially for the whole file, which negates the ?=. This should be fixed now that I've set the definition inside the e2e target instead of globally.
Takes advantage of changes made to isolate test runs to execute as many tests in parallel as possible. For tests that must be run serially, the @serial tag has been added to the beginning of relevant feature file(s). Signed-off-by: Daniel Franz <dfranz@redhat.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
| e2e: #EXHELP Run the e2e tests. | ||
| go test -count=1 -v ./test/e2e/features_test.go -timeout=$(E2E_TIMEOUT) $(if $(GODOG_ARGS),-args $(GODOG_ARGS)) | ||
| ifeq ($(strip $(GODOG_ARGS)),) | ||
| set +e; \ |
There was a problem hiding this comment.
Nit (non-blocking): with set +e, Ctrl+C during the parallel phase kills go test but the serial phase still starts — the user needs to Ctrl+C a second time. Adding a trap before set +e would make it exit immediately:
trap 'exit 130' INT; \
set +e; \Minor ergonomic improvement for local dev. Fine to skip or do in a follow-up.
|
@pedjak has basically given approval except for a nit. I want to try this downstream to see how it behaves. |
|
I tested this downstream, and it passed. |
openshift-merge-bot
Bot
merged commit ea60813
into
operator-framework:main
Description
Takes advantage of changes made to isolate test runs (#2651) to execute as many tests in parallel as possible. For tests that must be run serially, the
@Serialtag has been added to the beginning of relevant feature file(s).Tests are now executed in two steps: parallel followed by serial. Since this creates two distinct test outputs, the final output has been modified to combine any step failures into one smaller output at the end, while still maintaining live output to stdout. Ideally this could be formatted into a nicer looking markdown output in the test summary that we already output, but I'm saving that for a future PR.
go testexecution speedup results (compared to a recent run):Reviewer Checklist