miandmhoennig
260a84935f
BuildExecutorIntegrationTest stays excluded from PIT (PR#94, PR#97); instead, the decisions of BuildExecutor move into small classes with fast `*UnitTest` specs. - `FinalStatus`, `BuildDeadline`, `CpuSampler`, `PhaseLedger`, `OverheadBudget`, `RestartDrain`, `BuildResultUpdate`, `GiteaStatusText`. - No change of behaviour; the ordering in `execute()` is unchanged (terminal status before leaving `currentBuilds()`, drain flag before the slot is released). - All 43 cases of the integration spec stay. - PIT locally: 58 of 66 mutants of the new classes detected in 50 s, against 18 of 284 BuildExecutor mutants before without the integration spec; the 8 left are equivalent. See docs/prs/2026-10-03-PR#99-build-executor-decisions-in-unit-tests.md. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: mhoennig <michael@hoennig.de> Reviewed-on: #99
77 lines
5.2 KiB
Markdown
77 lines
5.2 KiB
Markdown
> **WARNING:** This document describes only the change applied in this PR.
|
|
> It may already be outdated once the next PR is merged.
|
|
> Historic PR-documentation is not maintained along with new PRs — treat it as a snapshot, not as current documentation.
|
|
|
|
## Related Links
|
|
|
|
- [PR#94](2026-10-03-PR%2394-mutation-build-fixes.md) — took `BuildExecutorIntegrationTest` out of PIT.
|
|
- [PR#97](2026-10-03-PR%2397-build-executor-test-independent-of-load.md) — made that spec independent of the host's load, kept it excluded, and asked for this refactoring.
|
|
|
|
## The Problem
|
|
|
|
`BuildExecutorIntegrationTest` is excluded from PIT since PR#94.
|
|
Under PIT it costs 24-29 s per covering mutant, and a local run before the exclusion counted 284 `BuildExecutor` mutants: 96 killed, 65 timed out, 103 survived, 20 without coverage.
|
|
|
|
With the spec excluded, `BuildExecutor` looks almost uncovered in the nightly Mutation-CRAP report.
|
|
Its decisions sat between threads, processes and the result repository, so no fast test could reach them.
|
|
|
|
## Non-Goals
|
|
|
|
- No change of behaviour, of the configuration, of the API or of the UI.
|
|
- `BuildExecutorIntegrationTest` stays excluded from PIT, and all its cases stay: it remains the end-to-end proof.
|
|
- Threads, processes, log pumps, the slot semaphore and the drain thread stay in `BuildExecutor`.
|
|
|
|
## The Solution
|
|
|
|
The decisions of `BuildExecutor` move into seven small classes in `de.hoennig.werkator.build`, without Spring and without threads.
|
|
Each one has a `*UnitTest` spec that runs in milliseconds.
|
|
|
|
| Class | Decision |
|
|
|---------------------|---------------------------------------------------------------------------------------------------------------|
|
|
| `FinalStatus` | the terminal status after an exit code and after an exception: cancelled, success, timed out, interrupted, failed |
|
|
| `BuildDeadline` | one deadline per build, the remaining wait (zero, never negative), the exit code after a timeout (124) |
|
|
| `CpuSampler` | the largest reading, not the last one; `recorded` keeps null-not-zero |
|
|
| `PhaseLedger` | phases in order of first occurrence; a phase entered twice keeps its first start and adds up |
|
|
| `OverheadBudget` | parsing `executor.overheadBudget`, the warning text with the slowest phase, the phase format |
|
|
| `RestartDrain` | only the first `SUCCESS` of a `restartsInstance` build starts the drain |
|
|
| `BuildResultUpdate` | what a transition writes into the stored result, and the result recreated when it is gone |
|
|
| `GiteaStatusText` | the description of a commit status |
|
|
|
|
`BuildExecutor` only asks these classes.
|
|
The order in its `finally` block is unchanged: terminal status, artifacts, phases and overhead warning, then the drain claim, then `builds.remove`, then `slot.release`.
|
|
So the terminal status is stored before the build leaves `currentBuilds()`, and the drain flag is set before the slot is released.
|
|
|
|
The scenarios of the integration spec that name a decision are now also verified by a unit test:
|
|
Scenario#48.03 (`BuildDeadlineUnitTest`), Scenario#65.02 and Scenario#65.06 (`RestartDrainUnitTest`), Scenario#67.03 and Scenario#67.04 (`CpuSamplerUnitTest`), Scenario#70.01 (`PhaseLedgerUnitTest`), Scenario#70.03 and Scenario#70.04 (`OverheadBudgetUnitTest`).
|
|
|
|
### PIT effect
|
|
|
|
Local runs on a developer machine, with `BuildExecutorIntegrationTest` excluded as in the nightly run.
|
|
|
|
| Mutants | Before | After |
|
|
|--------------------------------------------|-------------------------|---------------------|
|
|
| in `BuildExecutor` | 284, 18 killed | 228, none covered |
|
|
| in the new classes | — | 66, 58 detected |
|
|
| detected without the integration spec | 18 | 58 |
|
|
|
|
The new classes reach 100 % line coverage.
|
|
PIT needs 50 s for them: 12 s coverage analysis, 37 s mutation analysis.
|
|
The 18 "before" kills are the run before PR#94, counted without the kills of `BuildExecutorIntegrationTest`.
|
|
|
|
Two mutants inside the inlined `maxByOrNull` of `OverheadBudget.warningFor` survived at first: of two equally slow phases, the last one would have been named.
|
|
A test now says the first one is named.
|
|
|
|
The 8 that remain undetected are not a gap in the tests:
|
|
|
|
- 5 removed `Intrinsics.checkNotNullExpressionValue` calls, which Kotlin adds and nothing can observe.
|
|
- `>` against `>=` in `CpuSampler.observe`: a reading equal to the highest one changes nothing.
|
|
- The `?: return null` after `maxByOrNull` in `OverheadBudget.warningFor`: no overhead phase means no overhead, which never exceeds the budget.
|
|
- The getter of `CpuSampler.thread`, which only the sampler itself reads.
|
|
|
|
The 228 mutants left in `BuildExecutor` are its threads, processes, log pumps, slots and the drain thread.
|
|
They need real processes, and only `BuildExecutorIntegrationTest` covers them.
|
|
|
|
## Additional Changes
|
|
|
|
- None.
|