Skip to content

Add test suite (75 tests) + fix platform-neutral install hints#1

Open
gatosa12 wants to merge 8 commits into
bradautomates:mainfrom
gatosa12:main
Open

Add test suite (75 tests) + fix platform-neutral install hints#1
gatosa12 wants to merge 8 commits into
bradautomates:mainfrom
gatosa12:main

Conversation

@gatosa12

Copy link
Copy Markdown

What this PR does

This is the first test suite for the project, plus two bug fixes for Linux/Windows users.

New: tests/ — 75 unit tests (no external binaries required)

File Tests Covers
tests/test_transcribe.py 22 parse_vtt, _dedupe, filter_range, format_transcript
tests/test_frames.py 25 parse_time, format_time, _clamp_fps, auto_fps, auto_fps_focus
tests/test_download.py 28 is_url, resolve_local, _pick_subtitle, _pick_video, VIDEO_EXTS

All tests run with just pip install pytest — no ffmpeg, yt-dlp, or API keys needed.

New: .github/workflows/tests.yml — CI workflow

Runs the test suite on every push/PR against Python 3.10, 3.11, and 3.12.

Bug fixes: platform-neutral install hints

scripts/download.py — the error message when yt-dlp isn't found previously said brew install yt-dlp on all platforms. Fixed to show the right command per OS.

scripts/frames.py — same issue in two places for ffmpeg/ffprobe. Fixed to show macOS / Linux / Windows install commands.

How to run tests locally

pip install pytest
pytest tests/ -v

Add tests for VTT parsing logic in transcribe.py
This file contains unit tests for the time parsing and FPS budget logic in scripts/frames.py. It includes tests for parse_time, format_time, _clamp_fps, auto_fps, and auto_fps_focus functions.
…for URL detection and file resolutionnctionality

This file contains tests for the download module, including URL detection, local file resolution, and subtitle/video file picking logic. The tests cover various scenarios for each function, ensuring correct behavior.
Add .github/workflows/tests.yml: CI runs pytest on Python 3.10-3.12
…yt-dlp install hint (not just brew)essage

Updated error message for missing yt-dlp installation to include installation instructions for both macOS and Linux/Windows.
…eg install hints (not just brew)ation instructions

@gatosa12 gatosa12 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Great first test suite! Here's my full review:

✅ What's solid

tests/test_transcribe.py (22 tests)

  • Good coverage of parse_vtt edge cases: HTML tag stripping, rolling/identical deduplication, comma decimal separators, and empty files.
    • The _vtt() helper keeps fixtures concise.
      • TestDedupe and TestFilterRange test the pure functions cleanly in isolation.
        • TestFormatTranscript roundtrip via format_timeparse_time is a nice sanity check.
          tests/test_frames.py (25 tests)
  • TestParseTime covers all timestamp formats (MM:SS, HH:MM:SS, decimal variants) and passthrough of numeric types.
    • TestAutoFpsFocus.test_denser_than_auto_fps_for_short_clips is a good behavioral assertion (not just a value check).
      • fail-fast: false in the CI matrix is the right call — you want to see all Python version failures at once.
        tests/test_download.py (28 tests)
  • TestIsUrl covers platform-specific paths (Windows backslash path, tilde, bare filename) — these are the cases most likely to regress.
    • TestPickSubtitle tests the en preference logic and the en-US/en-GB variant handling correctly.
      scripts/download.py & scripts/frames.py bug fixes
  • Platform-neutral install hints are correct and consistent across both files.
    • The auto_fps_focus docstring fix (em-dash → hyphen) is a minor but appreciated cleanup.

⚠️ Suggestions / things to watch

  1. test_unknown_extension_warns_but_proceeds may be fragile. The test asserts "warning" in captured.err.lower() or "xyz" in captured.err. If resolve_local doesn't currently print to stderr for unknown extensions, this test will silently pass on the or branch without actually validating the warning. Consider splitting into two separate assertions or using pytest.warns if a warning is raised.
  2. test_tilde_expansion doesn't actually test tilde expansion. The comment says "Monkeypatch Path.expanduser to return tmp_path / 'video.mp4'" but the test just passes a real absolute path. It would be more valuable to call resolve_local("~/video.mp4") with a monkeypatched expanduser, or remove the test and rename it to test_absolute_path_works.
  3. TestAutoFps.test_30_second_boundary uses a hard-coded expected max target of 30. If the fps budget algorithm changes, this test breaks without a clear explanation of why 30 is expected. Consider adding a brief comment explaining the intent (e.g., "30s clip should get at most 1 fps").
  4. CI workflow only runs on ubuntu-latest. The bug fixes are specifically about macOS/Windows install hints. It might be worth adding a windows-latest runner row to actually exercise the Windows code path — even if just for the platform-neutral error message test.
  5. No conftest.py or __init__.py in tests/. This is fine for now with pytest, but worth noting for future contributors. A conftest.py could house the _vtt() helper if more test files need it.

Summary

This is a clean, well-structured contribution. The tests are readable, isolated, and don't require any external binaries — exactly right for a CI-first setup. The bug fixes are correct. The suggestions above are minor; none are blockers. Ready to merge once CI passes.

Fix test failure: auto_fps_focus(3.0) was returning only 6 frames instead of the expected >=10. The _clamp_fps function was recalculating target after capping fps to MAX_FPS, which reduced the frame count. Now it preserves the originally intended target frame count while still capping the fps.
Fix indentation error from previous commit. Also fixes the frame calculation bug where auto_fps_focus(3.0) returned only 6 frames instead of >=10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant