CNTRLPLANE-3237: Add inParallel helper for running encryption test steps concurrently#2352
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds a new ChangesParallel test helper
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/library/encryption/helpers.go (1)
485-509: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo cancellation/timeout for hung steps.
If one step blocks indefinitely (e.g. a network call in an encryption test),
wg.Wait()will block forever, relying solely on the globalgo test -timeoutto eventually abort. Consider threading acontext.Context(with a deadline) throughtestStepso hung steps can be detected/cancelled independently.As per path instructions, "context.Context for cancellation and timeouts" is expected for Go code in this area.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/library/encryption/helpers.go` around lines 485 - 509, The inParallel helper can hang forever because it only waits on a WaitGroup and has no per-step cancellation or timeout handling. Update the testStep flow used by inParallel to thread a context.Context with a deadline through the testFunc path, and ensure each step can observe cancellation so a blocked step can be detected and aborted independently. Keep the fix localized around inParallel and the testStep/testFunc contract so hung encryption test steps don’t rely solely on the global test timeout.Source: Path instructions
test/library/encryption/helpers_test.go (1)
119-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTiming-based assertion risks flakiness in CI.
Asserting
elapsed < 1sfor three 500ms sleeps run in parallel is a reasonable 2x margin locally, but under loaded/throttled CI runners (or lowGOMAXPROCS) this can flake. Consider asserting oncompleted/goroutine-count evidence of concurrency instead of, or in addition to, wall-clock timing, or use a looser/relative threshold.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/library/encryption/helpers_test.go` around lines 119 - 133, The concurrency test in TestInParallelRunsConcurrently relies on a strict wall-clock threshold, which can be flaky under slow or heavily loaded CI. Update the test to validate concurrency using more deterministic evidence from inParallel/testStep behavior, such as completion ordering or a shared signal/counter, and if timing remains, loosen it to a relative threshold. Use the existing TestInParallelRunsConcurrently and inParallel identifiers to locate the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/library/encryption/helpers.go`:
- Around line 480-509: The inParallel helper does not preserve the
FailNow/require stop-immediately behavior because each step runs inside a wg.Go
goroutine in test/library/encryption/helpers.go. Update inParallel so the
combined testFunc checks whether any step failed after wg.Wait() and then
triggers a real FailNow from the caller goroutine (where supported), and adjust
the comment on inParallel/testStep to reflect that require/t.Fatal in parallel
steps only fails that goroutine unless explicitly propagated.
---
Nitpick comments:
In `@test/library/encryption/helpers_test.go`:
- Around line 119-133: The concurrency test in TestInParallelRunsConcurrently
relies on a strict wall-clock threshold, which can be flaky under slow or
heavily loaded CI. Update the test to validate concurrency using more
deterministic evidence from inParallel/testStep behavior, such as completion
ordering or a shared signal/counter, and if timing remains, loosen it to a
relative threshold. Use the existing TestInParallelRunsConcurrently and
inParallel identifiers to locate the assertion.
In `@test/library/encryption/helpers.go`:
- Around line 485-509: The inParallel helper can hang forever because it only
waits on a WaitGroup and has no per-step cancellation or timeout handling.
Update the testStep flow used by inParallel to thread a context.Context with a
deadline through the testFunc path, and ensure each step can observe
cancellation so a blocked step can be detected and aborted independently. Keep
the fix localized around inParallel and the testStep/testFunc contract so hung
encryption test steps don’t rely solely on the global test timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 57516a8f-b999-4c41-af9c-9da41e8b2aa4
📒 Files selected for processing (2)
test/library/encryption/helpers.gotest/library/encryption/helpers_test.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| @@ -0,0 +1,133 @@ | |||
| package encryption | |||
There was a problem hiding this comment.
yes. I would prefer having tests that ensure errors from individual runs will be reported and propagated to the caller.
| wrapped := make([]testStep, len(steps)) | ||
| for i, s := range steps { | ||
| orig := s.testFunc | ||
| wrapped[i] = testStep{name: s.name, testFunc: func(t testing.TB) { |
There was a problem hiding this comment.
instead of closing over the loop variable, can we consider passing s.testFunc into the closure.
There was a problem hiding this comment.
pushed, see if you like it.
| if rec.Failed() != tt.expectFailed { | ||
| t.Errorf("expected Failed()=%v, actual %v", tt.expectFailed, rec.Failed()) | ||
| } | ||
| if actual := completed.Load(); actual != tt.expectCompleted { |
There was a problem hiding this comment.
checking completed.Load() immediately may not give goroutines enough time to complete.
can we add small grace time here?
There was a problem hiding this comment.
I think that wg.Wait() guarantees all goroutines will finish before inParallel returns.
Does it make sense ?
p0lyn0mial
force-pushed
the
add-inparallel-helper
branch
from
82af32a to
d318a48
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, p0lyn0mial 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 |
|
@p0lyn0mial: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
p0lyn0mial
changed the title
|
@p0lyn0mial: This pull request references CNTRLPLANE-3237 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 story to target the "5.0.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. |
openshift-ci-robot
added
the
jira/valid-reference
3 participants
could be used as a more generic mechanism to run arbitrary scenarios for #2349
Summary by CodeRabbit