diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 0ed297a..c7fdc5c 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -80,7 +80,7 @@ reviews: - path: "tests/**/*.py" instructions: | - Tests should use pytest with fixtures from conftest.py - - Verify test naming follows given_when_then pattern + - Verify test naming follows test_request_{method}_{resource}_{param_or_context}_response_{outcome} pattern - Check that TestClient is used for endpoint testing - Ensure test data uses stubs (e.g., player_stub.py) - Tests should use async test functions where appropriate @@ -287,7 +287,8 @@ code_generation: - path: "tests/**/*.py" instructions: | - Use pytest framework with async support (pytest-asyncio) - - Follow given_when_then or arrange_act_assert patterns + - Follow test_request_{method}_{resource}_{param_or_context}_response_{outcome} pattern + - Tests use arrange-act-assert structure within test body - Use fixtures from conftest.py for TestClient - Use test stubs for consistent test data - Ensure async tests are properly decorated diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 7669ab2..0563ad2 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -47,6 +47,7 @@ Request → Routes → Services → SQLAlchemy → SQLite ``` **Key Directories:** + - `routes/` - API endpoints (player_route.py, health_route.py) - `services/` - Business logic (player_service.py) - `models/` - Pydantic validation (camelCase JSON API) @@ -56,6 +57,7 @@ Request → Routes → Services → SQLAlchemy → SQLite - `tests/` - pytest suite (test_main.py, conftest.py) **Config Files:** + - `.flake8` - Linter (max-line-length=88, complexity=10) - `pyproject.toml` - Black formatter (line-length=88) - `.coveragerc` - Coverage config (80% target) @@ -65,6 +67,7 @@ Request → Routes → Services → SQLAlchemy → SQLite ## API Endpoints All async with `AsyncSession` injection: + - `POST /players/` → 201|409|422 - `GET /players/` → 200 (cached 10min) - `GET /players/{player_id}` → 200|404 @@ -78,11 +81,13 @@ JSON: camelCase (e.g., `squadNumber`, `firstName`) ## CI/CD **python-ci.yml** (push/PR to master): + 1. Lint: commitlint → `flake8 .` → `black --check .` 2. Test: `pytest -v` → coverage 3. Upload to Codecov **python-cd.yml** (tags `v*.*.*-*`): + 1. Validate semver + coach name 2. Run tests 3. Build Docker (amd64/arm64) @@ -92,6 +97,7 @@ JSON: camelCase (e.g., `squadNumber`, `firstName`) ## Critical Patterns ### Async Everywhere + ```python # Always use async/await async def get_player(async_session: AsyncSession, player_id: int): @@ -99,12 +105,14 @@ async def get_player(async_session: AsyncSession, player_id: int): result = await async_session.execute(stmt) return result.scalar_one_or_none() ``` + - All routes: `async def` - Database: `AsyncSession` (never `Session`) - Driver: `aiosqlite` (not `sqlite3`) - SQLAlchemy 2.0: `select()` (not `session.query()`) ### camelCase API Contract + ```python class PlayerModel(BaseModel): model_config = ConfigDict(alias_generator=to_camel) @@ -113,7 +121,9 @@ class PlayerModel(BaseModel): ``` ### Database Schema Changes + ⚠️ No Alembic yet - manual process: + 1. Update `schemas/player_schema.py` 2. Manually update `storage/players-sqlite3.db` (SQLite CLI/DB Browser) 3. Preserve 26 players @@ -121,6 +131,7 @@ class PlayerModel(BaseModel): 5. Update services + tests ### Caching + - Key: `"players"` (hardcoded) - TTL: 600s (10min) - Cleared on POST/PUT/DELETE @@ -129,7 +140,7 @@ class PlayerModel(BaseModel): ## Common Issues 1. **SQLAlchemy errors** → Always catch + rollback in services -2. **Test file** → `test_main.py` excluded from Black (preserves long names) +2. **Test file** → `test_main.py` excluded from Black 3. **Database location** → Local: `./storage/`, Docker: `/storage/` (volume) 4. **Pydantic validation** → Returns 422 (not 400) 5. **Import order** → stdlib → third-party → local @@ -155,6 +166,43 @@ curl http://localhost:9000/players # 200 OK - Line length: 88 - Complexity: ≤10 +## Test Naming Convention + +Integration tests follow an action-oriented pattern: + +**Pattern:** + +```text +test_request_{method}_{resource}_{param_or_context}_response_{outcome} +``` + +**Components:** + +- `method` - HTTP verb: `get`, `post`, `put`, `delete` +- `resource` - `players` (collection) or `player` (single resource) +- `param_or_context` - Request details: `id_existing`, `squadnumber_nonexistent`, `body_empty` +- `response` - Literal separator +- `outcome` - What's asserted: `status_ok`, `status_not_found`, `body_players`, `header_cache_miss` + +**Examples:** + +```python +def test_request_get_players_response_status_ok(client): + """GET /players/ returns 200 OK""" + +def test_request_get_player_id_existing_response_body_player_match(client): + """GET /players/{player_id} with existing ID returns matching player""" + +def test_request_post_player_body_empty_response_status_unprocessable(client): + """POST /players/ with empty body returns 422 Unprocessable Entity""" +``` + +**Docstrings:** + +- Single-line, concise descriptions +- Complements test name (doesn't repeat) +- No "Expected:" prefix (redundant) + ## Commit Messages Follow Conventional Commits format (enforced by commitlint in CI): @@ -162,12 +210,14 @@ Follow Conventional Commits format (enforced by commitlint in CI): **Format:** `type(scope): description (#issue)` **Rules:** + - Max 80 characters - Types: `feat`, `fix`, `docs`, `test`, `refactor`, `chore`, `ci`, `perf`, `style`, `build` - Scope: Optional (e.g., `api`, `db`, `service`, `route`) - Issue number: Required suffix **Examples:** + ```text feat(api): add player stats endpoint (#42) fix(db): resolve async session leak (#88) diff --git a/.github/workflows/python-cd.yml b/.github/workflows/python-cd.yml index 3d756b3..0b2de55 100644 --- a/.github/workflows/python-cd.yml +++ b/.github/workflows/python-cd.yml @@ -120,6 +120,11 @@ jobs: else echo "📝 Generating changelog from $PREVIOUS_TAG to ${{ steps.version.outputs.tag_name }}" CHANGELOG=$(git log --pretty=format:"- %s (%h)" ${PREVIOUS_TAG}..${{ steps.version.outputs.tag_name }}) + + # Guard against empty changelog (e.g., re-tagging same commit) + if [ -z "$CHANGELOG" ]; then + CHANGELOG="No new changes since $PREVIOUS_TAG" + fi fi # Write changelog to file diff --git a/AGENTS.md b/AGENTS.md index a21448f..6d4535d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,7 +71,8 @@ async def get_stats( 1. Tests (`tests/test_main.py`): ```python -async def test_given_get_when_player_stats_then_returns_200(): +def test_request_get_player_id_existing_stats_response_status_ok(client): + """GET /players/{player_id}/stats with existing ID returns 200 OK""" ... ``` @@ -110,10 +111,22 @@ No Alembic migrations: ## Testing Strategy +**Naming Pattern:** + +```text +test_request_{method}_{resource}_{param_or_context}_response_{outcome} +``` + +- `players` (collection) vs `player` (single resource) +- Examples: `test_request_get_players_response_status_ok`, `test_request_post_player_body_empty_response_status_unprocessable` + +**Guidelines:** + - Use `tests/player_stub.py` for data - Test real DB (fixtures handle setup) - Cover: happy paths + errors + edges - Cache tests: verify `X-Cache` header +- Docstrings: Single-line, concise, no "Expected:" prefix ## Planning Tips @@ -130,7 +143,7 @@ No Alembic migrations: - `test_main.py`: Excluded from Black - SQLAlchemy errors: Catch + rollback in services - Validation errors: 422 (not 400) -- Test file naming: `test_given_when_then` pattern +- Test naming: `test_request_{method}_{resource}_{context}_response_{outcome}` pattern ## Cross-Tool Notes diff --git a/storage/players-sqlite3.db b/storage/players-sqlite3.db index 07bb620..a5f904c 100644 Binary files a/storage/players-sqlite3.db and b/storage/players-sqlite3.db differ diff --git a/tests/player_stub.py b/tests/player_stub.py index 34bd629..2b458c9 100644 --- a/tests/player_stub.py +++ b/tests/player_stub.py @@ -54,16 +54,16 @@ def nonexistent_player(): Creates a test stub for a nonexistent (new) Player. """ return Player( - id=12, - first_name="Leandro", - middle_name="Daniel", - last_name="Paredes", - date_of_birth="1994-06-29T00:00:00.000Z", - squad_number=5, - position="Defensive Midfield", - abbr_position="DM", - team="AS Roma", - league="Serie A", + id=24, + first_name="Thiago", + middle_name="Ezequiel", + last_name="Almada", + date_of_birth="2001-04-26T00:00:00.000Z", + squad_number=16, + position="Attacking Midfield", + abbr_position="AM", + team="Atlanta United FC", + league="Major League Soccer", starting11=0, ) diff --git a/tests/test_main.py b/tests/test_main.py index c32946b..4b48c4e 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -16,7 +16,6 @@ - Conflict and edge case behaviors """ -import json from tests.player_stub import existing_player, nonexistent_player, unknown_player PATH = "/players/" @@ -24,14 +23,8 @@ # GET /health/ ----------------------------------------------------------------- -def test_given_get_when_request_path_is_health_then_response_status_code_is_200( - client, -): - """ - Given GET /health/ - when request - then response Status Code is 200 (OK) - """ +def test_request_get_health_response_status_ok(client): + """GET /health/ returns 200 OK""" # Act response = client.get("/health/") # Assert @@ -42,14 +35,8 @@ def test_given_get_when_request_path_is_health_then_response_status_code_is_200( # GET /players/ ---------------------------------------------------------------- -def test_given_get_when_request_is_initial_then_response_header_x_cache_is_miss( - client, -): - """ - Given GET /players/ - when request is initial - then response Header X-Cache value is MISS - """ +def test_request_get_players_response_header_cache_miss(client): + """GET /players/ initial request returns X-Cache: MISS""" # Act response = client.get(PATH) @@ -58,14 +45,8 @@ def test_given_get_when_request_is_initial_then_response_header_x_cache_is_miss( assert response.headers.get("X-Cache") == "MISS" -def test_given_get_when_request_is_subsequent_then_response_header_x_cache_is_hit( - client, -): - """ - Given GET /players/ - when request is subsequent - then response Header X-Cache is HIT - """ +def test_request_get_players_response_header_cache_hit(client): + """GET /players/ subsequent request returns X-Cache: HIT""" # Act client.get(PATH) # initial response = client.get(PATH) # subsequent (cached) @@ -75,49 +56,32 @@ def test_given_get_when_request_is_subsequent_then_response_header_x_cache_is_hi assert response.headers.get("X-Cache") == "HIT" -def test_given_get_when_request_path_has_no_id_then_response_status_code_is_200( - client, -): - """ - Given GET /players/ - when request path has no ID - then response Status Code is 200 (OK) - """ +def test_request_get_players_response_status_ok(client): + """GET /players/ returns 200 OK""" # Act response = client.get(PATH) # Assert assert response.status_code == 200 -def test_given_get_when_request_path_has_no_id_then_response_body_is_list_of_players( - client, -): - """ - Given GET /players/ - when request path has no ID - then response Body is list of players - """ +def test_request_get_players_response_body_players(client): + """GET /players/ returns list of players""" # Act response = client.get(PATH) # Assert players = response.json() - player_id = 0 - for player in players: - player_id += 1 - assert player["id"] == player_id + assert ( + len(players) == 25 + ) # Database has 25 players (ID 24 reserved for creation test) + assert all("id" in player for player in players) # Each player has an ID + assert all(player["id"] != 24 for player in players) # ID 24 not in database yet # GET /players/{player_id} ----------------------------------------------------- -def test_given_get_when_request_path_is_nonexistent_id_then_response_status_code_is_404( - client, -): - """ - Given GET /players/{player_id} - when request path is nonexistent ID - then response Status Code is 404 (Not Found) - """ +def test_request_get_player_id_nonexistent_response_status_not_found(client): + """GET /players/{player_id} with nonexistent ID returns 404 Not Found""" # Arrange player_id = nonexistent_player().id # Act @@ -126,14 +90,8 @@ def test_given_get_when_request_path_is_nonexistent_id_then_response_status_code assert response.status_code == 404 -def test_given_get_when_request_path_is_existing_id_then_response_status_code_is_200( - client, -): - """ - Given GET /players/{player_id} - when request path is existing ID - then response Status Code is 200 (OK) - """ +def test_request_get_player_id_existing_response_status_ok(client): + """GET /players/{player_id} with existing ID returns 200 OK""" # Arrange player_id = existing_player().id # Act @@ -142,14 +100,8 @@ def test_given_get_when_request_path_is_existing_id_then_response_status_code_is assert response.status_code == 200 -def test_given_get_when_request_path_is_existing_id_then_response_is_matching_player( - client, -): - """ - Given GET /players/{player_id} - when request path is existing ID - then response is matching Player - """ +def test_request_get_player_id_existing_response_body_player_match(client): + """GET /players/{player_id} with existing ID returns matching player""" # Arrange player_id = existing_player().id # Act @@ -162,14 +114,8 @@ def test_given_get_when_request_path_is_existing_id_then_response_is_matching_pl # GET /players/squadnumber/{squad_number} -------------------------------------- -def test_given_get_when_request_path_is_nonexistent_squad_number_then_response_status_code_is_404( - client, -): - """ - Given GET /players/squadnumber/{squad_number} - when request path is nonexistent Squad Number - then response Status Code is 404 (Not Found) - """ +def test_request_get_player_squadnumber_nonexistent_response_status_not_found(client): + """GET /players/squadnumber/{squad_number} with nonexistent number returns 404 Not Found""" # Arrange squad_number = nonexistent_player().squad_number # Act @@ -178,14 +124,8 @@ def test_given_get_when_request_path_is_nonexistent_squad_number_then_response_s assert response.status_code == 404 -def test_given_get_when_request_path_is_existing_squad_number_then_response_status_code_is_200( - client, -): - """ - Given GET /players/squadnumber/{squad_number} - when request path is existing Squad Number - then response Status Code is 200 (OK) - """ +def test_request_get_player_squadnumber_existing_response_status_ok(client): + """GET /players/squadnumber/{squad_number} with existing number returns 200 OK""" # Arrange squad_number = existing_player().squad_number # Act @@ -194,14 +134,8 @@ def test_given_get_when_request_path_is_existing_squad_number_then_response_stat assert response.status_code == 200 -def test_given_get_when_request_path_is_existing_squad_number_then_response_is_matching_player( - client, -): - """ - Given GET /players/squadnumber/{squad_number} - when request path is existing Squad Number - then response is matching Player - """ +def test_request_get_player_squadnumber_existing_response_body_player_match(client): + """GET /players/squadnumber/{squad_number} with existing number returns matching player""" # Arrange squad_number = existing_player().squad_number # Act @@ -214,52 +148,30 @@ def test_given_get_when_request_path_is_existing_squad_number_then_response_is_m # POST /players/ --------------------------------------------------------------- -def test_given_post_when_request_body_is_empty_then_response_status_code_is_422( - client, -): - """ - Given POST /players/ - when request body is empty - then response Status Code is 422 (Unprocessable Entity) - """ - # Arrange - body = {} +def test_request_post_player_body_empty_response_status_unprocessable(client): + """POST /players/ with empty body returns 422 Unprocessable Entity""" # Act - response = client.post(PATH, data=body) + response = client.post(PATH, json={}) # Assert assert response.status_code == 422 -def test_given_post_when_request_body_is_existing_player_then_response_status_code_is_409( - client, -): - """ - Given POST /players/ - when request body is existing Player - then response Status Code is 409 (Conflict) - """ +def test_request_post_player_body_existing_response_status_conflict(client): + """POST /players/ with existing player returns 409 Conflict""" # Arrange player = existing_player() - body = json.dumps(player.__dict__) # Act - response = client.post(PATH, data=body) + response = client.post(PATH, json=player.__dict__) # Assert assert response.status_code == 409 -def test_given_post_when_request_body_is_nonexistent_player_then_response_status_code_is_201( - client, -): - """ - Given POST /players/ - when request body is nonexistent Player - then response Status Code is 201 (Created) - """ +def test_request_post_player_body_nonexistent_response_status_created(client): + """POST /players/ with nonexistent player returns 201 Created""" # Arrange player = nonexistent_player() - body = json.dumps(player.__dict__) # Act - response = client.post(PATH, data=body) + response = client.post(PATH, json=player.__dict__) # Assert assert response.status_code == 201 @@ -267,57 +179,38 @@ def test_given_post_when_request_body_is_nonexistent_player_then_response_status # PUT /players/{player_id} ----------------------------------------------------- -def test_given_put_when_request_body_is_empty_then_response_status_code_is_422( +def test_request_put_player_id_existing_body_empty_response_status_unprocessable( client, ): - """ - Given PUT /players/{player_id} - when request body is empty - then response Status Code is 422 (Unprocessable Entity) - """ + """PUT /players/{player_id} with empty body returns 422 Unprocessable Entity""" # Arrange player_id = existing_player().id - body = {} # Act - response = client.put(PATH + str(player_id), data=body) + response = client.put(PATH + str(player_id), json={}) # Assert assert response.status_code == 422 -def test_given_put_when_request_path_is_unknown_id_then_response_status_code_is_404( - client, -): - """ - Given PUT /players/{player_id} - when request path is unknown ID - then response Status Code is 404 (Not Found) - """ +def test_request_put_player_id_unknown_response_status_not_found(client): + """PUT /players/{player_id} with unknown ID returns 404 Not Found""" # Arrange player_id = unknown_player().id player = unknown_player() - body = json.dumps(player.__dict__) # Act - response = client.put(PATH + str(player_id), data=body) + response = client.put(PATH + str(player_id), json=player.__dict__) # Assert assert response.status_code == 404 -def test_given_put_when_request_path_is_existing_id_then_response_status_code_is_204( - client, -): - """ - Given PUT /players/{player_id} - when request path is existing ID - then response Status Code is 204 (No Content) - """ +def test_request_put_player_id_existing_response_status_no_content(client): + """PUT /players/{player_id} with existing ID returns 204 No Content""" # Arrange player_id = existing_player().id player = existing_player() player.first_name = "Emiliano" player.middle_name = "" - body = json.dumps(player.__dict__) # Act - response = client.put(PATH + str(player_id), data=body) + response = client.put(PATH + str(player_id), json=player.__dict__) # Assert assert response.status_code == 204 @@ -325,14 +218,8 @@ def test_given_put_when_request_path_is_existing_id_then_response_status_code_is # DELETE /players/{player_id} -------------------------------------------------- -def test_given_delete_when_request_path_is_unknown_id_then_response_status_code_is_404( - client, -): - """ - Given DELETE /players/{player_id} - when request path is unknown ID - then response Status Code is 404 (Not Found) - """ +def test_request_delete_player_id_unknown_response_status_not_found(client): + """DELETE /players/{player_id} with unknown ID returns 404 Not Found""" # Arrange player_id = unknown_player().id # Act @@ -341,16 +228,10 @@ def test_given_delete_when_request_path_is_unknown_id_then_response_status_code_ assert response.status_code == 404 -def test_given_delete_when_request_path_is_existing_id_then_response_status_code_is_204( - client, -): - """ - Given DELETE /players/{player_id} - when request path is existing ID - then response Status Code is 204 (No Content) - """ +def test_request_delete_player_id_existing_response_status_no_content(client): + """DELETE /players/{player_id} with existing ID returns 204 No Content""" # Arrange - player_id = 12 # nonexistent_player() previously created + player_id = 24 # Thiago Almada - created by POST test, now deleted (atomic) # Act response = client.delete(PATH + str(player_id)) # Assert