tests: add SOLID compliance tests#30
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new pytest module intended to enforce SOLID-oriented architectural constraints around the converter/registry patterns, increasing regression protection for the plugin registration and orchestration pipeline.
Changes:
- Introduces
tests/test_solid.pywith SRP/OCP/LSP/ISP/DIP checks for extractor/writer, registries, parsers/formatters, and converter wiring. - Adds tests covering decorator-based registration and factory-based dependency injection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sig = inspect.signature(GitHubToMarkdownConverter.__init__) | ||
| hints = get_type_hints(GitHubToMarkdownConverter.__init__) |
| from typing import Any, get_type_hints | ||
|
|
||
| import pytest | ||
|
|
| methods = [ | ||
| m for m in dir(GitHubExtractor) if not m.startswith("_") or m == "__init__" | ||
| ] |
| parsers = {p.section_key: p for p in [ProfileParser(), ReposParser(), ContributionsParser()]} | ||
| formatters = {f.section_key: f for f in [ProfileFormatter(), ReposFormatter(), ContributionsFormatter()]} |
| def test_writer_only_writes(self): | ||
| """MarkdownFileWriter: sole job is writing content to files.""" | ||
| writer = MarkdownFileWriter(Path("/tmp")) | ||
| sig = inspect.signature(writer.write) |
| def test_create_converter_factory_injects_dependencies(self): | ||
| """Factory function wires concrete deps behind abstract interfaces.""" | ||
| output_dir = Path("/tmp/test_github2md_dip") | ||
| conv = create_converter(output_dir) |
|
|
||
|
|
||
| class TestDependencyInversion: | ||
| """High-level modules should not depend on low-level details; both should depend on abstractions.""" |
| assert registered_after == registered_before + 1 | ||
|
|
||
| def test_converter_uses_registry_not_hardcoded_list(self): | ||
| """Converter iterates registry — adding a new parser/formatter works automatically.""" |
| """Subtypes must be substitutable for their base types/protocols.""" | ||
|
|
||
| def test_parsers_implement_section_parser_protocol(self): | ||
| """All parsers satisfy SectionParser and can be used where SectionParser is expected.""" |
|
Thanks for the PR! 🙌 Copilot's review caught a few ruff lint issues that need fixing before merge:
Could you address these? The tests look solid architecturally — just need the lint cleanup. Happy to merge once CI passes! 🚀 |
juanmanueldaza
left a comment
juanmanueldaza
left a comment
There was a problem hiding this comment.
Code Review: SOLID Compliance Tests
Nice work @Muhtasim-Munif-Fahim! This is a thorough test suite covering all five SOLID principles.
Positive:
- Comprehensive coverage — S, O, L, I, D all tested ✅
- Tests are well-organized by principle in separate classes ✅
- Uses protocols (
DataExtractor,OutputWriter) for dependency inversion tests ✅ - Registry returns copies to prevent external mutation ✅
- Edge cases: empty data, mock implementations ✅
Suggestion: Consider splitting the test_converter_depends_on_protocols_not_concretions test — it uses get_type_hints which could fail with NameError if forward refs are used in the future. A simpler assertion checking the parameter names (like test_converter_depends_on_abstractions) is more robust.
CI is running now. Will approve once it passes. 🚀
juanmanueldaza
left a comment
juanmanueldaza
left a comment
There was a problem hiding this comment.
CI Results: ❌ 3 lint errors
The CI ran and found 3 I001 (import block un-sorted) errors in tests/test_solid.py:
- Line 11 — Top-level imports not properly organized
- Line 418 —
test_parsers_reference_only_abstractions— inline imports not organized - Line 430 —
test_formatters_reference_only_abstractions— inline imports not organized
All 3 are auto-fixable with ruff check --fix. Could you:
- Run
ruff check --fix tests/test_solid.pylocally - Commit and push the fixes
These tests also import from production modules (github2md.converter, etc.) which may not be available in the CI environment unless the package is installed — double check that your pyproject.toml has the right dev dependencies.
Other than the lint, the test structure and coverage is solid (pun intended). 🚀
juanmanueldaza
dismissed
their stale review
Fix applied — pushed ruff import sorting fix directly
- Rename format param from 'parsed_data' to 'data' in SectionFormatter protocol - Ruff format test_solid.py (blank lines after class defs) - Fix ruff I001 import ordering in test_solid.py
Closes #12
Adds SOLID principle compliance tests: