test(dev_tools): fix dead render mock in test_successful_batter_flow (#169) #170

Claude · 2026-04-11T17:33:29Z

Claude commented

2026-04-11 17:33:29 +00:00

Closes #169

What

test_successful_batter_flow patched db_get to stub the card-render step, but the render call in _execute_refractor_test uses aiohttp.ClientSession directly — not db_get. The mock was never consumed; the test silently hit the non-fatal except branch (⚠️ Card render failed) instead of the happy path.

Fix

Removed dead patch("cogs.dev_tools.db_get") and unused mock_get.return_value
Added aiohttp.ClientSession mock (status 200) using explicit __aenter__/__aexit__ on MagicMock (required because sess.get(url) must return an async context manager, not a coroutine)
Added assertion: assert "✅ Card rendered + S3 upload triggered" in result_text

Files changed

tests/test_dev_tools.py — TestRefractorTestExecute::test_successful_batter_flow

Tests

14/14 pass (python -m pytest tests/test_dev_tools.py -v)

Closes #169 ## What `test_successful_batter_flow` patched `db_get` to stub the card-render step, but the render call in `_execute_refractor_test` uses `aiohttp.ClientSession` directly — not `db_get`. The mock was never consumed; the test silently hit the non-fatal `except` branch (`⚠️ Card render failed`) instead of the happy path. ## Fix - Removed dead `patch("cogs.dev_tools.db_get")` and unused `mock_get.return_value` - Added `aiohttp.ClientSession` mock (status 200) using explicit `__aenter__`/`__aexit__` on `MagicMock` (required because `sess.get(url)` must return an async context manager, not a coroutine) - Added assertion: `assert "✅ Card rendered + S3 upload triggered" in result_text` ## Files changed - `tests/test_dev_tools.py` — `TestRefractorTestExecute::test_successful_batter_flow` ## Tests 14/14 pass (`python -m pytest tests/test_dev_tools.py -v`)

Claude added 1 commit 2026-04-11 17:33:30 +00:00

test(dev_tools): fix dead render mock in test_successful_batter_flow (#169 )

Ruff Lint / lint (pull_request) Successful in 16s

Details

00d746bf5a

Replaces dead db_get mock (never consumed by the render step) with a
proper aiohttp.ClientSession mock. The render call uses aiohttp directly,
so the old mock hit the non-fatal except branch instead of the 200-OK path.
Adds explicit assertion for '✅ Card rendered + S3 upload triggered'.

Closes #169

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

Claude referenced this pull request

2026-04-11 17:33:37 +00:00

test(dev_tools): refractor-test render step mock is a dead mock #169

Claude added the

ai-reviewing

label 2026-04-11 17:45:52 +00:00

Claude reviewed 2026-04-11 17:47:19 +00:00

Claude left a comment

AI Code Review

Files Reviewed

tests/test_dev_tools.py (modified — TestRefractorTestExecute::test_successful_batter_flow)

Findings

Correctness

No issues found. The mock structure correctly mirrors production usage:

aiohttp.ClientSession(headers=AUTH_TOKEN) returns mock_sess via return_value=mock_sess on the patched class ✓
async with mock_sess as sess → mock_sess.__aenter__ returns mock_sess itself ✓
sess.get(render_url) → get_cm (a proper async context manager) ✓
async with get_cm as r → mock_render_resp with status = 200 ✓
r.status == 200 → branch appends "✅ Card rendered + S3 upload triggered" ✓

The removed patch("cogs.dev_tools.db_get") was genuinely dead — _execute_refractor_test makes no db_get calls (only db_post + direct aiohttp). Removal is safe.

The new assertion assert "✅ Card rendered + S3 upload triggered" in result_text closes the gap flagged in the PR #168 review: previously only "✅" and "game" were checked, which passed even while the render step silently failed.

Security

No issues found. Test file only.

Style & Conventions

No issues found. Docstring accurately documents the historical bug and fix. Mock setup is idiomatic unittest.mock.

Suggestions

None.

Verdict: APPROVED

Directly addresses the dead-mock issue flagged in the PR #168 review. Mock wiring is correct for the double async context manager pattern (ClientSession → sess.get). Happy-path assertion is now meaningful.

Automated review by Claude PR Reviewer (posted as COMMENT — Gitea blocks self-approval)

## AI Code Review ### Files Reviewed - `tests/test_dev_tools.py` (modified — `TestRefractorTestExecute::test_successful_batter_flow`) ### Findings #### Correctness No issues found. The mock structure correctly mirrors production usage: - `aiohttp.ClientSession(headers=AUTH_TOKEN)` returns `mock_sess` via `return_value=mock_sess` on the patched class ✓ - `async with mock_sess as sess` → `mock_sess.__aenter__` returns `mock_sess` itself ✓ - `sess.get(render_url)` → `get_cm` (a proper async context manager) ✓ - `async with get_cm as r` → `mock_render_resp` with `status = 200` ✓ - `r.status == 200` → branch appends `"✅ Card rendered + S3 upload triggered"` ✓ The removed `patch("cogs.dev_tools.db_get")` was genuinely dead — `_execute_refractor_test` makes no `db_get` calls (only `db_post` + direct `aiohttp`). Removal is safe. The new assertion `assert "✅ Card rendered + S3 upload triggered" in result_text` closes the gap flagged in the PR #168 review: previously only `"✅"` and `"game"` were checked, which passed even while the render step silently failed. #### Security No issues found. Test file only. #### Style & Conventions No issues found. Docstring accurately documents the historical bug and fix. Mock setup is idiomatic `unittest.mock`. #### Suggestions None. ### Verdict: APPROVED Directly addresses the dead-mock issue flagged in the PR #168 review. Mock wiring is correct for the double async context manager pattern (`ClientSession` → `sess.get`). Happy-path assertion is now meaningful. --- *Automated review by Claude PR Reviewer (posted as COMMENT — Gitea blocks self-approval)*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-04-11 17:47:52 +00:00

Claude removed the

ai-reviewed

label 2026-04-11 19:23:44 +00:00

claude-reviewer added the

ai-reviewing

label 2026-04-11 20:55:04 +00:00

claude-reviewer approved these changes 2026-04-11 20:56:37 +00:00

claude-reviewer left a comment

AI Code Review

Files Reviewed

tests/test_dev_tools.py (modified — TestRefractorTestExecute::test_successful_batter_flow only)

Findings

Correctness

No issues found.

Mock wiring is correct for the double async context manager pattern at dev_tools.py:375-376:

patch("cogs.dev_tools.aiohttp.ClientSession", return_value=mock_sess) — correct patch target; aiohttp is imported as a module in dev_tools.py so cogs.dev_tools.aiohttp.ClientSession is the right path
aiohttp.ClientSession(headers=AUTH_TOKEN) → returns mock_sess ✓
async with mock_sess as sess: → mock_sess.__aenter__ returns mock_sess → sess = mock_sess ✓
sess.get(render_url) → returns get_cm ✓
async with get_cm as r: → get_cm.__aenter__ returns mock_render_resp with status=200 ✓
r.status == 200 → True → appends "✅ Card rendered + S3 upload triggered" ✓

Dead mock removal is correct: _execute_refractor_test makes zero db_get calls (all API calls use db_post; render uses raw aiohttp). The old mock was silently dropping into the except branch at dev_tools.py:384.

New assertion assert "✅ Card rendered + S3 upload triggered" in result_text closes the coverage gap from PR #168 suggestion #1.

Security

No issues found.

Style & Conventions

No issues found. Mock setup follows the existing pattern in the test file. Docstring updated to explain the bug and fix inline.

Suggestions

None.

Verdict: APPROVED

Mock wiring is correct, dead mock removed, assertion added. Closes the silent test regression identified in PR #168. Clean.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `tests/test_dev_tools.py` (modified — `TestRefractorTestExecute::test_successful_batter_flow` only) ### Findings #### Correctness No issues found. Mock wiring is correct for the double async context manager pattern at `dev_tools.py:375-376`: - `patch("cogs.dev_tools.aiohttp.ClientSession", return_value=mock_sess)` — correct patch target; `aiohttp` is imported as a module in `dev_tools.py` so `cogs.dev_tools.aiohttp.ClientSession` is the right path - `aiohttp.ClientSession(headers=AUTH_TOKEN)` → returns `mock_sess` ✓ - `async with mock_sess as sess:` → `mock_sess.__aenter__` returns `mock_sess` → `sess = mock_sess` ✓ - `sess.get(render_url)` → returns `get_cm` ✓ - `async with get_cm as r:` → `get_cm.__aenter__` returns `mock_render_resp` with `status=200` ✓ - `r.status == 200` → True → appends `"✅ Card rendered + S3 upload triggered"` ✓ Dead mock removal is correct: `_execute_refractor_test` makes zero `db_get` calls (all API calls use `db_post`; render uses raw `aiohttp`). The old mock was silently dropping into the `except` branch at `dev_tools.py:384`. New assertion `assert "✅ Card rendered + S3 upload triggered" in result_text` closes the coverage gap from PR #168 suggestion #1. #### Security No issues found. #### Style & Conventions No issues found. Mock setup follows the existing pattern in the test file. Docstring updated to explain the bug and fix inline. #### Suggestions None. ### Verdict: APPROVED Mock wiring is correct, dead mock removed, assertion added. Closes the silent test regression identified in PR #168. Clean. --- *Automated review by Claude PR Reviewer*

claude-reviewer added

ai-reviewed

and removed

ai-reviewing

labels 2026-04-11 20:56:55 +00:00

cal merged commit 9127c9a00b into main

2026-04-12 14:36:27 +00:00

cal deleted branch issue/169-test-dev-tools-refractor-test-render-step-mock-is

2026-04-12 14:36:27 +00:00

cal referenced this issue from a commit

2026-04-12 14:36:28 +00:00

Merge pull request 'test(dev_tools): fix dead render mock in test_successful_batter_flow (#169)' (#170) from issue/169-test-dev-tools-refractor-test-render-step-mock-is into main