test: implement test_validate_transaction_exception_handling (#35) #67

cal · 2026-03-05T08:04:51Z

cal commented

2026-03-05 08:04:51 +00:00

Summary

Removes the @pytest.mark.skip from test_validate_transaction_exception_handling in tests/test_services_transaction.py
Implements the test using a patch on services.transaction_service.RosterValidation that raises RuntimeError on the first call, triggering the except clause in validate_transaction
Verifies the method returns is_legal=False with error message "Validation error: Simulated validation failure" instead of propagating the exception

What was fixed

The skipped test covered the critical exception-handling path at services/transaction_service.py:187-192. The except Exception clause catches any runtime error during validation and returns a failed RosterValidation — this is now covered by the test.

The test follows the existing patterns in the file (using patch from unittest.mock and pytest.mark.asyncio).

Test results

tests/test_services_transaction.py: 20 passed (previously 19 passed + 1 skipped)
Full suite: 925 passed, 2 skipped (6 pre-existing failures in test_commands_profile_images.py unrelated to this change)

Files changed

tests/test_services_transaction.py — removed skip, implemented test body

🤖 Generated with Claude Code

## Summary - Removes the `@pytest.mark.skip` from `test_validate_transaction_exception_handling` in `tests/test_services_transaction.py` - Implements the test using a `patch` on `services.transaction_service.RosterValidation` that raises `RuntimeError` on the first call, triggering the `except` clause in `validate_transaction` - Verifies the method returns `is_legal=False` with error message `"Validation error: Simulated validation failure"` instead of propagating the exception ## What was fixed The skipped test covered the critical exception-handling path at `services/transaction_service.py:187-192`. The `except Exception` clause catches any runtime error during validation and returns a failed `RosterValidation` — this is now covered by the test. The test follows the existing patterns in the file (using `patch` from `unittest.mock` and `pytest.mark.asyncio`). ## Test results ``` tests/test_services_transaction.py: 20 passed (previously 19 passed + 1 skipped) Full suite: 925 passed, 2 skipped (6 pre-existing failures in test_commands_profile_images.py unrelated to this change) ``` ## Files changed - `tests/test_services_transaction.py` — removed skip, implemented test body 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 1 commit 2026-03-05 08:04:52 +00:00

test: implement test_validate_transaction_exception_handling (#35 )

Build Docker Image / build (pull_request) Successful in 1m6s

Details

ebec9a720b

Removes the @pytest.mark.skip and implements the test using a
patched RosterValidation that raises on the first call, triggering
the except clause in validate_transaction. Verifies the method
returns is_legal=False with a descriptive error message.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

cal referenced this pull request

2026-03-05 08:04:58 +00:00

Implement `test_validate_transaction_exception_handling` #35

cal added the

ai-reviewing

label 2026-03-05 08:15:31 +00:00

cal reviewed 2026-03-05 08:16:25 +00:00

cal left a comment

AI Code Review

Files Reviewed

tests/test_services_transaction.py (modified)

Findings

Correctness

The core implementation is correct. Tracing the execution:

patched_rv is called when validate_transaction first instantiates RosterValidation inside the try block — this raises RuntimeError("Simulated validation failure").
The except Exception as e: clause at services/transaction_service.py:187 catches it and calls RosterValidation(is_legal=False, errors=[f"Validation error: {str(e)}"]) — this is the second call, which delegates to _real(...) and returns a genuine RosterValidation.
The post-with assertions check against the original unpatched RosterValidation class imported in the test file — isinstance works correctly because _real was captured before patching.

The call_count = [0] closure trick is idiomatic Python for mutable state in nested functions. The expected error string "Validation error: Simulated validation failure" exactly matches the f-string in the production except clause. All assertions are sound.

The test docstring explains both "what" and "why" per project conventions. ✓

Security

No issues found. Test file only; no production code changed.

Style & Conventions

The diff also reformats the entire existing file (single → double quotes, trailing commas, line-length splits). This is a purely mechanical style pass — no behavioral changes and nothing that contradicts CLAUDE.md conventions. Slightly noisy diff but harmless.

One pre-existing minor note (out of scope for this PR): import time remains inside the test_performance_with_large_dataset function body. Not introduced by this PR, no action needed.

Suggestions

No blocking suggestions. The call_count approach could alternatively use mock.call_count on a MagicMock wrapping _real, but the current pattern is clear and well-understood.

Verdict: COMMENT (APPROVED — self-review restriction)

Solid implementation. Removes a permanently-skipped test stub and replaces it with a well-targeted test that exercises the exact except Exception path. Logic is correct, docstring is thorough, test results confirm 20/20 passing. Ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `tests/test_services_transaction.py` (modified) ### Findings #### Correctness The core implementation is correct. Tracing the execution: 1. `patched_rv` is called when `validate_transaction` first instantiates `RosterValidation` inside the `try` block — this raises `RuntimeError("Simulated validation failure")`. 2. The `except Exception as e:` clause at `services/transaction_service.py:187` catches it and calls `RosterValidation(is_legal=False, errors=[f"Validation error: {str(e)}"])` — this is the second call, which delegates to `_real(...)` and returns a genuine `RosterValidation`. 3. The post-`with` assertions check against the original unpatched `RosterValidation` class imported in the test file — `isinstance` works correctly because `_real` was captured before patching. The `call_count = [0]` closure trick is idiomatic Python for mutable state in nested functions. The expected error string `"Validation error: Simulated validation failure"` exactly matches the f-string in the production except clause. All assertions are sound. The test docstring explains both "what" and "why" per project conventions. ✓ #### Security No issues found. Test file only; no production code changed. #### Style & Conventions The diff also reformats the entire existing file (single → double quotes, trailing commas, line-length splits). This is a purely mechanical style pass — no behavioral changes and nothing that contradicts CLAUDE.md conventions. Slightly noisy diff but harmless. One pre-existing minor note (out of scope for this PR): `import time` remains inside the `test_performance_with_large_dataset` function body. Not introduced by this PR, no action needed. #### Suggestions No blocking suggestions. The `call_count` approach could alternatively use `mock.call_count` on a `MagicMock` wrapping `_real`, but the current pattern is clear and well-understood. ### Verdict: COMMENT (APPROVED — self-review restriction) Solid implementation. Removes a permanently-skipped test stub and replaces it with a well-targeted test that exercises the exact `except Exception` path. Logic is correct, docstring is thorough, test results confirm 20/20 passing. Ready to merge. --- *Automated review by Claude PR Reviewer*

cal added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-05 08:16:43 +00:00

cal changed target branch from main to next-release

2026-03-07 03:31:44 +00:00

cal merged commit 1a6300854e into next-release

2026-03-07 03:32:24 +00:00

cal deleted branch ai/major-domo-v2#35

2026-03-07 03:32:24 +00:00

cal referenced this issue from a commit