Skip to content

fix: bump dependencies to latest#135

Merged
Besthope-Official merged 5 commits intomainfrom
fix/bump-new-dep
Mar 5, 2026
Merged

fix: bump dependencies to latest#135
Besthope-Official merged 5 commits intomainfrom
fix/bump-new-dep

Conversation

@Besthope-Official
Copy link
Copy Markdown
Contributor

Related Issue

Closes #129

Summary of Changes

  • bump to latest version of dependencies
  • reuse fixture in integration tests
  • use session level of fixture to prevent early clean of test db

Breaking Changes

N/A

Checklist

  • Issue discussion completed before opening PR
  • Scope is small and focused (single feature/fix)
  • All functions have full type annotations
  • Async/await used for all I/O operations
  • Tests added for new behaviors

@Besthope-Official Besthope-Official added the bug Something isn't working label Mar 5, 2026
@Besthope-Official
Copy link
Copy Markdown
Contributor Author

@claude do code quality check for this PR

@claude
Copy link
Copy Markdown

claude Bot commented Mar 5, 2026

Claude finished @Besthope-Official's task in 4m 52s —— View job


Code Quality Check — PR #135

  • Gather context and read changed files
  • Analyze code quality
  • Post findings

Summary

The PR correctly consolidates duplicate fixtures and switches to a session-scoped engine for efficiency. Dependency bumps look clean. A few issues are worth addressing:


Issues Found

1. Multiple long-lived sessions held open during test execution — tests/integration/conftest.py:34–108

admin_user, regular_user, and superuser_user all yield while inside async with test_db() as session:. This keeps a DB connection checked out from the pool for the entire test duration. When all three are injected into the same test (e.g. rbac_audit_client), three sessions are simultaneously open alongside any sessions created by the test body itself.

SQLite serializes writes, so this won't corrupt data, but it risks database is locked timeouts—especially with timeout = 5 in pytest config. The fix is to commit and close before yielding:

# Instead of:
async with test_db() as session:
    ...
    await session.commit()
    await session.refresh(user)
    yield user            # ← session stays open here

# Prefer:
async with test_db() as session:
    ...
    await session.commit()
    await session.refresh(user)
    user_id = user.id    # capture what's needed

yield user               # ← session is already closed

Fix this →


2. PasswordHelper imported inside fixture bodies — tests/integration/conftest.py:37,64,91

@pytest.fixture
async def admin_user(test_db, rbac_data):
    from fastapi_users.password import PasswordHelper   # ← deferred import

PasswordHelper is imported three times inside separate fixture bodies. It should be a single top-level import alongside the existing ones on line 1–4. Deferred imports here provide no benefit and obscure dependencies.

Fix this →


3. test_db fixture scope is implicit — tests/conftest.py:97

test_engine is explicitly scope="session", but test_db (which depends on it and calls drop_all/create_all) has no explicit scope, defaulting to function. The asymmetry is not wrong, but it's easy to miss. Adding scope="function" explicitly would make the intent clear.


4. rbac_data not consumed by test_audit_rbac.py tests that previously passed with fewer permissions

The old test_audit_rbac.py rbac_data only gave role_id=1 permissions id=1 and id=2. The new shared rbac_data gives role 1 four permissions (including admin:*). The existing audit tests only assert on specific audit log entries and don't enumerate all permissions, so they still pass—but this is worth noting in case future audit tests assert on the exact permission set in extra.


Positive Notes

  • Moving to file-based SQLite + session-scoped engine with tmp_path_factory is the right approach to avoid the "early drop" problem described in the PR.
  • Using @event.listens_for(engine.sync_engine, "connect") for the PRAGMA is the correct pattern for async engines.
  • Adding engine.dispose() in test_mixins.py is a good cleanup improvement.
  • try/finally around yields in test_db and mixin_db correctly guards against teardown being skipped on test failure.
  • asyncio_default_fixture_loop_scope = "session" and asyncio_default_test_loop_scope = "session" in pyproject.toml is the correct fix for the loop scope deprecation warning.

@Besthope-Official Besthope-Official merged commit 18e33fe into main Mar 5, 2026
3 checks passed
@Besthope-Official Besthope-Official deleted the fix/bump-new-dep branch March 5, 2026 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: test_concurrent_requests_have_unique_request_ids fails on Python 3.13

1 participant