Merged
Conversation
spkane31
commented
Mar 25, 2026
| // taskQueueStatsSuite encapsulates the test environment and parameters for task queue stats tests. | ||
| taskQueueStatsSuite struct { | ||
| testcore.Env | ||
| usePriMatcher bool |
Contributor
Author
There was a problem hiding this comment.
We could add a lint to prevent embedding testing.T in new suites, this is causing issues for our tests
Contributor
There was a problem hiding this comment.
Since #9536 decouples Env and assertions, that might already help a lot.
stephanos
reviewed
Mar 25, 2026
|
|
||
| req.ReportTaskQueueStats = true | ||
| resp, err := s.FrontendClient().DescribeWorkerDeploymentVersion(ctx, req) | ||
| require.NoError(s.T(), err) |
stephanos
approved these changes
Mar 25, 2026
Comment on lines
+1088
to
+1090
| if len(allEvents) == 0 { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Why do we still need this one if the assertion checks it's 1 already?
awln-temporal
pushed a commit
that referenced
this pull request
Mar 26, 2026
## What changed? 1. tests/task_queue_stats_test.go — Remove the `t *testing.T` field from `taskQueueStatsSuite` and replace all `s.t` usages with `s.T()`. Critically, fix `validateDescribeWorkerDeploymentVersion` to use `a.NoError(err)` instead of `require.NoError(s.T(), err)` inside the callback. 2. tests/xdc/history_replication_signals_and_updates_test.go — Fix `pollWorkflowResult` to handle errors from `GetWorkflowExecutionHistory` gracefully: return the error from the inner function, retry with a nil page token on transient errors (e.g. CurrentBranchChanged after conflict resolution), and abort cleanly when the context expires. ## Why? TestTaskQueueStats panic: Calling require.NoError(s.T(), err) inside an EventuallyWithT callback is unsafe. EventuallyWithT runs its callback in a separate goroutine on a ticker. If the outer test context expires and the test completes while this callback is still running, the require.NoError call invokes t.Fail() on a completed test, which panics the entire test binary with panic: Fail in goroutine after TestXxx has completed. The fix uses the CollectT-scoped a.NoError(err) so assertions are buffered and safe. TestConflictResolutionGetResult hang/crash: pollWorkflowResult was calling responseInner.History.Events without checking if responseInner was nil. When GetWorkflowExecutionHistory returns an error (e.g. CurrentBranchChanged after conflict resolution resets the branch token), the response is nil, causing a nil pointer dereference that panics the goroutine. Because the goroutine crashed before sending to workflowResultCh, the main test goroutine blocked on <-workflowResultCh for the entire 30-minute CI timeout. The fix returns the error from the inner function and retries with a nil page token on transient errors, which allows the poll to succeed on the updated branch. ## How did you test it? - [ ] built - [X] run locally and tested manually - [X] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) ## Potential risks The XDC fix changes `pollWorkflowResult` to retry indefinitely on any non-context error. If a non-transient error were to occur repeatedly, this could loop until the context expires rather than failing immediately. This is acceptable since the context timeout provides a hard bound, and the test failure message via `c.t.s.NoError(ctx.Err(), ...)` will make the root cause clear.
chaptersix
pushed a commit
to chaptersix/temporal
that referenced
this pull request
Apr 2, 2026
## What changed? 1. tests/task_queue_stats_test.go — Remove the `t *testing.T` field from `taskQueueStatsSuite` and replace all `s.t` usages with `s.T()`. Critically, fix `validateDescribeWorkerDeploymentVersion` to use `a.NoError(err)` instead of `require.NoError(s.T(), err)` inside the callback. 2. tests/xdc/history_replication_signals_and_updates_test.go — Fix `pollWorkflowResult` to handle errors from `GetWorkflowExecutionHistory` gracefully: return the error from the inner function, retry with a nil page token on transient errors (e.g. CurrentBranchChanged after conflict resolution), and abort cleanly when the context expires. ## Why? TestTaskQueueStats panic: Calling require.NoError(s.T(), err) inside an EventuallyWithT callback is unsafe. EventuallyWithT runs its callback in a separate goroutine on a ticker. If the outer test context expires and the test completes while this callback is still running, the require.NoError call invokes t.Fail() on a completed test, which panics the entire test binary with panic: Fail in goroutine after TestXxx has completed. The fix uses the CollectT-scoped a.NoError(err) so assertions are buffered and safe. TestConflictResolutionGetResult hang/crash: pollWorkflowResult was calling responseInner.History.Events without checking if responseInner was nil. When GetWorkflowExecutionHistory returns an error (e.g. CurrentBranchChanged after conflict resolution resets the branch token), the response is nil, causing a nil pointer dereference that panics the goroutine. Because the goroutine crashed before sending to workflowResultCh, the main test goroutine blocked on <-workflowResultCh for the entire 30-minute CI timeout. The fix returns the error from the inner function and retries with a nil page token on transient errors, which allows the poll to succeed on the updated branch. ## How did you test it? - [ ] built - [X] run locally and tested manually - [X] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) ## Potential risks The XDC fix changes `pollWorkflowResult` to retry indefinitely on any non-context error. If a non-transient error were to occur repeatedly, this could loop until the context expires rather than failing immediately. This is acceptable since the context timeout provides a hard bound, and the test failure message via `c.t.s.NoError(ctx.Err(), ...)` will make the root cause clear.
chaptersix
pushed a commit
to chaptersix/temporal
that referenced
this pull request
Apr 2, 2026
## What changed? 1. tests/task_queue_stats_test.go — Remove the `t *testing.T` field from `taskQueueStatsSuite` and replace all `s.t` usages with `s.T()`. Critically, fix `validateDescribeWorkerDeploymentVersion` to use `a.NoError(err)` instead of `require.NoError(s.T(), err)` inside the callback. 2. tests/xdc/history_replication_signals_and_updates_test.go — Fix `pollWorkflowResult` to handle errors from `GetWorkflowExecutionHistory` gracefully: return the error from the inner function, retry with a nil page token on transient errors (e.g. CurrentBranchChanged after conflict resolution), and abort cleanly when the context expires. ## Why? TestTaskQueueStats panic: Calling require.NoError(s.T(), err) inside an EventuallyWithT callback is unsafe. EventuallyWithT runs its callback in a separate goroutine on a ticker. If the outer test context expires and the test completes while this callback is still running, the require.NoError call invokes t.Fail() on a completed test, which panics the entire test binary with panic: Fail in goroutine after TestXxx has completed. The fix uses the CollectT-scoped a.NoError(err) so assertions are buffered and safe. TestConflictResolutionGetResult hang/crash: pollWorkflowResult was calling responseInner.History.Events without checking if responseInner was nil. When GetWorkflowExecutionHistory returns an error (e.g. CurrentBranchChanged after conflict resolution resets the branch token), the response is nil, causing a nil pointer dereference that panics the goroutine. Because the goroutine crashed before sending to workflowResultCh, the main test goroutine blocked on <-workflowResultCh for the entire 30-minute CI timeout. The fix returns the error from the inner function and retries with a nil page token on transient errors, which allows the poll to succeed on the updated branch. ## How did you test it? - [ ] built - [X] run locally and tested manually - [X] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) ## Potential risks The XDC fix changes `pollWorkflowResult` to retry indefinitely on any non-context error. If a non-transient error were to occur repeatedly, this could loop until the context expires rather than failing immediately. This is acceptable since the context timeout provides a hard bound, and the test failure message via `c.t.s.NoError(ctx.Err(), ...)` will make the root cause clear.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
What changed?
t *testing.Tfield fromtaskQueueStatsSuiteand replace alls.tusages withs.T(). Critically, fixvalidateDescribeWorkerDeploymentVersionto usea.NoError(err)instead ofrequire.NoError(s.T(), err)inside the callback.pollWorkflowResultto handle errors fromGetWorkflowExecutionHistorygracefully: return the error from the inner function, retry with a nil page token on transient errors (e.g. CurrentBranchChanged after conflict resolution), and abort cleanly when the context expires.Why?
TestTaskQueueStats panic: Calling require.NoError(s.T(), err) inside an EventuallyWithT callback is unsafe. EventuallyWithT runs its callback in a separate goroutine on a ticker. If the outer test context expires and the test completes while this callback is still running, the require.NoError call invokes t.Fail() on a completed test, which panics the entire test binary with panic: Fail in goroutine after TestXxx has completed. The fix uses the CollectT-scoped a.NoError(err) so assertions are buffered and safe.
TestConflictResolutionGetResult hang/crash: pollWorkflowResult was calling responseInner.History.Events without checking if responseInner was nil. When GetWorkflowExecutionHistory returns an error (e.g. CurrentBranchChanged after conflict resolution resets the branch token), the response is nil, causing a nil pointer dereference that panics the goroutine. Because the goroutine crashed before sending to workflowResultCh, the main test goroutine blocked on <-workflowResultCh for the entire 30-minute CI timeout. The fix returns the error from the inner function and retries with a nil page token on transient errors, which allows the poll to succeed on the updated branch.
How did you test it?
Potential risks
The XDC fix changes
pollWorkflowResultto retry indefinitely on any non-context error. If a non-transient error were to occur repeatedly, this could loop until the context expires rather than failing immediately. This is acceptable since the context timeout provides a hard bound, and the test failure message viac.t.s.NoError(ctx.Err(), ...)will make the root cause clear.