pkg/ddl/tests/tiflash: stabilize flaky TestTiFlashBackoff#68115
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/M
|
Hi @flaky-claw. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
📝 WalkthroughWalkthroughThe test Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #68115 +/- ##
================================================
+ Coverage 77.7579% 78.3218% +0.5639%
================================================
Files 1990 2022 +32
Lines 551594 572774 +21180
================================================
+ Hits 428908 448607 +19699
+ Misses 121766 121464 -302
- Partials 920 2703 +1783
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 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.
🧹 Nitpick comments (1)
pkg/ddl/tests/tiflash/ddl_tiflash_test.go (1)
777-804: Guarantee poll shutdown on all exit paths
ddl.DisableTiFlashPollis only called at Line 799. If anyrequire.*before that fails, teardown still closes the mock status server while polling may remain active, which can re-open the teardown race on failure paths. Register cleanup immediately after Line 777 so poll is always disabled.Proposed fix
ddl.EnableTiFlashPoll(s.dom.DDL()) + t.Cleanup(func() { + ddl.DisableTiFlashPoll(s.dom.DDL()) + }) // 1, 1.5, 2.25, 3.375, 5.5625 // (1), 1, 1, 2, 3, 5 time.Sleep(ddl.PollTiFlashInterval * 5) @@ - ddl.DisableTiFlashPoll(s.dom.DDL()) + ddl.DisableTiFlashPoll(s.dom.DDL())As per coding guidelines: “Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/tests/tiflash/ddl_tiflash_test.go` around lines 777 - 804, After calling ddl.EnableTiFlashPoll(s.dom.DDL()), register a test cleanup to always call ddl.DisableTiFlashPoll(s.dom.DDL()) so polling is disabled on all exit paths (use t.Cleanup to register the call immediately after EnableTiFlashPoll). This guarantees shutdown even if earlier require.* assertions fail; you can keep or remove the later explicit ddl.DisableTiFlashPoll call at Line 799 but the cleanup must be added right after EnableTiFlashPoll to ensure deterministic teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ddl/tests/tiflash/ddl_tiflash_test.go`:
- Around line 777-804: After calling ddl.EnableTiFlashPoll(s.dom.DDL()),
register a test cleanup to always call ddl.DisableTiFlashPoll(s.dom.DDL()) so
polling is disabled on all exit paths (use t.Cleanup to register the call
immediately after EnableTiFlashPoll). This guarantees shutdown even if earlier
require.* assertions fail; you can keep or remove the later explicit
ddl.DisableTiFlashPoll call at Line 799 but the cleanup must be added right
after EnableTiFlashPoll to ensure deterministic teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c27cd877-acd9-41da-a898-1b3840a5050e
📒 Files selected for processing (1)
pkg/ddl/tests/tiflash/ddl_tiflash_test.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
|
/check-issue-triage-complete |
|
@wuhuizuo: PRs from untrusted users cannot be marked as trusted with 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. |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, D3Hunter, YangKeao 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 |
|
@yinsustart: PRs from untrusted users cannot be marked as trusted with 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. |
|
/test pull-unit-test-ddlv1 |
|
@yinsustart: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@flaky-claw: PRs from untrusted users cannot be marked as trusted with 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. |
|
/test pull-unit-test-ddlv1 |
|
@flaky-claw: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@yinsustart: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@yinsustart: PRs from untrusted users cannot be marked as trusted with 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. |
|
/test pull-br-integration-test |
|
@yinsustart: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@yinsustart: PRs from untrusted users cannot be marked as trusted with 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. |
5 participants
What problem does this PR solve?
Issue Number: close #68044
Problem Summary:
Flaky test
TestTiFlashBackoffinpkg/ddl/tests/tiflashintermittently fails, so this PR stabilizes that path.What changed and how does it work?
Root Cause
TestTiFlashBackoffwas a test teardown race: it returned after replica availability became true even though the TiFlash poller can still perform later available-table progress refreshes against the mock status server.Fix
The added progress-cache witness is necessary to prove that post-availability polling both happened and then stopped before teardown, which closes the reviewer-reported quiescence gap without changing product code.
Verification
Spec:
pkg/ddl/tests/tiflash :: TestTiFlashBackofftidb.go_flaky.defaultBASELINE_ONLYbaseline_onlyObserved result:
Gate checklist:
Commands:
go test -json ./pkg/ddl/tests/tiflash -run '^TestTiFlashBackoff$' -count=1go test -json ./pkg/ddl/tests/tiflash -count=1make buildCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Fixes #68044
Summary by CodeRabbit
TestTiFlashBackofftest reliability by directly controlling TiFlash state and removing fragile mocking mechanisms. Enhanced validation to ensure TiFlash replica progress is properly refreshed when availability changes occur and polling is active.