render機能のテストカバレッジ向上のためのテストケース追加 #29
Conversation
📝 WalkthroughWalkthroughRemoved broad test-planning docs and added focused render-module test tasks and tests; added two Taskipy tasks in pyproject.toml; adjusted pytest marker detection in conftest.py; added comprehensive tests for PDF/image export, dependency checks, error handling, and filename sanitization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/render/test_render_init.py (1)
209-227: Consider simplifying the import mock.The
_fake_importfunction matches the fullbuiltins.__import__signature but could be more concise.🔎 Simplified alternative
def test_require_pdfium_missing_dependency(monkeypatch: pytest.MonkeyPatch) -> None: """_require_pdfium raises MissingDependencyError when import fails.""" - real_import = builtins.__import__ - - def _fake_import( - name: str, - globals_dict: dict[str, object] | None, - locals_dict: dict[str, object] | None, - fromlist: tuple[str, ...], - level: int, - ) -> object: - if name == "pypdfium2": - raise ImportError("missing") - return real_import(name, globals_dict, locals_dict, fromlist, level) - - monkeypatch.setattr(builtins, "__import__", _fake_import) + def _fake_import(name: str, *args: object, **kwargs: object) -> object: + if name == "pypdfium2": + raise ImportError("missing") + return builtins.__import__(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _fake_import) with pytest.raises(MissingDependencyError, match="pypdfium2"): render._require_pdfium()This reduces boilerplate while maintaining the same behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/agents/TASKS.mdpyproject.tomltests/conftest.pytests/render/test_render_init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Add type hints to all function and method arguments and return values, must be mypy strict compliant
UseAnytype only at external library boundaries (xlwings, pandas, numpy, etc.)
Return Pydantic BaseModel instances instead of dictionaries or tuples for structured data
Each function must have a single responsibility; cyclomatic complexity must not exceed 12
Organize imports in the following order: 1) standard library, 2) third-party packages, 3) exstruct internal modules
Add Google-style docstrings to all functions and classes
Do not depend on internal structures of external libraries (xlwings, pandas, numpy); normalize external entities to Pydantic models at the boundary layer
Generated code must pass mypy strict mode with zero errors
Generated code must pass Ruff linter checks (E, W, F, I, B, UP, N, C90 rules) with zero errors
Avoid God Functions (large monolithic functions that do too much)
Avoid God Objects (classes that have too many responsibilities)
Eliminate unnecessary nesting and deeply nested conditional branches
Avoid circular dependencies between modules
Write functions that are testable and avoid unnecessary side effects
Files:
tests/conftest.pytests/render/test_render_init.py
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:32:54.706Z
Learnt from: CR
Repo: harumiWeb/exstruct PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T14:32:54.706Z
Learning: Applies to **/*.py : Generated code must pass Ruff linter checks (E, W, F, I, B, UP, N, C90 rules) with zero errors
Applied to files:
pyproject.toml
🪛 GitHub Actions: pytest
tests/render/test_render_init.py
[error] 1-1: Test failing due to the above TypeError in export_sheet_images; pytest exit code != 0.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
pyproject.toml (2)
127-127: LGTM! Helpful documentation comment.The inline comment documents the required dependency groups for running render tests, which aids developer onboarding.
132-133: LGTM! Useful task additions.The new
codecov-comanddocstasks improve the development workflow by providing convenient shortcuts for code coverage uploads and documentation serving.docs/agents/TASKS.md (1)
5-8: LGTM! Task tracking updated.The completed tasks align with the comprehensive test coverage added in
tests/render/test_render_init.py.tests/conftest.py (2)
79-79: LGTM! More idiomatic marker detection.Using
get_closest_marker()is the recommended pytest approach for marker detection, providing better type safety and explicitness compared to keyword string checks.
84-84: LGTM! Consistent marker detection pattern.Consistent with the render marker check, this follows pytest best practices for marker-based test filtering.
tests/render/test_render_init.py (8)
1-11: LGTM! Imports properly organized.Import order follows the coding guidelines: standard library, third-party packages, then internal modules, with appropriate grouping.
14-117: LGTM! Well-structured test stubs.The stub classes effectively mock xlwings and pypdfium2 dependencies with:
- Proper type hints on all methods and parameters
- Clear, concise docstrings
- Focused, single-responsibility implementations
- Consistent patterns for parameter ignoring using
_
119-129: LGTM! Clean factory pattern.The factory function properly uses closure to capture configuration and returns a typed callable, facilitating flexible test setup.
132-165: LGTM! Comprehensive success and failure path testing.These tests properly exercise both success and error scenarios for Excel app initialization and PDF export:
- Proper type hints and docstrings
- Clean monkeypatching patterns
- Clear assertions
168-206: LGTM! Thorough error handling tests.The tests validate error wrapping and edge cases effectively. The
_copywrapper intest_export_pdf_missing_output_raisesis a bit intricate but necessary to simulate the missing output file scenario.
230-272: LGTM! Comprehensive image export testing.The tests cover success paths, error propagation, and proper file generation with sanitized filenames.
275-313: LGTM! Unknown error wrapping validated.The test correctly verifies that unexpected errors during PDF processing are wrapped in
RenderErrorwith an appropriate message. TheExplodingPdfDocumentstub effectively simulates the failure scenario.
316-319: LGTM! Sanitization logic tested.The test validates character replacement and default fallback behavior for invalid sheet names.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/render/test_render_init.py (1)
110-123: Fix the FakeImage.save parameter name to match PIL API.The
savemethod usesimage_formatas the parameter name (line 116), but the actual code callspil_image.save(img_path, format="PNG", dpi=(dpi, dpi))with the keyword argumentformat. While**kwargsmight capture this, the mock should match the real PIL Image.save API signature for clarity and correctness.🔎 Fix the parameter name
class FakeImage: """Stub of a PIL image with a save method.""" def save( self, path: Path, - image_format: str | None = None, + format: str | None = None, dpi: tuple[int, int] | None = None, **kwargs: object, ) -> None: - _ = image_format + _ = format _ = dpi _ = kwargs path.write_bytes(b"PNG")
🧹 Nitpick comments (1)
tests/render/test_render_init.py (1)
216-234: Align _fake_import signature with builtins.import.The
_fake_importfunction signature (lines 220-226) requires all parameters, but the realbuiltins.__import__has default values:__import__(name, globals=None, locals=None, fromlist=(), level=0). While the test may work if all calls provide all arguments, matching the real signature improves correctness and prevents potential issues.🔎 Proposed fix to match builtins signature
def _fake_import( name: str, - globals_dict: dict[str, object] | None, - locals_dict: dict[str, object] | None, - fromlist: tuple[str, ...], - level: int, + globals_dict: dict[str, object] | None = None, + locals_dict: dict[str, object] | None = None, + fromlist: tuple[str, ...] = (), + level: int = 0, ) -> object: if name == "pypdfium2": raise ImportError("missing") return real_import(name, globals_dict, locals_dict, fromlist, level)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/render/test_render_init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Add type hints to all function and method arguments and return values, must be mypy strict compliant
UseAnytype only at external library boundaries (xlwings, pandas, numpy, etc.)
Return Pydantic BaseModel instances instead of dictionaries or tuples for structured data
Each function must have a single responsibility; cyclomatic complexity must not exceed 12
Organize imports in the following order: 1) standard library, 2) third-party packages, 3) exstruct internal modules
Add Google-style docstrings to all functions and classes
Do not depend on internal structures of external libraries (xlwings, pandas, numpy); normalize external entities to Pydantic models at the boundary layer
Generated code must pass mypy strict mode with zero errors
Generated code must pass Ruff linter checks (E, W, F, I, B, UP, N, C90 rules) with zero errors
Avoid God Functions (large monolithic functions that do too much)
Avoid God Objects (classes that have too many responsibilities)
Eliminate unnecessary nesting and deeply nested conditional branches
Avoid circular dependencies between modules
Write functions that are testable and avoid unnecessary side effects
Files:
tests/render/test_render_init.py
🧬 Code graph analysis (1)
tests/render/test_render_init.py (2)
src/exstruct/errors.py (2)
MissingDependencyError(24-25)RenderError(28-29)src/exstruct/render/__init__.py (4)
_require_excel_app(17-25)export_pdf(28-62)_require_pdfium(65-73)export_sheet_images(76-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
tests/render/test_render_init.py (9)
1-12: LGTM! Imports are well-organized.The imports follow the coding guidelines with proper separation: standard library, third-party packages, and internal modules.
14-66: LGTM! Well-structured mock classes for xlwings.The mock classes (FakeSheet, FakeBookApi, FakeBook, FakeBooks, FakeApp) appropriately stub xlwings components with proper type hints and clear single responsibilities.
69-107: LGTM! PDF rendering mocks are properly implemented.The FakeBitmap, FakePage, and FakePdfDocument classes correctly implement the context manager protocol and pypdfium2 API stubs with appropriate type hints.
126-136: LGTM! Factory pattern is well-implemented.The
_fake_app_factoryhelper uses a clean closure pattern with proper type hints to generate xlwings.App mocks.
139-158: LGTM! Excel app initialization tests are thorough.Both success and failure paths for
_require_excel_appare properly tested with clear assertions and appropriate error matching.
161-213: LGTM! PDF export tests cover key scenarios.The three test functions comprehensively validate the
export_pdfflow: successful export, error wrapping, and missing output detection. The monkeypatching intest_export_pdf_missing_output_raisesappropriately simulates a failed copy operation.
237-259: LGTM! Sheet image export test validates key functionality.The test properly verifies image generation with filename sanitization, checking both the sanitized names ("Sheet/1" → "Sheet_1", " " → "sheet") and file existence.
261-320: LGTM! Error handling tests are comprehensive.Both tests properly validate error propagation and wrapping behavior. The
ExplodingPdfDocumentstub effectively simulates a failure during PDF processing.
323-326: LGTM! Filename sanitization test covers edge cases.The test validates both special character replacement and the whitespace-only default case for
_sanitize_sheet_filename.
Summary by CodeRabbit
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.