Skip to content

test: stabilize flaky TestHotTestSuite/TestBuckets #10937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
flaky-claw wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −0
Open

test: stabilize flaky TestHotTestSuite/TestBuckets #10937

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tools/pd-ctl/tests/hot/hot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,4 +555,5 @@ func (suite *hotTestSuite) checkBuckets(cluster *pdTests.TestCluster) {
// 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))

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

}
Loading