feat(downloader): backend-execution tests + per-backend executable paths#53
Open
mads-jm wants to merge 6 commits into
Open
feat(downloader): backend-execution tests + per-backend executable paths#53mads-jm wants to merge 6 commits into
mads-jm wants to merge 6 commits into
Conversation
Add offline, fixture-driven Vitest coverage for the yt-dlp/spotDL/Spytify CLI invocation paths (progress parsing, completion-path heuristics incl. the yt-dlp informational-line regression, error classification, checkInstalled states, cancellation) plus the subprocess lifecycle (line buffering, inactivity timeout, process kill). Recorded fixtures live in __tests__/fixtures/; backends are driven via a documented vi.mock seam over runCommand/spawnLines and child_process.spawn (helpers/fixture-process.ts). Backends gain an optional executable-path constructor arg used in place of the bare command name (default behaviour unchanged) — groundwork for #46. Fix latent bug: SpotdlBackend declared the singular 'spotify-id' capability while DownloadInput.type uses 'spotify-ids' (plural), making that path unreachable. InputType and supportedInputs now use the plural form.
Users whose yt-dlp/spotDL/Spytify binary is not on PATH can now configure a custom executable path, persisted in a main-process settings store (downloader-config-store.ts, userData JSON — localStorage can't reach main). downloader-ipc reads the path before constructing each backend and invalidates the cached instance on change; resolved path + version flow through the existing download:check-backends channel (BackendStatusResult gains an additive 'path' field). New download:get-backend-paths / download:set-backend-path channels + preload methods. DownloadSettings lets users set/clear a path per backend and shows the resolved path + version; the 'planned for a future release' note is removed. BackendGate distinguishes 'not installed' from a misconfigured custom path and points at the settings entry point. Spytify stays Windows-gated with real-time PoC copy; its path editor is inert on non-Windows. No bundling introduced. Status interpretation is extracted to a pure, unit-tested helper (backend-status.ts).
…ro-exit Inspector ITERATE blocker: epic #45 AC requires all three checkInstalled states (installed, spawn-error, non-zero exit) for all three backends. Spytify covered only the installed case; add the two missing Windows-path cases mirroring the yt-dlp/spotDL test patterns. No production change.
|
The Architect handoff notes are working artifacts of the agent cycle, not project source. They were the only add/add conflict when integrating the Wave-1 lanes, and they do not belong in the reviewable diff. Removing them here keeps the PR diff to real changes; .claude/ scratch is gitignored on mvp.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short Description
Brings the download backends from "parsers tested, execution not" up to covered: integration tests that drive the actual yt-dlp / spotDL / Spytify CLI invocation through a fixture/mock-process harness (#45), plus per-backend custom executable-path config with presence checks so binaries no longer have to be assumed-in-PATH (#46). Also fixes a real latent bug — the spotDL input-type mismatch that made its capability unreachable.
Background
The downloader was the most mature pillar, but its tests stopped at parsers and mappers; the subprocess lifecycle and each backend's argv/stdout/stderr handling were unverified. This adds a
fixture-processharness that replays captured CLI output so the execution paths run deterministically, then layers the user-facing path config (#46) on top.Two deviations worth calling out up front: (1) yt-dlp completion detection now uses a
WHATNEXT_FILEPATH=sentinel rather than the spec's stdout line-heuristic — strictly safer, no locale/format guessing; (2)spotDLsupportedInputsdeclared the singular'spotify-id'while the capability check expected'spotify-ids', so that path was dead — fixed and now reachable.What Has Changed
Backend-execution tests (#45):
__tests__/helpers/fixture-process.ts+load-fixture.ts— mock-process harness replaying captured CLI outputytdlp-backend.test.ts(11),spotdl-backend.test.ts(13),spytify-backend.test.ts(8),subprocess.test.ts(10) — each backend'scheckInstalled()covers all three states (installed / spawn-error / non-zero-exit); progress/destination/error parsing; subprocess kill + timeout__tests__/fixtures/(success/skip/error/benign-stderr per backend)ytdlp-backend.ts—WHATNEXT_FILEPATH=sentinel for completed-path capture (replaces the stdout heuristic)spotdl-backend.ts—'spotify-id'→'spotify-ids'input-type fix (was unreachable)Binary path config (#46):
downloader-config-store.ts(new) +downloader-ipc.ts+ipc-protocol.ts+preload.ts— per-backend custom executable path, persisted; cache-invalidating presence checksbackend-status.ts+BackendGate.tsx+DownloadSettings.tsx— presence UX (gate + path editor); logic extracted tobackend-status.tsfor testabilityservice/downloader/index.ts—createBackend(id, execPath?)additive optional arg (groundwork)Steps for Reviewing
fixture-process.ts— the harness everything else leans on; confirm it faithfully models spawn/stdout/stderr/exit.ytdlp-backend.ts— judge theWHATNEXT_FILEPATH=approach vs the spec's heuristic (deliberate deviation). ✅ inspector confirmed sound'spotify-ids'makes the capability reachable.backend-status.ts+BackendGate.tsx: testable logic is extracted; the JSX shells are not unit-tested (no jsdom).src/+service/paths. ✅Checklist
DownloadSettings.tsx/BackendGate.tsxJSX untested (no component harness) — needs manual QAchore/fix-dev-env-lint;service/files sit outside the app eslint rootCloses #45
Closes #46
Comments
Carry-forwards (non-blocking):
Spytify cancel()path untested;isBackendIdguard on cache invalidation;createBackendJSDoc.createBackend(id, execPath?)'s new arg is currently unused by the IPC wiring (which builds viagetBackend) — harmless groundwork for when custom-path construction is wired through./Windows/ierror-text heuristic since the renderer lacksprocess.platform.