test: improve e2e error handling #525
Conversation
WalkthroughTemporal test helpers now accept a maximum page size (25) and iterate workflows concurrently per page. The api_connectors test call is updated to pass the new page-size argument; workflow-id matching logic was simplified to rely on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TS as Test Suite
participant Iter as iterateThroughTemporalWorkflowExecutions
participant TC as Temporal Client
participant WF as Workflows (per page)
participant CB as Callback goroutines
participant Ch as Results Channel
TS->>Iter: iterateThroughTemporalWorkflowExecutions(ctx, cl, 25, cb)
loop Pages until NextPageToken == nil or shouldStop
Iter->>TC: ListOpenWorkflowExecutions(MaximumPageSize=25, NextPageToken)
TC-->>Iter: Executions + NextPageToken
par For each execution
Iter->>CB: spawn callback in goroutine
CB-->>Ch: bool (shouldStop for this item)
and continue...
end
Iter->>Iter: wait for all goroutines, drain Ch
alt Any result == true
Iter-->>TS: return (shouldStop)
else No result true
Note right of Iter: Continue to next page if token present
end
end
Iter-->>TS: done
sequenceDiagram
autonumber
participant TS as Test Suite
participant Flush as flushRemainingWorkflows
participant Iter as iterateThroughTemporalWorkflowExecutions
participant Term as Terminate Workflow
TS->>Flush: flushRemainingWorkflows(ctx, cl)
Flush->>Iter: iterate(..., maxPageSize=25, callback=terminate)
loop Each workflow (concurrent inside iterate)
Iter->>Term: Terminate(workflow)
Term-->>Flush: error or nil
end
Flush->>Flush: collect errors, ignore serviceerror.NotFound
alt Any non-NotFound error
Flush->>TS: assert failure
else No actionable errors
Flush-->>TS: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #525 +/- ##
=======================================
Coverage 66.71% 66.72%
=======================================
Files 761 761
Lines 39749 39749
=======================================
+ Hits 26518 26521 +3
+ Misses 11844 11842 -2
+ Partials 1387 1386 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/suite_test.go (1)
191-205: Early-stop signal isn’t propagated beyond the current page.Breaking the results loop doesn’t stop outer pagination; function should return when any callback returns true.
Apply:
- // wait for this batch of goroutines to finish before allowing the loop to continue + // wait for this batch of goroutines to finish before allowing the loop to continue wg.Wait() close(ch) - for shouldStop := range ch { - if shouldStop { - break - } - } + stopAll := false + for shouldStop := range ch { + if shouldStop { + stopAll = true + break + } + } + if stopAll { + return + } if len(workflowRes.NextPageToken) == 0 { break }
🧹 Nitpick comments (2)
test/e2e/suite_test.go (1)
140-144: Avoid magic number 25 for page size.Define a shared const and reuse here and at the call site to keep them in sync.
Example:
const temporalPageSize = 25test/e2e/api_connectors_test.go (1)
456-456: Use shared page-size const instead of literal 25.Keeps this call aligned with suite_test.go.
- iterateThroughTemporalWorkflowExecutions(ctx, cl, 25, func(info *v17.WorkflowExecutionInfo) bool { + iterateThroughTemporalWorkflowExecutions(ctx, cl, temporalPageSize, func(info *v17.WorkflowExecutionInfo) bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/e2e/api_connectors_test.go(1 hunks)test/e2e/suite_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (1)
test/e2e/suite_test.go (1)
8-8: Imports look good.Needed for new error handling paths.
Also applies to: 21-21
…w how many workflows will be fetched
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/e2e/suite_test.go (2)
145-152: Good: error aggregation avoids the deadlock risk.Switching from a buffered channel to a mutex-protected slice fixes the “drain-after-iteration” deadlock the earlier code risked.
184-192: Fix compile error (chan capacity type) and loop variable capture.
- make(chan …, maxPageSize) fails: capacity must be int, not int32.
- Goroutine captures the loop variable info; pass it as a parameter.
- ch := make(chan bool, maxPageSize) - wg := &sync.WaitGroup{} + pageSize := int(maxPageSize) + ch := make(chan bool, pageSize) + var wg sync.WaitGroup for _, info := range workflowRes.Executions { - wg.Add(1) - go func() { - defer wg.Done() - ch <- callbackFn(info) - }() + wg.Add(1) + go func(info *v17.WorkflowExecutionInfo) { + defer wg.Done() + ch <- callbackFn(info) + }(info) }
🧹 Nitpick comments (3)
test/e2e/suite_test.go (3)
140-140: Prefer a const for page size.Use a const to document intent and avoid accidental mutation.
- maxPageSize := 25 + const maxPageSize = 25
142-144: Minor: avoid pointer mutex and preallocate errs capacity.Slightly simpler and avoids extra allocs.
- mu := &sync.Mutex{} - errs := make([]error, 0) + var mu sync.Mutex + errs := make([]error, 0, maxPageSize)
169-170: Type consistency for page size.maxPageSize is int32 for the RPC (good), but channel capacities are int. Cast where used for channels (see next comment) or consider taking an int here and casting to int32 only for the request.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/e2e/suite_test.go(3 hunks)
🔇 Additional comments (3)
test/e2e/suite_test.go (3)
8-8: Imports look correct and necessary.Both errors and serviceerror are used below.
Also applies to: 21-21
155-162: LGTM: selective NotFound handling.Treating NotFound as non-fatal and asserting others keeps the flush robust.
177-180: LGTM: request construction.Namespace, paging token, and MaximumPageSize usage are correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/suite_test.go (1)
155-162: Filter NotFound at source to reduce locking and allocations.Skip appending NotFound inside the callback to avoid taking the mutex and growing errs unnecessarily.
iterateThroughTemporalWorkflowExecutions(ctx, cl, int32(maxPageSize), func(info *v17.WorkflowExecutionInfo) bool { - err := cl.TerminateWorkflow(ctx, info.Execution.WorkflowId, info.Execution.RunId, "system flush") - if err != nil { - mu.Lock() - errs = append(errs, err) - mu.Unlock() - } + if err := cl.TerminateWorkflow(ctx, info.Execution.WorkflowId, info.Execution.RunId, "system flush"); err != nil { + var notFoundErr *serviceerror.NotFound + if errors.As(err, ¬FoundErr) { + return false + } + mu.Lock() + errs = append(errs, err) + mu.Unlock() + } return false })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/e2e/suite_test.go(3 hunks)
🔇 Additional comments (6)
test/e2e/suite_test.go (6)
8-8: Imports LGTM.Brings in errors and Temporal serviceerror used below; consistent.
Also applies to: 21-21
140-151: Good: eliminates the earlier error-channel deadlock and bounds termination concurrency.Mutex-protected slice avoids backpressure during pagination; concurrency now limited to page size.
175-181: Request construction looks correct.MaximumPageSize uses int32 as required by Temporal; pagination token handled.
199-203: Good: labeled break cleanly stops pagination across pages.No leak risk since channel is closed after wg.Wait; senders are done.
205-206: LGTM.Correctly terminates when there’s no next page.
169-171: iterateThroughTemporalWorkflowExecutions call sites updated Verified all usages now include the newmaxPageSizeargument.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/e2e/api_connectors_test.go(1 hunks)
🔇 Additional comments (1)
test/e2e/api_connectors_test.go (1)
457-463: Confirm ConnectorID.String() covers legacy IDs before narrowing match
Ensure all child workflows—install (“run-tasks-…”), uninstall, and reset—are generated usingConnectorID.String()(and that this returns the same legacy Reference value) so thestrings.Contains(..., connectorID.String())check won’t miss any existing workflows.
| if (strings.Contains(info.Execution.WorkflowId, connectorID.Reference.String()) || | ||
| strings.Contains(info.Execution.WorkflowId, connectorID.String())) && | ||
| strings.HasPrefix(info.Execution.WorkflowId, searchKeyword) { | ||
| iterateThroughTemporalWorkflowExecutions(ctx, cl, 25, func(info *v17.WorkflowExecutionInfo) bool { |
There was a problem hiding this comment.
Fix arg type: pass int32 to iterator to avoid compile error
iterateThroughTemporalWorkflowExecutions now takes maxPageSize int32. Passing 25 (untyped int) won’t compile.
Apply:
- iterateThroughTemporalWorkflowExecutions(ctx, cl, 25, func(info *v17.WorkflowExecutionInfo) bool {
+ iterateThroughTemporalWorkflowExecutions(ctx, cl, int32(25), func(info *v17.WorkflowExecutionInfo) bool {Optionally declare a typed const near this function and reuse it:
const temporalMaxPageSize int32 = 25🤖 Prompt for AI Agents
In test/e2e/api_connectors_test.go at line ~456, the call
iterateThroughTemporalWorkflowExecutions(ctx, cl, 25, ...) fails to compile
because the function now expects maxPageSize int32; change the literal to an
int32 by either replacing 25 with int32(25) or (preferably) declare a typed
constant like temporalMaxPageSize int32 = 25 and pass that constant to the
function so the argument type matches.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
We don't want unbounded goroutines generated by this flush command, so this moves the concurrency control to the iteration command which can limit them to the number of pages fetched