Skip to content

🐛 fix(tests): fix DI container setup, mock signature, and update deps#81

Closed
itisnotyourenv wants to merge 2 commits into
mainfrom
fix/test-di-and-deps
Closed

🐛 fix(tests): fix DI container setup, mock signature, and update deps#81
itisnotyourenv wants to merge 2 commits into
mainfrom
fix/test-di-and-deps

Conversation

@itisnotyourenv

Copy link
Copy Markdown
Owner

Pull Request

Description

Fixes two test failures introduced/exposed by the DI provider refactor in #79, and updates project dependencies.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Configuration change

Changes

Test Fixes

  • tests/integration/conftest.py: Removed invalid worker_postgres_config argument from DBProvider() — it was being passed as Dishka's scope parameter, causing UnknownScopeError in all 9 integration tests. The worker-specific config is already passed via context={Config: worker_config}.
  • tests/unit/presentation/bot/routers/test_commands.py: Added component positional parameter to mock container's get() method to match Dishka's AsyncContainer.get(dep_type, component) signature.

Dependency Updates

  • Updated uv.lock with latest dependency versions.

Testing

  • Tests pass locally with my changes (227 passed)
  • New and existing unit tests pass locally with my changes

Code Quality

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Pre-commit hooks pass
  • Ruff linting passes (ruff check src/ tests/)
  • Ruff formatting passes (ruff format src/ tests/ --check)

Related Issues

Follows up on #79


🤖 Generated with Claude Code

- Remove invalid `worker_postgres_config` arg from `DBProvider()` in
  test conftest — it was interpreted as Dishka's `scope` parameter,
  causing UnknownScopeError in all integration tests.
- Add `component` positional parameter to mock container's `get()` to
  match Dishka's `AsyncContainer.get(dep_type, component)` signature.
- Update uv.lock with latest dependency versions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 14, 2026

Copy link
Copy Markdown

PR Review — 🐛 fix(tests): fix DI container setup, mock signature, and update deps

Summary

Focused, well-scoped bug fix. Both test failures are correctly diagnosed and fixed. The PR description is clear, the commit message follows the project's conventional-commit + emoji convention, and all checks reportedly pass (227 tests green). Overall: approve with one minor note.


Critical Issues

None.


Change-by-Change Analysis

1. tests/integration/conftest.py — Remove worker_postgres_config from DBProvider()

Correct fix. DBProvider (a Dishka Provider subclass) takes scope as its only constructor argument. Passing worker_postgres_config (a Pydantic model) there was silently interpreted as scope, causing UnknownScopeError across all 9 integration tests. The worker config is already wired correctly via context={Config: worker_config, ...}, so the fix is minimal and correct.

2. tests/unit/.../test_commands.py — Add component to mock_get

Correct fix. Dishka's AsyncContainer.get(dep_type, component) has a positional component parameter. The previous mock only accepted **kwargs, so the call site raised a TypeError. The fix adds component: object = None with a default, keeping the mock backward-compatible while matching the real signature.

Minor note: component is silently ignored inside mock_get. That's fine for current tests, but if future tests need component-scoped dependencies, the mock will need updating. Worth a small # component unused — this mock handles single-component cases only comment if the function grows.

3. uv.lock — Dependency updates

Standard lockfile refresh. Notable bumps visible in the diff: aiofiles 24.1.0 → 25.1.0. The author states 227 tests pass, which is the practical signal that nothing broke.


Minor Suggestions (non-blocking)

Inconsistent URL splitting in conftest.py

Line 71 was correctly changed to rsplit("/", maxsplit=1)[-1], but line 152 still uses split("/")[-1]:

# line 71 (fixed) ✅
worker_db_name = worker_url.rsplit("/", maxsplit=1)[-1]

# line 152 (unchanged) ⚠️
worker_db_name = worker_db_url.split("/")[-1]

Both extract the DB name from a URL, so they should use the same approach. For typical postgresql+asyncpg://user:pass@host:port/dbname URLs without query strings this doesn't matter, but the inconsistency is a code smell. Consider aligning line 152 to use rsplit("/", maxsplit=1)[-1] as well.

Raw SQL CREATE DATABASE (pre-existing, not introduced here)

await conn.execute(text(f'CREATE DATABASE "{worker_db_name}"'))

worker_db_name is derived from the test config (not user input), so injection risk is essentially zero. Still, worth a brief comment noting that parameterized DDL isn't supported by PostgreSQL for CREATE DATABASE, so the f-string is intentional. This is pre-existing code and out of scope for this PR, just flagging for awareness.


Questions

None — the PR description covers the "why" clearly and the related PR (#79) gives sufficient context.


Overall Assessment

✅ Approve. Both fixes are minimal, correct, and well-motivated. The lockfile update is routine. The only actionable follow-up is aligning splitrsplit at line 152, which can be done in this PR or as a trivial follow-up.

@itisnotyourenv itisnotyourenv deleted the fix/test-di-and-deps branch April 14, 2026 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant