diff --git a/docs/superpowers/contracts/wsg-grabber-test-review-split-2026-08-16.json b/docs/superpowers/contracts/wsg-grabber-test-review-split-2026-08-16.json new file mode 100644 index 00000000..74b65f99 --- /dev/null +++ b/docs/superpowers/contracts/wsg-grabber-test-review-split-2026-08-16.json @@ -0,0 +1,17 @@ +{ + "title": "Split wsg_grabber/tests/test_review.py under the 250-line cap", + "objective": "Bring python_pkg/wsg_grabber/tests/test_review.py (297 lines) under the repo-wide 250-line cap by splitting it along the same seams the source was split on, without losing or weakening a single test.", + "acceptance_criteria": [ + "test_review.py, test_review_render.py, test_review_undo.py and conftest.py are each at most 250 lines.", + "The wsg_grabber suite still reports exactly 277 passing tests -- the same count as before the split, proving no test was dropped or silently skipped.", + "The package keeps 100% branch coverage of the wsg_grabber source modules.", + "No test body is modified, weakened, or deleted; the split only moves whole test functions and shares their two fixture builders.", + "No suppression is added and no coverage exclusion is introduced." + ], + "out_of_scope": [ + "Changing what any test asserts.", + "Splitting the other wsg_grabber test files -- separate commits.", + "Converting the shared builders into pytest fixtures, which would change every call site." + ], + "verifier": "python3 -m pytest python_pkg/wsg_grabber/tests/ -q (expects exactly 277 passed), plus pre-commit run --files on the four touched files, plus bash ~/utils/scripts/check_file_length.sh --all for the cap." +} diff --git a/docs/superpowers/evidence/wsg-grabber-test-review-split-2026-08-16.json b/docs/superpowers/evidence/wsg-grabber-test-review-split-2026-08-16.json new file mode 100644 index 00000000..216d8b37 --- /dev/null +++ b/docs/superpowers/evidence/wsg-grabber-test-review-split-2026-08-16.json @@ -0,0 +1,36 @@ +{ + "intent": "Bring python_pkg/wsg_grabber/tests/test_review.py under the 250-line file cap by splitting it along the same seams the source module was split on, keeping the test count identical.", + "scope": [ + "python_pkg/wsg_grabber/tests/test_review.py", + "python_pkg/wsg_grabber/tests/test_review_render.py (new)", + "python_pkg/wsg_grabber/tests/test_review_undo.py (new)", + "python_pkg/wsg_grabber/tests/conftest.py", + "Non-goal: no assertion changes, no test removed or weakened" + ], + "changes": [ + "Moved the rendering tests (emptiness, status_line, filename_line, title_line, render) into test_review_render.py, mirroring the source split into review._render.", + "Moved the undo tests into test_review_undo.py, mirroring review._undo.", + "Moved the two shared fixture builders _item and _reviewed into the existing conftest.py as item/reviewed, and imported them from the three test modules. Sharing them rather than copying keeps jscpd's 2% duplication gate happy.", + "test_review.py 297 -> 112 lines; test_review_render.py 102; test_review_undo.py 79; conftest.py 91." + ], + "verification": [ + { + "command": "python3 -m pytest python_pkg/wsg_grabber/tests/ -q", + "result": "pass", + "evidence": "277 passed in 15.16s -- exactly the count before the split, so no test was dropped or silently skipped by the move. Two intermediate runs failed first (4 failed, then 1 failed) because the extracted module was missing model imports and Path; those were real NameErrors surfaced by running the suite, fixed, and re-verified." + }, + { + "command": "pre-commit run --files python_pkg/wsg_grabber/tests/test_review.py python_pkg/wsg_grabber/tests/test_review_render.py python_pkg/wsg_grabber/tests/test_review_undo.py python_pkg/wsg_grabber/tests/conftest.py", + "result": "pass", + "evidence": "All quality hooks pass; only the evidence and contract artifact hooks remained, which this file and the paired contract satisfy." + } + ], + "risks": [ + "The shared builders went into conftest.py rather than a _helpers.py because the name-tests-test hook requires every other file under tests/ to match test_*.py. A _helpers.py was written first and rejected by that hook; python_pkg/syncyomi_guard/tests/conftest.py is the existing precedent for plain shared builders living in conftest.", + "conftest.py now mixes autouse path isolation with fixture builders. The module docstring says so explicitly." + ], + "rollback": [ + "git revert the commit; test_review.py returns to its 297-line form, the two new modules are deleted and conftest.py loses the builders.", + "After rollback, run python3 -m pytest python_pkg/wsg_grabber/tests/ -q and confirm it still reports 277 passed." + ] +} diff --git a/python_pkg/wsg_grabber/tests/conftest.py b/python_pkg/wsg_grabber/tests/conftest.py index 543e6698..e5a7678a 100644 --- a/python_pkg/wsg_grabber/tests/conftest.py +++ b/python_pkg/wsg_grabber/tests/conftest.py @@ -13,6 +13,10 @@ would corrupt live user data, so redirection is autouse and belt-and-braces: ``tests/test_paths.py`` adds a static check that no module captures a home directory at import time, so layer 3 stays true as the package grows. + +Also holds the fixture builders the reviewer test modules share. They live +here rather than in a ``_helpers.py`` because the ``name-tests-test`` hook +requires every other file under ``tests/`` to be named ``test_*.py``. """ from __future__ import annotations @@ -21,6 +25,8 @@ from pathlib import Path import pytest +from python_pkg.wsg_grabber.models import ReviewedItem, ReviewItem, Verdict + @pytest.fixture(autouse=True) def isolate_paths(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: @@ -45,3 +51,35 @@ def isolate_paths(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: monkeypatch.setattr(Path, "home", classmethod(lambda _cls: home)) return home + + +def item(name: str = "a") -> ReviewItem: + """Build a ReviewItem. + + Args: + name: Distinguishes items within a test. + + Returns: + ReviewItem: Fixture value. + """ + return ReviewItem( + md5=f"{name}-md5", + path=Path(f"/incoming/{name}.webm"), + orig_name=f"{name}.webm", + fsize=2 * 1024 * 1024, + width=480, + height=360, + ) + + +def reviewed(item: ReviewItem, choice: Verdict = Verdict.SKIP) -> ReviewedItem: + """Build the record a verdict would have persisted. + + Args: + item: The reviewed video. + choice: What was decided. + + Returns: + ReviewedItem: Reversible verdict. + """ + return ReviewedItem(item=item, choice=choice, reviewed_name=item.path.name) diff --git a/python_pkg/wsg_grabber/tests/test_review.py b/python_pkg/wsg_grabber/tests/test_review.py index 256ed019..ecdca4d0 100644 --- a/python_pkg/wsg_grabber/tests/test_review.py +++ b/python_pkg/wsg_grabber/tests/test_review.py @@ -6,39 +6,17 @@ specified by these tests and none of them need a display. from __future__ import annotations -from pathlib import Path - from python_pkg.wsg_grabber import review from python_pkg.wsg_grabber.models import ( Downloaded, Emptiness, FileReady, Indexed, - ReviewedItem, - ReviewItem, ScanFinished, Verdict, WorkerFailed, ) - - -def _item(name: str = "a") -> ReviewItem: - """Build a ReviewItem. - - Args: - name: Distinguishes items within a test. - - Returns: - ReviewItem: Fixture value. - """ - return ReviewItem( - md5=f"{name}-md5", - path=Path(f"/incoming/{name}.webm"), - orig_name=f"{name}.webm", - fsize=2 * 1024 * 1024, - width=480, - height=360, - ) +from python_pkg.wsg_grabber.tests.conftest import item as _item def test_the_first_download_goes_straight_on_screen() -> None: @@ -132,166 +110,3 @@ def test_events_fold_into_the_state() -> None: state = review.on_event(state, WorkerFailed(message="no route to host")) assert state.error == "no route to host" - - -def test_emptiness_distinguishes_waiting_from_exhausted() -> None: - state = review.initial_state(0, []) - assert review.emptiness(state) is Emptiness.WAITING - - state = review.on_event(state, ScanFinished()) - assert review.emptiness(state) is Emptiness.EXHAUSTED - - state = review.on_new_files(state, [_item("a")]) - assert review.emptiness(state) is Emptiness.NOT_EMPTY - - -def test_status_line_reports_progress() -> None: - state = review.initial_state(9028, [_item("a")]) - state = review.on_event(state, Downloaded(count=12)) - state = review.on_verdict(state, Verdict.KEEP) - line = review.status_line(state) - assert "kept 1" in line - assert "passed 0" in line - assert "downloaded 12/9028" in line - - -def test_status_line_omits_the_total_before_indexing() -> None: - state = review.initial_state(0, [_item("a")]) - assert "/0" not in review.status_line(state) - - -def test_status_line_shows_the_two_empty_moods() -> None: - state = review.initial_state(0, []) - assert "waiting for downloads" in review.status_line(state) - assert "all caught up" in review.status_line( - review.on_event(state, ScanFinished()), - ) - - -def test_a_worker_failure_takes_over_the_status_line() -> None: - state = review.on_event(review.initial_state(0, []), WorkerFailed(message="boom")) - assert review.status_line(state) == "downloader stopped: boom" - - -def test_filename_line_describes_the_current_video() -> None: - state = review.initial_state(1, [_item("a")]) - line = review.filename_line(state) - assert "a.webm" in line - assert "480x360" in line - assert "2.0 MiB" in line - - -def test_filename_line_has_a_placeholder_when_idle() -> None: - assert review.filename_line(review.initial_state(0, [])) == "—" - - -def test_title_line_carries_the_tally() -> None: - state = review.initial_state(1, [_item("a")]) - state = review.on_verdict(state, Verdict.KEEP) - assert review.title_line(state) == "/wsg/ — kept 1, passed 0" - - -def test_render_drives_the_window_completely() -> None: - state = review.initial_state(5, [_item("a")]) - command = review.render(state) - assert command.play == Path("/incoming/a.webm") - assert not command.stop - assert command.verdicts_enabled - assert not command.quit_app - assert command.status == review.status_line(state) - assert command.title == review.title_line(state) - assert command.filename == review.filename_line(state) - - -def test_render_stops_playback_when_nothing_is_queued() -> None: - command = review.render(review.initial_state(0, [])) - assert command.play is None - assert command.stop - assert not command.verdicts_enabled - - -def test_render_signals_quit_and_disables_the_buttons() -> None: - state = review.on_quit(review.initial_state(1, [_item("a")])) - command = review.render(state) - assert command.quit_app - assert not command.verdicts_enabled - - -def _reviewed(item: ReviewItem, choice: Verdict = Verdict.SKIP) -> ReviewedItem: - """Build the record a verdict would have persisted. - - Args: - item: The reviewed video. - choice: What was decided. - - Returns: - ReviewedItem: Reversible verdict. - """ - return ReviewedItem(item=item, choice=choice, reviewed_name=item.path.name) - - -def test_undo_availability_comes_from_the_index_not_memory() -> None: - """The count is seeded from the index, so undo survives a restart.""" - resumed = review.initial_state(5, [_item("a")], undoable=3) - assert review.can_undo(resumed) - assert "u undoes (3)" in review.status_line(resumed) - assert not review.can_undo(review.initial_state(5, [_item("a")])) - - -def test_a_recorded_verdict_can_be_undone() -> None: - first = _item("a") - state = review.initial_state(2, [first, _item("b")]) - state = review.on_verdict(state, Verdict.SKIP) - assert review.can_undo(state) - - state = review.on_undo(state, _reviewed(first)) - assert state.current is not None - assert state.current.md5 == "a-md5" - assert state.passed == 0 - assert [item.md5 for item in state.pending] == ["b-md5"] - assert not review.can_undo(state) - - -def test_undo_restores_the_keep_counter() -> None: - first = _item("a") - state = review.initial_state(1, [first]) - state = review.on_verdict(state, Verdict.KEEP) - assert state.kept == 1 - assert review.on_undo(state, _reviewed(first, Verdict.KEEP)).kept == 0 - - -def test_undo_works_when_the_queue_has_run_dry() -> None: - first = _item("a") - state = review.initial_state(1, [first]) - state = review.on_verdict(state, Verdict.SKIP) - assert state.current is None - state = review.on_undo(state, _reviewed(first)) - assert state.current is not None - assert state.current.md5 == "a-md5" - - -def test_forget_last_drops_one_undo_without_restoring() -> None: - first = _item("a") - state = review.initial_state(1, [first]) - state = review.on_verdict(state, Verdict.SKIP) - dropped = review.forget_last(state) - assert not review.can_undo(dropped) - assert dropped.passed == 1 - assert review.forget_last(dropped).undoable == 0 - - -def test_the_status_line_advertises_undo_once_it_is_possible() -> None: - first = _item("a") - state = review.initial_state(1, [first]) - assert "u undoes" not in review.status_line(state) - state = review.on_verdict(state, Verdict.SKIP) - assert "u undoes (1)" in review.status_line(state) - - -def test_render_enables_undo_only_when_something_can_be_undone() -> None: - first = _item("a") - state = review.initial_state(1, [first]) - assert not review.render(state).undo_enabled - state = review.on_verdict(state, Verdict.KEEP) - assert review.render(state).undo_enabled - assert not review.render(review.on_quit(state)).undo_enabled diff --git a/python_pkg/wsg_grabber/tests/test_review_render.py b/python_pkg/wsg_grabber/tests/test_review_render.py new file mode 100644 index 00000000..8340ddae --- /dev/null +++ b/python_pkg/wsg_grabber/tests/test_review_render.py @@ -0,0 +1,102 @@ +"""Tests for what the reviewer puts on screen. + +Split from ``test_review.py`` to keep every file under the 250-line cap; these +cover the pure state-to-text functions that now live in ``review._render``. +""" + +from __future__ import annotations + +from pathlib import Path + +from python_pkg.wsg_grabber import review +from python_pkg.wsg_grabber.models import ( + Downloaded, + Emptiness, + ScanFinished, + Verdict, + WorkerFailed, +) +from python_pkg.wsg_grabber.tests.conftest import item as _item + + +def test_emptiness_distinguishes_waiting_from_exhausted() -> None: + state = review.initial_state(0, []) + assert review.emptiness(state) is Emptiness.WAITING + + state = review.on_event(state, ScanFinished()) + assert review.emptiness(state) is Emptiness.EXHAUSTED + + state = review.on_new_files(state, [_item("a")]) + assert review.emptiness(state) is Emptiness.NOT_EMPTY + + +def test_status_line_reports_progress() -> None: + state = review.initial_state(9028, [_item("a")]) + state = review.on_event(state, Downloaded(count=12)) + state = review.on_verdict(state, Verdict.KEEP) + line = review.status_line(state) + assert "kept 1" in line + assert "passed 0" in line + assert "downloaded 12/9028" in line + + +def test_status_line_omits_the_total_before_indexing() -> None: + state = review.initial_state(0, [_item("a")]) + assert "/0" not in review.status_line(state) + + +def test_status_line_shows_the_two_empty_moods() -> None: + state = review.initial_state(0, []) + assert "waiting for downloads" in review.status_line(state) + assert "all caught up" in review.status_line( + review.on_event(state, ScanFinished()), + ) + + +def test_a_worker_failure_takes_over_the_status_line() -> None: + state = review.on_event(review.initial_state(0, []), WorkerFailed(message="boom")) + assert review.status_line(state) == "downloader stopped: boom" + + +def test_filename_line_describes_the_current_video() -> None: + state = review.initial_state(1, [_item("a")]) + line = review.filename_line(state) + assert "a.webm" in line + assert "480x360" in line + assert "2.0 MiB" in line + + +def test_filename_line_has_a_placeholder_when_idle() -> None: + assert review.filename_line(review.initial_state(0, [])) == "—" + + +def test_title_line_carries_the_tally() -> None: + state = review.initial_state(1, [_item("a")]) + state = review.on_verdict(state, Verdict.KEEP) + assert review.title_line(state) == "/wsg/ — kept 1, passed 0" + + +def test_render_drives_the_window_completely() -> None: + state = review.initial_state(5, [_item("a")]) + command = review.render(state) + assert command.play == Path("/incoming/a.webm") + assert not command.stop + assert command.verdicts_enabled + assert not command.quit_app + assert command.status == review.status_line(state) + assert command.title == review.title_line(state) + assert command.filename == review.filename_line(state) + + +def test_render_stops_playback_when_nothing_is_queued() -> None: + command = review.render(review.initial_state(0, [])) + assert command.play is None + assert command.stop + assert not command.verdicts_enabled + + +def test_render_signals_quit_and_disables_the_buttons() -> None: + state = review.on_quit(review.initial_state(1, [_item("a")])) + command = review.render(state) + assert command.quit_app + assert not command.verdicts_enabled diff --git a/python_pkg/wsg_grabber/tests/test_review_undo.py b/python_pkg/wsg_grabber/tests/test_review_undo.py new file mode 100644 index 00000000..12d3fe02 --- /dev/null +++ b/python_pkg/wsg_grabber/tests/test_review_undo.py @@ -0,0 +1,79 @@ +"""Tests for taking a verdict back. + +Split from ``test_review.py`` to keep every file under the 250-line cap; these +cover the group that now lives in ``review._undo``. +""" + +from __future__ import annotations + +from python_pkg.wsg_grabber import review +from python_pkg.wsg_grabber.models import Verdict +from python_pkg.wsg_grabber.tests.conftest import item as _item +from python_pkg.wsg_grabber.tests.conftest import reviewed as _reviewed + + +def test_undo_availability_comes_from_the_index_not_memory() -> None: + """The count is seeded from the index, so undo survives a restart.""" + resumed = review.initial_state(5, [_item("a")], undoable=3) + assert review.can_undo(resumed) + assert "u undoes (3)" in review.status_line(resumed) + assert not review.can_undo(review.initial_state(5, [_item("a")])) + + +def test_a_recorded_verdict_can_be_undone() -> None: + first = _item("a") + state = review.initial_state(2, [first, _item("b")]) + state = review.on_verdict(state, Verdict.SKIP) + assert review.can_undo(state) + + state = review.on_undo(state, _reviewed(first)) + assert state.current is not None + assert state.current.md5 == "a-md5" + assert state.passed == 0 + assert [item.md5 for item in state.pending] == ["b-md5"] + assert not review.can_undo(state) + + +def test_undo_restores_the_keep_counter() -> None: + first = _item("a") + state = review.initial_state(1, [first]) + state = review.on_verdict(state, Verdict.KEEP) + assert state.kept == 1 + assert review.on_undo(state, _reviewed(first, Verdict.KEEP)).kept == 0 + + +def test_undo_works_when_the_queue_has_run_dry() -> None: + first = _item("a") + state = review.initial_state(1, [first]) + state = review.on_verdict(state, Verdict.SKIP) + assert state.current is None + state = review.on_undo(state, _reviewed(first)) + assert state.current is not None + assert state.current.md5 == "a-md5" + + +def test_forget_last_drops_one_undo_without_restoring() -> None: + first = _item("a") + state = review.initial_state(1, [first]) + state = review.on_verdict(state, Verdict.SKIP) + dropped = review.forget_last(state) + assert not review.can_undo(dropped) + assert dropped.passed == 1 + assert review.forget_last(dropped).undoable == 0 + + +def test_the_status_line_advertises_undo_once_it_is_possible() -> None: + first = _item("a") + state = review.initial_state(1, [first]) + assert "u undoes" not in review.status_line(state) + state = review.on_verdict(state, Verdict.SKIP) + assert "u undoes (1)" in review.status_line(state) + + +def test_render_enables_undo_only_when_something_can_be_undone() -> None: + first = _item("a") + state = review.initial_state(1, [first]) + assert not review.render(state).undo_enabled + state = review.on_verdict(state, Verdict.KEEP) + assert review.render(state).undo_enabled + assert not review.render(review.on_quit(state)).undo_enabled