Skip to content

test: stabilize flaky TestMainBackupLoop#69364

Open
flaky-claw wants to merge 1 commit into
pingcap:masterfrom
flaky-claw:flakyfixer/case_86683a1709be-a7
Open

test: stabilize flaky TestMainBackupLoop#69364
flaky-claw wants to merge 1 commit into
pingcap:masterfrom
flaky-claw:flakyfixer/case_86683a1709be-a7

Conversation

@flaky-claw

@flaky-claw flaky-claw commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #68203

Problem Summary:
Flaky test TestMainBackupLoop in br/pkg/backup intermittently fails, so this PR stabilizes that path.

What changed and how does it work?

Root Cause

Random mock response assignment could leave a live store with no responses, causing the mock sender and RunLoop to wait until timeout.

Fix

Deterministic round-robin assignment keeps both mock stores participating in every subcase and removes the invalid empty-live-store setup.

Verification

Spec:

  • target: br/pkg/backup :: TestMainBackupLoop
  • strategy: tidb.issue_scoped.v2
  • plan mode: BASELINE_ONLY
  • requirements: required case must execute; no skip; repeat count = 1
  • execution surface: GO_TEST_WITH_TAGS
  • build tags: intest, deadlock
  • baseline gates: required_flaky_gate, build_safety_gate, intent_guard_gate
  • feedback surface source: baseline_only

Observed result:

  • status: passed
  • required case executed: yes
  • submission decision: ALLOWED
  • note: Required flaky case executed during validation.
    Required flaky case was not skipped.
    target_flaky passed.
    build passed.

Gate checklist:

  • target_flaky: PASS
  • package_raw: SKIPPED
  • package_failpoint: SKIPPED
  • build: PASS

Commands:

  • go test -json -tags=intest,deadlock ./br/pkg/backup -run '^TestMainBackupLoop$' -count=1
  • make build

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Fixes #68203

Summary by CodeRabbit

  • Tests
    • Improved test stability by using deterministic round-robin mapping for backup responses instead of random selection.

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 749b915d-9f55-43d1-91e2-d32c5053b184

📥 Commits

Reviewing files that changed from the base of the PR and between 6bb13b7 and fe61277.

📒 Files selected for processing (1)
  • br/pkg/backup/client_test.go

📝 Walkthrough

Walkthrough

In TestMainBackupLoop, a new storeIDForSplit(i) helper is introduced that uses modulo indexing (stores[i%len(stores)].GetId()) to deterministically assign store IDs. This helper replaces the previous random store selection across all five test case scenarios when populating mockBackupResponses.

Changes

Fix flaky TestMainBackupLoop with deterministic store selection

Layer / File(s) Summary
storeIDForSplit helper and all test case updates
br/pkg/backup/client_test.go
Adds storeIDForSplit(i) that picks stores[i%len(stores)].GetId(), then applies it in all five split-loop blocks (normal run, canceled, store drops never returns, store drops comes back, store drops while watched) replacing the prior random store selection.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

size/S, release-note-none, approved, lgtm

Suggested reviewers

  • GMHDBJD
  • YuJuncen
  • Leavrth

Poem

🐇 No more random hops for me,
Round-robin stores, deterministically!
Flaky tests shall flap no more,
Modulo maps each range to store.
Steady builds, the warren cheers! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'test: stabilize flaky TestMainBackupLoop' accurately describes the main change—stabilizing a flaky test—and is concise and clear.
Description check ✅ Passed The description includes all required template sections: problem statement with issue link, root cause analysis, fix explanation, verification details, test checklist with unit test selected, and release notes.
Linked Issues check ✅ Passed The PR successfully addresses the objective of stabilizing the flaky TestMainBackupLoop test (#68203) by implementing deterministic round-robin mock response assignment to eliminate timeout issues.
Out of Scope Changes check ✅ Passed All changes are scoped to the test file (br/pkg/backup/client_test.go) and directly address the flaky test stabilization objective with no out-of-scope modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: TestMainBackupLoop in br/pkg/backup

1 participant