fix: invoke actual cog callback in test_error_handling_and_logging (#39) #54

cal · 2026-03-05T08:35:12Z

cal commented

2026-03-05 08:35:12 +00:00

Summary

Fixes #39 — test_error_handling_and_logging (lines 500-524) was a no-op: it patched api_calls.db_get and pygsheets.authorize, called those mocks directly, caught the exceptions, and passed. The actual cog method was never invoked, so the test passed even when all cog code was deleted.

Changes

tests/players_refactor/test_team_management.py

Removed the @patch('logging.getLogger') decorator and no-op mock-only test body.
Replaced with a test that:
1. Retrieves pull_roster_command.callback from the real TeamManagement cog instance (skips gracefully if the cog can't be imported).
2. Patches get_context_user, get_team_by_owner, and get_rosters at the correct module-level names (cogs.players_new.team_management.*).
3. Configures get_rosters to raise an Exception (simulating a Google Sheets connection error).
4. Calls the actual cog method via cmd.callback(cog, ctx).
5. Asserts ctx.send was called with 'Could not retrieve rosters from your sheet.' — the exact error message in the cog's exception handler.

This test now fails if the pull_roster_command method body is deleted or the exception-handling branch is removed.

Notes

The formatter hook reformatted the entire test file (quote normalization, argument wrapping). Only the functional change at test_error_handling_and_logging was intentional; the rest is formatter-driven.
Other observations (out of scope): several other tests in this file also define local mock_*_command functions instead of calling real cog methods — the same pattern as the fixed test. Line 279 also references sample_team_data which is undefined in that scope (pre-existing bug).

## Summary Fixes #39 — `test_error_handling_and_logging` (lines 500-524) was a no-op: it patched `api_calls.db_get` and `pygsheets.authorize`, called those mocks directly, caught the exceptions, and passed. The actual cog method was never invoked, so the test passed even when all cog code was deleted. ## Changes **`tests/players_refactor/test_team_management.py`** - Removed the `@patch('logging.getLogger')` decorator and no-op mock-only test body. - Replaced with a test that: 1. Retrieves `pull_roster_command.callback` from the real `TeamManagement` cog instance (skips gracefully if the cog can't be imported). 2. Patches `get_context_user`, `get_team_by_owner`, and `get_rosters` at the correct module-level names (`cogs.players_new.team_management.*`). 3. Configures `get_rosters` to raise an `Exception` (simulating a Google Sheets connection error). 4. Calls the actual cog method via `cmd.callback(cog, ctx)`. 5. Asserts `ctx.send` was called with `'Could not retrieve rosters from your sheet.'` — the exact error message in the cog's exception handler. This test now fails if the `pull_roster_command` method body is deleted or the exception-handling branch is removed. ## Notes - The formatter hook reformatted the entire test file (quote normalization, argument wrapping). Only the functional change at `test_error_handling_and_logging` was intentional; the rest is formatter-driven. - Other observations (out of scope): several other tests in this file also define local `mock_*_command` functions instead of calling real cog methods — the same pattern as the fixed test. Line 279 also references `sample_team_data` which is undefined in that scope (pre-existing bug).

cal added 1 commit 2026-03-05 08:35:13 +00:00

fix: invoke actual cog callback in test_error_handling_and_logging (#39 )

Build Docker Image / build (pull_request) Successful in 1m14s

Details

4009be1e44

The previous test patched api_calls.db_get and pygsheets.authorize then
called those mocks directly—never invoking any cog method. The test
passed even when all cog code was deleted.

Replace with a test that retrieves the real pull_roster_command.callback
from the cog instance, patches dependencies at the correct module-level
names, calls the callback, and asserts ctx.send was called with the
expected error message. If the cog cannot be imported, the test skips
gracefully via pytest.skip.

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

cal referenced this pull request

2026-03-05 08:35:24 +00:00

Strengthen `players_refactor` tests to assert on actual cog method calls #39

cal added the

ai-reviewing

label 2026-03-05 08:45:31 +00:00

cal reviewed 2026-03-05 08:47:12 +00:00

cal left a comment

AI Code Review

Files Reviewed

tests/players_refactor/test_team_management.py (modified)

Findings

Correctness

The functional replacement in test_error_handling_and_logging is correct. The patch paths (cogs.players_new.team_management.get_context_user, get_team_by_owner, get_rosters) match the actual import-time bindings in the module.
get_rosters is called synchronously in the cog (rosters = get_rosters(team, self.bot)); patching it with side_effect=Exception(...) (not AsyncMock) is correct.
get_team_by_owner is awaited in the cog; patching with new=AsyncMock(return_value=team_with_sheet) is correct.
get_context_user returns mock_context.author (which has .id = 12345), so the get_team_by_owner call receives the right argument.
The team_with_sheet fixture has "gsheet": "valid-sheet-id" — this correctly passes the if not team.get('gsheet') or team.get('gsheet') == 'None' guard in the cog, reaching the get_rosters call.
The assert message 'Could not retrieve rosters from your sheet.' matches exactly what the cog sends at line 264 of cogs/players_new/team_management.py.
Calling via cmd.callback(cog, ctx) bypasses command decorators (legal_channel, role check) as intended — the test is scoped to business logic only.

Security

No issues found.

Style & Conventions

Test name: test_error_handling_and_logging no longer describes the test. It now specifically tests pull_roster_command's exception path; a name like test_pull_roster_command_get_rosters_exception_sends_error would be more discoverable. Minor — doesn't block merging.
Inline fixture: team_with_sheet is defined inline rather than using the sample_team fixture from conftest. tests/CLAUDE.md says to use factory data when possible. The divergence is justified here — the existing sample_team fixture has 'gsheet': 'None' which would fail the gsheet guard and never reach get_rosters, so a custom value is necessary.
Formatter-driven changes (quote normalisation, argument wrapping) are consistent with the rest of the file.

Suggestions

Rename test_error_handling_and_logging to something that reflects the specific path being tested (e.g. test_pull_roster_command_exception_sends_error_message).
team_with_sheet could be derived from the sample_team fixture: {**sample_team, "gsheet": "valid-sheet-id"} — slightly cleaner.

Verdict: APPROVED

The core fix is sound. The old test was a genuine no-op (all paths caught their own exceptions and called pass); the new test calls the real cog callback and asserts the actual error message. Patch paths, mock types, and the assertion string are all correct. Minor naming and fixture-style suggestions are non-blocking.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `tests/players_refactor/test_team_management.py` (modified) ### Findings #### Correctness - The functional replacement in `test_error_handling_and_logging` is correct. The patch paths (`cogs.players_new.team_management.get_context_user`, `get_team_by_owner`, `get_rosters`) match the actual import-time bindings in the module. - `get_rosters` is called synchronously in the cog (`rosters = get_rosters(team, self.bot)`); patching it with `side_effect=Exception(...)` (not `AsyncMock`) is correct. - `get_team_by_owner` is awaited in the cog; patching with `new=AsyncMock(return_value=team_with_sheet)` is correct. - `get_context_user` returns `mock_context.author` (which has `.id = 12345`), so the `get_team_by_owner` call receives the right argument. - The `team_with_sheet` fixture has `"gsheet": "valid-sheet-id"` — this correctly passes the `if not team.get('gsheet') or team.get('gsheet') == 'None'` guard in the cog, reaching the `get_rosters` call. - The assert message `'Could not retrieve rosters from your sheet.'` matches exactly what the cog sends at line 264 of `cogs/players_new/team_management.py`. - Calling via `cmd.callback(cog, ctx)` bypasses command decorators (`legal_channel`, role check) as intended — the test is scoped to business logic only. #### Security - No issues found. #### Style & Conventions - **Test name**: `test_error_handling_and_logging` no longer describes the test. It now specifically tests `pull_roster_command`'s exception path; a name like `test_pull_roster_command_get_rosters_exception_sends_error` would be more discoverable. Minor — doesn't block merging. - **Inline fixture**: `team_with_sheet` is defined inline rather than using the `sample_team` fixture from conftest. `tests/CLAUDE.md` says to use factory data when possible. The divergence is justified here — the existing `sample_team` fixture has `'gsheet': 'None'` which would fail the gsheet guard and never reach `get_rosters`, so a custom value is necessary. - Formatter-driven changes (quote normalisation, argument wrapping) are consistent with the rest of the file. #### Suggestions - Rename `test_error_handling_and_logging` to something that reflects the specific path being tested (e.g. `test_pull_roster_command_exception_sends_error_message`). - `team_with_sheet` could be derived from the `sample_team` fixture: `{**sample_team, "gsheet": "valid-sheet-id"}` — slightly cleaner. ### Verdict: APPROVED The core fix is sound. The old test was a genuine no-op (all paths caught their own exceptions and called `pass`); the new test calls the real cog callback and asserts the actual error message. Patch paths, mock types, and the assertion string are all correct. Minor naming and fixture-style suggestions are non-blocking. --- *Automated review by Claude PR Reviewer*

cal added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-05 08:47:22 +00:00

cal changed target branch from main to next-release

2026-03-07 07:32:28 +00:00

cal force-pushed ai/paper-dynasty-discord-39 from 4009be1e44 to 7b76b429a8

2026-03-07 07:34:03 +00:00

Compare

cal merged commit 25bae26f1e into next-release

2026-03-07 07:41:29 +00:00