test(cli): add subprocess-based unit tests for v0.17.0 task scripts (#293)#326
test(cli): add subprocess-based unit tests for v0.17.0 task scripts (#293)#326MScottAdams merged 3 commits intomasterfrom
Conversation
c31de4b to
decff2a
Compare
Greptile SummaryThis PR adds Confidence Score: 5/5Safe to merge — the only finding is a P2 reliability note about GPG signing in test fixtures, which does not affect CI (task CLI is skipped there) or production code. All findings are P2 or lower. The LINK_CHECK_STRICT isolation concern from prior rounds is correctly resolved. The GPG signing gap in _setup_repo is a local-dev reliability edge case, not a correctness or security issue, and the affected tests are already skip-guarded in CI via @requires_task. tests/cli/test_task_scripts.py — _setup_repo helper (lines 344–360) should add commit.gpgsign=false for GPG-signing environments Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: tests/cli/test_task_scripts.py
Line: 344-360
Comment:
**GPG signing not disabled in test commits**
`_setup_repo` configures `user.email` and `user.name` locally but does not disable `commit.gpgsign`. If a developer's global `~/.gitconfig` has `commit.gpgsign=true`, the `git commit` call (with `check=True`) will raise `CalledProcessError` rather than yielding a clean fixture commit. All four `TestCommitLint` tests would error rather than run.
Since these are ephemeral test-fixture commits, passing `-c commit.gpgsign=false` is the standard guard:
```suggestion
def _setup_repo(self, tmp_path: Path, message: str) -> None:
"""Create a git repo with one commit using the given message."""
subprocess.run(["git", "init"], cwd=str(tmp_path), capture_output=True, check=True)
subprocess.run(
["git", "config", "user.email", "test@test.com"],
cwd=str(tmp_path), capture_output=True, check=True,
)
subprocess.run(
["git", "config", "user.name", "Test"],
cwd=str(tmp_path), capture_output=True, check=True,
)
subprocess.run(
["git", "config", "commit.gpgsign", "false"],
cwd=str(tmp_path), capture_output=True, check=True,
)
(tmp_path / "f.txt").write_text("x", encoding="utf-8")
subprocess.run(["git", "add", "."], cwd=str(tmp_path), capture_output=True, check=True)
subprocess.run(
["git", "commit", "-m", message],
cwd=str(tmp_path), capture_output=True, check=True,
)
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (4): Last reviewed commit: "fix(test): address Greptile round-2 find..." | Re-trigger Greptile |
- P1: flip t3.3.4 SPECIFICATION.md status from [pending] to [completed] -- implementing PR should close its own spec task, not defer to stale-flip PR - P2: add check=True to all subprocess.run calls in _setup_repo -- prevents silent git failures from propagating as cryptic assertion errors - P2: rename test_timeout_handling to test_timeout_parameter_present_in_source -- clarifies the structural-check scope; no behavioral change
- P1: clear LINK_CHECK_STRICT in warning-mode test to prevent env-variable leak -- run_script inherits parent env; setting "" explicitly overrides any set value - P2: remove unused run_task helper (dead code; task tests use inline subprocess.run) - P2: add requires_all_tools guard for test_happy_path_all_tools_present -- @requires_task only checked for task; all 5 tools (go, uv, task, git, gh) now verified
…293) - Create tests/cli/test_task_scripts.py with 25 subprocess-based tests - Cover toolchain-check.py: happy path, missing tool exits, NOT FOUND reporting, timeout - Cover verify-stubs.py: clean source, TODO/FIXME/HACK/bare-pass detection, excluded dirs, encoding - Cover validate-links.py: valid links, broken strict/warning, external URL skip, archive exclusion, --strict - Cover change:init task: directory structure, path traversal rejection, empty name, duplicate - Cover commit:lint task: valid conventional commit, missing type, breaking change, all 11 types - Fill t3.3.4 acceptance criteria in SPECIFICATION.md - Add CHANGELOG entry under [Unreleased] - Coverage at 87.58% (>=85% threshold)
- P1: flip t3.3.4 SPECIFICATION.md status from [pending] to [completed] -- implementing PR should close its own spec task, not defer to stale-flip PR - P2: add check=True to all subprocess.run calls in _setup_repo -- prevents silent git failures from propagating as cryptic assertion errors - P2: rename test_timeout_handling to test_timeout_parameter_present_in_source -- clarifies the structural-check scope; no behavioral change
- P1: clear LINK_CHECK_STRICT in warning-mode test to prevent env-variable leak -- run_script inherits parent env; setting "" explicitly overrides any set value - P2: remove unused run_task helper (dead code; task tests use inline subprocess.run) - P2: add requires_all_tools guard for test_happy_path_all_tools_present -- @requires_task only checked for task; all 5 tools (go, uv, task, git, gh) now verified
3cb711e to
0755c1d
Compare
|
Rebase-only force-push onto updated master (absorbed #298 CHANGELOG/SPECIFICATION changes). No logic changes -- pure rebase. |
Summary
Add subprocess-based unit tests for all v0.17.0 deterministic task scripts.
Closes #293
Changes
Checklist