✨ 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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rashmigottipati, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Suggestion: Simplify by moving orchestration to the MakefileThe two-phase orchestration in Go (dual Alternative approachKeep the e2e:
go test -count=1 -v ./test/e2e/features_test.go -timeout=$(E2E_TIMEOUT) \
$(if $(GODOG_ARGS),-args $(GODOG_ARGS),-args --godog.tags="~@Serial" --godog.concurrency=100)
go test -count=1 -v ./test/e2e/features_test.go -timeout=$(E2E_TIMEOUT) \
$(if $(GODOG_ARGS),-args $(GODOG_ARGS),-args --godog.tags="@Serial" --godog.concurrency=1)This would mean:
Safety net: skip
|
|
New changes are detected. LGTM label has been removed. |
|
I've restored the go test code in favor of splitting the runs in the makefile - PTAL @pedjak The test results will be a little more difficult to see but we can probably do something about that in a future PR. |
|
Been seeing test failures; if they continue I may need to run that test as |
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>
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.
| echo "e2e tests failed: tests timed out"; \ | ||
| exit 1; \ | ||
| elif [[ $$parallelExitCode -ne 0 ]] || [[ $$serialExitCode -ne 0 ]]; then \ | ||
| echo "e2e tests failed: parallel=$$parallelExitCode serial=$$serialExitCode"; \ |
There was a problem hiding this comment.
Bug: variable scoping silently swallows test failures
parallelExitCode and serialExitCode are set inside the bash -c '...' subshell, but read outside it. Variables set in a subshell are not visible to the calling shell.
In bash, [[ "" -ne 0 ]] treats the empty string as 0, so the elif is always false. Additionally, the last statement inside bash -c is a variable assignment (serialExitCode=$?), which always exits 0, so timeout also returns 0 regardless of test results.
Net effect: if either test phase fails, make e2e still exits 0. CI passes today because the tests pass — this only manifests on failure, which is exactly when you need it.
Suggested fix — move the exit-code logic inside the bash -c subshell:
ifeq ($(strip $(GODOG_ARGS)),)
set +e; \
timeout -s SIGTERM $(E2E_TIMEOUT) bash -c ' \
go test -count=1 -v ./test/e2e/features_test.go -args --godog.tags="~@Serial" --godog.concurrency=100; \
parallelExit=$$?; \
go test -count=1 -v ./test/e2e/features_test.go -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'; \
exitCode=$$?; \
if [[ $$exitCode -ne 0 ]]; then \
exit 1; \
fi
else| # restores the original configuration during cleanup, so scenarios are independent. | ||
|
|
||
| # This feature file is run serially to avoid potential issues modifying the catalogd | ||
| # deployment during functional testing. |
There was a problem hiding this comment.
Nit: trailing whitespace on lines 9 and 11.
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