(Feat): Add Unit Test Suite for RecommendationService / Recommendation Use Case Interactor - #184
Conversation
…on object using the helpers
…dation service for proper logic
…orreect input, simplfying it following ssr
…yer id was called in recommendation service
…er has no season data
…ns not used for unit test
WalkthroughEnforces a 9-player minimum in RecommendationService, removes intermediate logging/timing, adds in-memory mock repositories and helpers for tests, introduces comprehensive unit tests for RecommendationService, updates an integration test to use FastAPI dependency_overrides, and adds Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant API as FastAPI Endpoint
participant Service as RecommendationService
participant RHelper as RosterHelper (mock)
participant Repo as Repositories (mock)
Note over Test,API: Test posts 9 player_ids
Test->>API: POST /recommendations (9 ids)
API->>Service: recommend_players(player_ids)
Service->>RHelper: validate_player_ids(player_ids)
alt validation fails
RHelper-->>Service: raises InputValidationError
Service-->>API: 4xx error
else
Service->>Repo: get_players_seasons_data(player_ids)
Repo-->>Service: seasons data
Service->>RHelper: calculate_roster_averages(seasons)
RHelper-->>Service: roster averages
Service->>RHelper: compute_team_weakness_scores(...)
RHelper-->>Service: weakness scores
Service->>Service: filter candidates by weakest position
loop per candidate
Service->>RHelper: compute_adjustment_sum(candidate_stats,...)
RHelper-->>Service: adjustment score & contributions
end
Service->>Service: rank candidates (highest contributions first)
Service-->>API: List[PlayerSearchResult]
end
API-->>Test: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 9
🧹 Nitpick comments (3)
server/services/tests/test_recommendation_service.py (3)
13-18: Fix line formatting and improve readability.Line 15 is excessively long and difficult to read. Consider breaking the dictionary initialization across multiple lines.
Apply this diff to improve readability:
def create_season(year: int, **stats) -> Dict: """Create a season dict with default stats if not provided""" - defaults = {"strikeout_rate": 0.20,"walk_rate": 0.08,"isolated_power": 0.15,"on_base_percentage": 0.32,"base_running": 0.0, - "plate_appearances": 500 } + defaults = { + "strikeout_rate": 0.20, + "walk_rate": 0.08, + "isolated_power": 0.15, + "on_base_percentage": 0.32, + "base_running": 0.0, + "plate_appearances": 500 + } defaults.update(stats) return defaults
105-106: Improve comment clarity.The comment has a grammatical issue and unclear phrasing.
assert "A valid roster must contain at least 9 players" in str(exc_info.value) - # cant check full string cause of the full error string is included iwth the input validation erro + # Note: Only checking partial string since InputValidationError wraps the full message
220-222: Improve comment clarity.The comment has awkward phrasing that makes it harder to understand.
- """If the weakest player which means, lowest adjustment score, has no position, input - validation error is raised as the algorithm could not and would not accuartely return the recommendations""" + """If the weakest player (lowest adjustment score) has no position, InputValidationError + is raised because the recommendation algorithm cannot accurately determine replacements"""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/pytest.ini(1 hunks)server/services/recommendation_service.py(3 hunks)server/services/tests/mocks/mock_repositories.py(1 hunks)server/services/tests/mocks/mock_use_case_helpers.py(1 hunks)server/services/tests/test_recommendation_service.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/services/tests/mocks/mock_use_case_helpers.py (4)
server/dtos/roster_dtos.py (2)
RosterAvgResponse(58-66)PlayerAvgStats(45-56)server/useCaseHelpers/roster_helper.py (1)
RosterDomain(6-269)server/useCaseHelpers/player_helper.py (1)
PlayerDomain(8-195)server/useCaseHelpers/errors.py (1)
InputValidationError(19-21)
server/services/tests/mocks/mock_repositories.py (3)
server/repositories/player_repository.py (1)
PlayerRepository(4-30)server/repositories/roster_avg_repository.py (1)
RosterRepository(4-28)client/src/api/client.ts (1)
get(58-61)
server/services/recommendation_service.py (1)
server/useCaseHelpers/errors.py (2)
InputValidationError(19-21)QueryError(29-31)
🪛 Ruff (0.14.5)
server/services/tests/test_recommendation_service.py
13-13: Unused function argument: year
(ARG001)
21-21: Unused function argument: player_id
(ARG001)
196-196: Unused method argument: mock_roster_helper
(ARG002)
196-196: Unused method argument: mock_player_repo
(ARG002)
server/services/tests/mocks/mock_use_case_helpers.py
27-27: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Unused method argument: players_stats
(ARG002)
61-61: Unused method argument: team_avg
(ARG002)
61-61: Unused method argument: league_avg
(ARG002)
61-61: Unused method argument: league_std
(ARG002)
76-76: Unused method argument: seasons
(ARG002)
92-92: Unused method argument: player_latest_stats
(ARG002)
93-93: Unused method argument: league_avg
(ARG002)
94-94: Unused method argument: league_std
(ARG002)
95-95: Unused method argument: team_weakness
(ARG002)
114-114: Unused method argument: player_data
(ARG002)
114-114: Unused method argument: seasons
(ARG002)
server/services/recommendation_service.py
51-51: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Local variable start_time is assigned to but never used
Remove assignment to unused variable start_time
(F841)
78-78: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (13)
server/pytest.ini (1)
29-30: LGTM! Standard pytest configuration.This addition allows tests to import project modules without requiring external PYTHONPATH setup, which is a common and recommended practice for pytest projects.
server/services/tests/test_recommendation_service.py (5)
57-89: LGTM! Well-structured pytest fixtures.The fixtures properly instantiate mock dependencies and compose them into the service under test, following pytest best practices for dependency injection.
92-158: LGTM! Comprehensive validation tests.The test class properly validates both the error case (< 9 players) and the success case (exactly 9 players). The assertions correctly verify that:
- InputValidationError is raised for insufficient players
- Exactly 5 RF recommendations are returned for valid rosters
- Only position-matched candidates are included in results
160-189: LGTM! Important safeguard test.This test ensures
validate_player_idsis always called, preventing future regressions where player IDs might be processed without validation.
191-211: LGTM! Proper error handling test.The test correctly validates that missing season data triggers a QueryError, ensuring data integrity requirements are enforced.
213-249: LGTM! Edge case properly handled.The test correctly validates that the service raises InputValidationError when it cannot determine a position for the weakest player, preventing invalid recommendations.
server/services/recommendation_service.py (3)
50-51: LGTM! Essential roster validation added.The validation correctly enforces a minimum roster size of 9 players before processing, preventing invalid recommendations. This aligns with the test expectations in the test suite.
78-78: LGTM! Clearer error message.The revised error message is more specific and user-friendly, clearly indicating that the player has no season data available for recommendations.
165-165: LGTM! Simplified return.Direct return of results is cleaner now that intermediate logging has been removed.
server/services/tests/mocks/mock_repositories.py (2)
1-47: LGTM! Well-structured mock repository.MockRosterRepository provides clean in-memory implementations with helper methods for test data setup. The async methods correctly match the repository interface.
49-75: LGTM! Clean mock player repository.The implementation properly supports in-memory player storage with both list and dict-based lookups, matching the repository interface requirements.
server/services/tests/mocks/mock_use_case_helpers.py (2)
9-104: LGTM! Comprehensive roster helper mock.The mock provides configurable behavior for all roster domain operations, enabling thorough testing of different scenarios including validation errors, default calculations, and cached results.
106-161: LGTM! Complete player helper mock.MockPlayerHelper properly implements position retrieval and search result building with configurable behavior for testing edge cases. The years_active calculation logic is correct.
…t least 9 batters to avoid input validation error and remove @patch to fastapi dependency override to properly add the depednecy into the test and use the mock
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/tests/integration/test_recommendations_integration.py (1)
7-7: Remove unusedpatchimport.The
patchimport is no longer used after migrating to FastAPI's dependency override approach.Apply this diff:
-from unittest.mock import patch, Mock, AsyncMock +from unittest.mock import Mock, AsyncMock
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/tests/integration/test_recommendations_integration.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/tests/integration/test_recommendations_integration.py (1)
server/dependency/dependencies.py (1)
get_recommendation_service(90-96)
🔇 Additional comments (1)
server/tests/integration/test_recommendations_integration.py (1)
62-62: LGTM: Clean signature after removing patch-based mocking.Removing the
mock_service_getterparameter is consistent with migrating to dependency overrides.
… use case interactor used in debugging
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/services/recommendation_service.py (1)
48-49: Roster size check is good; consider centralizing the “9 players” requirement.The explicit
< 9guard with anInputValidationErrorgives a clear, fast‑fail path for undersized rosters and matches the domain expectation of a 9‑player lineup. To avoid “9” becoming a magic number duplicated across services, helpers, and tests, consider lifting it into a shared constant (e.g., onRosterDomainor a settings module) and referencing that here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/services/recommendation_service.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/services/recommendation_service.py (1)
server/useCaseHelpers/errors.py (2)
InputValidationError(19-21)QueryError(29-31)
🪛 Ruff (0.14.5)
server/services/recommendation_service.py
49-49: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
server/services/recommendation_service.py (2)
72-75: ClearerQueryErrormessage for missing season data.Raising
QueryErrorwhenseasons is Nonewith a specific message about missing season data forplayer_idis sensible and should make downstream error handling and debugging easier. No functional issues from this change.
161-163: Explicitreturn resultsis appropriate.Returning the accumulated
resultslist at the end cleanly handles both the “no suitable candidates” case (empty list) and normal flows. No further changes needed here.
…e case interactor code for more intuitive approach and understanding
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/services/recommendation_service.py (2)
5-6: Remove unused imports.Both
loggingandtimeare imported but never used after the logging and timing code was removed.Apply this diff to remove the unused imports:
-import logging -import time - from dtos.player_dtos import PlayerSearchResult
32-46: Fix typos in docstring.The docstring contains several typographical errors that should be corrected:
- Line 33: "recommmended" → "recommended", "rooster" → "roster"
- Line 37: "iwth" → "with"
- Line 38: "replcae" → "replace", "there positions" → "their positions"
- Line 42: "hypothetiically" → "hypothetically", "ne" → "new"
- Line 46: "higest" → "highest"
Apply this diff:
async def recommend_players(self, player_ids: List[int]) -> List[PlayerSearchResult]: """ - Return 5 recommmended players for there rooster based off of the + Return 5 recommended players for their roster based on the following: 1. Get current team weakness vector (weakness_s) → where each stat's value > 0 means team underperforms the league average. - 2. Find the player iwth the lowest adjustment score, this is the - player we will replcae and there positions + 2. Find the player with the lowest adjustment score; this is the + player we will replace and their positions 3. Fetch all players from firebase (there mlbid) whose position matches with what we need - 4. for each team hypothetiically create a ne weakness vector with this player instead of the replaced player + 4. For each team hypothetically create a new weakness vector with this player instead of the replaced player 5. Store the player id with the difference between the sum of old weakness vector - new weakness vector - 6. Return the top 5 mlbid with the higest difference (maybe in a hashmap with mlbid, and the difference)""" + 6. Return the top 5 mlbid with the highest difference (maybe in a hashmap with mlbid, and the difference)"""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/services/recommendation_service.py(4 hunks)server/tests/integration/test_recommendations_integration.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/tests/integration/test_recommendations_integration.py
🧰 Additional context used
🧬 Code graph analysis (1)
server/services/recommendation_service.py (1)
server/useCaseHelpers/errors.py (2)
InputValidationError(19-21)QueryError(29-31)
🪛 Ruff (0.14.5)
server/services/recommendation_service.py
49-49: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
server/services/recommendation_service.py (3)
48-49: LGTM!The minimum roster size validation correctly enforces the 9-player requirement as specified in the PR objectives and test suite.
74-74: LGTM!The error message is more specific and clearly indicates the failure reason.
161-161: LGTM!The simplified return statement is clean and appropriate after removing the timing/logging code.
…ks for edge cases in filtering candidates and another on skpping invalid candidates
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
server/services/tests/test_recommendation_service.py (3)
47-55: Normalize league std helper dict formatting and float literalThe helpers are fine functionally, but you can improve readability and fix the
0.literal increate_league_std:def create_league_avg() -> Dict[str, float]: """Create default league average stats""" - return {"strikeout_rate": 0.22,"walk_rate": 0.08,"isolated_power": 0.16,"on_base_percentage": 0.32,"base_running": 0.0} + return { + "strikeout_rate": 0.22, + "walk_rate": 0.08, + "isolated_power": 0.16, + "on_base_percentage": 0.32, + "base_running": 0.0, + } @@ def create_league_std() -> Dict[str, float]: """Create default league standard deviations""" - return {"strikeout_rate": 0.03,"walk_rate": 0.02,"isolated_power": 0.04,"on_base_percentage": 0.03,"base_running": 0.} + return { + "strikeout_rate": 0.03, + "walk_rate": 0.02, + "isolated_power": 0.04, + "on_base_percentage": 0.03, + "base_running": 0.0, + }
33-45: Fix typo in default-season commentMinor spelling fix to keep comments clear (matches earlier feedback):
- # dfault: 2023 sseason + # default: 2023 season
196-211: Clean up docstring/comment typos and unused fixture argumentsThe behavior under test is correct, but the doc/comment have typos, and Ruff flags
mock_roster_helperandmock_player_repoas unused. You can simplify the signature and fix the text:- async def test_missing_player_season_data_raises_query_error(self, service, mock_roster_repo, mock_roster_helper, mock_player_repo): - """Chceks if QueryError is raised when a player dosent have any data from any seasons""" + async def test_missing_player_season_data_raises_query_error(self, service, mock_roster_repo): + """Checks if QueryError is raised when a player doesn't have any data from any seasons""" @@ - # QueryError should be raised from to missing season data + # QueryError should be raised due to missing season dataThis keeps the fixture wiring intact (the
servicefixture still depends on the other mocks) while resolving ARG002 and making the test docs clearer.
🧹 Nitpick comments (4)
server/services/tests/test_recommendation_service.py (4)
13-30: Address unused parameters and avoid mutating caller-provided dicts in helpers
create_seasondoesn't useyear, andcreate_player_seasonsdoesn't useplayer_id; Ruff flags both as unused, andseason.pop("year", 2023)mutates the caller’s dict. You can keep behavior while quieting lint and avoiding mutation:-def create_season(year: int, **stats) -> Dict: - """Create a season dict with default stats if not provided""" - defaults = {"strikeout_rate": 0.20,"walk_rate": 0.08,"isolated_power": 0.15,"on_base_percentage": 0.32,"base_running": 0.0, - "plate_appearances": 500 } +def create_season(_year: int, **stats) -> Dict: + """Create a season dict with default stats if not provided""" + defaults = { + "strikeout_rate": 0.20, + "walk_rate": 0.08, + "isolated_power": 0.15, + "on_base_percentage": 0.32, + "base_running": 0.0, + "plate_appearances": 500, + } defaults.update(stats) return defaults @@ -def create_player_seasons(player_id: int, *seasons) -> Dict: +def create_player_seasons(_player_id: int, *seasons) -> Dict: @@ - elif isinstance(season, dict): - year = season.pop("year", 2023) - seasons_dict[str(year)] = create_season(year, **season) + elif isinstance(season, dict): + data = dict(season) + year = data.pop("year", 2023) + seasons_dict[str(year)] = create_season(year, **data)
110-158: Optionally assertPlayerSearchResulttype in the success-path testThe flow and expectations for
test_roster_with_9_players_passes_validationlook good and robust. Since you already importPlayerSearchResultand comment that results should be of that type, you could assert it explicitly:# should return exactly 5 players assert isinstance(result, list) assert len(result) == 5, f"Expected 5 recommendations, got {len(result)}" - # all results must be PlayerSearchResult and match the RF candidate IDs - returned_ids = [player.id for player in result] + # all results must be PlayerSearchResult and match the RF candidate IDs + assert all(isinstance(player, PlayerSearchResult) for player in result) + returned_ids = [player.id for player in result]
218-247: Tighten wording/typos in weakest-position test for clarityThe test logic is sound; only the docstring and comments are a bit hard to read. Consider:
- """If the weakest player which means, lowest adjustment score, has no position, input - validation error is raised as the algorithm could not and would return the recommendations""" + """If the weakest player (lowest adjustment score) has no position, an + InputValidationError is raised because the algorithm cannot determine + recommendations.""" @@ - # add season data to playes + # add season data to players @@ - # raise error when detected no position + # raise error when no position can be determined
252-335: Polish candidate-filtering test docs and commentsBoth candidate-filtering tests exercise the intended edge cases well. You can make them easier to read by fixing a few typos in class/test docs and comments:
-class TestCandidateFiltering: - """Tests for edge cases in filtering candidates and skipping invalid candidates that has the same positoin""" +class TestCandidateFiltering: + """Tests edge cases in filtering candidates and skipping invalid candidates + with the same position.""" @@ - # add mock mlb id with player inn position Rf + # add valid RF candidates @@ - # add candidate with invalid mlbam_id (here we do string ) which should be skipped + # add candidate with invalid mlbam_id (string) which should be skipped @@ - # mostly same setup as below with rest of the unit test + # mostly same setup as above as the rest of the unit test @@ - # player + rooster data setup + # player + roster data setup @@ - # add the RF candidates + # add the RF candidates @@ - # add candidate with valid mlbam_id bit no season data is added and thus shiud be skipped + # add candidate with valid mlbam_id but no season data; should be skipped
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/services/tests/test_recommendation_service.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
server/services/tests/test_recommendation_service.py
13-13: Unused function argument: year
(ARG001)
21-21: Unused function argument: player_id
(ARG001)
196-196: Unused method argument: mock_roster_helper
(ARG002)
196-196: Unused method argument: mock_player_repo
(ARG002)
🔇 Additional comments (2)
server/services/tests/test_recommendation_service.py (2)
81-89: Fixtures and service wiring look solidThe
servicefixture correctly wires the four mock dependencies and keeps tests isolated via per-test fixture instantiation. No changes needed here.
160-190: Good pattern for asserting helper invocation
test_recommend_calls_validate_player_idsusespatch.objecton the helper instance andassert_called_once_with, which is exactly what you want to guard against accidental removal of that validation. No issues here.
…ics for the recommendation use case interactor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/services/recommendation_service.py (4)
5-6: Remove unused imports.The
loggingandtimeimports are no longer used after the cleanup that removed timing and logging statements. These should be removed to keep the code clean.-import logging -import time
31-47: Fix typos in docstring.The docstring contains several spelling and grammar issues that should be corrected for clarity:
- Line 33: "rooster" → "roster", "there" → "their"
- Line 38: "iwth" → "with"
- Line 39: "replcae" → "replace", "there" → "their"
- Line 41: "there" → "their"
- Line 43: "hypothetiically" → "hypothetically"
- Line 47: "higest" → "highest"
async def recommend_players(self, player_ids: List[int]) -> List[PlayerSearchResult]: """ - Return 5 recommmended players for there rooster based off of the + Return 5 recommended players for their roster based off of the 1. Get current team weakness vector (weakness_s) → where each stat's value > 0 means team underperforms the league average. - 2. Find the player iwth the lowest adjustment score, this is the - player we will replcae and there positions + 2. Find the player with the lowest adjustment score, this is the + player we will replace and their positions - 3. Fetch all players from firebase (there mlbid) whose position matches with what we need + 3. Fetch all players from firebase (their mlbid) whose position matches with what we need - 4. for each team hypothetiically create a new weakness vector with this player instead of the replaced player + 4. for each team hypothetically create a new weakness vector with this player instead of the replaced player 5. Store the player id with the difference between the new weakness vector - old weakness vector - 6. Return the top 5 mlbid with the higest difference""" + 6. Return the top 5 mlbid with the highest difference"""
146-156: Algorithm change is correctly documented and implemented.The contribution calculation and sorting now align with the updated z-score semantics where higher values indicate better performance. The inline comments explaining this change are helpful for future maintainers.
Minor typos in the comments: "ALGOIRTHM" → "ALGORITHM", "veector" → "vector".
- # NOTE: DUE TO ALGOIRTHM CHANGE, weakness veector with higher values are good and lower is bad since we are using + # NOTE: DUE TO ALGORITHM CHANGE, weakness vector with higher values are good and lower is bad since we are using
111-112: Update stale comment to match new algorithm.This comment still describes the old formula ("difference between the sum of original weakness vector and sum of this vector"), but the algorithm now calculates
new - old. Update for consistency with line 146.- player_contributions = {} # dictionary of key is mlbam_id and value is the difference between the sum of original weakness vector - # and sum of this vector + player_contributions = {} # dictionary of key is mlbam_id and value is the difference between the sum of new weakness vector + # and sum of original vector (new - old); higher values indicate better candidates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/services/recommendation_service.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/services/recommendation_service.py (1)
server/useCaseHelpers/errors.py (2)
InputValidationError(19-21)QueryError(29-31)
🪛 Ruff (0.14.5)
server/services/recommendation_service.py
50-50: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
server/services/recommendation_service.py (1)
49-50: LGTM!The roster size validation is correctly placed at the start of the method for fail-fast behavior, and the error message is clear.
2 participants
Summary
Added isolated unit tests for recommendation use case interactor with mock repositories and use case helpers.
Changes
Files Added
Closes #183
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.