tikv / tikv/pd

test: migrate failpoint.Inject to failpoint.InjectCall for cleaner test injection

Open
#10,297 0 comments 0 reactions 0 assignees View on GitHub
type/enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.