From e03fbd17e967f2e46614b8490beba23d6faa28dc Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sun, 16 Aug 2026 16:24:48 +0200 Subject: [PATCH] Split wsg_grabber/tests/test_review.py under the 250-line cap Split along the same seams the source took: test_review_render.py covers what now lives in review._render, test_review_undo.py covers review._undo. The two fixture builders they share moved into the existing conftest.py rather than a _helpers.py -- the name-tests-test hook requires everything else under tests/ to be named test_*.py, and syncyomi_guard/tests/conftest.py is the precedent for plain shared builders living there. Sharing beats copying: jscpd fails the commit above 2% duplication and this repo already carries four partN test files. 277 passed, the same count as before, so nothing was dropped. Two intermediate runs caught real NameErrors from imports the extracted module needed. test_review.py 297 -> 112, test_review_render.py 102, test_review_undo.py 79. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011w2Pj4Ek3ZNMQVYcFHgXDY --- ...-grabber-test-review-split-2026-08-16.json | 17 ++ ...-grabber-test-review-split-2026-08-16.json | 36 ++++ python_pkg/wsg_grabber/tests/conftest.py | 38 ++++ python_pkg/wsg_grabber/tests/test_review.py | 187 +----------------- .../wsg_grabber/tests/test_review_render.py | 102 ++++++++++ .../wsg_grabber/tests/test_review_undo.py | 79 ++++++++ 6 files changed, 273 insertions(+), 186 deletions(-) create mode 100644 docs/superpowers/contracts/wsg-grabber-test-review-split-2026-08-16.json create mode 100644 docs/superpowers/evidence/wsg-grabber-test-review-split-2026-08-16.json create mode 100644 python_pkg/wsg_grabber/tests/test_review_render.py create mode 100644 python_pkg/wsg_grabber/tests/test_review_undo.py 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