diff --git a/README.md b/README.md index 5925ac3..ee66cc6 100644 --- a/README.md +++ b/README.md @@ -96,6 +96,11 @@ subminer --start video.mkv # optional explicit overlay start when plugin auto_st For full guides on configuration, Anki, Jellyfin, and more, see [docs.subminer.moe](https://docs.subminer.moe). +## Verification + +- Run `bun run test:subtitle` to verify subtitle sync coverage for the maintained `alass`/`ffsubsync` test surface. +- That lane reuses `src/core/services/subsync.test.ts` and `src/subsync/utils.test.ts`, which are also included in the broader `bun run test:core` suite. + ## Acknowledgments Built on the shoulders of [GameSentenceMiner](https://github.com/bpwhelan/GameSentenceMiner), [texthooker-ui](https://github.com/Renji-XD/texthooker-ui), [mpvacious](https://github.com/Ajatt-Tools/mpvacious), [Anacreon-Script](https://github.com/friedrich-de/Anacreon-Script), and [autosubsync-mpv](https://github.com/joaquintorres/autosubsync-mpv). Subtitles powered by [Jimaku.cc](https://jimaku.cc). Dictionary lookups via [Yomitan](https://github.com/yomidevs/yomitan). diff --git a/backlog/tasks/task-87.2 - Subtitle-sync-verification-replace-the-no-op-subtitle-lane-with-real-automated-coverage.md b/backlog/tasks/task-87.2 - Subtitle-sync-verification-replace-the-no-op-subtitle-lane-with-real-automated-coverage.md index 4de414f..fa23a71 100644 --- a/backlog/tasks/task-87.2 - Subtitle-sync-verification-replace-the-no-op-subtitle-lane-with-real-automated-coverage.md +++ b/backlog/tasks/task-87.2 - Subtitle-sync-verification-replace-the-no-op-subtitle-lane-with-real-automated-coverage.md @@ -3,10 +3,11 @@ id: TASK-87.2 title: >- Subtitle sync verification: replace the no-op subtitle lane with real automated coverage -status: To Do -assignee: [] +status: Done +assignee: + - Kyle Yasuda created_date: '2026-03-06 03:19' -updated_date: '2026-03-06 03:21' +updated_date: '2026-03-06 08:06' labels: - tests - subsync @@ -27,27 +28,57 @@ priority: high ## Description - SubMiner advertises subtitle syncing with alass and ffsubsync, but the dedicated test:subtitle command currently does not run any tests. There is already lower-level coverage in src/core/services/subsync.test.ts, but the test matrix and contributor-facing commands do not reflect that reality. This task should replace the no-op lane with real verification, align scripts with the existing subsync test surface, and make the user-facing docs honest about how subtitle sync is verified. - ## Acceptance Criteria - - -- [ ] #1 The test:subtitle entrypoint runs real automated verification instead of echoing a placeholder message. -- [ ] #2 The subtitle verification lane covers both alass and ffsubsync behavior, including at least one non-happy-path scenario relevant to current functionality. -- [ ] #3 Contributor-facing documentation points to the real subtitle verification command and no longer implies a dedicated test lane exists when it does not. -- [ ] #4 The resulting verification strategy integrates cleanly with the repository-wide test matrix without duplicating or hiding existing subsync coverage. +- [x] #1 The test:subtitle entrypoint runs real automated verification instead of echoing a placeholder message. +- [x] #2 The subtitle verification lane covers both alass and ffsubsync behavior, including at least one non-happy-path scenario relevant to current functionality. +- [x] #3 Contributor-facing documentation points to the real subtitle verification command and no longer implies a dedicated test lane exists when it does not. +- [x] #4 The resulting verification strategy integrates cleanly with the repository-wide test matrix without duplicating or hiding existing subsync coverage. ## Implementation Plan +Plan of record: -1. Audit the existing subtitle-sync test surface, especially src/core/services/subsync.test.ts, and decide whether test:subtitle should reuse or regroup that coverage. -2. Replace the placeholder script with a real automated command and keep the matrix legible alongside TASK-87.1 work. -3. Update README or related docs so the advertised subtitle verification path matches reality. -4. Verify both alass and ffsubsync behavior remain covered by the resulting lane. +1. Replace the placeholder package-script lane with a real `test:subtitle:src` command that runs the maintained subtitle-sync tests directly (`src/core/services/subsync.test.ts` and `src/subsync/utils.test.ts`), and point `test:subtitle` at that lane instead of build+echo behavior. +2. Add one focused ffsubsync non-happy-path test in `src/core/services/subsync.test.ts` so the dedicated lane explicitly covers both engines plus failure propagation relevant to current functionality. +3. Update `README.md` contributor guidance to name `bun run test:subtitle` as the subtitle verification command and explain that it reuses the maintained subsync tests already included in broader core coverage. +4. Verify the final strategy by running `bun run test:subtitle` and `bun run test:core:src` so the dedicated lane stays aligned with the repository-wide matrix instead of creating a divergent hidden suite. + +Detailed execution plan saved at `docs/plans/2026-03-06-subtitle-sync-verification.md`. + +## Implementation Notes + + +Reviewed task references and current subtitle verification surface. Existing coverage already lives in `src/core/services/subsync.test.ts` and `src/subsync/utils.test.ts`; `test:subtitle` is still a placeholder build+echo wrapper. The referenced report `docs/reports/2026-02-22-task-100-dead-code-report.md` is not present in the workspace, so planning used the task body plus repository state instead. + +Implementation plan written and saved to `docs/plans/2026-03-06-subtitle-sync-verification.md`. Proceeding with execution per the task request. + +Replaced the placeholder subtitle lane with `test:subtitle:src` in `package.json`, pointing `test:subtitle` directly at `src/core/services/subsync.test.ts` and `src/subsync/utils.test.ts` instead of build+echo behavior. + +Added explicit ffsubsync failure-path coverage in `src/core/services/subsync.test.ts`, asserting non-zero command failures surface detailed `ffsubsync synchronization failed` messaging alongside existing alass coverage. + +Updated `README.md` verification guidance to point contributors at `bun run test:subtitle` and explain that the lane reuses the maintained subsync tests already included in `bun run test:core`. + +Verification: `bun run test:subtitle` passed (15 tests across 2 files). `bun run test:core:src` also passed (373 pass, 6 skip, 0 fail), confirming the dedicated subtitle lane stays aligned with the broader matrix. + + +## Final Summary + + +Implemented a real subtitle verification lane by replacing the placeholder `test:subtitle` build+echo flow with a source-level `test:subtitle:src` command that runs the maintained subtitle-sync tests directly from `src/core/services/subsync.test.ts` and `src/subsync/utils.test.ts`. This keeps subtitle verification explicit for contributors while still reusing the same maintained test surface already covered by `test:core`. + +Expanded subtitle-sync coverage with an explicit ffsubsync failure-path test so the dedicated lane now exercises both engines plus a user-visible non-happy path. Updated `README.md` to document `bun run test:subtitle` as the contributor-facing subtitle verification command and to explain its relationship to the broader core suite. + +Verification run: +- `bun run test:subtitle` +- `bun run test:core:src` + +Notes: +- The task reference `docs/reports/2026-02-22-task-100-dead-code-report.md` was not present in the workspace during execution, so implementation used the task body and live repository state as the source of truth. + diff --git a/docs/plans/2026-03-06-subtitle-sync-verification.md b/docs/plans/2026-03-06-subtitle-sync-verification.md new file mode 100644 index 0000000..d21607b --- /dev/null +++ b/docs/plans/2026-03-06-subtitle-sync-verification.md @@ -0,0 +1,121 @@ +# Subtitle Sync Verification Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Replace the no-op `test:subtitle` lane with real automated subtitle-sync verification that reuses the maintained subsync tests and documents the real contributor workflow. + +**Architecture:** Repoint the subtitle verification command at the existing source-level subsync tests instead of inventing a second hidden suite. Add one focused ffsubsync failure-path test so the subtitle lane explicitly covers both engines plus a non-happy path, then update contributor docs to describe the dedicated subtitle lane and how it relates to `test:core`. + +**Tech Stack:** TypeScript, Bun test, Node test/assert, npm package scripts, Markdown docs. + +--- + +### Task 1: Lock subtitle lane to real subsync tests + +**Files:** + +- Modify: `package.json` + +**Step 1: Write the failing test** + +Define the intended command shape first: `test:subtitle:src` should run `src/core/services/subsync.test.ts` and `src/subsync/utils.test.ts`, `test:subtitle` should invoke that real source lane, and no placeholder echo should remain. + +**Step 2: Run test to verify it fails** + +Run: `bun run test:subtitle` +Expected: It performs a build and prints `Subtitle tests are currently not configured`, proving the lane is still a no-op. + +**Step 3: Write minimal implementation** + +Update `package.json` so: + +- `test:subtitle:src` runs `bun test src/core/services/subsync.test.ts src/subsync/utils.test.ts` +- `test:subtitle` runs the new source lane directly +- `test:subtitle:dist` is removed if it is no longer the real verification path + +**Step 4: Run test to verify it passes** + +Run: `bun run test:subtitle` +Expected: PASS with Bun executing the real subtitle-sync test files. + +### Task 2: Add explicit ffsubsync non-happy-path coverage + +**Files:** + +- Modify: `src/core/services/subsync.test.ts` +- Test: `src/core/services/subsync.test.ts` + +**Step 1: Write the failing test** + +Add a test that runs `runSubsyncManual({ engine: 'ffsubsync' })` with a stub ffsubsync executable that exits non-zero and writes stderr, then assert: + +- `result.ok === false` +- `result.message` starts with `ffsubsync synchronization failed` +- the failure message includes command details surfaced to the user + +**Step 2: Run test to verify it fails** + +Run: `bun test src/core/services/subsync.test.ts` +Expected: FAIL because ffsubsync failure propagation is not asserted yet. + +**Step 3: Write minimal implementation** + +Keep production code unchanged unless the new test exposes a real bug. If needed, tighten failure assertions or message propagation in `src/core/services/subsync.ts` without changing successful behavior. + +**Step 4: Run test to verify it passes** + +Run: `bun test src/core/services/subsync.test.ts` +Expected: PASS with both alass and ffsubsync paths covered, including a non-happy path. + +### Task 3: Make contributor docs match the real verification path + +**Files:** + +- Modify: `README.md` +- Modify: `package.json` + +**Step 1: Write the failing test** + +Use the repository state as the failure signal: README currently advertises subtitle sync as a feature but does not tell contributors that `bun run test:subtitle` is the real verification lane. + +**Step 2: Run test to verify it fails** + +Run: `bun run test:subtitle && bun test src/subsync/utils.test.ts` +Expected: Tests pass, but docs still do not explain the lane; this is the remaining acceptance-criteria gap. + +**Step 3: Write minimal implementation** + +Update `README.md` with a short contributor-facing verification note that: + +- points to `bun run test:subtitle` for subtitle-sync coverage +- states that the lane reuses the maintained subsync tests already included in broader core coverage +- avoids implying there is a separate hidden subtitle test harness beyond those tests + +**Step 4: Run test to verify it passes** + +Run: `bun run test:subtitle` +Expected: PASS, with docs and scripts now aligned around the same subtitle verification strategy. + +### Task 4: Verify matrix integration stays clean + +**Files:** + +- Modify: `package.json` (only if Task 1/3 exposed cleanup needs) + +**Step 1: Write the failing test** + +Treat duplication as the failure condition: confirm the dedicated subtitle lane reuses the same maintained files already present in `test:core:src` rather than creating a second divergent suite. + +**Step 2: Run test to verify it fails** + +Run: `bun run test:subtitle && bun run test:core:src` +Expected: If file lists diverge unexpectedly, this review step exposes it before handoff. + +**Step 3: Write minimal implementation** + +If needed, do the smallest script cleanup necessary so subtitle coverage remains explicit without hiding or duplicating existing core coverage. + +**Step 4: Run test to verify it passes** + +Run: `bun run test:subtitle && bun run test:core:src` +Expected: PASS, confirming the dedicated lane and the broader core suite agree on subtitle coverage. diff --git a/package.json b/package.json index 7161ea0..ac2d06e 100644 --- a/package.json +++ b/package.json @@ -23,12 +23,12 @@ "test:core:dist": "bun test dist/cli/args.test.js dist/cli/help.test.js dist/core/services/cli-command.test.js dist/core/services/ipc.test.js dist/core/services/anki-jimaku-ipc.test.js dist/core/services/field-grouping-overlay.test.js dist/core/services/numeric-shortcut-session.test.js dist/core/services/secondary-subtitle.test.js dist/core/services/mpv-render-metrics.test.js dist/core/services/overlay-content-measurement.test.js dist/core/services/mpv-control.test.js dist/core/services/mpv.test.js dist/core/services/runtime-options-ipc.test.js dist/core/services/runtime-config.test.js dist/core/services/config-hot-reload.test.js dist/core/services/discord-presence.test.js dist/core/services/tokenizer.test.js dist/core/services/tokenizer/annotation-stage.test.js dist/core/services/tokenizer/parser-selection-stage.test.js dist/core/services/tokenizer/parser-enrichment-stage.test.js dist/core/services/subsync.test.js dist/core/services/overlay-bridge.test.js dist/core/services/overlay-manager.test.js dist/core/services/overlay-shortcut-handler.test.js dist/core/services/mining.test.js dist/core/services/anki-jimaku.test.js dist/core/services/jimaku-download-path.test.js dist/core/services/jellyfin.test.js dist/core/services/jellyfin-remote.test.js dist/core/services/immersion-tracker-service.test.js dist/core/services/overlay-runtime-init.test.js dist/core/services/app-ready.test.js dist/core/services/startup-bootstrap.test.js dist/core/services/subtitle-processing-controller.test.js dist/core/services/anilist/anilist-token-store.test.js dist/core/services/anilist/anilist-update-queue.test.js dist/renderer/error-recovery.test.js dist/renderer/subtitle-render.test.js dist/renderer/handlers/mouse.test.js dist/renderer/handlers/keyboard.test.js dist/renderer/modals/jimaku.test.js dist/subsync/utils.test.js dist/main/anilist-url-guard.test.js dist/window-trackers/x11-tracker.test.js", "test:core:smoke:dist": "bun test dist/cli/help.test.js dist/core/services/runtime-config.test.js dist/core/services/ipc.test.js dist/core/services/overlay-manager.test.js dist/core/services/anilist/anilist-token-store.test.js dist/core/services/startup-bootstrap.test.js dist/renderer/error-recovery.test.js dist/main/anilist-url-guard.test.js dist/window-trackers/x11-tracker.test.js", "test:smoke:dist": "bun run test:config:smoke:dist && bun run test:core:smoke:dist", - "test:subtitle:dist": "echo \"Subtitle tests are currently not configured\"", + "test:subtitle:src": "bun test src/core/services/subsync.test.ts src/subsync/utils.test.ts", "test": "bun run test:config && bun run test:core", "test:config": "bun run test:config:src", "test:launcher": "bun run test:launcher:src", "test:core": "bun run test:core:src", - "test:subtitle": "bun run build && bun run test:subtitle:dist", + "test:subtitle": "bun run test:subtitle:src", "test:fast": "bun run test:config:src && bun run test:core:src", "generate:config-example": "bun run build && bun dist/generate-config-example.js", "start": "bun run build && electron . --start", diff --git a/src/core/services/subsync.test.ts b/src/core/services/subsync.test.ts index e6cb440..28cb7c6 100644 --- a/src/core/services/subsync.test.ts +++ b/src/core/services/subsync.test.ts @@ -276,6 +276,59 @@ test('runSubsyncManual writes deterministic _retimed filename when replace is fa assert.equal(outputPath, path.join(tmpDir, 'episode.ja_retimed.srt')); }); +test('runSubsyncManual reports ffsubsync command failures with details', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subsync-ffsubsync-failure-')); + const ffsubsyncPath = path.join(tmpDir, 'ffsubsync.sh'); + const ffmpegPath = path.join(tmpDir, 'ffmpeg.sh'); + const alassPath = path.join(tmpDir, 'alass.sh'); + const videoPath = path.join(tmpDir, 'video.mkv'); + const primaryPath = path.join(tmpDir, 'primary.srt'); + + fs.writeFileSync(videoPath, 'video'); + fs.writeFileSync(primaryPath, 'sub'); + writeExecutableScript(ffmpegPath, '#!/bin/sh\nexit 0\n'); + writeExecutableScript(alassPath, '#!/bin/sh\nexit 0\n'); + writeExecutableScript(ffsubsyncPath, '#!/bin/sh\necho "reference audio missing" >&2\nexit 1\n'); + + const deps = makeDeps({ + getMpvClient: () => ({ + connected: true, + currentAudioStreamIndex: null, + send: () => {}, + requestProperty: async (name: string) => { + if (name === 'path') return videoPath; + if (name === 'sid') return 1; + if (name === 'secondary-sid') return null; + if (name === 'track-list') { + return [ + { + id: 1, + type: 'sub', + selected: true, + external: true, + 'external-filename': primaryPath, + }, + ]; + } + return null; + }, + }), + getResolvedConfig: () => ({ + defaultMode: 'manual', + alassPath, + ffsubsyncPath, + ffmpegPath, + }), + }); + + const result = await runSubsyncManual({ engine: 'ffsubsync', sourceTrackId: null }, deps); + + assert.equal(result.ok, false); + assert.equal(result.message.startsWith('ffsubsync synchronization failed'), true); + assert.match(result.message, /code=1/); + assert.match(result.message, /reference audio missing/); +}); + test('runSubsyncManual constructs alass command and returns failure on non-zero exit', async () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subsync-alass-')); const alassLogPath = path.join(tmpDir, 'alass-args.log');