Update agent code review guidelines flaky test findings#9669
Merged
Conversation
stephanos
reviewed
Mar 25, 2026
tests/activity_test.go
Outdated
| select { | ||
| case <-cancelCh: | ||
| case <-ctx.Done(): | ||
| s.Fail("Test timed out after 90s waiting for activity cancellation", ctx.Err()) |
Contributor
There was a problem hiding this comment.
nit: let's not hard-code timeouts (90s) as they might change
| case <-listReqDone: | ||
| case <-ctx.Done(): | ||
| s.Fail("timed out waiting for list nexus endpoints request to complete") | ||
| } |
Contributor
There was a problem hiding this comment.
non-blocking musings: I wonder if this could be a helper on TestEnv; like s.WaitForChn and it uses the right context automatically. I didn't think if happens that often, but now seeing multiple examples ...
.github/copilot-instructions.md
Outdated
| - Use `require.ErrorAs(t, err, &specificErr)` for specific error type checks | ||
| - Prefer `require` over `assert` - it's rarely useful to continue a test after a failed assertion | ||
| - Add comments explaining why `Eventually` is needed (e.g., eventual consistency) | ||
| - Do not use single-value type assertions on errors (`err.(*T)`); this panics instead of failing the test when the type doesn't match. Use `errors.As` with a guarded return, or the two-value form `v, ok := err.(*T)` followed by an explicit `require.True(t, ok)`. |
Contributor
There was a problem hiding this comment.
I'd suggest only errors.As and not the longer/less helpful version of checked type casting.
Contributor
There was a problem hiding this comment.
This 1-liner is now possible in latest Go:
s.ErrorAs(err, new(*serviceerror.NotFound))
That might be actually the best advice IMO.
.github/copilot-instructions.md
Outdated
| - Never call testify assertions (`s.NoError`, `s.Equal`, `require.NoError`, even `assert.NoError`) inside a `go func()` — if the goroutine outlives the test, the assertion panics the binary with `panic: Fail in goroutine after TestXxx has completed`. Move assertions to the test goroutine or use a buffered error channel. | ||
| - Any `<-ch` that isn't inside a `select` with `ctx.Done()` will hang indefinitely if the sender never sends. Always provide a context cancellation fallback. | ||
| - Never write to package-level or global variables in tests — parallel tests share the same process; thread values through function parameters instead. | ||
| - Proto message fields accessed outside the workflow lock must be cloned, not aliased: use `common.CloneProto(...)` rather than returning the pointer directly. |
Contributor
There was a problem hiding this comment.
Is this test specific? or does it belong to Concurrency and Safety?
.github/copilot-instructions.md
Outdated
| - Never write to package-level or global variables in tests — parallel tests share the same process; thread values through function parameters instead. | ||
| - Proto message fields accessed outside the workflow lock must be cloned, not aliased: use `common.CloneProto(...)` rather than returning the pointer directly. | ||
| - Never use `time.Sleep` or `time.Since(start) > threshold` to enforce ordering — use channels, `sync.WaitGroup`, or `EventuallyWithT` instead. | ||
| - Do not use `RealTimeSource` with a zero-duration delay in unit tests — it creates an infinite spin loop. Use a mock/event time source with a non-zero delay and advance the clock explicitly. |
Contributor
There was a problem hiding this comment.
I don't quite follow that one; is that referring to NewTimer?
Contributor
Author
There was a problem hiding this comment.
I'm removing this one, it was a one-off that I don't think happens frequently enough for a rule
.github/copilot-instructions.md
Outdated
| - Proto message fields accessed outside the workflow lock must be cloned, not aliased: use `common.CloneProto(...)` rather than returning the pointer directly. | ||
| - Never use `time.Sleep` or `time.Since(start) > threshold` to enforce ordering — use channels, `sync.WaitGroup`, or `EventuallyWithT` instead. | ||
| - Do not use `RealTimeSource` with a zero-duration delay in unit tests — it creates an infinite spin loop. Use a mock/event time source with a non-zero delay and advance the clock explicitly. | ||
| - When combining a short-lived background operation with a propagation wait, verify the timeouts are compatible: if the background op can time out before the condition is satisfied, the propagation wait will never succeed. |
Contributor
There was a problem hiding this comment.
Maybe AI will get it, but I don't :D What's "propagation wait" meaning here?
Contributor
Author
There was a problem hiding this comment.
Meaning an Eventually that is waiting on an async operation to take affect. Rewording to be more clear
stephanos
approved these changes
Mar 30, 2026
.github/copilot-instructions.md
Outdated
| - Prefer `sync.Mutex` over `sync.RWMutex` almost always, except when reads are much more common than writes (>1000×) or readers hold the lock for significant time | ||
| - Don't do IO while holding locks - use side effect tasks | ||
| - Clone data before releasing locks if it might be modified | ||
| - Be suspicious of `go s.someHelper(ctx, ...)` calls where the goroutine runs exactly once and the test then immediately waits for something that helper was supposed to cause. If the operation can fail transiently (network, tight deadline, busy CI), the single attempt may fail silently and the wait will never succeed. Either loop the goroutine until `ctx.Done()`, or check that the operation succeeded before proceeding. |
Contributor
There was a problem hiding this comment.
nit: should this be in ## 3. Testify Suite Correctness and Reliability ?
chaptersix
pushed a commit
to chaptersix/temporal
that referenced
this pull request
Apr 2, 2026
) ## What changed? Add new review guidelines for test reliability and fix instances found in the current codebase. ## Why? I have found these to be the most common patterns across the last two weeks of targeting flaky tests. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) ## Potential risks Minimal
chaptersix
pushed a commit
to chaptersix/temporal
that referenced
this pull request
Apr 2, 2026
) ## What changed? Add new review guidelines for test reliability and fix instances found in the current codebase. ## Why? I have found these to be the most common patterns across the last two weeks of targeting flaky tests. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) ## Potential risks Minimal
chaptersix
pushed a commit
that referenced
this pull request
Apr 2, 2026
## What changed? Add new review guidelines for test reliability and fix instances found in the current codebase. ## Why? I have found these to be the most common patterns across the last two weeks of targeting flaky tests. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) ## Potential risks Minimal
chaptersix
pushed a commit
to chaptersix/temporal
that referenced
this pull request
Apr 2, 2026
) ## What changed? Add new review guidelines for test reliability and fix instances found in the current codebase. ## Why? I have found these to be the most common patterns across the last two weeks of targeting flaky tests. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) ## Potential risks Minimal
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?
Add new review guidelines for test reliability and fix instances found in the current codebase.
Why?
I have found these to be the most common patterns across the last two weeks of targeting flaky tests.
How did you test it?
Potential risks
Minimal