🧪 test: Add unit tests for get_version fallback mechanisms#219
🧪 test: Add unit tests for get_version fallback mechanisms#219bashandbone wants to merge 4 commits intomainfrom
Conversation
Added comprehensive unit tests for `get_version` in `tests/unit/test_init.py`. The tests verify that all mechanisms to retrieve the package version function as intended: - Reading `codeweaver._version.__version__` - Using `importlib.metadata.version` - Falling back to `git describe` - Handling missing `git` command - Handling broader unexpected exceptions All fallback scenarios now properly ensure the code returns the default `0.0.0` string or the valid version. No functional code was changed, only new tests were introduced. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdds comprehensive unit tests for the get_version() function to cover all fallback paths (version file, importlib.metadata, git-based resolution, absence/failure of git, and generic exceptions), using monkeypatch and mocks to simulate each scenario deterministically. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In tests where you want
codeweaver._versionto be absent, consider usingmonkeypatch.delitem(sys.modules, 'codeweaver._version', raising=False)instead of setting it toNoneto more accurately reflect the module not being importable. - There is quite a bit of repeated setup for mocking
sys.modules['codeweaver._version']andimportlib.metadata.version; extracting this into a small helper or pytest fixture would reduce duplication and make the intent of each test clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In tests where you want `codeweaver._version` to be absent, consider using `monkeypatch.delitem(sys.modules, 'codeweaver._version', raising=False)` instead of setting it to `None` to more accurately reflect the module not being importable.
- There is quite a bit of repeated setup for mocking `sys.modules['codeweaver._version']` and `importlib.metadata.version`; extracting this into a small helper or pytest fixture would reduce duplication and make the intent of each test clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Adds unit tests to cover codeweaver.get_version() across its primary path and multiple fallback/error paths to close a testing gap around exception-driven version resolution.
Changes:
- Introduces unit tests for version resolution via
_version,importlib.metadata, andgit describe. - Adds tests for git-related failure modes (non-zero exit, missing git, unexpected exception).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| from codeweaver import get_version | ||
|
|
||
| def test_get_version_from_version_file(monkeypatch): |
|
|
||
| assert get_version() == "1.2.3-file" | ||
|
|
||
| def test_get_version_from_importlib_metadata(monkeypatch): |
|
|
||
| assert get_version() == "1.2.3-metadata" | ||
|
|
||
| def test_get_version_from_git_success(monkeypatch): |
|
|
||
| assert get_version() == "0.0.0" | ||
|
|
||
| def test_get_version_no_git(monkeypatch): |
|
|
||
| assert get_version() == "0.0.0" | ||
|
|
||
| def test_get_version_exception(monkeypatch): |
tests/unit/test_init.py
Outdated
| # Mock codeweaver._version to not exist | ||
| monkeypatch.setitem(sys.modules, "codeweaver._version", None) | ||
|
|
||
| # Mock importlib.metadata.version to raise PackageNotFoundError | ||
| def mock_version(pkg_name): | ||
| raise importlib.metadata.PackageNotFoundError("code-weaver") | ||
| monkeypatch.setattr(importlib.metadata, "version", mock_version) |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review: Test Coverage for get_version Function
Thank you for adding comprehensive test coverage for the get_version function! This is valuable work that tests all the fallback scenarios. However, there are several issues that need to be addressed before this can be merged.
Critical Issue: Missing Copyright Headers ❌
The CI is failing because the new test file is missing the required copyright and licensing information. This is blocking the merge.
Required Fix:
Add the following header at the top of tests/unit/test_init.py:
# SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0This must come before all other code (including imports).
Code Quality Issues
1. Import Organization
Missing from __future__ import annotations which is used consistently across the codebase. Add this as the first import after the copyright header.
2. Test Function Isolation Issues
The tests have a critical flaw: they're modifying sys.modules["codeweaver._version"] but the get_version() function has already been called at module import time (line 75 in __init__.py). This means:
- The mocking in tests won't affect the already-cached
__version__value - Tests may pass due to timing/ordering but don't truly test the fallback logic
- The function needs to be tested in a way that accounts for module-level caching
Recommendation: Consider whether these tests actually validate the intended behavior, or if a different testing approach is needed (e.g., testing the function before it's cached, or refactoring to make it more testable).
3. Inconsistent Style
- Lines 62-67: Inconsistent blank line spacing compared to other test functions
- Consider using
pytest.fixturefor common mocking setups to reduce duplication
4. Missing Type Annotations
While not required for test functions, parameter types could improve clarity (e.g., monkeypatch: pytest.MonkeyPatch).
Test Coverage Assessment ✅
Positive aspects:
- All 6 fallback paths are tested
- Good use of pytest's
monkeypatchfixture - Clear docstrings for each test
- Appropriate use of mocks to avoid external dependencies
Coverage scenarios tested:
- ✅ Happy path (_version module)
- ✅ importlib.metadata fallback
- ✅ git describe success
- ✅ git describe failure
- ✅ Missing git executable
- ✅ Exception handling
Security Considerations
The tests appropriately mock subprocess calls, which is good practice. No security concerns identified.
Recommendations
- Immediate: Add the copyright header to fix the failing CI
- Important: Address the test isolation issue - the current tests may not be testing what they appear to test due to module-level caching
- Nice to have: Add
from __future__ import annotations, clean up formatting inconsistencies
Verdict
Request Changes - The PR cannot be merged due to:
- Failing CI checks (missing copyright header)
- Potential test isolation issues that may lead to false positives
Once the copyright header is added and the test isolation issue is addressed, this will be a valuable addition to the test suite.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds unit tests to cover codeweaver.get_version() resolution order and its fallback behavior across multiple failure modes.
Changes:
- Introduces a new unit test module covering version retrieval via
_version,importlib.metadata, andgit describe. - Adds tests for git failures, missing git executable, and exception-driven fallback to
"0.0.0".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return "1.2.3-metadata" | ||
| raise importlib.metadata.PackageNotFoundError("code-weaver") | ||
|
|
||
| monkeypatch.setattr(importlib.metadata, "version", mock_version) |
There was a problem hiding this comment.
The Copilot reviewer is correct about this issue. The problem is that get_version() uses lazy imports - it imports importlib.metadata, shutil, and subprocess inside the function at runtime (see lines 46, 51-52 in src/codeweaver/init.py:37-72).
When you patch shutil.which or importlib.metadata.version at the stdlib module level, the import shutil statement inside get_version() creates a fresh local reference that may not see your monkeypatch.
The fix: Patch these imports where they're actually used. Since get_version() is in codeweaver module, you need to patch them there. However, since these are lazy imports (imported inside the function), the best approach is to:
- For
importlib.metadata.version: Patch it atimportlib.metadata.versionlevel (this should work since it's called asimportlib.metadata.version()) - For
shutil.whichandsubprocess.run: These are trickier because they're imported inside the function
Recommended approach:
Use unittest.mock.patch with the actual import path. For lazy imports inside functions, you need to patch at the point where the module is imported. Here's an example pattern:
from unittest.mock import patch
def test_get_version_from_git_success():
with patch('codeweaver._version.__version__', side_effect=ImportError):
with patch('importlib.metadata.version', side_effect=importlib.metadata.PackageNotFoundError):
with patch('shutil.which', return_value='/usr/bin/git'):
with patch('subprocess.run') as mock_run:
mock_run.return_value.returncode = 0
mock_run.return_value.stdout = '1.2.3-git\n'
assert get_version() == '1.2.3-git'Alternatively, since the imports happen at runtime inside the function, you could use monkeypatch.setattr on the actual module objects before they're imported, or use importlib machinery to ensure your mocks are in place.
The key insight: lazy imports (imports inside functions) require different mocking strategies than module-level imports.
| monkeypatch.setattr(importlib.metadata, "version", mock_version) | ||
|
|
||
| # Mock shutil.which to return a git path | ||
| monkeypatch.setattr(shutil, "which", lambda cmd: "/usr/bin/git" if cmd == "git" else None) |
| return mock_result | ||
| return MagicMock(returncode=1) | ||
|
|
||
| monkeypatch.setattr(subprocess, "run", mock_run) |
tests/unit/test_init.py
Outdated
| monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False) | ||
| def mock_version(pkg_name): | ||
| raise importlib.metadata.PackageNotFoundError("code-weaver") | ||
| monkeypatch.setattr(importlib.metadata, "version", mock_version) |
| def test_get_version_exception(monkeypatch): | ||
| """Test general exception in git block returns 0.0.0.""" |
tests/unit/test_init.py
Outdated
| # Mock shutil.which to raise an exception | ||
| def mock_which(cmd): | ||
| raise RuntimeError("Something went wrong") | ||
| monkeypatch.setattr(shutil, "which", mock_which) |
There was a problem hiding this comment.
Code Review Findings
Thank you for adding comprehensive test coverage for the get_version function! The test scenarios are thorough and cover all the fallback paths. However, I've identified several issues that need to be addressed:
Critical Issues
1. Test Isolation Problem
The current test implementation has a fundamental issue with module import timing:
- Line 7:
from codeweaver import get_versiontriggers the import ofcodeweaver.__init__ - When
__init__.pyloads, line 75 executes:__version__: Final[str] = get_version() - This means
get_version()runs before any test can set up mocks - The module-level
__version__is already computed and cached with the real environment
Impact: Tests like test_get_version_from_version_file are setting up mocks AFTER the import has already happened, which may not test what's intended.
Suggested Fix: Consider using importlib.import_module within each test to reload the module, or mock at a different level. Alternatively, import the function lazily or use pytest fixtures to ensure proper isolation.
2. Missing SPDX Copyright Headers
All test files in this repository include SPDX headers (see tests/unit/ui/test_unified_ux.py:1-4 or tests/unit/conftest.py:1-4). Please add:
# SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0Minor Issues
3. Missing from __future__ import annotations
For consistency with other test files (see tests/unit/conftest.py:8), consider adding this import after the SPDX headers.
4. Missing Module Docstring
Other test files include descriptive docstrings. Consider adding one after the SPDX headers to explain what's being tested.
Recommendations
- Fix the import isolation issue - This is the most critical issue that may cause tests to pass incorrectly
- Add SPDX headers - Required for licensing compliance
- Add
from __future__ import annotations- For consistency - Add module docstring - For better documentation
CI Status
Note: Some test jobs are currently failing or pending. Please ensure all tests pass after addressing these issues.
Let me know if you'd like help implementing these fixes!
There was a problem hiding this comment.
Code Review of PR #219: Unit Tests for get_version Function
Thank you for adding comprehensive test coverage for the get_version function! This PR adds valuable tests for all the fallback scenarios. However, there are several critical issues that need to be addressed before this can be merged.
🚨 Critical Issues
1. Missing Copyright Header
The test file is missing the required SPDX copyright header that is used consistently across the codebase. This needs to be added at the top of the file:
# SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0See tests/unit/conftest.py:1-4 or tests/unit/core/test_discovery.py:1-4 for reference.
2. Test Isolation Problem - Tests Don't Actually Test the Fallback Logic
This is a fundamental issue with the test design. The get_version() function is called at module import time (line 75 of src/codeweaver/__init__.py):
__version__: Final[str] = get_version()When you import get_version in your test file (line 7):
from codeweaver import get_versionThe function has already been executed and the version has been cached. Your tests are calling get_version() again with mocked modules, but:
- The import statements inside
get_version()are executed fresh each time it's called - However, the tests assume that the mocking of
sys.moduleswill affect these imports
The problem: When you use monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False), this doesn't actually make the import fail in subsequent calls to get_version() because:
- The
from codeweaver._version import __version__statement inget_version()will still try to import from disk - If the module exists on disk, it will still import successfully
Testing approach needed:
- These tests need to be restructured to actually test the fallback logic properly
- Consider using
importlib.reload()or testing in isolated subprocess - Or mock at a different level (e.g., mock
__import__or useunittest.mock.patch)
3. Missing Type Annotations
The codebase uses type hints consistently. Add type annotations to test functions:
def test_get_version_from_version_file(monkeypatch: pytest.MonkeyPatch) -> None:4. Missing pytest Imports and Markers
Looking at other test files in the codebase (e.g., tests/unit/core/test_discovery.py:11-17), you should:
- Import
pytest - Add the
pytestmark = [pytest.mark.unit]marker to identify these as unit tests
💡 Code Quality Suggestions
5. Code Duplication
There's significant repetition in the test setup. Consider using a pytest fixture for common mocking scenarios:
@pytest.fixture
def mock_no_version_module(monkeypatch: pytest.MonkeyPatch) -> None:
"""Mock codeweaver._version to not exist."""
monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False)
@pytest.fixture
def mock_no_metadata(monkeypatch: pytest.MonkeyPatch) -> None:
"""Mock importlib.metadata.version to raise PackageNotFoundError."""
def mock_version(pkg_name: str) -> str:
raise importlib.metadata.PackageNotFoundError("code-weaver")
monkeypatch.setattr(importlib.metadata, "version", mock_version)6. Inconsistent Formatting
- Lines 66-67: Missing blank line after
monkeypatch.delitem(inconsistent with other tests) - Consider running the codebase's formatter/linter
7. Better Mock Verification
In test_get_version_from_git_success, the mock_run function checks specific indices which is fragile:
if args[0][0] == "/usr/bin/git" and args[0][1] == "describe":Consider using more robust mocking with unittest.mock.patch or checking the full command list.
✅ What Works Well
- Comprehensive coverage of all 6 fallback paths
- Clear, descriptive docstrings for each test
- Good use of
monkeypatchfixture for test isolation (in principle) - Appropriate mocking to avoid external dependencies
- Tests are focused and test one scenario each
📊 CI Status
The tests are currently failing in CI for Python 3.13t, 3.14, and 3.14t. The copyright header issue is likely blocking, but the test isolation problem may also be causing failures.
🔧 Recommended Actions
- Immediate: Add the SPDX copyright header
- Critical: Fix the test isolation issue - the tests need to actually verify the fallback logic works
- Required: Add type annotations and pytest imports/markers
- Recommended: Reduce duplication with fixtures
- Nice-to-have: Format code consistently with the rest of the codebase
Verdict: Request Changes ⚠️
This PR cannot be merged until:
- The copyright header is added (blocking CI)
- The test isolation issue is resolved (tests may not be testing what they claim to test)
- Type annotations and pytest markers are added (to match codebase standards)
Once these issues are addressed, this will be a valuable addition to the test suite. Please let me know if you'd like help implementing any of these fixes!
There was a problem hiding this comment.
Code Review of PR #219: Unit Tests for get_version Function
Thank you for adding comprehensive test coverage for the get_version function! This PR adds valuable tests for all the fallback scenarios. However, there are several critical issues that need to be addressed before this can be merged.
🚨 Critical Issues
1. Missing Copyright Header
The test file is missing the required SPDX copyright header that is used consistently across the codebase. This needs to be added at the top of the file:
# SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0See tests/unit/conftest.py:1-4 or tests/unit/core/test_discovery.py:1-4 for reference.
2. Test Isolation Problem - Tests Don't Actually Test the Fallback Logic
This is a fundamental issue with the test design. The get_version() function is called at module import time (line 75 of src/codeweaver/__init__.py):
__version__: Final[str] = get_version()When you import get_version in your test file (line 7):
from codeweaver import get_versionThe function has already been executed and the version has been cached. Your tests are calling get_version() again with mocked modules, but:
- The import statements inside
get_version()are executed fresh each time it's called - However, the tests assume that the mocking of
sys.moduleswill affect these imports
The problem: When you use monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False), this doesn't actually make the import fail in subsequent calls to get_version() because:
- The
from codeweaver._version import __version__statement inget_version()will still try to import from disk - If the module exists on disk, it will still import successfully
Testing approach needed:
- These tests need to be restructured to actually test the fallback logic properly
- Consider using
importlib.reload()or testing in isolated subprocess - Or mock at a different level (e.g., mock
__import__or useunittest.mock.patch)
3. Missing Type Annotations
The codebase uses type hints consistently. Add type annotations to test functions:
def test_get_version_from_version_file(monkeypatch: pytest.MonkeyPatch) -> None:4. Missing pytest Imports and Markers
Looking at other test files in the codebase (e.g., tests/unit/core/test_discovery.py:11-17), you should:
- Import
pytest - Add the
pytestmark = [pytest.mark.unit]marker to identify these as unit tests
💡 Code Quality Suggestions
5. Code Duplication
There's significant repetition in the test setup. Consider using a pytest fixture for common mocking scenarios:
@pytest.fixture
def mock_no_version_module(monkeypatch: pytest.MonkeyPatch) -> None:
"""Mock codeweaver._version to not exist."""
monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False)
@pytest.fixture
def mock_no_metadata(monkeypatch: pytest.MonkeyPatch) -> None:
"""Mock importlib.metadata.version to raise PackageNotFoundError."""
def mock_version(pkg_name: str) -> str:
raise importlib.metadata.PackageNotFoundError("code-weaver")
monkeypatch.setattr(importlib.metadata, "version", mock_version)6. Inconsistent Formatting
- Lines 66-67: Missing blank line after
monkeypatch.delitem(inconsistent with other tests) - Consider running the codebase's formatter/linter
7. Better Mock Verification
In test_get_version_from_git_success, the mock_run function checks specific indices which is fragile:
if args[0][0] == "/usr/bin/git" and args[0][1] == "describe":Consider using more robust mocking with unittest.mock.patch or checking the full command list.
✅ What Works Well
- Comprehensive coverage of all 6 fallback paths
- Clear, descriptive docstrings for each test
- Good use of
monkeypatchfixture for test isolation (in principle) - Appropriate mocking to avoid external dependencies
- Tests are focused and test one scenario each
📊 CI Status
The tests are currently failing in CI for Python 3.13t, 3.14, and 3.14t. The copyright header issue is likely blocking, but the test isolation problem may also be causing failures.
🔧 Recommended Actions
- Immediate: Add the SPDX copyright header
- Critical: Fix the test isolation issue - the tests need to actually verify the fallback logic works
- Required: Add type annotations and pytest imports/markers
- Recommended: Reduce duplication with fixtures
- Nice-to-have: Format code consistently with the rest of the codebase
Verdict: Request Changes ⚠️
This PR cannot be merged until:
- The copyright header is added (blocking CI)
- The test isolation issue is resolved (tests may not be testing what they claim to test)
- Type annotations and pytest markers are added (to match codebase standards)
Once these issues are addressed, this will be a valuable addition to the test suite. Please let me know if you'd like help implementing any of these fixes!
There was a problem hiding this comment.
Code Review
Thanks for adding comprehensive test coverage for the get_version function! The tests cover all the fallback paths well. However, there are several issues that need to be addressed to match the project's coding standards:
Required Changes
- Missing SPDX Headers: Add the standard copyright headers at the top of the file:
# SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0-
Missing Module Docstring: Add a docstring after the headers explaining what this test file covers (see
tests/unit/telemetry/test_privacy_serialization.pyfor reference) -
Missing pytest markers: Add
pytestmark = [pytest.mark.unit]after the imports -
Missing future import: Add
from __future__ import annotationsas the first import -
Import Order: Reorganize imports to follow Python conventions:
- Standard library imports (importlib.metadata, shutil, subprocess, sys)
- Third-party imports (from unittest.mock import MagicMock)
- Local imports (from codeweaver import get_version)
- Import pytest
Test Design Consideration
There's a subtle issue with these tests: get_version() is called at module import time in src/codeweaver/__init__.py:75:
__version__: Final[str] = get_version()This means the version is cached when codeweaver is first imported. Your tests are currently testing the function in isolation (which is still valuable), but they may not fully reflect the module initialization behavior. Consider if this is the intended behavior or if you need to add module reloading logic.
Minor Issue
Some tests are failing on Python 3.13t, 3.14, and 3.14t (experimental). Please investigate these failures to ensure the tests are robust across all supported Python versions.
Overall, the test logic is sound and provides good coverage. Once the formatting and style issues are addressed, this will be a solid addition to the test suite.
There was a problem hiding this comment.
Review Summary
Thank you for adding comprehensive test coverage for the get_version() function! This PR addresses an important testing gap. However, there are two issues that need to be addressed before this can be merged:
🔴 Critical Issue: Test Implementation Bug
The monkeypatching approach used in these tests won't work correctly due to how get_version() uses lazy imports. The Copilot reviewer correctly identified this issue in comment 2940333916.
Problem: The get_version() function imports importlib.metadata, shutil, and subprocess inside the function at runtime (lazy imports on lines 46, 51-52 in src/codeweaver/init.py:37-72). When you patch these modules at the stdlib level using monkeypatch.setattr(shutil, 'which', ...), the import shutil statement that executes inside get_version() creates a fresh reference that won't see your monkeypatch.
Solution: You need to use unittest.mock.patch to intercept the imports at the module level. Here's the recommended pattern:
from unittest.mock import patch, MagicMock
def test_get_version_from_git_success():
"""Test getting version from git describe."""
with patch('codeweaver._version.__version__', side_effect=ImportError):
with patch('importlib.metadata.version') as mock_version:
mock_version.side_effect = importlib.metadata.PackageNotFoundError('code-weaver')
with patch('shutil.which', return_value='/usr/bin/git'):
with patch('subprocess.run') as mock_run:
mock_result = MagicMock()
mock_result.returncode = 0
mock_result.stdout = '1.2.3-git\n'
mock_run.return_value = mock_result
assert get_version() == '1.2.3-git'All six test functions need to be updated to use unittest.mock.patch instead of monkeypatch.
🔴 CI Failure: Missing REUSE Headers
The CI is failing because tests/unit/test_init.py is missing the required copyright and licensing headers. Every file in this project needs SPDX headers.
Add this at the top of the file:
# SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0
"""Unit tests for get_version fallback mechanisms."""See tests/unit/cli/test_httpx_lazy_import.py:1-4 for the format.
✅ What's Good
- Excellent test coverage - You've identified all 6 fallback paths in
get_version() - Clear documentation - Good docstrings explaining each test scenario
- Comprehensive edge cases - Testing success and failure paths for git operations
📋 Action Items
- Add REUSE compliance headers to tests/unit/test_init.py
- Replace monkeypatch with
unittest.mock.patchfor all lazy imports - Test that the mocks actually work by running the tests locally
Once these issues are fixed, this will be a valuable addition to the test suite!
PR Review for #219: Unit tests for get_version fallback mechanismsThank you for adding comprehensive test coverage for the Critical Issues1. Missing SPDX Headers ❌The test file is missing the required copyright and licensing information headers. This is causing the CI checks to fail. Required headers (add at the top of # SPDX-FileCopyrightText: 2026 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0Code Quality Issues2. Test Isolation Concerns
|
Added unit tests for `get_version` in `tests/unit/test_init.py`. The tests verify that all mechanisms to retrieve the package version function as intended: - Reading `codeweaver._version.__version__` - Using `importlib.metadata.version` - Falling back to `git describe` - Handling missing `git` command - Handling broader unexpected exceptions Uses `monkeypatch` safely by keeping original references to avoid polluting the global namespace and Pytest internal packages. No functional code changes. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
| import sys | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest |
Code ReviewSummaryThis PR adds comprehensive unit tests for the Positive Aspects✅ Comprehensive coverage: All six fallback scenarios are properly tested:
✅ Proper mocking strategy: Uses ✅ Clear test names: Each test function clearly describes what scenario it's testing ✅ Preserves original behavior: Stores Issues to Address1. Missing Copyright Header ❗The new test file lacks the standard SPDX copyright header used throughout the codebase. All test files include: # SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0See: tests/unit/cli/test_doctor_command.py:1-4 2. Missing Module Docstring ❗The file should have a module-level docstring explaining its purpose, following the pattern in other test files: """Unit tests for get_version function fallback mechanisms."""See: tests/unit/telemetry/test_privacy_serialization.py:5-10 3. Missing Pytest Marker ❗The file should include the standard pytest marker at the module level: pytestmark = [pytest.mark.unit]See: tests/unit/cli/test_cli_helpers.py:22 4. Missing
|
Added comprehensive unit tests for `get_version` in `tests/unit/test_init.py`. The tests verify that all mechanisms to retrieve the package version function as intended: - Reading `codeweaver._version.__version__` - Using `importlib.metadata.version` - Falling back to `git describe` - Handling missing `git` command - Handling broader unexpected exceptions Uses `monkeypatch` safely by keeping original references to avoid polluting the global namespace and Pytest internal packages. No functional code changes. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
| @@ -0,0 +1,16 @@ | |||
| import sys | |||
| import codeweaver | |||
There was a problem hiding this comment.
Pull request overview
Adds unit tests to cover codeweaver.get_version()’s fallback chain (bundled _version, importlib.metadata, and git-based resolution), aiming to close coverage gaps around exception-driven fallbacks.
Changes:
- Add unit tests covering version resolution via
_version,importlib.metadata, git describe, and failure fallbacks. - Add an extra top-level test/debug file exercising
get_version().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/unit/test_init.py | New unit tests for get_version() fallback behaviors. |
| test_import_final.py | Adds a standalone test/debug script that invokes pytest at import time. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| pytest.main(["-s", "test_import_final.py"]) |
| import importlib.metadata | ||
| import shutil | ||
| import subprocess | ||
| import sys | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest | ||
|
|
||
| from codeweaver import get_version | ||
|
|
||
| # Store the original version function to fall back to it for other packages | ||
| _original_version = importlib.metadata.version |
| def mock_run(*args, **kwargs): | ||
| return mock_result | ||
|
|
||
| monkeypatch.setattr(subprocess, "run", mock_run) |
| def mock_run(*args, **kwargs): | ||
| return mock_result | ||
|
|
||
| monkeypatch.setattr(subprocess, "run", mock_run) |
PR Review - Comprehensive AnalysisThank you for adding test coverage for the 🚨 Critical Issues1. Accidentally Committed Entire CodebaseThe commit Evidence: The commit includes files like:
Impact: This makes the PR extremely difficult to review and will cause merge conflicts. Required Action: The PR branch needs to be cleaned up to only include the intended changes:
2. Debug File Included in PRThe file Required Action: Remove 3. Missing SPDX Copyright HeadersThe test file Required headers (see # SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.04. Test File is Identical to Current Main BranchInterestingly, when I checked the current HEAD, Required Action: Verify whether these tests are already on main. If so, this PR may be redundant.
|
Code Review - PR #219Thank you for adding test coverage for the 🔴 Critical Issues1. Module Import Timing ProblemThe biggest issue is that __version__: Final[str] = get_version()This means the version is determined and cached when the from codeweaver import get_versionBy the time the tests run, Solution: You need to reload the import importlib
import codeweaver
def test_get_version_from_version_file(monkeypatch):
mock_module = MagicMock()
mock_module.__version__ = "1.2.3-file"
monkeypatch.setitem(sys.modules, "codeweaver._version", mock_module)
# Reload the module to re-execute get_version()
importlib.reload(codeweaver)
assert codeweaver.get_version() == "1.2.3-file"2. Incorrect Mocking of
|
PR Review SummaryThank you for adding comprehensive test coverage for the Critical Issues1. Missing SPDX Headers ❌File: The test file is missing required copyright and licensing headers. Based on the codebase standards, please add: # SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0
"""Unit tests for get_version fallback mechanisms."""2. Incorrect test file in repository root ❌File: This appears to be a scratch/debug file that should not be committed. It:
Recommendation: Remove this file entirely from the PR. Code Quality Issues3. Missing pytest markers
|
There was a problem hiding this comment.
Code Review: PR #219 - Unit Tests for get_version Function
Thank you for adding comprehensive test coverage for the get_version function! This addresses an important testing gap. However, after reviewing the code and CI feedback, I've identified several critical issues that need to be addressed before this can be merged.
🚨 Critical Issues
1. Unexpected File: test_import_final.py
The file test_import_final.py appears to be a scratch/debugging file that should not be included in this PR:
- It's in the repository root (not in
tests/directory) - Contains a single test that calls
pytest.main()directly (line 16) - Doesn't follow the project's testing patterns
- Not mentioned in the PR description
Action Required: Remove this file from the PR.
2. Missing SPDX Copyright Headers
The test file tests/unit/test_init.py is missing required copyright headers. All files in this project require SPDX headers.
Required addition at the top of tests/unit/test_init.py:
# SPDX-FileCopyrightText: 2025 Knitli Inc.
# SPDX-FileContributor: Adam Poulemanos <adam@knit.li>
#
# SPDX-License-Identifier: MIT OR Apache-2.0
"""Unit tests for get_version fallback mechanisms."""
from __future__ import annotationsReference: tests/unit/conftest.py:1-8
3. Test Isolation Issue
The current monkeypatching approach has a fundamental limitation. The get_version() function uses lazy imports (imports happen inside the function), which means:
# In test_get_version_from_importlib_metadata (line 24)
monkeypatch.setitem(sys.modules, "codeweaver._version", None)This sets the module to None, but when get_version() executes from codeweaver._version import __version__ (line 43 of __init__.py), Python will:
- See
sys.modules["codeweaver._version"] = None - Still attempt a fresh import from disk
- If the
_version.pyfile exists, import it successfully
The tests may pass in CI because the _version.py file might not exist in the test environment, but they don't accurately test the fallback logic.
Recommended approach: Use unittest.mock.patch to intercept the import at a deeper level, or use importlib.reload() with proper module cleanup. The Sourcery AI review correctly suggests using monkeypatch.delitem(sys.modules, 'codeweaver._version', raising=False) which is better than setting to None.
4. Missing Import and Markers
Looking at other test files (e.g., tests/unit/cli/test_httpx_lazy_import.py:29), the following are missing:
from __future__ import annotations # Should be first import after headers
# After imports, before test functions:
pytestmark = [pytest.mark.unit]💡 Code Quality Improvements
5. Code Duplication (Sourcery's suggestion is valid)
There's significant repetition in setting up mocks. Consider extracting common patterns into pytest fixtures:
@pytest.fixture
def mock_no_version_module(monkeypatch: pytest.MonkeyPatch) -> None:
"""Mock codeweaver._version to not exist."""
monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False)
@pytest.fixture
def mock_no_metadata(monkeypatch: pytest.MonkeyPatch) -> None:
"""Mock importlib.metadata.version to raise PackageNotFoundError."""
def mock_version(pkg_name: str) -> str:
if pkg_name == "code-weaver":
raise importlib.metadata.PackageNotFoundError("code-weaver")
return _original_version(pkg_name)
monkeypatch.setattr(importlib.metadata, "version", mock_version)Then tests can use:
def test_get_version_from_importlib_metadata(
monkeypatch: pytest.MonkeyPatch,
mock_no_version_module, # Use fixture
) -> None:
# Only the specific mocking for this test
...6. Type Annotations
Add type annotations to match project conventions:
def test_get_version_from_version_file(monkeypatch: pytest.MonkeyPatch) -> None:✅ What's Working Well
- Comprehensive coverage - All 6 fallback paths are tested
- Clear documentation - Good docstrings for each test
- Proper use of mocking - Avoids external dependencies (git, network)
- Focused tests - Each test targets one specific scenario
📊 CI Status
Current failures:
- Python 3.13t, 3.14, 3.14t tests are failing
- Likely due to missing copyright headers (REUSE compliance check)
🔧 Action Items (Priority Order)
- Remove
test_import_final.py- This file should not be in the PR - Add SPDX headers to
tests/unit/test_init.py - Add
from __future__ import annotationsandpytestmark - Fix monkeypatch approach - Use
deliteminstead ofsetitem(..., None) - Add type annotations to all test functions
- Consider refactoring - Extract common mocking into fixtures (optional but recommended)
Verdict: Request Changes ⚠️
This PR cannot be merged until issues #1-4 are resolved. Issue #5-6 are strongly recommended for code quality.
Once these are addressed, this will be a valuable addition to the test suite! The test logic is sound and the coverage is excellent - it just needs some structural fixes to align with project standards.
Let me know if you need any clarification on these points!
There was a problem hiding this comment.
PR Review: Test Coverage for get_version()
Summary
This PR adds comprehensive test coverage for the get_version() function in src/codeweaver/__init__.py. The tests cover all fallback paths including version file, importlib.metadata, git describe, and error scenarios. While the intent is excellent, there are several critical issues that need to be addressed.
Critical Issues
1. Function Caching Problem ⚠️
The most critical issue is that get_version() is called once at module import time to set __version__ (line 75 of __init__.py):
__version__: Final[str] = get_version()This means:
- When you import
codeweaverorget_versionin your tests, the function has already been called - The
__version__module-level variable is already set - Subsequent calls to
get_version()will execute again, but the module's__version__won't change - Your mocks are being set up after the initial call
Impact: Tests may pass or fail inconsistently depending on the order they run and what the actual environment looks like.
Solution: Consider one of these approaches:
- Reload the module after setting up mocks:
import importlib
import codeweaver
# ... setup mocks ...
importlib.reload(codeweaver)
version = codeweaver.__version__- Test the function directly by importing it lazily or resetting
sys.modulesmore thoroughly - Use pytest fixtures with
autouse=Trueto ensure clean state between tests
2. Stray Test File 🗑️
The file test_import_final.py at the repository root should be removed. It appears to be a debug/exploration file that shouldn't be committed:
- It calls
pytest.main()which would cause issues - It doesn't follow the project's test structure
- It's not in the
tests/directory
Code Quality Issues
3. Mock Setup - Using None vs delitem
In multiple tests, you use:
monkeypatch.setitem(sys.modules, "codeweaver._version", None)Issue: Setting a module to None in sys.modules doesn't truly simulate it being missing. Python will find None in sys.modules and treat it as "already imported (but empty)".
Better approach:
monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False)This more accurately simulates the module not being available.
4. Code Duplication
As Sourcery AI pointed out, there's significant duplication in mock setup. Consider creating a fixture:
@pytest.fixture
def mock_no_version_file(monkeypatch):
"""Remove _version module from sys.modules."""
monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False)
@pytest.fixture
def mock_no_metadata(monkeypatch, mock_no_version_file):
"""Mock importlib.metadata to raise PackageNotFoundError."""
def mock_version(pkg_name):
if pkg_name == "code-weaver":
raise importlib.metadata.PackageNotFoundError("code-weaver")
return _original_version(pkg_name)
monkeypatch.setattr(importlib.metadata, "version", mock_version)Test Coverage Analysis
The tests cover:
- ✅ Version from
_versionfile - ✅ Version from
importlib.metadata - ✅ Version from git describe (success)
- ✅ Git describe failure (non-zero exit)
- ✅ Missing git binary
- ✅ Generic exception handling
This is excellent coverage of all the fallback paths.
Minor Issues
5. Hardcoded Paths
monkeypatch.setattr(shutil, "which", lambda cmd: "/usr/bin/git" if cmd == "git" else None)The hardcoded path /usr/bin/git works but could be more generic:
monkeypatch.setattr(shutil, "which", lambda cmd: "git" if cmd == "git" else None)The actual path doesn't matter for the test; just that it's truthy.
6. Test Documentation
The docstrings are clear and helpful. Good work! ✅
Current Test Failures
The CI shows failures on Python 3.13t, 3.14, and 3.14t. Without access to the detailed logs, I suspect these failures are related to:
- The caching issue described above
- Potential timing issues with module imports
- Possible conflicts with the actual installed version of codeweaver in the test environment
Recommendations
To fix this PR:
- Remove
test_import_final.py - Fix the caching issue by implementing module reload or refactoring how tests import
get_version - Use
deliteminstead of setting modules toNone - Create fixtures to reduce duplication
- Investigate test failures - run tests locally with
pytest tests/unit/test_init.py -vto see the actual failures
Example of a fixed test:
def test_get_version_from_version_file(monkeypatch):
"""Test getting version from codeweaver._version.__version__."""
# Clean up any existing imports
monkeypatch.delitem(sys.modules, "codeweaver._version", raising=False)
monkeypatch.delitem(sys.modules, "codeweaver", raising=False)
# Set up mock BEFORE importing
mock_module = MagicMock()
mock_module.__version__ = "1.2.3-file"
monkeypatch.setitem(sys.modules, "codeweaver._version", mock_module)
# Now import and reload
import importlib
import codeweaver
importlib.reload(codeweaver)
assert codeweaver.__version__ == "1.2.3-file"Verdict
Request Changes
While the test coverage strategy is excellent and the PR addresses an important gap, the implementation has critical issues that cause test failures. The main problem is the module-level caching of __version__ which requires careful handling in tests.
Once the issues above are addressed, this will be a valuable addition to the test suite!
Positive Notes 👍
- Excellent identification of a testing gap
- Comprehensive coverage of all fallback scenarios
- Clear test naming and documentation
- Good use of pytest and monkeypatch
|
Jules, please address the comments in the below PR review:
|
🎯 What: The testing gap addressed
The
get_versionfunction insrc/codeweaver/__init__.pylacked proper test coverage, specifically regarding its multi-level exception handling fallback logic.📊 Coverage: What scenarios are now tested
codeweaver._version.__version__.importlib.metadata.version.✨ Result: The improvement in test coverage
The
get_versionfunction is now fully tested under all its supported fallback conditions and failure states, providing safety for any future changes to version retrieval logic.PR created automatically by Jules for task 4264728002802036473 started by @bashandbone
Summary by Sourcery
Add comprehensive test coverage for get_version’s version resolution and fallback behavior.
Tests: