test: migrate failpoint.Inject to failpoint.InjectCall for cleaner test injection
- Dominant language
- Go
- Stars
- 1.2k
- Forks
- 783
- Avg merge
- 4d 15h
- Merged PRs (30d)
- 33
Description
## Enhancement Task
### Background
The codebase currently uses ~165 `failpoint.Inject` call sites. The newer `failpoint.InjectCall` / `failpoint.EnableCall` API offers a cleaner pattern:
- **Production code stays minimal**: `failpoint.InjectCall("name")` is a pure stub with no closure logic polluting production code.
- **Test logic stays in tests**: `failpoint.EnableCall("path", func() { ... })` gives full control to the test side via Go callbacks, instead of encoding behavior in string expressions like `` `return(true)` ``.
- **Type safety**: Callbacks are real Go functions — no runtime `interface{}` casting from `failpoint.Value`.
- **Better test flexibility**: Tests can inject channel synchronization, counters, or conditional logic directly in callbacks, without modifying production code structure.
We already have ~7 `InjectCall` sites that demonstrate this pattern working well (e.g., `blockCheckStores`, `versionChangeConcurrency`, `syncRegionChannelFull`).
### Scope
A codebase-wide audit identified the following migration candidates:
**Batch 1 — High priority (~19 sites, trivial changes):**
Pure `time.Sleep` / nil closure / empty closure failpoints where the Inject body has zero interaction with local variables. These can be directly converted:
| Example | Current | After |
|---------|---------|-------|
| `delayStartServerLoop` | `Inject("name", func() { time.Sleep(2s) })` | `InjectCall("name")` |
| `delayProcess` | `Inject("name", nil)` | `InjectCall("name")` |
| `raftclusterIsBusy` | `Inject("name", func() {})` | `InjectCall("name")` |
Key candidates: `delayStartServerLoop`, `memberNil`, `raftclusterIsBusy`, `customTimeout`, `delayProcess`, `slowRequest`, `concurrentBucketHeartbeat` (×2), `concurrentRegionHeartbeat`, `delaySyncTimestamp`, `concurrentRemoveOperator`, `slowTxn`, `pauseFinishSplitBeforeTxn`, `delayWatcher`, `onGetAllKeyspacesGCStatesFinish`, `delayDispatchTSORequest`, `backOffExecute`, `triggerUpdate` (group_controller).
**Batch 2 — Medium priority (~20 sites, uniform pattern):**
"Speed-up ticker" pattern where the closure only does `ticker.Reset(100ms)`. These can use `InjectCall("name", ticker)` with a typed callback:
```go
// production
failpoint.InjectCall("highFrequencyClusterJobs", ticker)
// test
failpoint.EnableCall("path", func(t *time.Ticker) {
t.Reset(time.Millisecond)
})
```
Key candidates: `highFrequencyClusterJobs` (×4 inject sites, 9 test usages), `changeCoordinatorTicker` (8 usages), `fastUpdateMember` (6), `acceleratedAllocNodes` (6), `speedUpMemberLoop`, `fastUpdatePhysicalInterval`, `fastUpdateServiceMode`, `fastPersist`, `fastCleanupTicker`, `fastTick`, `changeAvailabilityCheckInterval`.
**Not in scope (~110 sites):**
Failpoints using `failpoint.Return` / `Continue` / `Goto` macros, or closures that read/write complex local variables. These should remain as `failpoint.Inject`.
### Benefits
1. **Cleaner production code** — Remove ~40 closure bodies from production files; replace with single-line stubs.
2. **Test-side control** — All injection behavior is defined where it's tested, improving readability and maintainability.
3. **Consistency** — Establish `InjectCall` as the default pattern for new failpoints; reduce two different styles coexisting.
4. **Reduced flaky test risk** — Typed callbacks avoid the string-expression parsing path (`return(true)`, `1*return(true)`, `pause`, etc.).
Contributor guide
Research direction
Start by auditing the listed Batch 1 failpoint sites, then review the Batch 2 ticker pattern and its existing InjectCall examples such as blockCheckStores, versionChangeConcurrency, and syncRegionChannelFull. Migrate only the trivial and ticker candidates, updating their test-side callbacks, while leaving sites using Return, Continue, Goto, or complex local-variable access unchanged. Done means the in-scope sites use the newer API and tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- testing-qa
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100