test(cli): assert exact fallback value in _get_current_version test#35
Merged
cmeans-claude-dev[bot] merged 2 commits intoApr 22, 2026
Merged
Conversation
Closes #32. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cmeans
reviewed
Apr 22, 2026
Owner
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #35
Verdict: QA Failed
One substantive finding blocks signoff (missing CHANGELOG entry). The test tightening itself is correct and matches issue #32 exactly.
Verification
- Diff is exactly scoped to the issue:
tests/test_cli.py+2/-1 — adds thecli_modimport and replacesisinstance(..., str)with== cli_mod.__version__. Nothing else touched. - Fallback contract is tested meaningfully now.
monkeypatch.setattr(importlib.metadata, "version", boom)forces theexceptbranch in_get_current_version()atsrc/yt_dont_recommend/cli.py:49. That branch is documented toreturn __version__, which is re-imported intocli.pyfromconfig.py:88(__version__: str = _resolve_version()). Asserting equality withcli_mod.__version__means a regression likereturn "error"orreturn ""would now fail the test — whereas the oldisinstance(..., str)passed for any string. - Mirrors the analogous tightening in
tests/test_config.py::TestResolveVersion::test_falls_back_to_0_0_0_when_metadata_unavailable(asserts exact"0.0.0"), which was the precedent called out in issue #32. Philosophy consistency looks good. - Tests —
tests/test_cli.py::TestVersionHelpers→ 6/6 PASSED in 0.08s; full suite → 449 passed in 33.97s. Both match the PR body. - CI all required checks green on
test/cli-version-fallback-exact-value.
Findings
| # | Finding | Severity |
|---|---|---|
| 1 | No CHANGELOG.md ## [Unreleased] entry for this PR. Project convention is that every PR — test-only included — adds an Unreleased entry, and new tests go under ### Added. The existing ### Added — Test coverage push to 100% on pure-logic modules entry describes PR #30's bulk coverage work, not this follow-on tightening of the _get_current_version fallback assertion (closes #32). Suggested: add a ### Added entry along the lines of "Tightened TestVersionHelpers::test_get_current_version_falls_back_on_metadata_failure from isinstance(str) to equality with cli.__version__, mirroring the exact-value assertion already used for _resolve_version's "0.0.0" fallback in TestResolveVersion (closes #32)." |
substantive |
Notes
- No observations / nits beyond the CHANGELOG gap.
- Once the Unreleased entry lands, I'd re-run
tests/test_cli.py::TestVersionHelpers -v+ full suite on the refreshed branch and promote toReady for QA Signoffassuming no regressions.
…ning Addresses QA feedback on #35. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cmeans
reviewed
Apr 22, 2026
Owner
cmeans
left a comment
There was a problem hiding this comment.
QA Review — Round 2 on PR #35
Verdict: Ready for QA Signoff
Round-1 finding addressed; re-verification clean.
Re-verification
- CHANGELOG
## [Unreleased] → ### Addedentry added — closes #32, explains the weaken-to-strict motivation and cross-references theTestResolveVersionprecedent. Correct section and category for a test addition. - Test diff unchanged from round 1 (still
+2/-1intests/test_cli.py) —assert _get_current_version() == cli_mod.__version__is the documented fallback contract; would fail if a regression changed theexceptbranch to return anything other thancli.__version__. - Tests:
tests/test_cli.py::TestVersionHelpers -v→ 6/6 PASSED in 0.08s. Full suite → 449 passed in 33.96s. - Lint:
.venv/bin/ruff check src/ tests/→ all checks passed. - CI green on the refreshed branch; mergeable.
Promoting to Ready for QA Signoff. Maintainer can apply QA Approved.
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.
Summary
Tighten `TestVersionHelpers::test_get_current_version_falls_back_on_metadata_failure` from `isinstance(_get_current_version(), str)` to `_get_current_version() == cli_mod.version`. The old assertion passed for any string — it would not catch a regression that silently returned `"error"`, `""`, or any other value from the fallback branch.
Matches the shape already adopted in `tests/test_config.py::TestResolveVersion::test_falls_back_to_0_0_0_when_metadata_unavailable`, consistent with PR #30's "the number should mean what it claims" philosophy.
Closes #32.
Test plan