diff --git a/docs/decisions.md b/docs/decisions.md index 9364af441..9f6f970c6 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -202,7 +202,7 @@ another doc or an old issue comment should land here and then follow the link. - 2026-07-21 — `WorkAheadSlots.Release()` clamps before decrementing and reports unbalance in-band (#539) — [`ffmpeg.work-ahead-slot-release-never-negative`](decisions/records/ffmpeg/work-ahead-slot-release-never-negative.md) - 2026-07-21 — `from-lineup` advanced overrides express "clear to none" via a typed `clear` enum list (#135) — [`api.from-lineup-clear-to-none`](decisions/records/api/from-lineup-clear-to-none.md) - 2026-07-22 — Channel-level graphics-element attachment + seeded On Now/Next text element (#74) — [`graphics.channel-level-attachment`](decisions/records/graphics/channel-level-attachment.md) -- 2026-07-22 — Sequential (YAML) playout gets a golden; Scripted is excluded from the golden net by construction (#381) — [`testing.scripted-playout-golden-deferred`](decisions/records/testing/scripted-playout-golden-deferred.md) +- 2026-07-22 — Sequential (YAML) playout gets a golden; Scripted is excluded from the golden net by construction (#381) — [`testing.scripted-playout-golden-deferred`](decisions/archive/testing/scripted-playout-golden-deferred.md) (superseded by `testing.scripted-engine-in-process-net`) - 2026-07-22 — per-schedule clock-boundary padding is a synthetic content-less Pad over the existing per-episode machinery (#392) — [`sched.clock-padding-schedule-toggle`](decisions/records/sched/clock-padding-schedule-toggle.md) - 2026-07-23 — Channel health = a server-derived `health` object on the channel DTOs, built-timeline detection (#415) — [`api.channel-health-object`](decisions/records/api/channel-health-object.md) - 2026-07-23 — Channel origin is immutable creation-provenance, stamped at insert, not a health signal (#414) — [`channel.origin-marker`](decisions/records/channel/origin-marker.md) @@ -216,3 +216,4 @@ another doc or an old issue comment should land here and then follow the link. - 2026-07-25 — The rationale-edit marker is a git trailer, not a substring anywhere in the commit range (#609) — [`ci.decisions-edit-trailer`](decisions/records/ci/decisions-edit-trailer.md) - 2026-07-25 — UI-E2E: headless Playwright flows in the existing `functional-e2e` job, browser baked into the CI image (#445) — [`ci.ui-e2e-harness`](decisions/records/ci/ui-e2e-harness.md) - 2026-07-26 — Facet-value typeahead restated: every artist source covered; the JSON-column source is paged by row position with no residual SQL predicate (#578) — [`api.search-field-values-sources`](decisions/records/api/search-field-values-sources.md) +- 2026-09-05 — Scripted playout is covered in-process, engine plus controller adapter; the Cli.Wrap process and the HTTP transport are permanently out of the automated suite (#563) — [`testing.scripted-engine-in-process-net`](decisions/records/testing/scripted-engine-in-process-net.md) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index c165a701c..7fd053dd4 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -224,7 +224,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `testing.live-e2e-prepush-timing` | Run live-E2E via `scripts/e2e-local.sh` before pushing a write-path or UI change, and exercise download endpoints with curl, never a browser tab. | 2026-07-21 | [link](records/testing/live-e2e-prepush-timing.md) | | `testing.mutation-claims-are-executed` | A `MUTATION` row in `docs/guard-inventory.md` is not a statement that someone once witnessed a red. It carries a DECLARED clause mutation in `scripts/tests/mutation_manifest.py`, and `scripts/tests/test_mutation_harness.py` applies that mutation to an isolated copy of the repository on every run and requires the row's OWN named test to go red. The manifest and the MUTATION rows are compared for SET EQUALITY in both directions, so a row cannot claim the grade without a mutation and a mutation cannot outlive the grade it justifies. EXIT STATUS IS NOT THE VERDICT: each entry also declares the DIAGNOSTIC its red must carry, matched against pytest's exception output alone, because pytest reports a crashing test exactly as it reports a detecting one and a red for an unrelated reason is evidence about nothing. WHERE THE GUARD IS ITSELF A TEST, `target` may differ from `guard` and the exact-once check applies to the declared TARGET. Two shapes are admissible and the choice is not free. Where the guard's assertion IS the check — a completeness comparison against a Markdown inventory — the mutation goes into the guarded ARTIFACT, per `testing.guard-ships-with-mutation-proof`'s checker-guard exception, because mutating such a checker's own POPULATION demonstrates a false POSITIVE while proving nothing about the detection the row claims. Where the guard is a test module wrapping a separately mutable DETECTOR or helper, the clause may be in that detector, since disarming it is a real clause disarm and the module's own assertion is what notices. THE MUTATION IS DECLARED, NEVER INFERRED: a harness that guessed which clause of a 90-line hook is the guard would manufacture the confident-but-empty coverage this exists to prevent, which is why `testing.guard-ships-with-mutation-proof` rejected a generic runner. Where a proof test already names its clause in source, the manifest reuses THAT string, so a retarget in either place is caught by the other. COARSENESS IS RECORDED, NOT HIDDEN: each entry is graded `CLAUSE` or `DETECTOR`, and a `DETECTOR` entry — one whose detector accumulates faults from independent arms, so disarming any single arm leaves its proof test green — must CARRY the finer mutation that survived, which is re-run every time and required to keep surviving. Guards that are not graded `MUTATION` each carry a STATED reason in that same manifest, keyed on the guard and compared for SET EQUALITY against the inventory's `GUARD` rows in both directions — so a new guard cannot arrive without someone writing what a proof would need, and a reason cannot outlive the row it is about. Keying the reason on the row's GRADE instead is tautological (a new guard inherits one and nobody looks at it) and a pinned COUNT moves only on net change; both were tried and are rejected. The sandbox is a real git repository built from `git ls-files` with working-tree content, never a filesystem walk. THE POPULATION IS EVERY DECLARED MUTATION-OUTCOME CLAIM, NOT ONLY THE INVENTORY'S ROWS (ersatztv#881): a sentence asserting that a specific mutation reddens, or does not redden, a named test is the same kind of claim wherever it is written — a code comment, a test docstring, a decision record — and is either a `CLAIMS` entry in `scripts/tests/mutation_manifest.py` that executes every run, or it is not written. A claim entry is keyed on the PROSE rather than on a guard: it carries the tracked `site` and the verbatim `quote`, both checked every run, so a reworded sentence reports as a RETARGET instead of drifting away from the entry that justifies it. THE QUOTE SPANS THE MUTATION AND THE OUTCOME IT ASSERTS: the outcome half is the checkable part, so a quote stopping at the mutation binds the uncontroversial words and leaves the assertion, and the test it names, outside the binding entirely. It declares `RED` or `GREEN`, and GREEN is the half no `MUTATION` row can express — a claim that a mutation is NOT noticed is read by THREE separate clauses, because each alone certifies a run that proves nothing: the run exited 0, something actually PASSED, and the named proof was shown to REACH the mutated clause. THE TWO DIRECTIONS ARE NOT SYMMETRIC, WHICH IS WHY THE THIRD CLAUSE EXISTS ON ONLY ONE OF THEM. A RED claim checks its own relevance for free — a proof that never executes the mutated file stays green and is refused in the same words as a clause that has stopped being load-bearing — while a GREEN claim gets precisely the green it asserts from a proof that reads a different file entirely, so exit status and a passing test TOGETHER still certify a run that never executed the clause. A GREEN entry therefore also declares a SECOND mutation of the SAME clause, required to REDDEN the same proof and carrying its own declared diagnostic, executed through the same red-reading path the inventory rows use; only once that passes is the green evidence about this clause at all. It is worth what the `DETECTOR` survivor was worth: re-run, and required to KEEP surviving. THE REFERENT IS DECLARED, NEVER INFERRED, exactly as the clause is: a checker hunting for sentences that merely SOUND like mutation claims would be a string predicate over prose, which this repo refuses to make load-bearing, so what is enforced is a claim that named its own referent, and the direction that cannot be mechanised — prose that declares nothing — is stated rather than papered over. TWO NEIGHBOURING PROPOSALS ARE SETTLED HERE. A QUOTATION of another file is a claim about that file: adopted, and the `quote` field IS the check, wherever the referent is declared; a general quotation-checker is rejected for the same reason inference is, since a quotation whose referent is unnamed has nothing to resolve. ANCHORING PROSE TO A STATE THE COMMIT ITSELF MOVES ('the last edit to that path', 'the only X that'): rejected as a detector and kept as a phrasing rule. The rejection is measured, and the scan is PINNED IN THE BODY rather than summarised as a figure here, for the same reason the population scan is — a count quoted without the predicate that produced it is the defect this record is about. What it shows is that the shape is indistinguishable BY PATTERN from ordinary rationale ('X is the only thing that can Y'), which this rule carves out, so a detector would fire mostly on prose it must not flag, and an advisory check that does that stops being read. Anchor to a date or a sha under `docs.no-session-narrative`'s dated-measurement clause; inside this rule's own population the anchor is the manifest entry, which is re-executed and therefore cannot go stale. | 2026-08-22 | [link](records/testing/mutation-claims-are-executed.md) | | `testing.playwright-mcp-download-and-recovery` | In Playwright-MCP E2E, fetch file-download endpoints with curl — never a browser tab or `window.open` — and if browser tools stall repeatedly, `pkill -f ms-playwright-mcp` and drive a fresh session. | 2026-07-21 | [link](records/testing/playwright-mcp-download-and-recovery.md) | -| `testing.scripted-playout-golden-deferred` | The `PlayoutBuildGoldenTests` in-memory golden net covers Sequential (YAML) as of #381. Scripted's *end-to-end pipeline* is excluded — `ScriptedPlayoutBuilder` runs a user-authored external program that drives the engine over HTTP loopback, which the in-memory harness can't pin — so that full-pipeline (integration) harness is deferred to #563. But the scheduling *behavior* those scripts drive lives entirely in the in-process `SchedulingEngine` (the `ScriptedScheduleController` is a 1:1 pass-through to it), which IS directly unit/golden-testable; the earlier "Scripted is un-golden-able by construction" framing overstated the constraint by conflating transport with engine. #395 extracts that shared switch to `ContentEnumeratorBuilder` and adds a direct regression net (`ContentEnumeratorBuilderTests`) over it. | 2026-07-22 | [link](records/testing/scripted-playout-golden-deferred.md) | +| `testing.scripted-engine-in-process-net` | Scripted playout is characterized at two in-process levels and no further. (1) `SchedulingEngineTests` drives the engine build API directly with substituted repositories — the same setup order `ScriptedPlayoutBuilder` uses (`WithPlayoutId`/`WithMode`/`WithSeed`/`BuildBetween`/`WithReferenceData`/`RestoreOrReset`, and `WithReferenceData` MUST precede the last two or `PlayoutReferenceData.PlayoutHistory` NREs). (2) `ScriptedScheduleControllerTests` replays a committed script fixture (`ErsatzTV.Tests/Controllers/Fixtures/scripted-build.json`) through the REAL `ScriptedScheduleController` + `ScriptedPlayoutBuilderService.MockSession` + `SchedulingEngine` and pins the resulting `PlayoutItem`s as a snapshot in the golden line format — so exactly ONE action-to-engine mapping exists in the repo, the production one. The `Cli.Wrap` launch of the user-authored program (exit code, timeout, stdout capture) and the Kestrel/middleware/`ApiAuthorizationFilter` transport it calls back over are NOT tested and are not scheduled to be; that residue is named below rather than argued away. The controller is a thin adapter, NOT a 1:1 pass-through: an unparseable playback order is a 400, an unparseable filler kind SILENTLY degrades to `FillerKind.None`, an unknown build id is a 404, and the engine no-progress `InvalidOperationException` is translated to a 400 — each pinned by a test, because a prose claim of equivalence decays. | 2026-09-05 | [link](records/testing/scripted-engine-in-process-net.md) | | `testing.suite-isolated-from-production-hook-fire-log` | THE SUITE MUST NOT WRITE TO `$HOME/.cache/ersatztv/hook-fire/`, AND THAT PROPERTY IS ESTABLISHED STRUCTURALLY RATHER THAN OBSERVED. Two layers in `scripts/tests/conftest.py`, which close different routes and must both stay. FIRST, `pytest_configure` sets `ETV_HOOK_FIRE_LOG_DIR` BEFORE COLLECTION. An autouse fixture alone runs at test setup — after every module has been imported — so it cannot reach a module that snapshots `{**os.environ}` at import time (the ersatztv#785 defect, 58 leaked records per run) and cannot reach a MODULE- or SESSION-SCOPED FIXTURE at all; measured on the pre-change tree by instrumenting `subprocess.Popen` for a full run and asking which launches resolve to `$HOME/.cache/ersatztv/hook-fire`, 83 did — 37 during collection, 44 inside module- or session-scoped fixtures, and 2 from an environment built from scratch. INSTRUMENT `Popen` ALONE: `subprocess.run`, `call` and `check_output` all reach it, so a census wrapping `run` as well counts every launch twice and every figure doubles. STATE THE PREDICATE WITH THE COUNT: this one also UNDERCOUNTS the from-scratch route, because such an environment usually carries no `HOME` either and lands in the sink's OTHER default, which this predicate cannot see. The launches that carry no `ETV_HOOK_FIRE_LOG_DIR` and are built from scratch number 83 as well, and 81 of those carry no `HOME` — two different sets of the same size, which is precisely the shape a later reader reconciles wrongly. The per-test fixture stays, because a test that inspects its own log dir must not see another test's records. SECOND, a `Popen` wrapper fails EVERY launch that does not CARRY the isolated directory — the route left open when an environment is built FROM SCRATCH rather than derived from `os.environ`, where no snapshot is involved and import ordering cannot help. THE RULE IS "CARRIES AN ISOLATED DIR", NOT "IS NOT THE PRODUCTION ONE", and that is the difference between a guard and a guard-shaped hole: `etv_hook_fire_log_dir` has TWO default branches — `$HOME/.cache/...` and, when `HOME` is unset or empty, `/tmp/.cache/...` — so a check comparing against one resolved production path models one branch and waves the other through, and `/tmp/.cache/ersatztv/hook-fire` is just as shared, just as persistent and just as readable with `hook-fire-log.sh report --dir`. Measured: of the 83 from-scratch launches, 81 carry no `HOME` either. Requiring the variable covers both branches and any a future sink adds, and is independent of what `HOME` happens to be on the machine running the suite — which is also what keeps it from firing on legitimate launches where `HOME` is unset or is itself `/tmp`. The corollary is that every from-scratch environment in the suite must carry the variable, one line each. NO COUNT OF THOSE SITES IS KEPT HERE: it is a hand-written population literal that nothing derives, and the commit introducing this rule already falsified its own, by adding further construction sites in the tests that prove the rule. The guard itself is the enumerator; a missing site is a red, not a stale sentence. Its population is every launch that actually happens, derived at runtime, so a new suite is covered by existing rather than by remembering. NEVER OBSERVE THE PRODUCTION DIRECTORY TO PROVE THIS. The withdrawn guard snapshotted `st_mtime_ns` across it and required no change, which makes the oracle state every concurrent session on the machine writes: an unrelated session firing a hook inside the window failed the suite with `a test run modified the production hook-fire log`, an accusation about the suite when the writer was another process. Seen on three separate branches, green on every immediate re-run. The misattribution is the expensive part — it aims the next reader at the suite, and a guard that cries wolf gets waved through. THE RESOLVER IS A REIMPLEMENTATION AND FAILS OPEN, so it is differential-tested against `etv_hook_fire_log_dir` every run: a resolver that disagrees clears a launch the sink then points at the real log. Shell's `:-` treats an EMPTY value as absent, which `dict.get(k, default)` does not — that case is in the matrix because it is where a Python transliteration goes wrong. AND BIND A VIOLATION BEFORE ASSERTING ON IT: pytest rewrites an assert expression and prints the repr of every sub-expression, so `assert isolation_violation(os.environ) is None` dumps the environment — API keys included — into the failure output and from there into a CI log. | 2026-08-28 | [link](records/testing/suite-isolated-from-production-hook-fire-log.md) | | `testing.troubleshoot-path-cannot-test-branding` | Verify logo/watermark/bug changes through a real channel playout — a green troubleshoot run proves nothing about branding. | 2026-07-21 | [link](records/testing/troubleshoot-path-cannot-test-branding.md) | | `testing.verification-code-needs-its-own-proof` | VERIFICATION CODE IS CODE UNDER TEST. The obligation in `testing.guard-ships-with-mutation-proof` attaches to whatever emits a PASS/FAIL somebody acts on — a smoke test, a timeout wrapper, an operator-run checker, the harness that applies the mutations — and NOT only to files sitting in the guard population or named like guards; every one of the five defects measured below was in code nobody had counted as a guard. THE DISCRIMINATOR IS WHETHER A PROOF TEST CAN DRIVE THE CONSTRUCT HERMETICALLY, NOT WHETHER CI RUNS IT, and the difference is load-bearing rather than pedantic: `.claude/hooks/*.sh` and `.husky/pre-push` are operator-run and CI never fires them AS HOOKS, yet several of them carry `MUTATION` rows — among them `pretooluse-bom-guard.sh`, `pretooluse-worktree-guard.sh` and `.husky/pre-push` — because a pytest proof drives them in a sandbox. A does-CI-run-it test would sort those already-proven guards into the exempt branch. (Most of the remaining hooks are graded `NONE`; that is unproven debt, not evidence for the other discriminator.) "IT NEEDS A REAL SERVER" IS AN EXEMPTION CLAIM AND IT IS USUALLY FALSE: `scripts/mcp_smoke.py` takes its config path and server name as POSITIONAL ARGUMENTS, so a test hands it a synthetic config in a tmpdir pointing at a stub responder and drives it with no gitignored `.mcp.json` and no language server, which is how it is proven in `scripts/tests/test_mcp_smoke.py`. That coverage is a SUBSET and no count of it is kept here: the file's own docstring enumerates the cases it drives and names what it does not, so there is a list rather than a number to keep in step with it. A checker is never excused because its CALLER cannot run in CI. WHERE THE PROOF LIVES: the checker itself needs NO `docs/guard-inventory.md` row, because that population is derived from workflow and hook call sites and a transitively-reached script is rejected as a phantom; the proof goes in an ordinary `scripts/tests/test_*.py`, which joins the population automatically and carries the row, while the manifest entry names THAT test as `guard` and the checker as `target`. That shape already exists for `mutation_harness_lib.py` and needs no change to any population. ONLY WHAT NO PROOF TEST CAN DRIVE HERMETICALLY falls back to a WITNESSED NEGATIVE CONTROL — the failure path executed once, the false-green form reproduced, both recorded in the PR, and its decay stated because nothing re-runs it. `scripts/check-local-lsp.sh` is the genuine case: it asserts developer-machine installs (a global `csharp-ls`, a `host/fxr` under `DOTNET_ROOT`, a root `node_modules`) that no runner has. STOP-AND-SUBTRACT, adopted here as this record's own threshold: when a review round's finding was CREATED by the previous round's fix and that happens TWICE CONSECUTIVELY, delete the layer generating them rather than guarding it. READING THE CONSTRUCT IS NOT EXECUTING IT: ShellCheck 0.11.0 flagged NEITHER of the two shell defects below, and each is indistinguishable from correct by reading. | 2026-08-28 | [link](records/testing/verification-code-needs-its-own-proof.md) | diff --git a/docs/decisions/records/testing/scripted-playout-golden-deferred.md b/docs/decisions/archive/testing/scripted-playout-golden-deferred.md similarity index 77% rename from docs/decisions/records/testing/scripted-playout-golden-deferred.md rename to docs/decisions/archive/testing/scripted-playout-golden-deferred.md index 3111ebc59..ae21f0bc2 100644 --- a/docs/decisions/records/testing/scripted-playout-golden-deferred.md +++ b/docs/decisions/archive/testing/scripted-playout-golden-deferred.md @@ -1,13 +1,13 @@ --- key: testing.scripted-playout-golden-deferred title: 2026-07-22 — Sequential (YAML) playout gets a golden; Scripted is excluded from the golden net by construction (#381) -status: active +status: superseded since: '2026-07-22' supersedes: none -superseded-by: none -rule: 'The `PlayoutBuildGoldenTests` in-memory golden net covers Sequential (YAML) as of #381. Scripted''s *end-to-end pipeline* is excluded — `ScriptedPlayoutBuilder` runs a user-authored external program that drives the engine over HTTP loopback, which the in-memory harness can''t pin — so that full-pipeline (integration) harness is deferred to #563. But the scheduling *behavior* those scripts drive lives entirely in the in-process `SchedulingEngine` (the `ScriptedScheduleController` is a 1:1 pass-through to it), which IS directly unit/golden-testable; the earlier "Scripted is un-golden-able by construction" framing overstated the constraint by conflating transport with engine. #395 extracts that shared switch to `ContentEnumeratorBuilder` and adds a direct regression net (`ContentEnumeratorBuilderTests`) over it.' +superseded-by: testing.scripted-engine-in-process-net@2026-09-05 +rule: '(superseded) The `PlayoutBuildGoldenTests` in-memory golden net covers Sequential (YAML) as of #381. Scripted''s *end-to-end pipeline* is excluded — `ScriptedPlayoutBuilder` runs a user-authored external program that drives the engine over HTTP loopback, which the in-memory harness can''t pin — so that full-pipeline (integration) harness is deferred to #563. But the scheduling *behavior* those scripts drive lives entirely in the in-process `SchedulingEngine` (the `ScriptedScheduleController` is a 1:1 pass-through to it), which IS directly unit/golden-testable; the earlier "Scripted is un-golden-able by construction" framing overstated the constraint by conflating transport with engine. #395 extracts that shared switch to `ContentEnumeratorBuilder` and adds a direct regression net (`ContentEnumeratorBuilderTests`) over it.' signals: 'why is there no scripted golden; scripted transport vs engine; SchedulingEngine is in-process testable; Cli.Wrap external process is transport only; ScriptedScheduleController 1:1 pass-through; engine-level scripted regression net; EnumeratorForContent · paths: `ErsatzTV.Core.Tests/Scheduling/Goldens/PlayoutBuildGoldenTests.cs`, `ErsatzTV.Core.Tests/Scheduling/Engine/SchedulingEngineTests.cs`, `ErsatzTV.Core/Scheduling/Engine/SchedulingEngine.cs`, `ErsatzTV/Controllers/Api/ScriptedScheduleController.cs`, `ErsatzTV.Core/Scheduling/ScriptedScheduling/ScriptedPlayoutBuilder.cs` · issues: #381, #163, #395, #563' -mechanics: docs/testing.md → Golden-file nets +mechanics: superseded by `testing.scripted-engine-in-process-net` (ersatztv#563), which stands up the deferred coverage in-process and corrects the "1:1 pass-through" description of `ScriptedScheduleController` --- **Sequential (YAML) is golden-able and TZ-independent.** `SequentialPlayoutBuilder` reads a YAML schedule diff --git a/docs/decisions/records/testing/scripted-engine-in-process-net.md b/docs/decisions/records/testing/scripted-engine-in-process-net.md new file mode 100644 index 000000000..f0d285272 --- /dev/null +++ b/docs/decisions/records/testing/scripted-engine-in-process-net.md @@ -0,0 +1,54 @@ +--- +key: testing.scripted-engine-in-process-net +title: '2026-09-05 — Scripted playout is covered in-process, engine plus controller adapter; the Cli.Wrap process and the HTTP transport are permanently out of the automated suite (#563)' +status: active +since: '2026-09-05' +supersedes: testing.scripted-playout-golden-deferred@2026-07-22 +superseded-by: none +rule: 'Scripted playout is characterized at two in-process levels and no further. (1) `SchedulingEngineTests` drives the engine build API directly with substituted repositories — the same setup order `ScriptedPlayoutBuilder` uses (`WithPlayoutId`/`WithMode`/`WithSeed`/`BuildBetween`/`WithReferenceData`/`RestoreOrReset`, and `WithReferenceData` MUST precede the last two or `PlayoutReferenceData.PlayoutHistory` NREs). (2) `ScriptedScheduleControllerTests` replays a committed script fixture (`ErsatzTV.Tests/Controllers/Fixtures/scripted-build.json`) through the REAL `ScriptedScheduleController` + `ScriptedPlayoutBuilderService.MockSession` + `SchedulingEngine` and pins the resulting `PlayoutItem`s as a snapshot in the golden line format — so exactly ONE action-to-engine mapping exists in the repo, the production one. The `Cli.Wrap` launch of the user-authored program (exit code, timeout, stdout capture) and the Kestrel/middleware/`ApiAuthorizationFilter` transport it calls back over are NOT tested and are not scheduled to be; that residue is named below rather than argued away. The controller is a thin adapter, NOT a 1:1 pass-through: an unparseable playback order is a 400, an unparseable filler kind SILENTLY degrades to `FillerKind.None`, an unknown build id is a 404, and the engine no-progress `InvalidOperationException` is translated to a 400 — each pinned by a test, because a prose claim of equivalence decays.' +signals: 'why is there no scripted golden; scripted integration harness; scripted transport vs engine; SchedulingEngine is in-process testable; Cli.Wrap external process is transport only; shell-out scoped out; MockSession seam; in-process stand-in for a scripted script; engine build API characterization; committed script fixture replay; ScriptedScheduleController is an adapter not a pass-through; scripted filler kind silently falls back to None; scripted playout no-progress halt maps to 400; scripted build 401 blind spot; EnumeratorForContent · paths: `ErsatzTV.Core.Tests/Scheduling/Engine/SchedulingEngineTests.cs`, `ErsatzTV.Tests/Controllers/ScriptedScheduleControllerTests.cs`, `ErsatzTV.Tests/Controllers/Fixtures/scripted-build.json`, `ErsatzTV.Core/Scheduling/Engine/SchedulingEngine.cs`, `ErsatzTV/Controllers/Api/ScriptedScheduleController.cs`, `ErsatzTV.Core/Scheduling/ScriptedScheduling/ScriptedPlayoutBuilder.cs`, `ErsatzTV.Core.Tests/Scheduling/Goldens/PlayoutBuildGoldenTests.cs` · issues: #563, #381, #395, #163' +mechanics: docs/testing.md → "Scripted playout coverage" and "Golden-file nets" +--- + +**What carries over from the predecessor.** Sequential (YAML) is golden-able and TZ-independent, and +`PlayoutBuildGoldenTests` covers Classic / Block / Sequential. Scripted's *end-to-end pipeline* stays out +of that net: `ScriptedPlayoutBuilder` builds nothing itself, it `Cli.Wrap`-executes a user-authored +program, hands it `http://localhost:{Settings.UiPort}` plus a build id, and reads the result straight off +the in-process engine afterwards. #395's `ContentEnumeratorBuilderTests` remains the direct net over the +enumerator-construction switch Scripted and Sequential share. + +**What changed.** The predecessor deferred the whole harness to #563 and described the controller as a +"1:1 pass-through". Both are now settled: the coverage exists in-process, and the pass-through wording was +wrong — the four adapter mappings above are real behaviour that only a test can hold. + +**Why not the full Kestrel + `Cli.Wrap` harness.** Four measured obstacles, not a preference: + +1. *The production transport is not "a controller behind Kestrel".* `Startup` 404s any request whose Host + is not `localhost` and whose `Connection.LocalPort != Settings.UiPort` (a mutable process-wide static), + and `ApiAuthorizationFilter` fail-closes every mutating verb. A hand-rolled minimal host would test a + transport the product does not have; the real `Startup` drags in the whole DI graph (TvContext, ffmpeg + workers, search index, background services), which is what #381 deferred in the first place. +2. *The external program is user-authored by definition.* `Playout.ScheduleFile` is an arbitrary command + line, so any script we commit is a stand-in we wrote either way. The fork's own driver, + `scripts/scripted-schedules/entrypoint.py`, imports `etv_client`, a generated package that exists only + in the deploy environment, so it cannot run in the .NET test job without adding a client-generation step. +3. *It would put an interpreter and process spawning inside the pure in-memory .NET suite*, with the 30s + `Cli.Wrap` timeout nested inside CI's `--blame-hang-timeout 2m` — a new flake class on a shared runner. +4. *The stand-in loses nothing the harness would have added at the engine level.* Replaying the fixture + through the real controller and the real service keeps a single action-to-engine mapping under test; + only the two transport hops are absent. + +**The residue, named.** Uncovered: process launch, exit code and timeout handling; Kestrel plus the +forwarded-headers / compression / SPA-hosting middleware; and `ApiAuthorizationFilter`. That last one is +not hypothetical — `ScriptedScheduleController` carries no `[SkipApiAuthorization]` while +`ScriptedPlayoutBuilder` passes the script only host, build id and mode, with no `X-Api-Key`, and +`entrypoint.py` builds its client with no credential. Whether a scripted build therefore 401s on its first +POST in this fork is unverified against a live instance and is tracked separately; it is recorded here as +the concrete shape of what an engine-level net cannot see, so the scope-out is not read as a claim of +equivalence. + +**Determinism constraints on the fixtures.** Both use Chronological order and only instant-preserving +instructions (`AddCount`, `AddAll`, `AddDuration`, `PadUntilExact`). `WaitUntil(TimeOnly)` and +`PadUntil(string)` read the LOCAL day and time-of-day, and `PadToNext` becomes offset-sensitive once a +call has localized `CurrentTime`, so introducing any of them — or a `shuffle` order — reopens the +timezone and seed question the fixtures were built to close. diff --git a/docs/testing.md b/docs/testing.md index dbc04b5d5..d08021c09 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -35,14 +35,10 @@ Three golden-file suites guard the highest-value, most-subtle output: produces over a pinned build window. Covers the **Classic** (`PlaybackOrder.Chronological`), **Block**, and **Sequential (YAML)** builders. The **Scripted** *end-to-end pipeline* is excluded from this net — `ScriptedPlayoutBuilder` runs a user-authored external process that drives the engine over HTTP, which - the in-memory harness can't pin; that integration harness is tracked in ersatztv#563. The scheduling - *behavior* those scripts drive, though, lives in the in-process `SchedulingEngine` (the HTTP controller is a - 1:1 pass-through) and IS directly testable — `SchedulingEngineTests` news it up with substitutes, and - `ContentEnumeratorBuilderTests` (ersatztv#395) is the direct regression net over the enumerator-construction - helper the Scripted and Sequential/YAML engines now share (decision: - `testing.scripted-playout-golden-deferred`). The **Sequential** case - (`Sequential_yaml`, ersatztv#381) - builds from a committed YAML fixture (`Goldens/Fixtures/sequential-schedule.yml`) instead of a + the in-memory harness can't pin. Scripted is instead covered in-process at two levels, described under + "Scripted playout coverage" below (decision: `testing.scripted-engine-in-process-net`, ersatztv#563). + The **Sequential** case (`Sequential_yaml`, ersatztv#381) builds from a committed YAML fixture + (`Goldens/Fixtures/sequential-schedule.yml`) instead of a `ProgramSchedule`; it is TZ-independent (the `count`/`all`/`duration` handlers do UTC-only arithmetic — it passes, not skips, under a non-UTC `TZ`) so needs no `Assume` guard. A third case, `Classic_clock_padded` (ersatztv#77), locks clock-boundary padding: a `FillerMode.Pad` + @@ -67,6 +63,30 @@ diff during normal test runs means the code changed the output — regenerating away hides the change instead of surfacing it. Only a human who has confirmed the change is intentional should regenerate. +## Scripted playout coverage + +Scripted playout is characterized in-process at two levels, and the transport is deliberately not covered +at all (ersatztv#563, decision `testing.scripted-engine-in-process-net`). + +| Where | Covers | +| --- | --- | +| `ErsatzTV.Core.Tests/Scheduling/Engine/SchedulingEngineTests.cs` | The engine build API a script drives: `AddCollection`, `AddCount`, `AddAll`, `AddDuration` (stop-before-end and trim), `PadUntilExact`, EPG guide-group locking, per-item `PlayoutHistory`, the `IsDone` no-progress halt and its reset, and the `GetAnchor`/`RestoreOrReset` round-trip a Continue build resumes from. Substituted repositories, no database. | +| `ErsatzTV.Tests/Controllers/ScriptedScheduleControllerTests.cs` | A committed script fixture (`Controllers/Fixtures/scripted-build.json`) replayed through the real `ScriptedScheduleController` + `ScriptedPlayoutBuilderService.MockSession` + `SchedulingEngine`, with a pinned 13-item snapshot in the same line format the playout goldens use, plus the four adapter mappings: 404 on an unknown build id, 400 on an unparseable playback order, a silent fall back to `FillerKind.None` on an unparseable filler kind, and the engine's no-progress `InvalidOperationException` translated to a 400. | +| `ErsatzTV.Core.Tests/Scheduling/ContentEnumeratorBuilderTests.cs` | The enumerator-construction helper the Scripted and Sequential/YAML engines share (ersatztv#395). | + +The expected snapshot is a string constant in the test rather than a fourth golden file: the golden +harness lives in `ErsatzTV.Core.Tests`, which cannot reference a controller, and duplicating it would +create a second action-to-engine mapping — the thing the single-mapping design exists to avoid. + +**Not covered, and not scheduled to be** (measured 2026-09-05): the `Cli.Wrap` launch of the user's own +program — exit code, the `PlayoutScriptedScheduleTimeoutSeconds` timeout, stdout capture; Kestrel and the +`Startup` middleware, including the host/`Settings.UiPort` check that 404s a foreign request; and +`ApiAuthorizationFilter`, which fail-closes every mutating verb for an endpoint without +`[SkipApiAuthorization]`. Hosting the real `Startup` would drag in the whole DI graph, a hand-rolled +minimal host would test a transport the product does not have, and the script is user-authored by +definition — so any committed script is a stand-in either way. The decision record names the concrete +blind spot this leaves. + ## Timezone independence The suite is timezone-independent (ersatztv#24). When constructing test `PlayoutItem`s, always @@ -74,6 +94,17 @@ set a real `Start` (e.g. `startState.CurrentTime.UtcDateTime`) — never rely on `DateTime.MinValue`, which underflows `DateTimeOffset.MinValue` once a non-UTC local offset is applied (`StartOffset` calls `ToLocalTime()`). CI runs in UTC; local runs may not. +**Which `SchedulingEngine` calls are safe to put in a fixture** (audited 2026-09-05). `AddCount`, +`AddAll`, `AddDuration` and `PadUntilExact` preserve the instant — items are always written as +`_state.CurrentTime.UtcDateTime`, and `PadUntilExact`'s `ToLocalTime()` changes the offset the state +carries, not the moment. `WaitUntil(TimeOnly)` and `PadUntil(string)` are **not** safe: both read the +LOCAL day and time-of-day off `CurrentTime` and rebuild a target from them. `PadToNext` is safe only +until something localizes `CurrentTime` — it reads `.Year`/`.Month`/`.Day`/`.Hour`/`.Minute` in whatever +offset that value happens to carry, so a fixture that calls it after `PadUntilExact`, `WaitUntilExact` or +a Continue anchor becomes TZ-sensitive by ordering rather than by call. The scripted fixtures therefore +use Chronological order and only the instant-preserving instructions; they pass, rather than skip, under +`TZ=UTC`, `America/New_York`, `Australia/Lord_Howe` and `Asia/Kathmandu`. + ## Running tests Full .NET gate: