Makes the load-dependent cases of `BuildExecutorIntegrationTest` robust (follow-up of PR#94). Test-only change, v1.30.4. Under load (8 cores, 12 busy loops, parallel Gradle builds) the unchanged spec failed in 4 of 4 runs: Scenario#65.04 (drain flag read too early) three times, Scenario#67.03 and #73.01 (CPU-bound build exceeded the 30 s wait) once each. With this change: 16 of 16 runs green, 640 test cases, idle runtime unchanged at 37 s. Changes: gates instead of fixed sleeps (#65.05, live log), waiting for the drain (#65.04), `sleep 300` for builds the test cancels anyway, longer wait for CPU-bound builds, an already-ended command as the fixture for "too short to measure" (#67.04), a wider margin in #70.02, and #70.03/#70.04 read only their own branch's warning. No assertion became weaker. The spec stays excluded from PIT; re-adding it is an open question in the PR-doc. PR-doc: docs/prs/2026-10-03-PR#97-build-executor-test-independent-of-load.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: mhoennig <michael@hoennig.de> Reviewed-on: #97
74 lines
5.2 KiB
Markdown
74 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 after it failed PIT's coverage run once, and asked for this PR.
|
|
- [PR#71](2026-09-15-PR%2371-a-fraction-of-a-core.md) — an earlier load-dependent assertion of the same spec.
|
|
|
|
## The Problem
|
|
|
|
`BuildExecutorIntegrationTest` failed once in PIT's coverage run on mih09, without a mutant, and two probe runs could not reproduce it.
|
|
PR#94 guessed Scenario#67.04: `true` must consume no measurable processor time, and one clock tick of a shell's start breaks that.
|
|
|
|
Running the spec under load reproduced failures, but mostly elsewhere.
|
|
The unchanged spec ran repeatedly in a Gradle build on 8 cores, beside 12 busy loops and two more Gradle builds of copies (load average about 32).
|
|
Of 4 complete runs, 4 failed, in three different cases:
|
|
|
|
- Scenario#65.04, three times: `isDraining()` was read the moment the deployment's SUCCESS was stored.
|
|
The executor sets the drain flag a moment later, after the artifacts and before the slot is released.
|
|
- Scenario#67.03 and Scenario#73.01, once each: three seconds of processor time in `awk` took longer than the 30 s wait on a host whose cores were taken.
|
|
|
|
Other cases depend on the host's load by construction, even though this run did not catch them:
|
|
|
|
- Scenario#67.04: whether the shell's start costs less than one clock tick (10 ms) is the host's decision, and a loaded or virtualized host can charge more.
|
|
- Scenario#65.05: the deployment ran `sleep 2`, and a test thread kept waiting for two seconds never saw it RUNNING.
|
|
- "the live log grows": the test had three seconds between reading the first line and checking RUNNING.
|
|
- "the repository reports RUNNING", "with maxConcurrent 2" and "cancel only affects the addressed build": builds of `sleep 10` that had to be still running after a sequence of waits of up to 10 s and 30 s each.
|
|
- Scenario#70.02: the workspace phase had to stay below 900 ms.
|
|
- Scenario#70.03 and Scenario#70.04 read every warning of the `BuildExecutor` logger, which the whole JVM shares; under PIT, builds of earlier specs still finishing can write to it.
|
|
|
|
## Non-Goals
|
|
|
|
- No production code changes: every failure was a test reading too early or relying on a duration.
|
|
- No retry mechanism for flaky tests.
|
|
- `BuildExecutorIntegrationTest` stays excluded from PIT; whether to re-add it is decided separately (see Open Questions).
|
|
|
|
## The Solution
|
|
|
|
Each case now waits for the state it asserts, or holds the build until the test lets go, instead of relying on a duration.
|
|
None of the assertions became weaker; two became stronger.
|
|
|
|
- Scenario#65.04 waits for the drain to begin, as PR#98 meanwhile also made it do, and asserts in addition that the other build is still RUNNING at that point.
|
|
- Scenario#65.05 holds the deployment at a gate file the test opens once the second build is queued, like Scenario#65.04 already did.
|
|
- "the live log grows" holds the build between its two lines at a gate file, and asserts in addition that the second line is not there yet.
|
|
- Builds that the test cancels anyway run `sleep 300` instead of `sleep 10`, so they cannot end on their own while the test is still waiting.
|
|
- Scenario#67.03 and Scenario#73.01 wait up to 120 s for the CPU-bound build; Scenario#73.01 burns a tenth as much, since it asserts only the absence of a figure.
|
|
- Scenario#67.04 uses a runner whose command has already ended when the executor gets its process.
|
|
That is what "too short to measure" means: the sampler finds nothing to read.
|
|
The sampler, the result and the null-not-zero decision are the production ones; only the moment the command ends is the fixture's.
|
|
- Scenario#70.02 runs `sleep 2` and bounds the workspace phase by the command's own time, 1.9 s.
|
|
That bound still tells a workspace phase that included the command from one that did not.
|
|
- Scenario#70.03 and Scenario#70.04 read only the warning about their own branch.
|
|
|
|
The spec takes 37 s on an idle host, as before.
|
|
|
|
### Verification under load
|
|
|
|
Two loops ran side by side, 8 runs each, beside 12 busy loops; the host also swapped, because other builds ran on it.
|
|
All 16 runs passed, 640 test cases without a failure, at 63-107 s per run against 37 s idle.
|
|
Part of the runs had less load than the baseline; the runs at 96-107 s match the baseline's 105-115 s.
|
|
|
|
## Additional Changes
|
|
|
|
- None.
|
|
|
|
## Follow-up PRs
|
|
|
|
- `BuildExecutorIntegrationTest` stays excluded from PIT (decided with this PR).
|
|
Re-adding it would cost about an hour per full run: PR#94 measured 24-29 s per covering mutant, and `BuildExecutor` has about 260 covered mutants, 65 of which timed out in a local run.
|
|
A remaining load-dependent case would also abort the whole night again.
|
|
- Instead, `BuildExecutor` itself is refactored: its decisions (final status, processor time null or not, overhead warning, drain and restart, timeouts) move into small classes of their own, with unit tests that PIT runs in milliseconds.
|
|
The integration spec then keeps only what needs real processes and threads.
|