ci: run tests and the frontend build on pull requests - #89
Merged
Merged
Conversation
The Run Linters workflow is the only required check, and it ran neither
the test suite nor a build. tests/ existed but executed nowhere in CI,
and the frontend was built solely inside the Docker image — so a broken
frontend dependency surfaced only after merge, when the publish workflow
ran, and a backend regression surfaced only when a job hit it.
That left mypy as the sole guard over a weekly stream of automated
dependency bumps. Pin drift announces itself loudly; this class of
failure does not.
Both additions are cheap: 35 tests in ~1s, the build in ~2s. The runtime
dependencies they need are already installed, since requirements-lint.txt
starts with `-r requirements.txt`; only pytest itself was missing.
Because docker-publish.yml invokes this workflow via workflow_call before
building, tests and build now gate releases as well as pull requests.
Also drops both halves of the pre-commit.ci integration, neither of which
has ever done anything:
- the `ci:` block is read only by the hosted pre-commit.ci app, which
is not installed on this repository
- pre-commit-ci/lite-action cannot push, because the repository's
default workflow token is read-only
Commit e653f0d — a formatting fix applied by hand — is what the autofix
was supposed to have handled. Config that looks functional but is not is
worse than no config; autofix can be added deliberately later if wanted.
The first CI run of the new test step failed with pytest exit code 4 — a usage error, not a test failure. pyproject's addopts passes --timeout=20, which the pytest-timeout plugin provides, and that plugin was declared nowhere: not in requirements-lint.txt, not in the dev extras. It happened to be present in a local venv, so `pytest tests/` worked for whoever had installed it by hand and for nobody else. Adding tests to CI surfaced this on its first run, which is the argument for having them there. Declared in both places that claim to describe the test environment, and verified in a venv built only from requirements-lint.txt: 35 passed.
The test step's second failure was two real assertion failures, not a usage error: tests/test_container_extension.py and the subtitle cleanup test invoke real ffprobe and ffmpeg against fixtures in tests/fixtures/ rather than mocking the subprocess. GitHub's runner has neither binary, so both failed with a bare ENOENT. Installing FFmpeg is the honest fix — this project is an FFmpeg wrapper, and those two are among the most valuable tests in the suite precisely because they exercise the real thing. Stubbing them out to get a green check would remove the coverage that justifies running tests in CI. Adds roughly 20-30s to the job. AGENTS.md now records the requirement, since a contributor without FFmpeg sees the same two failures locally.
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.
Closes the gap where CI could go green on a change that broke the app.
The problem
Run Linters is the only required check, and it ran neither the test suite nor a build:
tests/exists — 35 tests — and executed nowhere in CI. Its only mention anywhere was in.github/copilot-instructions.md.That left
mypyas the sole guard over a weekly stream of automated dependency bumps. Pin drift is loud and self-announcing; this class of failure is silent.What changed
Two steps added after mypy:
Both are cheap — 35 tests in ~1.2s, the build in ~2.0s. Neither needed setup work:
npm ciandsvelte-kit syncalready run earlier in the job, andrequirements-lint.txtstarts with-r requirements.txt, so every runtime dependency the tests import was already installed. Onlypytestitself was missing, hence the one-line addition.Since docker-publish.yml invokes this workflow via
workflow_callbefore building, tests and build now gate releases too, not just PRs.Removing dead pre-commit.ci config
Both halves of the integration are inert, and have always been:
ci:block in.pre-commit-config.yamlgithub-actionspre-commit-ci/lite-actionstepdefault_workflow_permissionsisreadCommit e653f0d — "fix: ruff-format scripts/fix_container_mismatches.py", applied by hand — is precisely what the autofix was supposed to have handled.
Deleted rather than repaired. Granting workflow write permissions to revive an autofix nobody has missed is a real decision with a security dimension, and it should be made deliberately, not inherited from config that looked like it was working. Easy to add back if wanted.
Verification
pre-commit run --all-files— all hooks passpytest tests/ --ignore=tests/test_workers.py— 35 passed in 1.22snpm run build— exit 0, adapter-static writes cleanlyactionlint(in the pre-commit suite) passes on the edited workflowAGENTS.mdis updated in the same PR per #84: a new "What CI actually runs" section, since the old text named mypy as the extra step beyond pre-commit and that is no longer true.