feat(build): run unit tests in build via pytest#227
Open
scottschreckengaust wants to merge 3 commits into
Open
feat(build): run unit tests in build via pytest#227scottschreckengaust wants to merge 3 commits into
scottschreckengaust wants to merge 3 commits into
Conversation
Closes #226 (framework plumbing). The repo had no test task wired into CI — `mise run build` ran lint/fmt/validate/security but no tests, and the one existing test file (test_safe_query.py) was never invoked. Add the plumbing: - tools/run-tests.py — discovers `test_*.py` under tools/ and plugins/, runs each via `uv run`, exits non-zero if any fail. No test-runner dependency: each test file is self-contained (# /// script header + pytest shim + __main__ runner), so `uv run <file>` executes it standalone. Swappable for a single `pytest` call if pytest is adopted repo-wide later. - mise.toml — add a `test` task and insert it into the `build` chain (lint → fmt:check → validate → test → security). - AGENTS.md — document `mise run test` in the dev-commands list. Local/remote parity is automatic: CI's build job runs `mise run build` via mise-action, so the exact same command gates locally and in CI — no separate workflow needed. Verified: discovery finds test_safe_query.py (63 assertions pass); a failing test makes the runner exit non-zero (real gate); `mise run build` runs the test step and is bandit/lint/fmt-clean. Discovery excludes `.tmp`/node_modules by path relative to repo root (so it works from the repo's `.tmp/<name>` worktrees). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the 91-line tools/run-tests.py discovery/aggregation script with pytest, which already provides discovery, per-file execution, and result aggregation — and correctly collects the parametrized/fixture tests the custom runner's fallback shim silently skipped (65 vs 63 passing). - test:py runs `uv run --with pytest pytest tools plugins`. `--with` installs pytest into an ephemeral cached env for the run only, so no dependency is added to the repo (the reason the custom shim existed). Exit code 5 (no tests collected) is treated as success. - test:js runs `npm test` only when a package.json defines a test script; otherwise skips. Room for jest/other runners without a bespoke script. - test aggregates the per-language tasks; still gated in the build chain. - Delete tools/run-tests.py; update AGENTS.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
892ac08 to
6079ff4
Compare
mise run buildName test tasks by runtime for clarity: test:python (pytest) and test:node (npm/jest — covers JS and TS, which share the Node toolchain). No behavioral change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
theagenticguy
approved these changes
Jul 9, 2026
theagenticguy
left a comment
Contributor
There was a problem hiding this comment.
✅ Approve
Small, correct build-plumbing change (+27/−2 in mise.toml + AGENTS.md). Reviewed the full diff and verified the PR's claims against the committed head branch and CI config rather than the description alone.
Why this is a solid approve:
- Correct exit-code handling.
uv run --with pytest pytest tools plugins || [ $? -eq 5 ]masks only pytest's exit 5 (no tests collected) as success. Real failures (exit 1) and collection/usage errors (2–4) still propagate and break the build. - No new repo dependency.
--with pytestinstalls into an ephemeral cached env for the run only — the whole reason the earlier custom shim existed, now unnecessary. - Standard runner over bespoke code. pytest handles discovery/parametrize/fixtures natively; the deleted 91-line shim's fallback silently skipped parametrized/fixture tests (why the passing count rises 63→65).
- CI is genuinely exercised. The
buildjob runsmise run build, so the newteststep actually executes in CI (passed at 2m32s), and the mutation guard confirms the step writes nothing. All checks green. - Docs consistent. Edits land in
AGENTS.md(CLAUDE.mdis a symlink to it). The updated linelint + fmt:check + validate + test + securityalso corrects a pre-existing omission —validatewas already in the chain but undocumented.
Parser-covering tests are a documented fast-follow (#220), so their absence here is by design.
Non-blocking nits (fine to merge as-is):
- The PR body names the tasks
test:py/test:js, but the committed names aretest:python/test:node— description drift only. test:node'sgrep -q '"test"'is a loose match againstpackage.json;jq -e '.scripts.test'would be exact. Zero impact today (nopackage.json, so it always skips cleanly) — worth tightening only if a plugin ever ships one.
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
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.
Wires unit tests into the build so they run locally and in CI alike, using each language's standard test runner.
Related
mise run build#226 — establishes the test plumbing (a first test to cover the parser from feat(security): vendor Semgrep r/all ruleset with tracked exclusions #220 is a follow-up).Problem
mise run buildran lint / fmt / validate / security but no tests. The one existing test file (tools/evals/databases-on-aws/dsql/scripts/test_safe_query.py) was never invoked, so Python logic undertools/could regress silently.Approach
Use the ecosystem's standard runners rather than a bespoke script:
test:py→uv run --with pytest pytest tools plugins. pytest already does discovery, per-file execution, and result aggregation.--withinstalls pytest into an ephemeral cached env for the run only, so no dependency is added to the repo — which was the whole reason the earlier custom shim existed. Exit code 5 (no tests collected) is treated as success.test:js→npm testonly when apackage.jsondefines a test script; otherwise skips. Leaves room for jest/other runners with no custom code.testaggregates the per-language tasks and stays in the build chain:lint → fmt:check → validate → test → security.Why not a custom runner?
An earlier revision added a 91-line
tools/run-tests.pythat reimplemented discovery/aggregation/exclusion. pytest does all of that natively — and better: the custom runner's no-pytest fallback shim silently skipped parametrized and fixture-based tests. Switching to pytest raises the passing count from 63 → 65 while deleting ~91 lines.Verification
mise run test→test:py65 passed;test:jsskips cleanly (no package.json).mise run buildruns theteststep; lint / fmt:check pass.Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.
Generated with Claude Code