tests: stabilize TestGetAllKeyspaceGCStates TTL bounds#10275
tests: stabilize TestGetAllKeyspaceGCStates TTL bounds#10275okJiang wants to merge 2 commits intotikv:masterfrom
Conversation
Signed-off-by: okjiang <819421878@qq.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
release-note-none
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTest assertions in an integration test were relaxed: two TTL comparisons changed from strict greater-than to greater-or-equal, with comments noting TTL values are rounded to seconds. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
ti-chi-bot
bot
added
the
size/XS
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10275 +/- ##
==========================================
- Coverage 78.80% 78.78% -0.03%
==========================================
Files 523 525 +2
Lines 70529 70777 +248
==========================================
+ Hits 55580 55761 +181
- Misses 10955 11014 +59
- Partials 3994 4002 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integrations/client/client_test.go`:
- Around line 2710-2712: The assertion arguments to re.LessOrEqual are reversed:
currently it checks 2*time.Second <= res.GlobalGCBarriers[1].TTL but should
assert the TTL is at most 2s; change the call to
re.LessOrEqual(res.GlobalGCBarriers[1].TTL, 2*time.Second). Keep the preceding
re.GreaterOrEqual(res.GlobalGCBarriers[1].TTL, time.Second) unchanged so the TTL
remains asserted in the [1s,2s] range.
- Around line 2734-2736: The assertion arguments are reversed:
re.LessOrEqual(3*time.Second, state.GCBarriers[0].TTL) currently asserts 3s <=
TTL but the intent is TTL <= 3s; change the call to
re.LessOrEqual(state.GCBarriers[0].TTL, 3*time.Second) so
state.GCBarriers[0].TTL is asserted to be no greater than 3 seconds (paired with
the existing re.GreaterOrEqual(state.GCBarriers[0].TTL, 2*time.Second)).
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: okjiang <819421878@qq.com>
ti-chi-bot
bot
added
size/S
|
/retest |
|
/ok-to-retest |
|
/retest |
|
@MyonKeminta: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JmPotato, MyonKeminta The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What problem does this PR solve?
Issue Number: ref #10244
TestGetAllKeyspaceGCStatesis flaky in CI ("2s" is not greater than "2s").Root-cause evidence chain
tests/integrations/client/client_test.goand expects strict lower bounds (TTL > 2s/TTL > 1s).server/gc_service.go:math.Floor(b.ExpirationTime.Sub(now).Seconds()).ttl=3scan legitimately be returned as exactly2safter request/processing latency; similarly2scan become exactly1s.Greater(...)assertions inconsistent with implementation semantics and causes flaky boundary failures.Historical analog
flaky_stabilization+test_harness_alignment).tests: fix some unstable tests), which also touchedtests/integrations/client/client_test.goand includesTestGetAllKeyspaceGCStates.What is changed and how does it work?
TestGetAllKeyspaceGCStates, replace strict lower-bound assertions with inclusive bounds:res.GlobalGCBarriers[1].TTL > 1s->>= 1sstate.GCBarriers[0].TTL > 2s->>= 2sThis is a minimal test-only fix and preserves the original upper bounds (
<=2s,<=3s).Risk and impact
Verification commands and results
cd tests/integrations && make gotest GOTEST_ARGS='-tags without_dashboard ./client -run TestClientStatefulTestSuite/TestGetAllKeyspaceGCStates -count=30'FAIL) during stress run, consistent with flaky issue context.cd tests/integrations && make gotest GOTEST_ARGS='-tags without_dashboard ./client -run TestClientStatefulTestSuite/TestGetAllKeyspaceGCStates -count=1'listen tcp 127.0.0.1:0: bind: operation not permitted.cd tests/integrations && make gotest GOTEST_ARGS='-tags without_dashboard ./client -run TestDecodeHttpKeyRange -count=1'operation not permitted).Check List
Tests
Release note
Summary by CodeRabbit