Fix flaky timing tests #3487
Fix flaky timing tests #3487
Conversation
WalkthroughSleep durations in test files for cache, idempotency, limiter, and timeout middleware were increased by small increments. These adjustments ensure more reliable timing for cache expiration, idempotency keys, rate limiting, and timeout behaviors during tests. No changes were made to logic, control flow, or public interfaces. Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses flaky timing tests by increasing sleep durations and adding extra buffers in various middleware tests.
- Increase sleep duration in timeout tests to account for potential delays.
- Add extra buffer to limiter sleeps to prevent false negatives.
- Relax timing in idempotency and cache tests to stabilize intermittent failures.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| middleware/timeout/timeout_test.go | Increased sleep from 50ms to 70ms to reflect zero-timeout behavior adjustments. |
| middleware/limiter/limiter_test.go | Adjusted multiple sleep durations by adding extra buffers for rate limiter tests. |
| middleware/idempotency/idempotency_test.go | Increased sleep durations to ensure proper handling of delayed idempotency operations. |
| middleware/cache/cache_test.go | Updated sleep durations in cache tests to better cover expiration timing. |
| @@ -119,7 +119,7 @@ func TestTimeout_ZeroDuration(t *testing.T) { | |||
|
|
|||
| app.Get("/zero", New(func(c fiber.Ctx) error { | |||
| // Sleep 50ms, but there's no real 'deadline' since zero-timeout. | |||
| time.Sleep(50 * time.Millisecond) | |||
| time.Sleep(70 * time.Millisecond) | |||
There was a problem hiding this comment.
Consider extracting sleep duration values into named constants to improve test maintainability and readability.
| time.Sleep(70 * time.Millisecond) | |
| time.Sleep(sleepDurationZero) |
Copilot uses AI. Check for mistakes.
| @@ -46,7 +46,7 @@ func Test_Limiter_With_Max_Func_With_Zero_And_Limiter_Sliding(t *testing.T) { | |||
| require.NoError(t, err) | |||
| require.Equal(t, 200, resp.StatusCode) | |||
|
|
|||
| time.Sleep(4*time.Second + 500*time.Millisecond) | |||
| time.Sleep(4*time.Second + 700*time.Millisecond) | |||
There was a problem hiding this comment.
If similar sleep buffers are used throughout this test file, extracting them into constants could enhance clarity and ease future adjustments.
Copilot uses AI. Check for mistakes.
| @@ -72,7 +72,7 @@ func Test_Idempotency(t *testing.T) { | |||
| }) | |||
|
|
|||
| app.Post("/slow", func(c fiber.Ctx) error { | |||
| time.Sleep(3 * lifetime) | |||
| time.Sleep(3*lifetime + 100*time.Millisecond) | |||
There was a problem hiding this comment.
Extracting the modified sleep durations into constants can improve consistency and simplify future modifications in timing tests.
| time.Sleep(3*lifetime + 100*time.Millisecond) | |
| time.Sleep(slowRequestDuration) |
Copilot uses AI. Check for mistakes.
| @@ -59,7 +59,7 @@ func Test_Cache_Expired(t *testing.T) { | |||
| require.NoError(t, err) | |||
|
|
|||
| // Sleep until the cache is expired | |||
| time.Sleep(3 * time.Second) | |||
| time.Sleep(3*time.Second + 200*time.Millisecond) | |||
There was a problem hiding this comment.
Consider defining a named constant for the sleep duration to make it clear why this specific delay is required and to ease potential future changes.
Copilot uses AI. Check for mistakes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3487 +/- ##
==========================================
+ Coverage 83.76% 83.83% +0.06%
==========================================
Files 120 120
Lines 12254 12254
==========================================
+ Hits 10265 10273 +8
+ Misses 1564 1558 -6
+ Partials 425 423 -2
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/timeout/timeout_test.go (1)
122-122: Good improvement for test reliability.The increased sleep duration (50ms → 70ms) provides better buffer time to ensure the zero timeout test is more reliable while maintaining test validity.
Consider updating the comment on line 121 to reflect the new sleep duration:
- // Sleep 50ms, but there's no real 'deadline' since zero-timeout. + // Sleep 70ms, but there's no real 'deadline' since zero-timeout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
middleware/cache/cache_test.go(3 hunks)middleware/idempotency/idempotency_test.go(3 hunks)middleware/limiter/limiter_test.go(17 hunks)middleware/timeout/timeout_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: repeated
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (9)
middleware/idempotency/idempotency_test.go (3)
75-75: Timing improvement enhances test reliability.Adding 100ms buffer to the slow route sleep duration ensures concurrent idempotency tests are more reliable while maintaining the test's validity.
111-111: Cache expiration timing improvement.The additional 100ms buffer ensures the idempotency cache has reliably expired before testing with the same key, reducing flakiness in timing-dependent assertions.
128-128: Consistent timing buffer for cache expiration.Adding 100ms buffer aligns with other timing adjustments in this test and ensures the slow route cache has expired before subsequent validation.
middleware/limiter/limiter_test.go (3)
49-49: Consistent timing improvements for Fixed Window tests.Adding 200ms buffer to the 3-second sleep periods ensures rate limiter windows have reliably expired before testing reset behavior. This addresses flaky test issues while maintaining test validity for Fixed Window rate limiting.
Also applies to: 136-136, 180-180, 223-223, 262-262, 302-302, 419-419, 458-458, 573-573, 612-612
341-341: Extended timing buffers for Sliding Window tests.The increased buffer time (500ms → 700ms) for Sliding Window tests is appropriate as sliding windows may require longer periods to fully expire compared to fixed windows. This ensures more reliable test behavior.
Also applies to: 381-381, 496-496, 535-535, 650-650, 689-689
818-818: Additional timing improvements in sliding window test.The 200ms buffer additions ensure multiple sequential rate limiting cycles in the sliding window test are properly separated, improving test reliability.
Also applies to: 824-824, 830-830
middleware/cache/cache_test.go (3)
62-62: Cache expiration timing improvement.Adding 200ms buffer to the 3-second sleep ensures the 2-second cache expiration has reliably occurred, reducing flakiness in cache expiration tests.
431-431: Buffer time for negative expiration test.Increasing the sleep from 500ms to 600ms provides better reliability for testing cache behavior with negative expiration times (immediate expiry).
533-533: Custom expiration timing buffer.The adjustment from 1.1s to 1.2s ensures the 1-second custom cache expiration has definitely occurred before testing cache miss behavior.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: ed5044d | Previous: a590cdb | Ratio |
|---|---|---|---|
Benchmark_GenericParseTypeBytes/benchmark_genericParseTypeBytes |
30.87 ns/op 8 B/op 1 allocs/op |
20.05 ns/op 8 B/op 1 allocs/op |
1.54 |
Benchmark_GenericParseTypeBytes/benchmark_genericParseTypeBytes - ns/op |
30.87 ns/op |
20.05 ns/op |
1.54 |
Benchmark_GenericParseTypeBytes/benchmark_genericParseTypeBytes#03 |
25.94 ns/op 0 B/op 0 allocs/op |
13.15 ns/op 0 B/op 0 allocs/op |
1.97 |
Benchmark_GenericParseTypeBytes/benchmark_genericParseTypeBytes#03 - ns/op |
25.94 ns/op |
13.15 ns/op |
1.97 |
This comment was automatically generated by workflow using github-action-benchmark.
Summary
Testing
go test ./...