test: stabilize flaky TestHotTestSuite/TestBuckets#10937
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Hi @flaky-claw. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
ti-chi-bot
Bot
added
the
size/XS
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesHot peer cache test
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/pd-ctl/tests/hot/hot_test.go`:
- Line 558: The drained write stats check in hot_test.go is vacuous because
hotStat.GetHotPeerStats(utils.Write, 0) returns an allocated map even when
empty, so reusing re.NotNil does not verify population. Update the assertion
near the existing hotStat.GetHotPeerStats(utils.Write, 0) call in the hot test
to check for a non-empty result instead, using a size/length-based assertion or
equivalent on the returned map so the test actually confirms drained write peers
were recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e871b664-3b07-479c-bd56-d6d5a427c97a
📒 Files selected for processing (1)
tools/pd-ctl/tests/hot/hot_test.go
| // Drain async hot peer updates before TearDownTest resets the hot cache directly. | ||
| re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0)) | ||
| re.NotNil(hotStat.GetHotPeerStats(utils.Read, 0)) | ||
| re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a non-vacuous assertion for drained write stats
Line 558 duplicates Line 556, and NotNil is ineffective here because GetHotPeerStats returns an allocated map even when no hot peers exist. This does not validate the drain/population condition.
Suggested fix
- re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0))
- re.NotNil(hotStat.GetHotPeerStats(utils.Read, 0))
- re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0))
+ writeStats := hotStat.GetHotPeerStats(utils.Write, 0)
+ re.NotEmpty(writeStats)
+ found := false
+ for _, peers := range writeStats {
+ if len(peers) > 0 {
+ found = true
+ break
+ }
+ }
+ re.True(found)
+ re.NotNil(hotStat.GetHotPeerStats(utils.Read, 0))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0)) | |
| writeStats := hotStat.GetHotPeerStats(utils.Write, 0) | |
| re.NotEmpty(writeStats) | |
| found := false | |
| for _, peers := range writeStats { | |
| if len(peers) > 0 { | |
| found = true | |
| break | |
| } | |
| } | |
| re.True(found) | |
| re.NotNil(hotStat.GetHotPeerStats(utils.Read, 0)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/pd-ctl/tests/hot/hot_test.go` at line 558, The drained write stats
check in hot_test.go is vacuous because hotStat.GetHotPeerStats(utils.Write, 0)
returns an allocated map even when empty, so reusing re.NotNil does not verify
population. Update the assertion near the existing
hotStat.GetHotPeerStats(utils.Write, 0) call in the hot test to check for a
non-empty result instead, using a size/length-based assertion or equivalent on
the returned map so the test actually confirms drained write peers were
recorded.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024 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 |
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
flakyfixer/case_cf396bbf1769-a6
branch
from
94277ce to
14674d3
Compare
ti-chi-bot
Bot
added
dco-signoff: yes
|
/retest |
|
@flaky-claw: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
3 participants
What problem does this PR solve?
Issue Number: close #10651
Problem Summary:
Flaky test
TestHotTestSuite/TestBucketsingithub.com/tikv/pd/tools/pd-ctl/tests/hotintermittently fails, so this PR stabilizes that path.What changed and how does it work?
Root Cause
TestBuckets teardown could call CleanCache while a late async write hot-cache task still touched HotPeerCache maps.
Fix
The final write drain serializes teardown behind the last write-side hot-cache task without changing CLI assertions or product behavior.
Verification
Spec:
github.com/tikv/pd/tools/pd-ctl/tests/hot :: TestHotTestSuite/TestBucketspd.issue_scoped.v1BASELINE_ONLYbaseline_onlyObserved result:
Required flaky case was not skipped.
nested_target_json passed.
race_target passed.
Gate checklist:
Commands:
cd tools && go test -json -tags without_dashboard ./pd-ctl/tests/hot -run '^TestHotTestSuite/TestBuckets$' -count=1cd tools && go test -race -tags without_dashboard ./pd-ctl/tests/hot -run '^TestHotTestSuite/TestBuckets$' -count=5Release note
Fixes #10651
Summary by CodeRabbit