[DRAFT] Unit tests #87
Conversation
WalkthroughThis pull request introduces comprehensive changes to the testing infrastructure of the Transfermarkt API project. The modifications include adding a new GitHub Actions workflow for running tests, updating the project's dependency configuration, and restructuring the test suite. The changes involve removing several existing test files, introducing new JSON test data files, and creating new service-level test files that leverage a new Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
felipeall
force-pushed
the
refactor/tests
branch
from
327804c to
4e1ce4b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🛑 Comments failed to post (5)
tests/services/clubs/test_club_players.py (2)
4-7: 🛠️ Refactor suggestion
Add error handling test cases.
Missing test cases for error scenarios.
Example:
def test_club_players_invalid_id(): """Test handling of invalid club ID.""" tfmkt = TransfermarktClubPlayers(club_id="invalid", season_id="2023") with pytest.raises(ValueError): tfmkt.get_club_players() def test_club_players_invalid_season(): """Test handling of invalid season.""" tfmkt = TransfermarktClubPlayers(club_id="210", season_id="invalid") with pytest.raises(ValueError): tfmkt.get_club_players()
4-7: 🛠️ Refactor suggestion
Enhance test coverage with additional assertions and test cases.
The current test only verifies the club ID. Consider adding:
- Validation of the complete response structure
- Error handling test cases
- Edge cases (invalid IDs, seasons)
- Verification of actual player data
Example enhancement:
def test_club_players_210(): + """Test successful retrieval of players for club ID 210.""" tfmkt = TransfermarktClubPlayers(club_id="210", season_id="2023") response = tfmkt.get_club_players() assert response["id"] == "210" + assert "players" in response + assert isinstance(response["players"], list) + assert len(response["players"]) > 0 + + # Verify player data structure + player = response["players"][0] + required_fields = ["id", "name", "position"] + for field in required_fields: + assert field in player📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def test_club_players_210(): """Test successful retrieval of players for club ID 210.""" tfmkt = TransfermarktClubPlayers(club_id="210", season_id="2023") response = tfmkt.get_club_players() assert response["id"] == "210" assert "players" in response assert isinstance(response["players"], list) assert len(response["players"]) > 0 # Verify player data structure player = response["players"][0] required_fields = ["id", "name", "position"] for field in required_fields: assert field in player.github/workflows/tests.yml (3)
13-17: 🛠️ Refactor suggestion
Enhance Python setup with version and caching.
Add Python version specification and dependency caching for faster builds.
- uses: actions/setup-python@v5 + with: + python-version: '3.11' + cache: 'poetry' - name: Poetry run: | pip install poetry poetry install --no-interaction --no-root --with tests📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- uses: actions/setup-python@v5 with: python-version: '3.11' cache: 'poetry' - name: Poetry run: | pip install poetry poetry install --no-interaction --no-root --with tests🧰 Tools
🪛 actionlint (1.7.4)
13-13: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
12-13:
⚠️ Potential issueUpdate GitHub Actions versions.
The checkout and setup-python actions are using outdated versions.
- - uses: actions/checkout@v3 - - uses: actions/setup-python@v4 + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- uses: actions/checkout@v4 - uses: actions/setup-python@v5🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
13-13: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
18-20: 🛠️ Refactor suggestion
Add test reporting and coverage.
The current workflow doesn't capture test results or coverage information.
- name: Pytest run: | - poetry run pytest + poetry run pytest --junitxml=pytest.xml --cov=app --cov-report=xml + - name: Upload Test Results + uses: actions/upload-artifact@v4 + if: always() + with: + name: test-results + path: | + pytest.xml + coverage.xml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- name: Pytest run: | poetry run pytest --junitxml=pytest.xml --cov=app --cov-report=xml - name: Upload Test Results uses: actions/upload-artifact@v4 if: always() with: name: test-results path: | pytest.xml coverage.xml
felipeall
force-pushed
the
refactor/tests
branch
5 times, most recently
from
8ba8567 to
a2bb27e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/conftest.py (1)
8-16: Consider enhancing error handling for the test data loader.The implementation is clean and follows best practices. Consider adding error handling for common scenarios:
- File not found
- Invalid JSON format
- Unicode decode errors
@pytest.fixture def load_test_data() -> callable: def _load_test_data(file_name: str) -> dict: test_data_dir = Path(__file__).parent / "data" file_path = test_data_dir / f"{file_name}.json" + if not file_path.exists(): + raise FileNotFoundError(f"Test data file not found: {file_path}") with file_path.open("r", encoding="utf-8") as f: - return json.load(f) + try: + return json.load(f) + except json.JSONDecodeError as e: + raise ValueError(f"Invalid JSON in {file_path}: {e}") return _load_test_data.github/workflows/tests.yml (1)
10-10: Update the Ubuntu runner version.Consider using a more recent Ubuntu version for better security and performance.
- runs-on: ubuntu-20.04 + runs-on: ubuntu-22.04
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/workflows/tests.yml(1 hunks)pyproject.toml(0 hunks)tests/clubs/test_clubs_players.py(0 hunks)tests/clubs/test_clubs_profile.py(0 hunks)tests/clubs/test_clubs_search.py(0 hunks)tests/competitions/test_competitions_clubs.py(0 hunks)tests/competitions/test_competitions_search.py(0 hunks)tests/conftest.py(1 hunks)tests/data/clubs/players/131_2014.json(1 hunks)tests/data/clubs/players/131_None.json(1 hunks)tests/data/clubs/players/210_2017.json(1 hunks)tests/data/clubs/players/210_None.json(1 hunks)tests/players/test_players_achievements.py(0 hunks)tests/players/test_players_injuries.py(0 hunks)tests/players/test_players_market_value.py(0 hunks)tests/players/test_players_profile.py(0 hunks)tests/players/test_players_search.py(0 hunks)tests/players/test_players_stats.py(0 hunks)tests/players/test_players_transfers.py(0 hunks)tests/services/clubs/test_club_players.py(1 hunks)
💤 Files with no reviewable changes (13)
- pyproject.toml
- tests/players/test_players_injuries.py
- tests/players/test_players_search.py
- tests/competitions/test_competitions_clubs.py
- tests/clubs/test_clubs_profile.py
- tests/competitions/test_competitions_search.py
- tests/players/test_players_achievements.py
- tests/clubs/test_clubs_search.py
- tests/players/test_players_profile.py
- tests/players/test_players_market_value.py
- tests/clubs/test_clubs_players.py
- tests/players/test_players_transfers.py
- tests/players/test_players_stats.py
✅ Files skipped from review due to trivial changes (3)
- tests/data/clubs/players/131_None.json
- tests/data/clubs/players/131_2014.json
- tests/data/clubs/players/210_2017.json
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/tests.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
tests/services/clubs/test_club_players.py (1)
8-18: LGTM! Well-structured test parametrization.The test cases are well-organized with clear parameters and meaningful test data combinations.
tests/data/clubs/players/210_None.json (1)
1-584: LGTM! Comprehensive test data.The test data is well-structured and covers various scenarios:
- Multiple player positions
- Different contract situations
- Player status variations (injuries, loans)
- Various nationalities and age groups
| def test_club_players(club_id: str, season_id: str, load_test_data: callable): | ||
| tfmkt = TransfermarktClubPlayers(club_id=club_id, season_id=season_id) | ||
| response = tfmkt.get_club_players() | ||
| with open(f"tests/data/clubs/players/{club_id}_{season_id}.json", "w") as f: | ||
| json.dump(response, f, indent=2, ensure_ascii=False) | ||
| expected = load_test_data(f"clubs/players/{club_id}_{season_id}") | ||
|
|
||
| assert response == expected |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Separate test data generation from test execution.
Writing response data to files during test execution is an anti-pattern that:
- Makes tests dependent on file system operations
- May cause issues in parallel test execution
- Mixes test execution with test data management
Consider this alternative approach:
def test_club_players(club_id: str, season_id: str, load_test_data: callable):
tfmkt = TransfermarktClubPlayers(club_id=club_id, season_id=season_id)
response = tfmkt.get_club_players()
- with open(f"tests/data/clubs/players/{club_id}_{season_id}.json", "w") as f:
- json.dump(response, f, indent=2, ensure_ascii=False)
expected = load_test_data(f"clubs/players/{club_id}_{season_id}")
assert response == expectedCreate a separate utility script for generating test data when needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_club_players(club_id: str, season_id: str, load_test_data: callable): | |
| tfmkt = TransfermarktClubPlayers(club_id=club_id, season_id=season_id) | |
| response = tfmkt.get_club_players() | |
| with open(f"tests/data/clubs/players/{club_id}_{season_id}.json", "w") as f: | |
| json.dump(response, f, indent=2, ensure_ascii=False) | |
| expected = load_test_data(f"clubs/players/{club_id}_{season_id}") | |
| assert response == expected | |
| def test_club_players(club_id: str, season_id: str, load_test_data: callable): | |
| tfmkt = TransfermarktClubPlayers(club_id=club_id, season_id=season_id) | |
| response = tfmkt.get_club_players() | |
| expected = load_test_data(f"clubs/players/{club_id}_{season_id}") | |
| assert response == expected |
felipeall
force-pushed
the
refactor/tests
branch
2 times, most recently
from
f04e8a5 to
c24fc4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.github/workflows/tests.yml (1)
16-16: Consider using a more recent Python version.The workflow is using Python 3.9.16, which is quite old. Consider upgrading to a more recent version like 3.11 or 3.12 for better performance and security.
- python-version: "3.9.16" + python-version: "3.12"tests/services/clubs/test_club_players.py (2)
18-23: Consider enhancing test assertions.The current assertion only checks for exact equality. Consider adding more specific assertions to validate the structure and content of the response.
def test_club_players(club_id: str, season_id: str, load_test_data: callable): tfmkt = TransfermarktClubPlayers(club_id=club_id, season_id=season_id) response = tfmkt.get_club_players() expected = load_test_data(f"clubs/players/{club_id}_{season_id}") - assert response == expected + assert response["id"] == expected["id"], "Club ID mismatch" + assert isinstance(response["players"], list), "Players should be a list" + assert len(response["players"]) == len(expected["players"]), "Number of players mismatch" + + # Validate first player's data structure + if response["players"]: + player = response["players"][0] + assert all(key in player for key in ["id", "name", "position"]), "Missing required player fields"
26-41: Consider adding error case tests.The test covers only successful scenarios. Consider adding test cases for error conditions such as:
- Invalid club_id
- Non-existent club
- API errors
Example:
@pytest.mark.parametrize( "club_id,expected_error", [ ("invalid", "Invalid club ID format"), ("999999", "Club not found"), ], ) def test_clubs_profile_errors(club_id: str, expected_error: str): tfmkt = TransfermarktClubProfile(club_id=club_id) with pytest.raises(ValueError, match=expected_error): tfmkt.get_club_profile()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/tests.yml(1 hunks)pyproject.toml(0 hunks)tests/clubs/test_clubs_players.py(0 hunks)tests/clubs/test_clubs_profile.py(0 hunks)tests/clubs/test_clubs_search.py(0 hunks)tests/competitions/test_competitions_clubs.py(0 hunks)tests/competitions/test_competitions_search.py(0 hunks)tests/conftest.py(1 hunks)tests/data/clubs/players/11_2003.json(1 hunks)tests/data/clubs/players/131_2010.json(1 hunks)tests/data/clubs/players/131_None.json(1 hunks)tests/data/clubs/players/210_2017.json(1 hunks)tests/data/clubs/players/27_2019.json(1 hunks)tests/data/clubs/players/31_2018.json(1 hunks)tests/data/clubs/profile/11.json(1 hunks)tests/data/clubs/profile/131.json(1 hunks)tests/data/clubs/profile/210.json(1 hunks)tests/data/clubs/profile/27.json(1 hunks)tests/data/clubs/profile/31.json(1 hunks)tests/players/test_players_achievements.py(0 hunks)tests/players/test_players_injuries.py(0 hunks)tests/players/test_players_market_value.py(0 hunks)tests/players/test_players_profile.py(0 hunks)tests/players/test_players_search.py(0 hunks)tests/players/test_players_stats.py(0 hunks)tests/players/test_players_transfers.py(0 hunks)tests/services/clubs/test_club_players.py(1 hunks)
💤 Files with no reviewable changes (13)
- tests/players/test_players_injuries.py
- pyproject.toml
- tests/players/test_players_achievements.py
- tests/clubs/test_clubs_players.py
- tests/players/test_players_search.py
- tests/players/test_players_profile.py
- tests/players/test_players_stats.py
- tests/competitions/test_competitions_clubs.py
- tests/clubs/test_clubs_profile.py
- tests/competitions/test_competitions_search.py
- tests/clubs/test_clubs_search.py
- tests/players/test_players_market_value.py
- tests/players/test_players_transfers.py
✅ Files skipped from review due to trivial changes (7)
- tests/data/clubs/profile/131.json
- tests/data/clubs/profile/27.json
- tests/data/clubs/profile/11.json
- tests/data/clubs/profile/210.json
- tests/data/clubs/profile/31.json
- tests/data/clubs/players/31_2018.json
- tests/data/clubs/players/27_2019.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/data/clubs/players/131_None.json
- tests/conftest.py
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/tests.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (3)
.github/workflows/tests.yml (2)
21-23: Poetry configuration looks good.The configuration for Poetry is well-structured:
- Version is pinned to 1.5.1
- Virtual environments are created within the project directory
31-35: Test execution configuration is correct.The dependencies installation and test execution steps are properly configured:
- Uses
--with testsflag to include test dependencies- Executes pytest through Poetry
tests/services/clubs/test_club_players.py (1)
7-17: Well-structured test parametrization.The test parameters are well-organized with clear test cases and helpful comments indicating the club names.
| tests: | ||
| runs-on: ubuntu-20.04 | ||
| steps: | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update GitHub Actions to their latest versions.
The following actions have newer versions available:
- actions/checkout@v3 → v4
- actions/setup-python@v4 → v5
- actions/cache@v3 → v4
Apply this diff to update the actions:
- - uses: actions/checkout@v3
+ - uses: actions/checkout@v4
- - uses: actions/setup-python@v4
+ - uses: actions/setup-python@v5
- uses: actions/cache@v3
+ uses: actions/cache@v4Also applies to: 14-14, 26-26
🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| { | ||
| "id": "53309", | ||
| "name": "Jeffrén", | ||
| "position": "Right Winger", | ||
| "dateOfBirth": "Jan 20, 1988", | ||
| "age": "23", | ||
| "nationality": [ | ||
| "Venezuela", | ||
| "Spain" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,73m", | ||
| "foot": "left", | ||
| "joinedOn": "Aug 1, 2009", | ||
| "joined": "", | ||
| "signedFrom": "FC Barcelona Atlètic", | ||
| "contract": null, | ||
| "marketValue": "€3.00m", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "44675", | ||
| "name": "Bojan Krkic", | ||
| "position": "Second Striker", | ||
| "dateOfBirth": "Aug 28, 1990", | ||
| "age": "20", | ||
| "nationality": [ | ||
| "Spain", | ||
| "Serbia" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,70m", | ||
| "foot": "right", | ||
| "joinedOn": "Jul 1, 2013", | ||
| "joined": "", | ||
| "signedFrom": ": Ablöse €13.00m", | ||
| "contract": null, | ||
| "marketValue": "€15.00m", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "7980", | ||
| "name": "David Villa", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Dec 3, 1981", | ||
| "age": "29", | ||
| "nationality": [ | ||
| "Spain" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,74m", | ||
| "foot": "both", | ||
| "joinedOn": "Jul 1, 2010", | ||
| "joined": "", | ||
| "signedFrom": ": Ablöse €40.00m", | ||
| "contract": null, | ||
| "marketValue": "€50.00m", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "3455", | ||
| "name": "Zlatan Ibrahimović", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Oct 3, 1981", | ||
| "age": "29", | ||
| "nationality": [ | ||
| "Sweden", | ||
| "Bosnia-Herzegovina" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,95m", | ||
| "foot": "right", | ||
| "joinedOn": "Jul 27, 2009", | ||
| "joined": "", | ||
| "signedFrom": ": Ablöse €69.50m", | ||
| "contract": null, | ||
| "marketValue": "€35.00m", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "129476", | ||
| "name": "Gerard Deulofeu", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Mar 13, 1994", | ||
| "age": "17", | ||
| "nationality": [ | ||
| "Spain" | ||
| ], | ||
| "currentClub": "Udinese Calcio", | ||
| "height": "1,77m", | ||
| "foot": "right", | ||
| "joinedOn": "Jul 1, 2017", | ||
| "joined": "", | ||
| "signedFrom": "Everton FC", | ||
| "contract": null, | ||
| "marketValue": "-", | ||
| "status": "Knee surgery - Return unknown" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address data consistency issues in test fixtures.
The test data has several consistency issues that should be addressed:
- Inconsistent date formats: "Jan 14, 1982" vs empty strings
- Inconsistent market value formats: "€20.00m" vs "-"
- Empty strings vs null values
- Inconsistent status formats
Consider normalizing the data format:
{
"dateOfBirth": "1982-01-14", // ISO 8601 format
"marketValue": 20000000, // Numeric value in euros
"status": null, // null instead of empty string
"height": 1.83, // Numeric value in meters
}| { | ||
| "id": "3186", | ||
| "name": "Francis Jeffers", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Jan 25, 1981", | ||
| "age": "23", | ||
| "nationality": [ | ||
| "England" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,78m", | ||
| "foot": "right", | ||
| "joinedOn": "Jul 1, 2001", | ||
| "joined": "", | ||
| "signedFrom": ": Ablöse €15.30m", | ||
| "contract": null, | ||
| "marketValue": "-", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "3207", | ||
| "name": "Thierry Henry", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Aug 17, 1977", | ||
| "age": "26", | ||
| "nationality": [ | ||
| "France", | ||
| "Guadeloupe" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,88m", | ||
| "foot": "both", | ||
| "joinedOn": "Jan 6, 2012", | ||
| "joined": "", | ||
| "signedFrom": "New York Red Bulls", | ||
| "contract": null, | ||
| "marketValue": "-", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "3133", | ||
| "name": "Nwankwo Kanu", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Aug 1, 1976", | ||
| "age": "27", | ||
| "nationality": [ | ||
| "Nigeria" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,97m", | ||
| "foot": "right", | ||
| "joinedOn": "Jan 1, 1999", | ||
| "joined": "", | ||
| "signedFrom": ": Ablöse €6.25m", | ||
| "contract": null, | ||
| "marketValue": "-", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "3266", | ||
| "name": "Jérémie Aliadière", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Mar 30, 1983", | ||
| "age": "21", | ||
| "nationality": [ | ||
| "France", | ||
| "Algeria" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,83m", | ||
| "foot": "right", | ||
| "joinedOn": "Jul 1, 2001", | ||
| "joined": "", | ||
| "signedFrom": "Arsenal FC U18", | ||
| "contract": null, | ||
| "marketValue": "-", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "9329", | ||
| "name": "Michal Papadopulos", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Apr 14, 1985", | ||
| "age": "19", | ||
| "nationality": [ | ||
| "Czech Republic", | ||
| "Greece" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,83m", | ||
| "foot": "right", | ||
| "joinedOn": "Aug 1, 2003", | ||
| "joined": "", | ||
| "signedFrom": "FC Banik Ostrava", | ||
| "contract": null, | ||
| "marketValue": "-", | ||
| "status": "" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize transfer fee formats in test data.
The transfer fee format in signedFrom field is inconsistent:
: Ablöse €3.00m: Ablöse -- Empty strings
Standardize the format:
{
"signedFrom": {
"club": "FC Barcelona B",
"fee": 3000000, // Amount in euros, null if no fee
"currency": "EUR"
}
}| }, | ||
| { | ||
| "id": "195004", | ||
| "name": "Hernane", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Apr 8, 1986", | ||
| "age": "30", | ||
| "nationality": [ | ||
| "Brazil" | ||
| ], | ||
| "currentClub": "Nacional FC", | ||
| "height": "1,83m", | ||
| "foot": "right", | ||
| "joinedOn": "Feb 22, 2018", | ||
| "joined": "", | ||
| "signedFrom": "Esporte Clube Bahia", | ||
| "contract": null, | ||
| "marketValue": "€3.00m", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "58516", | ||
| "name": "Lucas Barrios", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Nov 13, 1984", | ||
| "age": "32", | ||
| "nationality": [ | ||
| "Paraguay", | ||
| "Argentina" | ||
| ], | ||
| "currentClub": "Retired", | ||
| "height": "1,89m", | ||
| "foot": "right", | ||
| "joinedOn": "Feb 24, 2017", | ||
| "joined": "", | ||
| "signedFrom": "Sociedade Esportiva Palmeiras", | ||
| "contract": null, | ||
| "marketValue": "€1.75m", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "379877", | ||
| "name": "Luis Henrique", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Mar 17, 1998", | ||
| "age": "18", | ||
| "nationality": [ | ||
| "Brazil", | ||
| "Italy" | ||
| ], | ||
| "currentClub": "Paternò Calcio", | ||
| "height": "1,84m", | ||
| "foot": "left", | ||
| "joinedOn": "", | ||
| "joined": "", | ||
| "signedFrom": "", | ||
| "contract": null, | ||
| "marketValue": "€750k", | ||
| "status": "unknown injury - Return unknown" | ||
| }, | ||
| { | ||
| "id": "203136", | ||
| "name": "Yuri Mamute", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "May 7, 1995", | ||
| "age": "21", | ||
| "nationality": [ | ||
| "Brazil" | ||
| ], | ||
| "currentClub": "SHB Da Nang FC", | ||
| "height": "1,80m", | ||
| "foot": "right", | ||
| "joinedOn": "Jan 1, 2013", | ||
| "joined": "", | ||
| "signedFrom": "Grêmio FBPA B (-2022)", | ||
| "contract": null, | ||
| "marketValue": "€650k", | ||
| "status": "" | ||
| }, | ||
| { | ||
| "id": "80815", | ||
| "name": "Jael", | ||
| "position": "Centre-Forward", | ||
| "dateOfBirth": "Oct 30, 1988", | ||
| "age": "28", | ||
| "nationality": [ | ||
| "Brazil" | ||
| ], | ||
| "currentClub": "Without Club", | ||
| "height": "1,86m", | ||
| "foot": "right", | ||
| "joinedOn": "Jan 18, 2017", | ||
| "joined": "", | ||
| "signedFrom": "Joinville Esporte Clube (SC)", | ||
| "contract": null, | ||
| "marketValue": "€450k", | ||
| "status": "" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize player status information.
The status field contains various formats:
- Injury information with inconsistent return dates
- Eligibility issues with varying formats
- Empty strings for no status
Standardize the status format:
{
"status": {
"type": "INJURY"|"INELIGIBLE"|"AVAILABLE",
"details": "Hip flexor problems", // Optional
"returnDate": "2025-01-13", // ISO 8601 format, optional
"reason": "Qatar Stars League" // Optional
}
}
felipeall
force-pushed
the
refactor/tests
branch
7 times, most recently
from
8e6497e to
f3158a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
.github/workflows/check.yml (4)
16-21: Update action version and consider Python version strategy
- The
actions/setup-pythonaction has a newer version available.- Consider using a minor version instead of a patch version for better maintainability.
Apply these changes:
- name: Set up Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: - python-version: '3.9.16' + python-version: '3.9' cache: 'pip'🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
22-26: Consider version pinning for development toolsWhile the setup is functional, consider pinning tool versions to ensure consistent behavior across different environments.
Consider either:
- Using a requirements-dev.txt file:
- name: Install dependencies run: | python -m pip install --upgrade pip - pip install ruff black interrogate + pip install -r requirements-dev.txt
- Or pinning versions directly:
- name: Install dependencies run: | python -m pip install --upgrade pip - pip install ruff black interrogate + pip install ruff==0.1.11 black==23.12.1 interrogate==1.5.0
27-34: Consider adding configuration files for toolsThe checks are well-organized, but consider moving tool configurations to dedicated config files for better maintainability.
- For Ruff: Add
ruff.tomlorpyproject.toml- For Black: Add
pyproject.toml- For Interrogate: Move settings to
pyproject.tomlExample
pyproject.toml:[tool.black] line-length = 88 target-version = ['py39'] [tool.interrogate] ignore-init-method = true ignore-init-module = true ignore-magic = false ignore-semiprivate = false ignore-private = false ignore-property-decorators = false ignore-module = false fail-under = 100 exclude = ["setup.py", "docs", "build"] verbose = 2 quiet = false color = true
14-14: Update checkout action versionThe
actions/checkoutaction has a newer version available.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
tests/services/test_competitions.py (1)
15-20: Consider adding response schema validationBoth test functions use exact matching (
assert response == expected), which makes tests brittle when dealing with dynamic data like market values or player counts that frequently change.Consider adding schema validation to ensure the response structure is correct while being more tolerant of value changes. Example:
from pydantic import BaseModel, Field class CompetitionResponse(BaseModel): name: str market_value: float = Field(gt=0) # Add other fields... def test_competitions_search(query: str, load_test_data: callable): tfmkt = TransfermarktCompetitionSearch(query=query) response = tfmkt.search_competitions() # Validate response structure CompetitionResponse.model_validate(response) # Test stable fields only expected = load_test_data(f"competitions/search/{query}") assert response["name"] == expected["name"]Also applies to: 31-36
tests/services/test_clubs.py (1)
1-60: Document test data structure in READMEThe tests rely heavily on JSON test data files, but their structure is not documented.
Consider adding a README.md in the tests/data directory explaining:
- The expected structure of JSON files
- Naming conventions for test data files
- How to generate/update test data
- Required fields and their types
Would you like me to help create this documentation?
.github/workflows/tests.yml (1)
34-35: Enhance test reporting and artifactsThe current test execution doesn't capture test results or generate reports.
Consider adding:
- JUnit XML reports for better CI integration
- Test coverage reports
- Test result artifacts
- - name: Run tests - run: poetry run pytest + - name: Run tests + run: | + poetry run pytest \ + --junitxml=junit/test-results.xml \ + --cov=app \ + --cov-report=xml \ + --cov-report=html + + - name: Upload test results + uses: actions/upload-artifact@v4 + if: always() # Run even if tests fail + with: + name: test-results + path: | + junit/test-results.xml + coverage.xml + htmlcov
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.github/workflows/check.yml(2 hunks).github/workflows/tests.yml(1 hunks)pyproject.toml(2 hunks)tests/clubs/test_clubs_players.py(0 hunks)tests/clubs/test_clubs_profile.py(0 hunks)tests/clubs/test_clubs_search.py(0 hunks)tests/competitions/test_competitions_clubs.py(0 hunks)tests/competitions/test_competitions_search.py(0 hunks)tests/conftest.py(1 hunks)tests/data/clubs/players/131_2010.json(1 hunks)tests/data/clubs/players/131_None.json(1 hunks)tests/data/clubs/players/210_2017.json(1 hunks)tests/data/clubs/players/27_2019.json(1 hunks)tests/data/clubs/players/31_2018.json(1 hunks)tests/data/clubs/players/5_2003.json(1 hunks)tests/data/clubs/profile/131.json(1 hunks)tests/data/clubs/profile/210.json(1 hunks)tests/data/clubs/profile/27.json(1 hunks)tests/data/clubs/profile/31.json(1 hunks)tests/data/clubs/profile/5.json(1 hunks)tests/data/clubs/search/Barcelona.json(1 hunks)tests/data/clubs/search/Bayern.json(1 hunks)tests/data/clubs/search/Grêmio.json(1 hunks)tests/data/clubs/search/Liverpool.json(1 hunks)tests/data/clubs/search/Milan.json(1 hunks)tests/data/competitions/clubs/BRA1.json(1 hunks)tests/data/competitions/clubs/ES1.json(1 hunks)tests/data/competitions/clubs/GB1.json(1 hunks)tests/data/competitions/search/Brasileirão.json(1 hunks)tests/data/competitions/search/La Liga.json(1 hunks)tests/data/competitions/search/Premier League.json(1 hunks)tests/players/test_players_achievements.py(0 hunks)tests/players/test_players_injuries.py(0 hunks)tests/players/test_players_market_value.py(0 hunks)tests/players/test_players_profile.py(0 hunks)tests/players/test_players_search.py(0 hunks)tests/players/test_players_stats.py(0 hunks)tests/players/test_players_transfers.py(0 hunks)tests/services/test_clubs.py(1 hunks)tests/services/test_competitions.py(1 hunks)
💤 Files with no reviewable changes (12)
- tests/players/test_players_injuries.py
- tests/competitions/test_competitions_clubs.py
- tests/players/test_players_search.py
- tests/clubs/test_clubs_profile.py
- tests/competitions/test_competitions_search.py
- tests/clubs/test_clubs_players.py
- tests/players/test_players_profile.py
- tests/players/test_players_achievements.py
- tests/clubs/test_clubs_search.py
- tests/players/test_players_transfers.py
- tests/players/test_players_stats.py
- tests/players/test_players_market_value.py
✅ Files skipped from review due to trivial changes (13)
- tests/data/clubs/search/Milan.json
- tests/data/clubs/search/Grêmio.json
- tests/data/competitions/search/Brasileirão.json
- tests/data/clubs/search/Bayern.json
- tests/data/clubs/search/Barcelona.json
- tests/data/clubs/search/Liverpool.json
- tests/data/competitions/clubs/BRA1.json
- tests/data/competitions/clubs/GB1.json
- tests/data/competitions/search/Premier League.json
- tests/data/competitions/search/La Liga.json
- tests/data/clubs/profile/5.json
- tests/data/competitions/clubs/ES1.json
- tests/data/clubs/players/5_2003.json
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/data/clubs/profile/27.json
- tests/data/clubs/profile/131.json
- tests/data/clubs/profile/210.json
- tests/data/clubs/players/131_None.json
- tests/data/clubs/players/131_2010.json
- tests/data/clubs/players/31_2018.json
- tests/data/clubs/profile/31.json
- tests/data/clubs/players/210_2017.json
- tests/data/clubs/players/27_2019.json
- pyproject.toml
- tests/conftest.py
🧰 Additional context used
🪛 GitHub Actions: Tests
tests/services/test_competitions.py
[error] 36-36: Test failure in test_competitions_search[Brasileirão]: Actual API response data doesn't match expected test data. Differences in player counts, market values, and mean market values across Brazilian league divisions.
🪛 actionlint (1.7.4)
.github/workflows/check.yml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/tests.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/check.yml (1)
Line range hint
1-11: LGTM! Good choice usingubuntu-latestUsing
ubuntu-latestensures you get the most recent stable Ubuntu version with security updates..github/workflows/tests.yml (1)
12-12: Update GitHub Actions to their latest versionsThe workflow uses older versions of GitHub Actions.
Also applies to: 14-14, 26-26
🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| "Brasileirão", | ||
| "Premier League", | ||
| "La Liga", |
There was a problem hiding this comment.
Fix failing test for "Brasileirão" search query
The test is failing due to differences in player counts, market values, and mean market values across Brazilian league divisions. This suggests that the test data might be outdated or the API response structure has changed.
Consider these approaches to fix the test:
- Update the test data to match the current API response
- Make the assertions more resilient to dynamic data by:
- Comparing only stable fields
- Using schema validation instead of exact matching
- Adding tolerance ranges for numerical values
Would you like me to help implement any of these solutions?
Also applies to: 34-36
| @pytest.mark.parametrize( | ||
| "club_id, season_id", | ||
| [ | ||
| ("5", "2003"), # Milan AC | ||
| ("27", "2019"), # FC Bayern Munich | ||
| ("31", "2018"), # Liverpool FC | ||
| ("131", "2010"), # Barcelona FC | ||
| ("131", None), # Barcelona FC | ||
| ("210", "2017"), # Grêmio FBPA | ||
| ], | ||
| ) | ||
| def test_club_players(club_id: str, season_id: str, load_test_data: callable): | ||
| tfmkt = TransfermarktClubPlayers(club_id=club_id, season_id=season_id) | ||
| response = tfmkt.get_club_players() | ||
| expected = load_test_data(f"clubs/players/{club_id}_{season_id}") | ||
|
|
||
| assert response == expected |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error case tests for club_players endpoint
The test cases cover only successful scenarios. Consider adding tests for error cases:
@pytest.mark.parametrize(
"club_id, season_id, expected_error",
[
("invalid", "2023", ValueError),
("210", "invalid", ValueError),
("999999", "2023", ValueError), # Non-existent club
],
)
def test_club_players_errors(club_id, season_id, expected_error):
tfmkt = TransfermarktClubPlayers(club_id=club_id, season_id=season_id)
with pytest.raises(expected_error):
tfmkt.get_club_players()
felipeall
force-pushed
the
refactor/tests
branch
from
f3158a5 to
403148b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
12-14: 🛠️ Refactor suggestionUpdate GitHub Actions versions.
The GitHub Actions versions are outdated and should be updated to their latest versions.
🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (9)
.github/workflows/tests.yml (4)
10-10: Consider upgrading to Ubuntu 22.04.Ubuntu 20.04 is approaching EOL in April 2024. Consider upgrading to Ubuntu 22.04 for longer support and security updates.
- runs-on: ubuntu-20.04 + runs-on: ubuntu-22.04
16-16: Consider upgrading Python version.Python 3.9.16 is quite old. Consider upgrading to a newer version (e.g., 3.12) to benefit from performance improvements and new features.
- python-version: "3.9.16" + python-version: "3.12"
18-23: Consider updating Poetry version.Poetry 1.5.1 could be updated to the latest version for bug fixes and improvements. The virtualenv configuration looks good.
- version: 1.5.1 + version: 1.7.1
34-35: Enhance test execution configuration.Consider adding test coverage reporting and improving test output visibility.
- run: poetry run pytest + run: poetry run pytest -v --cov=. --cov-report=xml --junit-xml=test-results.xmlThis change will:
- Increase test output verbosity (-v)
- Generate coverage reports (--cov)
- Create test results in JUnit format for better CI integration
.github/workflows/check.yml (5)
11-11: Consider pinning the Ubuntu version for reproducibility.While using
ubuntu-latestensures you get the most recent stable version, consider pinning to a specific version (e.g.,ubuntu-22.04) to ensure consistent behavior across workflow runs.- runs-on: ubuntu-latest + runs-on: ubuntu-22.04
14-14: Update checkout action to v4.The
actions/checkoutaction has a newer version (v4) available with performance improvements and bug fixes.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
16-20: Consider updating to a newer Python version.Python 3.9.16 is relatively old. Consider upgrading to Python 3.11 or 3.12 for better performance and access to newer language features.
with: - python-version: '3.9.16' + python-version: '3.11'🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-34: Consider adding configuration files for the linting tools.For better maintainability and consistency, consider adding configuration files:
pyproject.tomlfor Black configurationruff.tomlfor Ruff settings.interrogate.tomlfor Interrogate configurationThis would make it easier to maintain tool settings and share them with local development environments.
Would you like me to help generate these configuration files?
9-12: Add fail-fast and timeout configurations.Consider adding fail-fast and timeout configurations to improve workflow reliability:
jobs: check: + timeout-minutes: 10 runs-on: ubuntu-latest + strategy: + fail-fast: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.github/workflows/check.yml(2 hunks).github/workflows/tests.yml(1 hunks)pyproject.toml(2 hunks)tests/clubs/test_clubs_players.py(0 hunks)tests/clubs/test_clubs_profile.py(0 hunks)tests/clubs/test_clubs_search.py(0 hunks)tests/competitions/test_competitions_clubs.py(0 hunks)tests/competitions/test_competitions_search.py(0 hunks)tests/conftest.py(1 hunks)tests/data/clubs/players/131_2010.json(1 hunks)tests/data/clubs/players/131_None.json(1 hunks)tests/data/clubs/players/210_2017.json(1 hunks)tests/data/clubs/players/27_2019.json(1 hunks)tests/data/clubs/players/31_2018.json(1 hunks)tests/data/clubs/players/5_2003.json(1 hunks)tests/data/clubs/profile/131.json(1 hunks)tests/data/clubs/profile/210.json(1 hunks)tests/data/clubs/profile/27.json(1 hunks)tests/data/clubs/profile/31.json(1 hunks)tests/data/clubs/profile/5.json(1 hunks)tests/data/clubs/search/Barcelona.json(1 hunks)tests/data/clubs/search/Bayern.json(1 hunks)tests/data/clubs/search/Grêmio.json(1 hunks)tests/data/clubs/search/Liverpool.json(1 hunks)tests/data/clubs/search/Milan.json(1 hunks)tests/data/competitions/clubs/BRA1.json(1 hunks)tests/data/competitions/clubs/ES1.json(1 hunks)tests/data/competitions/clubs/GB1.json(1 hunks)tests/data/competitions/search/Brasileirão.json(1 hunks)tests/data/competitions/search/La Liga.json(1 hunks)tests/data/competitions/search/Premier League.json(1 hunks)tests/players/test_players_achievements.py(0 hunks)tests/players/test_players_injuries.py(0 hunks)tests/players/test_players_market_value.py(0 hunks)tests/players/test_players_profile.py(0 hunks)tests/players/test_players_search.py(0 hunks)tests/players/test_players_stats.py(0 hunks)tests/players/test_players_transfers.py(0 hunks)tests/services/test_clubs.py(1 hunks)tests/services/test_competitions.py(1 hunks)
💤 Files with no reviewable changes (12)
- tests/players/test_players_injuries.py
- tests/competitions/test_competitions_clubs.py
- tests/players/test_players_search.py
- tests/competitions/test_competitions_search.py
- tests/clubs/test_clubs_profile.py
- tests/players/test_players_market_value.py
- tests/players/test_players_achievements.py
- tests/clubs/test_clubs_players.py
- tests/clubs/test_clubs_search.py
- tests/players/test_players_profile.py
- tests/players/test_players_stats.py
- tests/players/test_players_transfers.py
🚧 Files skipped from review as they are similar to previous changes (26)
- tests/data/clubs/search/Barcelona.json
- tests/data/competitions/search/Premier League.json
- tests/data/clubs/search/Bayern.json
- tests/data/clubs/search/Grêmio.json
- tests/data/clubs/search/Milan.json
- tests/data/clubs/profile/5.json
- tests/data/competitions/clubs/GB1.json
- tests/data/clubs/profile/210.json
- tests/data/competitions/search/La Liga.json
- tests/data/clubs/profile/31.json
- tests/data/competitions/search/Brasileirão.json
- tests/data/clubs/profile/131.json
- tests/data/clubs/search/Liverpool.json
- tests/data/clubs/profile/27.json
- tests/data/clubs/players/5_2003.json
- tests/data/clubs/players/31_2018.json
- tests/data/clubs/players/131_2010.json
- tests/data/clubs/players/131_None.json
- tests/data/clubs/players/27_2019.json
- tests/data/clubs/players/210_2017.json
- tests/services/test_competitions.py
- tests/conftest.py
- tests/data/competitions/clubs/BRA1.json
- tests/data/competitions/clubs/ES1.json
- tests/services/test_clubs.py
- pyproject.toml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/check.yml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/tests.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/tests.yml (2)
1-6: LGTM! Well-configured workflow triggers.The workflow is properly configured to run on both push and pull request events targeting the main branch, ensuring comprehensive test coverage during development and integration.
25-30: LGTM! Effective caching strategy.The caching configuration is well-implemented, using the poetry.lock hash as the cache key and correctly targeting the virtualenv directory.
🧰 Tools
🪛 actionlint (1.7.4)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| - name: Install dependencies | ||
| run: | | ||
| pip install interrogate | ||
| interrogate app/services -vv | ||
| python -m pip install --upgrade pip | ||
| pip install ruff black interrogate |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin dependency versions for reproducibility.
Installing dependencies without version constraints can lead to inconsistent behavior across different environments and times.
run: |
python -m pip install --upgrade pip
- pip install ruff black interrogate
+ pip install ruff==0.1.9 black==23.12.1 interrogate==1.5.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install dependencies | |
| run: | | |
| pip install interrogate | |
| interrogate app/services -vv | |
| python -m pip install --upgrade pip | |
| pip install ruff black interrogate | |
| - name: Install dependencies | |
| run: | | |
| python -m pip install --upgrade pip | |
| pip install ruff==0.1.9 black==23.12.1 interrogate==1.5.0 |
felipeall
force-pushed
the
refactor/tests
branch
from
403148b to
f79562c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
tests/data/players/market_value/3373.json (2)
6-6: Consider standardizing data types and formats.A few suggestions to improve data consistency:
- Convert ages from strings to numbers (e.g.,
"24"→24) for easier numerical comparisons in tests.- Use a consistent format for market values, including the "Without Club" case (consider using
nullinstead of"-").Also applies to: 139-143
1-2: Consider adding metadata comments.It would be helpful to add a comment at the top of the file describing:
- The purpose of this test data
- The player it represents (appears to be Ronaldinho)
- The scenarios this data is designed to test
tests/data/players/injuries/8198.json (1)
12-13: Standardize empty value representation.The data shows inconsistent handling of empty values:
- Some records use empty string (
"") forgamesMissed- Some records use empty array (
[]) forgamesMissedClubsConsider standardizing empty value representation across all fields for better maintainability.
Also applies to: 32-33
tests/data/players/transfers/8198.json (2)
17-18: Standardize unknown value representation.The data shows inconsistent representation of unknown or empty values:
- Market values use "-"
- Fees use "-", "?" or "free transfer"
Consider standardizing the representation of unknown values across all fields.
Also applies to: 33-34, 97-98, 113-114, 129-130, 145-146, 161-162, 177-178, 193-194
197-200: Handle empty values in youthClubs array.The
youthClubsarray contains an empty string. Consider:
- Removing empty entries
- Using
nullfor unknown values- Documenting why empty strings are included
tests/data/players/achievements/68290.json (1)
9-12: Standardize season ID representation.The data shows inconsistent handling of season IDs:
- Some records have numeric IDs
- Some records have null IDs
- Both formats represent valid seasons
Consider standardizing the season ID representation across all records.
Also applies to: 21-24, 33-36, 45-48, 57-60, 69-72, 79-82, 89-92, 99-102, 109-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
.github/workflows/check.yml(2 hunks).github/workflows/tests.yml(1 hunks)pyproject.toml(2 hunks)tests/clubs/test_clubs_players.py(0 hunks)tests/clubs/test_clubs_profile.py(0 hunks)tests/clubs/test_clubs_search.py(0 hunks)tests/competitions/test_competitions_clubs.py(0 hunks)tests/competitions/test_competitions_search.py(0 hunks)tests/conftest.py(1 hunks)tests/data/clubs/players/131_2010.json(1 hunks)tests/data/clubs/players/131_None.json(1 hunks)tests/data/clubs/players/210_2017.json(1 hunks)tests/data/clubs/players/27_2019.json(1 hunks)tests/data/clubs/players/31_2018.json(1 hunks)tests/data/clubs/players/5_2003.json(1 hunks)tests/data/clubs/profile/131.json(1 hunks)tests/data/clubs/profile/210.json(1 hunks)tests/data/clubs/profile/27.json(1 hunks)tests/data/clubs/profile/31.json(1 hunks)tests/data/clubs/profile/5.json(1 hunks)tests/data/clubs/search/Barcelona.json(1 hunks)tests/data/clubs/search/Bayern.json(1 hunks)tests/data/clubs/search/Grêmio.json(1 hunks)tests/data/clubs/search/Liverpool.json(1 hunks)tests/data/clubs/search/Milan.json(1 hunks)tests/data/competitions/clubs/BRA1.json(1 hunks)tests/data/competitions/clubs/ES1.json(1 hunks)tests/data/competitions/clubs/GB1.json(1 hunks)tests/data/competitions/search/Brasileirão.json(1 hunks)tests/data/competitions/search/La Liga.json(1 hunks)tests/data/competitions/search/Premier League.json(1 hunks)tests/data/players/achievements/28003.json(1 hunks)tests/data/players/achievements/3373.json(1 hunks)tests/data/players/achievements/68290.json(1 hunks)tests/data/players/achievements/8198.json(1 hunks)tests/data/players/injuries/28003.json(1 hunks)tests/data/players/injuries/3373.json(1 hunks)tests/data/players/injuries/68290.json(1 hunks)tests/data/players/injuries/8198.json(1 hunks)tests/data/players/jersey_numbers/28003.json(1 hunks)tests/data/players/jersey_numbers/3373.json(1 hunks)tests/data/players/jersey_numbers/68290.json(1 hunks)tests/data/players/jersey_numbers/8198.json(1 hunks)tests/data/players/market_value/28003.json(1 hunks)tests/data/players/market_value/3373.json(1 hunks)tests/data/players/market_value/68290.json(1 hunks)tests/data/players/market_value/8198.json(1 hunks)tests/data/players/profile/28003.json(1 hunks)tests/data/players/profile/3373.json(1 hunks)tests/data/players/profile/68290.json(1 hunks)tests/data/players/profile/8198.json(1 hunks)tests/data/players/search/Cristiano.json(1 hunks)tests/data/players/search/Messi.json(1 hunks)tests/data/players/search/Neymar.json(1 hunks)tests/data/players/search/Ronaldo.json(1 hunks)tests/data/players/stats/28003.json(1 hunks)tests/data/players/stats/3373.json(1 hunks)tests/data/players/stats/68290.json(1 hunks)tests/data/players/stats/8198.json(1 hunks)tests/data/players/transfers/28003.json(1 hunks)tests/data/players/transfers/3373.json(1 hunks)tests/data/players/transfers/68290.json(1 hunks)tests/data/players/transfers/8198.json(1 hunks)tests/players/test_players_achievements.py(0 hunks)tests/players/test_players_injuries.py(0 hunks)tests/players/test_players_market_value.py(0 hunks)tests/players/test_players_profile.py(0 hunks)tests/players/test_players_search.py(0 hunks)tests/players/test_players_stats.py(0 hunks)tests/players/test_players_transfers.py(0 hunks)tests/services/test_clubs.py(1 hunks)tests/services/test_competitions.py(1 hunks)tests/services/test_players.py(1 hunks)
💤 Files with no reviewable changes (12)
- tests/players/test_players_injuries.py
- tests/players/test_players_search.py
- tests/competitions/test_competitions_clubs.py
- tests/clubs/test_clubs_profile.py
- tests/competitions/test_competitions_search.py
- tests/clubs/test_clubs_search.py
- tests/players/test_players_profile.py
- tests/clubs/test_clubs_players.py
- tests/players/test_players_market_value.py
- tests/players/test_players_transfers.py
- tests/players/test_players_achievements.py
- tests/players/test_players_stats.py
✅ Files skipped from review due to trivial changes (26)
- tests/data/players/jersey_numbers/3373.json
- tests/data/players/jersey_numbers/8198.json
- tests/data/players/jersey_numbers/28003.json
- tests/data/players/profile/8198.json
- tests/data/players/jersey_numbers/68290.json
- tests/data/players/injuries/3373.json
- tests/data/players/search/Neymar.json
- tests/data/players/transfers/3373.json
- tests/data/players/achievements/3373.json
- tests/data/players/profile/68290.json
- tests/data/players/profile/28003.json
- tests/data/players/stats/8198.json
- tests/data/players/market_value/28003.json
- tests/data/players/injuries/68290.json
- tests/data/players/stats/68290.json
- tests/data/players/search/Messi.json
- tests/data/players/stats/28003.json
- tests/data/players/search/Cristiano.json
- tests/data/players/stats/3373.json
- tests/data/players/injuries/28003.json
- tests/data/players/search/Ronaldo.json
- tests/data/players/transfers/68290.json
- tests/data/players/market_value/68290.json
- tests/data/players/market_value/8198.json
- tests/data/players/transfers/28003.json
- tests/data/players/profile/3373.json
🚧 Files skipped from review as they are similar to previous changes (25)
- tests/data/clubs/search/Milan.json
- tests/data/clubs/search/Barcelona.json
- tests/data/clubs/search/Bayern.json
- tests/data/competitions/search/Brasileirão.json
- tests/data/competitions/search/Premier League.json
- tests/data/clubs/profile/5.json
- tests/data/competitions/search/La Liga.json
- tests/data/clubs/search/Grêmio.json
- tests/data/clubs/players/131_None.json
- tests/data/clubs/players/27_2019.json
- tests/data/clubs/players/131_2010.json
- tests/data/clubs/search/Liverpool.json
- tests/data/clubs/players/5_2003.json
- tests/data/clubs/profile/131.json
- tests/data/clubs/profile/27.json
- tests/data/clubs/players/31_2018.json
- tests/data/clubs/profile/31.json
- tests/data/clubs/profile/210.json
- tests/data/clubs/players/210_2017.json
- tests/services/test_clubs.py
- tests/data/competitions/clubs/GB1.json
- pyproject.toml
- tests/data/competitions/clubs/ES1.json
- tests/services/test_competitions.py
- tests/data/competitions/clubs/BRA1.json
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/services/test_players.py
103-103: Undefined name load_test_data
(F821)
🪛 GitHub Actions: Code Check
tests/services/test_players.py
[error] 103-103: Undefined name load_test_data. The function load_test_data is referenced but not defined or imported.
🪛 actionlint (1.7.4)
.github/workflows/check.yml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/tests.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (11)
tests/data/players/market_value/3373.json (1)
1-147: LGTM! Well-structured test data.The JSON structure is well-organized and provides comprehensive market value history data suitable for testing various scenarios.
tests/services/test_players.py (1)
1-156: Well-structured test suite with comprehensive coverage!The test implementation follows best practices:
- Parameterized tests for multiple players
- Consistent test structure across all player services
- Data-driven testing using JSON fixtures
- Strong validation using Pydantic models
🧰 Tools
🪛 Ruff (0.8.2)
103-103: Undefined name
load_test_data(F821)
🪛 GitHub Actions: Code Check
[error] 103-103: Undefined name
load_test_data. The functionload_test_datais referenced but not defined or imported..github/workflows/check.yml (1)
24-25: Pin dependency versions for reproducibility.Installing dependencies without version constraints can lead to inconsistent behavior across different environments and times.
.github/workflows/tests.yml (2)
1-35: Well-structured test workflow!The workflow implementation follows best practices:
- Uses Poetry for dependency management
- Implements caching for dependencies
- Separates test dependencies installation
- Clear and focused test execution step
🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
12-30: 🛠️ Refactor suggestionUpdate GitHub Actions to their latest versions.
The actions being used are outdated according to static analysis.
Apply this diff to update the actions:
- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - - uses: actions/setup-python@v4 + - uses: actions/setup-python@v5 - uses: actions/cache@v3 + uses: actions/cache@v4Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
tests/conftest.py (1)
1-37: Excellent fixture implementation!The fixtures follow best practices:
- Well-documented purpose
- Clean and focused implementation
- Proper resource cleanup in override_pydantic_eq
- Type hints for better code clarity
tests/data/players/injuries/8198.json (1)
7-14: Review future dates in test data.The injury record contains dates in the future (24/25 season), which might affect test reliability. Consider using historical data or documenting that this is intentional test data.
✅ Verification successful
Future dates are intentional test fixtures
The presence of "24/25" season data is consistent across multiple test files (stats, injuries, jersey numbers, achievements) and players, following the same structure as historical data. This systematic pattern indicates these are intentionally designed test fixtures to verify the system's handling of future seasons.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other test files containing future dates # Expected: No other occurrences of future dates in test data rg -l "24/25" tests/data/Length of output: 388
Script:
#!/bin/bash # Check the structure of files containing future dates for file in $(rg -l "24/25" tests/data/); do echo "=== $file ===" head -n 20 "$file" echo -e "\n" doneLength of output: 5513
tests/data/players/transfers/8198.json (1)
3-196: Verify chronological order of transfers.The transfers appear to be in reverse chronological order. Let's verify this is consistent across all test data files.
✅ Verification successful
Chronological ordering verified
All transfer records across the test data files are correctly maintained in reverse chronological order (newest to oldest).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify chronological ordering in transfer data files # Expected: All transfer records should be in reverse chronological order # Find all transfer data files fd -t f . tests/data/players/transfers/ -x sh -c ' echo "Checking {}" jq -r ".transfers[] | .date" {} | while read -r date; do date -d "$date" +%s done | awk "{ if (NR>1 && prev<\$1) { print \"Order mismatch at line \" NR exit 1 } prev=\$1 }" 'Length of output: 476
tests/data/players/achievements/68290.json (1)
3-508: Verify achievement counts match details.Let's verify that the
countfield matches the length of thedetailsarray for each achievement.✅ Verification successful
Achievement counts verified successfully
All achievement counts correctly match their corresponding details array lengths in the file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify achievement counts match details array length # Expected: All count fields should match the number of details fd -t f . tests/data/players/achievements/ -x sh -c ' echo "Checking {}" jq -r ".achievements[] | select(.count != (.details | length)) | \"\(.title): count=\(.count) details=\(.details | length)\"" {} 'Length of output: 415
tests/data/players/achievements/8198.json (1)
239-249: Review duplicate season entries.The "Footballer of the Year" achievement contains duplicate entries for the 2007 season. Verify if this is intentional or needs deduplication.
✅ Verification successful
Duplicate seasons appear to be valid data entries
The duplicate season entries are consistent across multiple player achievement files and appear to be intentional, representing multiple instances of the same award won in a single season. This pattern is observed across different players and achievement types, suggesting it's a valid data design choice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate seasons within achievement details # Expected: No unintentional duplicate seasons fd -t f . tests/data/players/achievements/ -x sh -c ' echo "Checking {}" jq -r ".achievements[] | select(.details | map(.season.name) | group_by(.) | map(select(length > 1)) | length > 0) | \"\(.title): \(.details | map(.season.name) | group_by(.) | map(select(length > 1)))\"" {} 'Length of output: 911
tests/data/players/achievements/28003.json (1)
211-219: Review future dates in test data.The achievement record contains dates in the future (23/24 season). This is consistent with the injury data but might affect test reliability. Consider documenting that this is intentional test data.
✅ Verification successful
Future dates are part of the test data design
The presence of future dates (23/24 season) in the test data is intentional and consistent across multiple test files. This data is used for model validation testing and follows a systematic pattern across different entity types (achievements, injuries, transfers, etc.). No documentation is required as this is standard test fixture data used consistently throughout the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test files containing future dates # Expected: Document all occurrences of future dates rg -l "23/24|24/25" tests/data/Length of output: 598
Script:
#!/bin/bash # Check for documentation about test data rg -i "test.*(data|fixture)" README.md || echo "No matches in README.md" fd README tests/ cat tests/README.md 2>/dev/null || echo "No tests/README.md found" rg -i "test.*(data|fixture)" tests/ -g "!*.json"Length of output: 3269
| "clubId": "515" | ||
| } | ||
| ], | ||
| "ranking": {} |
There was a problem hiding this comment.
💡 Codebase verification
Add ranking data following the established structure
The empty ranking object deviates from the established pattern. Other test files consistently include:
- Worldwide ranking
- League ranking
- Club ranking
- Country ranking
- Position ranking
- Birth year ranking
🔗 Analysis chain
Consider adding ranking test data.
The empty ranking object might not provide sufficient test coverage. Consider adding sample ranking data to test ranking-related functionality.
Let me check if ranking data is used in the tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ranking-related test cases
rg -l "ranking" tests/
Length of output: 192
Script:
#!/bin/bash
# Examine ranking data structure in other test files
for file in tests/data/players/market_value/{68290,8198,28003}.json; do
echo "=== $file ==="
jq '.ranking' "$file"
done
Length of output: 1102
| def test_players_profile(player_id: str): | ||
| tfmkt = TransfermarktPlayerProfile(player_id=player_id) | ||
| response = PlayerProfile.model_validate(tfmkt.get_player_profile()) | ||
| expected = PlayerProfile.model_validate(load_test_data(f"players/profile/{player_id}")) |
There was a problem hiding this comment.
Add missing load_test_data parameter to function signature.
The test_players_profile function uses load_test_data but it's not declared in the function parameters.
Apply this diff to fix the function signature:
-def test_players_profile(player_id: str):
+def test_players_profile(player_id: str, load_test_data: callable):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_players_profile(player_id: str): | |
| tfmkt = TransfermarktPlayerProfile(player_id=player_id) | |
| response = PlayerProfile.model_validate(tfmkt.get_player_profile()) | |
| expected = PlayerProfile.model_validate(load_test_data(f"players/profile/{player_id}")) | |
| def test_players_profile(player_id: str, load_test_data: callable): | |
| tfmkt = TransfermarktPlayerProfile(player_id=player_id) | |
| response = PlayerProfile.model_validate(tfmkt.get_player_profile()) | |
| expected = PlayerProfile.model_validate(load_test_data(f"players/profile/{player_id}")) |
🧰 Tools
🪛 Ruff (0.8.2)
103-103: Undefined name load_test_data
(F821)
🪛 GitHub Actions: Code Check
[error] 103-103: Undefined name load_test_data. The function load_test_data is referenced but not defined or imported.
| - uses: actions/checkout@v3 | ||
| - uses: actions/setup-python@v4 | ||
| - name: ruff | ||
| run: | | ||
| pip install ruff | ||
| ruff check . | ||
| - name: black | ||
| run: | | ||
| pip install black | ||
| black --check . | ||
| - name: interrogate | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: '3.9.16' | ||
| cache: 'pip' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update GitHub Actions to their latest versions.
The actions being used are outdated according to static analysis.
Apply this diff to update the actions:
- - uses: actions/checkout@v3
+ - uses: actions/checkout@v4
- uses: actions/setup-python@v4
+ uses: actions/setup-python@v5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v3 | |
| - uses: actions/setup-python@v4 | |
| - name: ruff | |
| run: | | |
| pip install ruff | |
| ruff check . | |
| - name: black | |
| run: | | |
| pip install black | |
| black --check . | |
| - name: interrogate | |
| - name: Set up Python | |
| uses: actions/setup-python@v4 | |
| with: | |
| python-version: '3.9.16' | |
| cache: 'pip' | |
| - uses: actions/checkout@v4 | |
| - name: Set up Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: '3.9.16' | |
| cache: 'pip' |
🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
Testing
Workflow
Dependencies
pydanticto version 2.10.5.python-dateutildependency.schemadependency.Infrastructure