TEST: Add unit tests for pyrit/common/ utilities#1600
TEST: Add unit tests for pyrit/common/ utilities#1600romanlutz wants to merge 3 commits intomicrosoft:mainfrom
Conversation
Adds tests for 10 previously untested utility files: - apply_defaults.py (decorator, sentinel, registry) - csv_helper.py (read/write/roundtrip) - data_url_converter.py (format support, encoding) - deprecation.py (warnings for callables, classes) - display_response.py (notebook skip, image display) - path.py (constants, git_repo detection, default paths) - question_answer_helpers.py (prompt formatting) - singleton.py (identity, distinct classes) - utils.py (combine_list, to_sha256) - yaml_loadable.py (basic load, from_dict, errors) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # tests/unit/common/test_csv_helper.py
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds missing unit coverage for pyrit/common/ utilities by introducing new pytest tests across multiple helper modules.
Changes:
- Added unit tests for YAML loading (
YamlLoadable), hashing/list utilities, singleton metaclass behavior, and prompt construction helpers. - Added unit tests for filesystem/path constants/helpers, image display behavior, deprecation warnings, CSV read/write helpers, and data-url conversion.
- Introduced fixtures and mocking for async I/O and notebook-dependent behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/common/test_apply_defaults.py | Covers default registry behavior and @apply_defaults decorator scenarios |
| tests/unit/common/test_csv_helper.py | Adds coverage for CSV read/write and roundtrip behavior |
| tests/unit/common/test_data_url_converter.py | Adds coverage for supported formats, errors, and data URL output |
| tests/unit/common/test_deprecation.py | Validates warning emission/message contents across input types |
| tests/unit/common/test_display_response.py | Tests notebook gating, blocked content logging, and image display flow with mocks |
| tests/unit/common/test_path.py | Tests path constants plus in_git_repo / get_default_data_path |
| tests/unit/common/test_question_answer_helpers.py | Validates evaluation prompt formatting for Q/A entries |
| tests/unit/common/test_singleton.py | Tests singleton identity, class separation, and init-arg preservation |
| tests/unit/common/test_utils.py | Tests list-combining behavior and SHA256 determinism/known value |
| tests/unit/common/test_yaml_loadable.py | Tests YAML load success paths plus missing/invalid file error behavior |
| def test_db_data_path_exists(): | ||
| assert DB_DATA_PATH.exists() | ||
|
|
||
|
|
||
| def test_log_path_exists(): | ||
| assert LOG_PATH.exists() |
There was a problem hiding this comment.
These assertions make the test suite depend on the executing machine’s filesystem state (and possibly on import-time side effects that create these paths). This can be flaky in clean CI containers or when the package is installed in environments that don’t pre-create these directories. Prefer asserting properties that are stable (e.g., that the paths are absolute / under an expected base) or explicitly creating the directories in the test setup (or patching the base directory to a tmp_path) depending on the intended contract of pyrit.common.path.
| def test_db_data_path_exists(): | |
| assert DB_DATA_PATH.exists() | |
| def test_log_path_exists(): | |
| assert LOG_PATH.exists() | |
| def test_db_data_path_is_absolute(): | |
| assert DB_DATA_PATH.is_absolute() | |
| def test_log_path_is_absolute(): | |
| assert LOG_PATH.is_absolute() |
| @pytest.mark.asyncio | ||
| @patch("pyrit.common.display_response.is_in_ipython_session", return_value=True) | ||
| @patch("pyrit.common.display_response.Image") | ||
| @patch("builtins.display", create=True) |
There was a problem hiding this comment.
Patching builtins.display is fragile because the implementation may call display via a module import (e.g., IPython.display.display) or a symbol imported into pyrit.common.display_response, in which case this patch won’t intercept the call (and the test may become environment-dependent). Patch the display symbol in the namespace where it’s used (e.g., pyrit.common.display_response.display) to make the test deterministic.
| @patch("builtins.display", create=True) | |
| @patch("pyrit.common.display_response.display") |
| a = _MySingleton() | ||
| b = _MySingleton() | ||
| assert a is b | ||
|
|
||
| # Cleanup to avoid polluting other tests | ||
| Singleton._instances.pop(_MySingleton, None) |
There was a problem hiding this comment.
The cleanup relies on reaching into Singleton._instances and it won’t run if the test fails before the cleanup line (potentially polluting other tests). Consider moving this cleanup into a fixture/finalizer (or a try/finally) so state is always reset even on assertion failures, and to reduce coupling to the metaclass’s internal storage details.
|
|
||
| def test_from_yaml_file_nonexistent_raises(): | ||
| with pytest.raises(FileNotFoundError): | ||
| _SimpleYaml.from_yaml_file("nonexistent_file.yaml") |
There was a problem hiding this comment.
The other tests in this file pass a Path, but this one passes a string literal. If from_yaml_file is intended to accept Path objects, using Path("nonexistent_file.yaml") here would keep the tests consistent and avoid ambiguity about supported input types.
| _SimpleYaml.from_yaml_file("nonexistent_file.yaml") | |
| _SimpleYaml.from_yaml_file(Path("nonexistent_file.yaml")) |
rlundeen2
left a comment
rlundeen2
left a comment
There was a problem hiding this comment.
I like the copilot review suggestions, but no blockers!
Summary
Adds unit tests for 10 previously untested utility files in pyrit/common/.
New Test Files
Testing
All 71 tests pass locally.