From cd069aa3e4ce0f7f07b69d52e11f5369fe0c6427 Mon Sep 17 00:00:00 2001 From: Nano Taboada Date: Mon, 16 Feb 2026 05:19:41 -0300 Subject: [PATCH 1/6] refactor(tests): adopt action-oriented naming convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace BDD-style test names with action-oriented pattern: test_request_{method}_{resource}_{param_or_context}_response_{outcome} Test naming changes: - Rename all 19 integration tests for consistency - Apply players/player resource distinction (collection vs single) - Simplify docstrings (single-line, no \"Expected:\" prefix) Test data updates: - Update nonexistent_player() to ID 24 (Thiago Almada) - Use ID 21 (Alejandro Gómez) for deletion test - Update database: 25 players, ID 24 reserved for creation - GET /players/ test expects 25 players initially Documentation: - Add test naming convention to copilot-instructions.md - Update AGENTS.md with pattern and examples Test flow: - Initial: 25 players (missing ID 24, has ID 21) - POST creates ID 24 → 26 players - DELETE removes ID 21 → 25 players Aligns with multi-language comparison project standards. --- .github/copilot-instructions.md | 35 +++++- AGENTS.md | 17 ++- storage/players-sqlite3.db | Bin 16384 -> 20480 bytes tests/player_stub.py | 20 ++-- tests/test_main.py | 199 +++++++------------------------- 5 files changed, 103 insertions(+), 168 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 7669ab2..cc5e77f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -129,7 +129,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 +155,39 @@ curl http://localhost:9000/players # 200 OK - Line length: 88 - Complexity: ≤10 +## Test Naming Convention + +Integration tests follow an action-oriented pattern: + +**Pattern:** +``` +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): 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 07bb6204a409da4bc4484a291fc8c3a3a23dcaae..a5f904cd199f389f3e196685f574a6a0c359de5a 100644 GIT binary patch literal 20480 zcmeI3Piz~<6^EDl$0bE-MN2YG5sHQpWS1~EAt~FD3#2GXQHmT=rb*gr+@|4(JEX>5 z?n=8$RT7EDUy0<{Qg=tc`$Xi1n9JRafu2nm949qWSDXMRzzJ{yoB$`l32*|O04MP6C2;i8 zh&LRKdfuC|@Rmw;RD3`T)42`j%K1z+kE)q$A&;DYMBkwbTHmPV*Yf2AYEZ+pHgJoe zYW@dRv~eA-n}tFGwWy+~nf;^DfH!>P4ccode$86kVm!08bk}-9 zgIb>kHAFRwG>MU@^`JX0I;G6-pEB=4@@B6d9&wcOv&WnE(Z;M0TUwOYb_Gru;oBsBeCGHs~ zzzJ{yFD8MvX9GesGXJ_K9$JWs1>A=1Q@@7yC^3q#k#-(xT7axSx%>`$>evt ze_u%^lef?2h-Mi?Ue%4CU_;5RuEXYSpdwYVR)LL|pv>SQf6|g%biSRB6CMluR*W!ICYEZffv?TtTb3 zBEF*=s9+x~Dtf(6jJLyJG72UaLvmE?+|!ywbr9ur!ywF+%W`r_PF{6el*+WZZJ|vt zNsRI)*y=p6XkAA|tQb@&U{s(@Tt5q>gc$Saa#UPvQ$;W9iqUxpM;oTVCFV?OK!GzW zq(L;3?vZ0>R*GeURg@(bHBlC;dTWPv9ymsB%>;z`2>7rNnv04VmE6IaV(2Rv?C5Kq zPg=~>^0J&vpAaNeAdQ|Ah0RP36?N^fCr4{KAk59rz3!1iOEIyh@bUa55V}rv6Wmyq zQ%N~FAofHzkYLq4MRLU~YYtHI5@@hTjJs`%2@S+7 zVS~U^Hn1yldO(ZO(}1}{YwHa7B0xqggkFt{1!{I@?@tEc0|C42RypD%Ll%N7mn~wg zzD-P)4t7kH4AO#}azd&Sf>>fY%!gu85$3Mzrk!+|db{&63nj3Sl+y!7z}JQ)GZTeu zh3rvP#SpBHxOft%gh7ObZUhmd)%i$c8t5f*cu3&c?zBmu@|t_?W3IJ7C4Ll%nE>#y zP%192u?SH03}m_r(O|(1EI_G%#7ns?jC6CLyYg~&L$}xoeH+g2P&y3280u5}pCg5?vEr6o&(8aj#T3e-{ zXy=}~2fiIbfrCs+$x8#MeS=lGFBWsuI&iaY_*qp0GK-=3lCJ<=E-W3=sq?@r64PW3 zfS?dEm)vyx?xNPK{J2F?ZuOMvS@(e*VUbmc#kdcma-W(-Tt^vDfKlzrno6IrGkvl& z6d11^T~{IW2b>WhaOR#Lxhq(&fhDQ%(&e!MT%`a@#~T z2}?1zoh`3Ze`MGSe5_2Zr++1t%3 z-eM83LSgHGiVgYf)an5%6U8_h1X;e`WT13U03wLJnBE_N#t_|UWP zhAjnh%ExsJUg>BGi!^)V8A$9YC!X!X`UGDZ!(fSR&a7zrat6c{#o8L)Vzmj*4a8tx z!2EC^7|=cFn!gIQiD;}ey`fv?PTO!RUmap$40d8n`3=0?W~EQ18mq0{>5~JQKHFcH zo}e}HKs?QX*0E4L?puK*0+sVE3j8%4q&_}|L7}<|o3lF=!~Ml1V_NW{s$2g~gl%x_ zkq{aE%p?8!N&WxmXQ%Z4(EaoN|HbSMxYwKjC-B@5xbgU43~j?VKN$C&jQ>yjfB${K z{4dMG`+wg5+b_fH{&S!;eApKSP zo%A>9eQ94Zr1zwsNnc5iBvooi|CIhD{Zjfn`%(Uf6W|0m0ZxDu-~>1UPJk2O1ULas zfD_;ZdIUy?J)X0(Y@CU(aq=7+r7#=MoMmHZhK&=`Y#g0pW8@4Q#Sk0ECfPV5v2kdE zje+3Eu+I}n+a delta 889 zcmZozz}V2hI6+!aoPmLX1&En}m=TCMC+Zjri!7F0QQ1*pgV1n3R)Rkds)MT2vfgl3RepWO5F2bqsM;2yt}saaB-)$|xyl zaB(VSrYLcG`h~c2$}B3$fS3kSUtCz4nBrHOo0M9lG+B;UvK}Z_kYAixl9``}#m>Z} zq@n;6X^ZSY){Q_cQQ6 o=D*IrpZ_TTUH Date: Mon, 16 Feb 2026 05:20:11 -0300 Subject: [PATCH 2/6] chore(cd): add empty changelog guard for re-tagged releases Add fallback message when changelog is empty (e.g., re-tagging same commit without new changes). Changes: - Add conditional check after git log command - Display \"No new changes since {PREVIOUS_TAG}\" when empty - Prevents blank changelog section in GitHub releases Improves release notes clarity for edge cases like tag corrections or multiple coach tags on same commit. --- .github/workflows/python-cd.yml | 5 +++++ 1 file changed, 5 insertions(+) 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 From fc03b0d40fe47976ac6a90c6b60f89bdf7cd7132 Mon Sep 17 00:00:00 2001 From: Nano Taboada Date: Mon, 16 Feb 2026 05:26:38 -0300 Subject: [PATCH 3/6] test: make CRUD test flow atomic using single player ID --- tests/test_main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_main.py b/tests/test_main.py index 3f9b20c..2ad2321 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -239,7 +239,7 @@ def test_request_delete_player_id_unknown_response_status_not_found(client): 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 = 21 # Alejandro Gómez - intentionally removed from collection + player_id = 24 # Thiago Almada - created by POST test, now deleted (atomic) # Act response = client.delete(PATH + str(player_id)) # Assert From 1d07bdc5f6c6df9cebbcf372170de2eafc7dca4b Mon Sep 17 00:00:00 2001 From: Nano Taboada Date: Mon, 16 Feb 2026 05:39:44 -0300 Subject: [PATCH 4/6] test: use json parameter for proper Content-Type in POST/PUT --- tests/test_main.py | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/tests/test_main.py b/tests/test_main.py index 2ad2321..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/" @@ -151,10 +150,8 @@ def test_request_get_player_squadnumber_existing_response_body_player_match(clie def test_request_post_player_body_empty_response_status_unprocessable(client): """POST /players/ with empty body returns 422 Unprocessable Entity""" - # Arrange - body = {} # Act - response = client.post(PATH, data=body) + response = client.post(PATH, json={}) # Assert assert response.status_code == 422 @@ -163,9 +160,8 @@ 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 @@ -174,9 +170,8 @@ 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 @@ -190,9 +185,8 @@ def test_request_put_player_id_existing_body_empty_response_status_unprocessable """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 @@ -202,9 +196,8 @@ def test_request_put_player_id_unknown_response_status_not_found(client): # 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 @@ -216,9 +209,8 @@ def test_request_put_player_id_existing_response_status_no_content(client): 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 From 2fe88a4988d3d67ef0f1a21db35c10edf00151d5 Mon Sep 17 00:00:00 2001 From: Nano Taboada Date: Mon, 16 Feb 2026 05:40:18 -0300 Subject: [PATCH 5/6] style(docs): lint markdown in copilot-instructions --- .github/copilot-instructions.md | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index cc5e77f..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 @@ -160,11 +171,13 @@ curl http://localhost:9000/players # 200 OK 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` @@ -172,6 +185,7 @@ test_request_{method}_{resource}_{param_or_context}_response_{outcome} - `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""" @@ -184,6 +198,7 @@ def test_request_post_player_body_empty_response_status_unprocessable(client): ``` **Docstrings:** + - Single-line, concise descriptions - Complements test name (doesn't repeat) - No "Expected:" prefix (redundant) @@ -195,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) From 04c364525dadecb1a5236f8d479dcbcb06746b16 Mon Sep 17 00:00:00 2001 From: Nano Taboada Date: Mon, 16 Feb 2026 05:47:40 -0300 Subject: [PATCH 6/6] chore(config): align CodeRabbit rules with new test naming convention --- .coderabbit.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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