Add comprehensive test suite for utility modules and core models#297
Add comprehensive test suite for utility modules and core models#297amussara wants to merge 1 commit intoridgesai:mainfrom
Conversation
- tests/test_diff.py: Tests for file diff computation, validation, and application - tests/test_git_utils.py: Tests for git clone, commit verification, init, and reset - tests/test_cost_hash_map.py: Tests for in-memory cost tracking with cleanup - tests/test_inference_models.py: Tests for model cost calculation, tool conversion, request/response validation - tests/test_ttl_cache.py: Tests for TTL cache key generation, entry lifecycle, and decorator behavior - tests/test_temp_utils.py: Tests for temporary directory creation and cleanup - tests/test_logger.py: Tests for log levels, formatting, and debug toggle - tests/test_evaluation_models.py: Tests for evaluation run status, error codes, and model validation Covers edge cases including error code categorization, cache eviction, diff header formatting, and subprocess failure handling.
CryptoAardvark
left a comment
CryptoAardvark
left a comment
There was a problem hiding this comment.
Thanks again for putting this together, the naming conventions and class structure are really clear, and the error code range invariants (1xxx/2xxx/3xxx) in test_evaluation_models.py are a particularly nice touch. All of the observations above are minor and non-blocking from my perspective as an external reviewer. Feel free to take or leave any of them, and apologies if any of this overlaps with conventions I'm not aware of. 🙏
| """Tests for the validate_diff_for_local_repo function.""" | ||
|
|
||
| def _create_git_repo(self, files: dict) -> str: | ||
| repo_dir = tempfile.mkdtemp() |
There was a problem hiding this comment.
Suggestion: I noticed the directory created here doesn't appear to be cleaned up after the test finishes, so repeated test runs may leave behind temp directories. If it would be helpful, I think pytest's built-in tmp_path fixture handles cleanup automatically and could also eliminate the need for the try/finally blocks in _write_temp.
| with pytest.raises(Exception): | ||
| get_file_diff(existing, "/nonexistent/file.txt") |
There was a problem hiding this comment.
Suggestion(non-blocking): Since Exception is the root of most exception hierarchies, these assertions would also pass if the source code raised something unexpected like AttributeError from a typo. If the function raises a more specific type (e.g. FileNotFoundError or a custom exception), catching that explicitly would make the tests a bit stricter:
with pytest.raises(FileNotFoundError):
get_file_diff(existing, "/nonexistent/file.txt")
That said, I don't know the project's conventions here if broad catching is the preferred style, please disregard.
2 participants
Covers edge cases including error code categorization, cache eviction, diff header formatting, and subprocess failure handling.