CNTRLPLANE-1724:Fix suite parallelism and test output capture #46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes two important issues in the OTE framework: 1. **Respect suite's Parallelism field in run-suite command** - Modified pkg/cmd/cmdrun/runsuite.go to check suite.Parallelism - If suite.Parallelism > 0, use it instead of command-line flag - This allows suites to enforce serial execution (Parallelism: 1) - Fixes issue where serial/slow suites ran tests in parallel 2. **Capture and preserve test output (g.By() calls)** - Modified pkg/ginkgo/util.go to properly capture test logging - Use io.MultiWriter to write to both buffer and stderr - Filter out Ginkgo reporter summary lines from JSON output - Preserves real-time visibility while capturing programmatic output - Fixes issue where test output was empty in JSON results These changes enable proper serial test execution and detailed logging for test suites using the OTE framework.
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-1724 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @gangwgr. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
needs-ok-to-test
wangke19
reviewed
Nov 17, 2025
pkg/ginkgo/util.go
Outdated
Comment on lines
98
to
102
| originalWriter := ginkgo.GinkgoWriter | ||
| ginkgo.GinkgoWriter = captureWriter | ||
| defer func() { | ||
| ginkgo.GinkgoWriter = originalWriter | ||
| }() |
There was a problem hiding this comment.
Here, we modify the global ginkgo.GinkgoWriter, remove this entirely - it's not needed! The captureWriter parameter passed to RunSpec is sufficient.
wangke19
reviewed
Nov 17, 2025
pkg/ginkgo/util.go
Outdated
|
|
||
| // Create a buffer to capture g.By() output AND write to stderr for real-time visibility | ||
| var outputBuffer bytes.Buffer | ||
| multiWriter := io.MultiWriter(&outputBuffer, os.Stderr) |
There was a problem hiding this comment.
Here simplify by Using Ginkgo's Built-in Separation, the current approach creates a 40-line filter function, but Ginkgo already separates test output from reporter output!
// Just capture to buffer for stderr visibility
var outputBuffer bytes.Buffer
multiWriter := io.MultiWriter(&outputBuffer, os.Stderr)
captureWriter := ginkgo.NewWriter(multiWriter)
ginkgo.GetSuite().RunSpec(spec, ..., captureWriter, ...)
// Use Ginkgo's pre-separated output - NO FILTERING NEEDED!
result.Output = summary.CapturedGinkgoWriterOutput // Already clean!
result.Error = summary.CapturedStdOutErr
There was a problem hiding this comment.
Benefits:
- ✅ Removes 40+ lines of filtering code (Delete filterGinkgoReporterOutput() function (40 lines removed!))
- ✅ More robust (uses Ginkgo's API instead of text parsing)
- ✅ Thread-safe (no global manipulation)
- ✅ Future-proof (works with any Ginkgo version)
- ✅ Still maintains real-time stderr visibility
- Remove redundant global GinkgoWriter modification - Use CapturedGinkgoWriterOutput directly instead of custom filtering - Remove 40-line filterGinkgoReporterOutput function - Cleaner code leveraging Ginkgo's native functionality Ginkgo already separates test output from reporter output in CapturedGinkgoWriterOutput, so the custom filtering is unnecessary. The captureWriter parameter passed to RunSpec is sufficient.
|
/lgtm |
|
@wangke19: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Author
|
This commit adds a new serial suite for testing sequential execution and includes comprehensive debug tests to validate the output capture improvements. The changes also remove suite-level parallelism override to ensure
consistent behavior across test runs.
gangwgr
force-pushed
the
fix-parallelism-and-logging
branch
from
007f65f to
0259757
Compare
Contributor
Root Cause Investigation - Critical FindingI tested the current code empirically with tests that actually produce output (using The Current Code ALREADY WORKSTest results: # Test WITH By() calls - output IS captured:
./example-tests run-test "[sig-testing] openshift-tests-extension TEMP: test with By() output"
→ output_length: 226
→ Output contains all STEP messages from By() calls ✅
# Test WITHOUT By() calls - no output (expected):
./example-tests run-test "[sig-testing] openshift-tests-extension ordered should run beforeAll once"
→ output_length: 0
→ No output because test has no By() calls (just assertions) ✅Both Why the PR Might ExistPossible explanations:
Impact on PR ReviewGiven that:
Recommendation:
Would you like me to provide my test code so you can verify the output capture bug doesn't exist in the current codebase? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes two important issues in the OTE framework:
Respect suite's Parallelism field in run-suite command
Capture and preserve test output (g.By() calls)
These changes enable proper serial test execution and detailed logging for test suites using the OTE framework.