fix: remove unimplementable skipped caching tests (#33) #58

cal · 2026-03-06T16:33:43Z

cal commented

2026-03-06 16:33:43 +00:00

Summary

Removes the TestPlayerServiceCache class and its three @pytest.mark.skip tests from tests/unit/test_player_service.py

Why removed rather than implemented

The three skipped tests required:

test_cache_set_on_read — get_players() to write results to cache. PlayerService.get_players is a classmethod with no cache access; adding read-through caching requires a non-trivial architectural change.
test_cache_invalidation_on_update — patch_player() to call invalidate_related_cache() using the injected cache from ServiceConfig. The write methods use temp_service = cls() which creates a fresh instance that doesn't inherit the injected cache — so invalidation calls would go to a different cache object than the one the test observes.
test_cache_hit_rate — Two calls to get_players to produce a cache hit on the second call. Same root issue as test 1.

Implementing these tests correctly would require refactoring the classmethod + cls() pattern to propagate the injected cache, which is out of scope for a tech-debt cleanup issue.

Files changed

tests/unit/test_player_service.py — removed TestPlayerServiceCache (43 lines)

Note on diff size

The pre-commit formatter hook (ruff/black) auto-reformatted the file on save, producing a larger diff than the functional change. The only intentional change is the removal of the cache test class.

Other observations

test_cache_clear_on_create in TestPlayerServiceValidation (kept, not skipped) passes because cache.get_calls() is non-empty after the explicit cache.set(...) call before create_players — not because the service itself calls the cache. This may be a misleading test but it's out of scope for this issue.

## Summary - Removes the `TestPlayerServiceCache` class and its three `@pytest.mark.skip` tests from `tests/unit/test_player_service.py` ## Why removed rather than implemented The three skipped tests required: 1. **`test_cache_set_on_read`** — `get_players()` to write results to cache. `PlayerService.get_players` is a classmethod with no cache access; adding read-through caching requires a non-trivial architectural change. 2. **`test_cache_invalidation_on_update`** — `patch_player()` to call `invalidate_related_cache()` using the *injected* cache from `ServiceConfig`. The write methods use `temp_service = cls()` which creates a fresh instance that doesn't inherit the injected cache — so invalidation calls would go to a different cache object than the one the test observes. 3. **`test_cache_hit_rate`** — Two calls to `get_players` to produce a cache hit on the second call. Same root issue as test 1. Implementing these tests correctly would require refactoring the classmethod + `cls()` pattern to propagate the injected cache, which is out of scope for a tech-debt cleanup issue. ## Files changed - `tests/unit/test_player_service.py` — removed `TestPlayerServiceCache` (43 lines) ## Note on diff size The pre-commit formatter hook (ruff/black) auto-reformatted the file on save, producing a larger diff than the functional change. The only intentional change is the removal of the cache test class. ## Other observations - `test_cache_clear_on_create` in `TestPlayerServiceValidation` (kept, not skipped) passes because `cache.get_calls()` is non-empty after the explicit `cache.set(...)` call *before* `create_players` — not because the service itself calls the cache. This may be a misleading test but it's out of scope for this issue.

cal added 1 commit 2026-03-06 16:33:44 +00:00

fix: remove unimplementable skipped caching tests (#33 )

Build Docker Image / build (pull_request) Successful in 2m7s

Details

716674f679

The three skipped tests in TestPlayerServiceCache required caching
in get_players() (read-through cache) and cache propagation through
the cls() pattern in write methods — neither is implemented and the
architecture does not support it without significant refactoring.

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

cal referenced this pull request

2026-03-06 16:33:53 +00:00

Implement or remove the three skipped caching tests in `test_player_service.py` #33

cal added the

ai-reviewing

label 2026-03-06 16:45:34 +00:00

cal reviewed 2026-03-06 16:46:24 +00:00

cal left a comment

AI Code Review

Files Reviewed

tests/unit/test_player_service.py (modified)

Findings

Correctness

Functional change is exactly what the PR claims: TestPlayerServiceCache (3 @pytest.mark.skip tests) removed.
The technical analysis in the PR body is accurate:
- get_players is a classmethod with no cache read-through — test_cache_set_on_read and test_cache_hit_rate cannot pass without architectural refactor.
- patch_player uses temp_service = cls() which creates a fresh instance that doesn't inherit the injected cache — test_cache_invalidation_on_update cannot observe the same cache object the test injects.
No tests that were previously running are removed. No coverage regresses.

Security

No issues found.

Style & Conventions

Remaining diff is ruff/black reformatting (single→double quotes, trailing commas, line wrapping) — expected per project setup, not functional.
Pre-existing, not introduced by this PR: EnhancedMockCache remains imported but unused (line 14 in new file). This predates this PR and is out of scope.

Suggestions

The PR body correctly flags test_cache_clear_on_create in TestPlayerServiceValidation as misleading (passes because cache.get_calls() is non-empty from the explicit cache.set(...) call before create_players, not from any service-driven cache invalidation). Worth tracking as a follow-up issue but out of scope here.

Verdict: APPROVED

Clean removal of permanently-skipped dead tests with well-documented rationale. The architectural constraints identified in the PR body are accurate. No running tests removed, no coverage lost.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `tests/unit/test_player_service.py` (modified) ### Findings #### Correctness - Functional change is exactly what the PR claims: `TestPlayerServiceCache` (3 `@pytest.mark.skip` tests) removed. - The technical analysis in the PR body is accurate: - `get_players` is a classmethod with no cache read-through — `test_cache_set_on_read` and `test_cache_hit_rate` cannot pass without architectural refactor. - `patch_player` uses `temp_service = cls()` which creates a fresh instance that doesn't inherit the injected cache — `test_cache_invalidation_on_update` cannot observe the same cache object the test injects. - No tests that were previously running are removed. No coverage regresses. #### Security - No issues found. #### Style & Conventions - Remaining diff is ruff/black reformatting (single→double quotes, trailing commas, line wrapping) — expected per project setup, not functional. - **Pre-existing, not introduced by this PR**: `EnhancedMockCache` remains imported but unused (line 14 in new file). This predates this PR and is out of scope. #### Suggestions - The PR body correctly flags `test_cache_clear_on_create` in `TestPlayerServiceValidation` as misleading (passes because `cache.get_calls()` is non-empty from the explicit `cache.set(...)` call before `create_players`, not from any service-driven cache invalidation). Worth tracking as a follow-up issue but out of scope here. ### Verdict: APPROVED Clean removal of permanently-skipped dead tests with well-documented rationale. The architectural constraints identified in the PR body are accurate. No running tests removed, no coverage lost. --- *Automated review by Claude PR Reviewer*

cal added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-06 16:46:45 +00:00

cal changed target branch from main to next-release

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

cal merged commit 47734efb2f into next-release

2026-03-10 14:40:18 +00:00

cal deleted branch ai/major-domo-database-33

2026-03-10 14:40:19 +00:00

cal referenced this issue from a commit