stabilize interrupted stream writer test#3669
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe test Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented changes to stabilize the Test_Ctx_SendStreamWriter_Interrupted test. The core issue was flakiness due to timing inconsistencies in how the stream writer behaved before interruption. My changes fine-tune the timing within the test by adjusting how long the stream sleeps between writing lines and by tightening the overall test timeout. This ensures the test consistently simulates the intended interruption scenario, making it more reliable.
Highlights
- Conditional Stream Writer Sleep: I've adjusted the sleep duration within the
Test_Ctx_SendStreamWriter_Interruptedfunction. Instead of a fixed 500ms sleep for every line, the first two lines now sleep for only 50ms, allowing them to flush more quickly. Subsequent lines still sleep for 500ms. - Reduced Test Timeout: I've reduced the overall
Timeoutfor theTest_Ctx_SendStreamWriter_Interruptedtest from 1400ms to 400ms. This change, combined with the conditional sleep, ensures the test reliably hits the intended interruption point, making it more stable.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR stabilizes a flaky test for the SendStreamWriter interrupt functionality by adjusting timing parameters to ensure consistent test behavior. The changes modify the stream writing delays and reduce the overall test timeout to create more predictable test conditions.
- Adjusted sleep durations in the stream writer to have shorter delays for the first 3 lines
- Reduced the test timeout from 1400ms to 400ms to better align with the new timing
There was a problem hiding this comment.
Code Review
This pull request aims to stabilize a flaky test, Test_Ctx_SendStreamWriter_Interrupted, by adjusting the timing logic. The changes modify the sleep durations within the stream writer and reduce the overall test timeout. My review focuses on improving the maintainability of this timing-sensitive test by suggesting the use of named constants instead of magic numbers. This will make the test logic clearer and easier to adjust in the future.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ctx_test.go (2)
5384-5388: Good timing strategy; consider naming delays and guarding the invariant.The fast-then-barrier sleeps make the 3-flush threshold deterministic. To make the intent self-documenting and prevent future regressions, extract these durations into named constants and assert the invariant (test timeout < post-third-line barrier). This also centralizes tuning if CI ever needs adjustment.
Apply within this block:
- if lineNum < 3 { - time.Sleep(50 * time.Millisecond) - } else { - time.Sleep(500 * time.Millisecond) - } + if lineNum < 3 { + time.Sleep(fastFlushPause) + } else { + time.Sleep(postThirdBarrier) + }Add near the top of the test (outside this range):
const ( fastFlushPause = 50 * time.Millisecond postThirdBarrier = 500 * time.Millisecond testTimeout = 400 * time.Millisecond ) require.Less(t, testTimeout, postThirdBarrier, "testTimeout must be less than postThirdBarrier")
5398-5398: Timeout aligns with the barrier; minor robustness and readability tweaks.400ms ensures three flushes occur before attempting the 4th. To encode intent and reduce magic numbers, reuse a named timeout const; also prefer type-safe error check for EOF.
Within this line:
- Timeout: 400 * time.Millisecond, + Timeout: testTimeout,Outside this range, prefer:
// Replace string compare with error identity check require.ErrorIs(t, err, io.ErrUnexpectedEOF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ctx_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
ctx_test.go
📚 Learning: 2024-09-25T17:05:06.991Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
ctx_test.go
⏰ 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). (5)
- GitHub Check: repeated
- GitHub Check: Analyse
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, macos-13)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3669 +/- ##
=======================================
Coverage 91.84% 91.84%
=======================================
Files 113 113
Lines 11447 11447
=======================================
Hits 10514 10514
Misses 669 669
Partials 264 264
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ctx_test.go (1)
5383-5385: Extract timing magic numbers into named constantsMakes intent explicit and keeps timings easy to tweak in CI. This echoes a prior suggestion.
Within the test function, add:
const ( flushPause = 500 * time.Millisecond testTimeout = 200 * time.Millisecond )Then update usages:
- if lineNum == 3 { - time.Sleep(500 * time.Millisecond) - } + if lineNum == 3 { + time.Sleep(flushPause) + }- Timeout: 200 * time.Millisecond, + Timeout: testTimeout,Also applies to: 5395-5395
🧹 Nitpick comments (1)
ctx_test.go (1)
5411-5412: Prefer require.Truef (remove unnecessary fmt.Sprintf)Simplifies the assertion and satisfies testifylint “formatter” hint.
- v := flushErrLine.Load() - require.True(t, v == 0 || v >= 4, fmt.Sprintf("unexpected flush error on line %d", v)) + v := flushErrLine.Load() + require.Truef(t, v == 0 || v >= 4, "unexpected flush error on line %d", v)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ctx_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
ctx_test.go
📚 Learning: 2024-09-25T17:09:03.756Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Applied to files:
ctx_test.go
📚 Learning: 2024-09-25T17:05:06.991Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
ctx_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
ctx_test.go
📚 Learning: 2024-06-30T00:38:06.580Z
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-16T12:12:30.506Z
Learnt from: gaby
PR: gofiber/fiber#3170
File: ctx_test.go:1721-1724
Timestamp: 2024-10-16T12:12:30.506Z
Learning: In the Go unit tests in `ctx_test.go`, it is acceptable to use invalid CIDR notation such as `"0.0.0.1/31junk"` for testing purposes.
Applied to files:
ctx_test.go
🪛 GitHub Check: lint
ctx_test.go
[failure] 5375-5375:
G115: integer overflow conversion int -> int32 (gosec)
[failure] 5412-5412:
formatter: remove unnecessary fmt.Sprintf (testifylint)
🪛 GitHub Actions: golangci-lint
ctx_test.go
[error] 5375-5375: golangci-lint (gosec): integer overflow conversion int -> int32 (G115)
⏰ 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). (6)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: Analyse
| t.Parallel() | ||
| app := New() | ||
| var flushed atomic.Int32 | ||
| var flushErrLine atomic.Int32 |
There was a problem hiding this comment.
Fix gosec G115: avoid narrowing int→int32 cast by using atomic.Int64
The int→int32 cast on Line 5375 triggers gosec (G115). Use atomic.Int64 and store int64(lineNum) to prevent the narrowing conversion and unblock CI.
- var flushErrLine atomic.Int32
+ var flushErrLine atomic.Int64- flushErrLine.Store(int32(lineNum))
+ flushErrLine.Store(int64(lineNum))Also applies to: 5375-5375
🤖 Prompt for AI Agents
In ctx_test.go at line 5368, replace the declaration of flushErrLine from
atomic.Int32 to atomic.Int64. Then, at line 5375, update the store operation to
use int64(lineNum) instead of casting to int32. This change avoids the narrowing
int to int32 conversion flagged by gosec G115 and resolves the security warning.
Summary
Testing
go test -run Test_Ctx_SendStreamWriter_Interrupted -count=50make testhttps://chatgpt.com/codex/tasks/task_e_6895b762ad64832688709704748ab6b8