Skip to content

test-quality: tests/test_storage_vector.py 'vector refresh failed' assertion couples two exception layers #44

@aksOps

Description

@aksOps

Summary

`tests/test_storage_vector.py:114-115` (rc3 / PR #41) asserts both `"vector delete failed"` and `"vector refresh failed"` log messages when a fake vector store raises on both `delete()` and `add_documents()`. The first assertion is internal to `_refresh_vector()`; the second relies on the exception propagating up to `SessionStore.save()`'s outer `try/except`, which logs via the same helper with `operation="refresh"`.

Code-reviewer flagged: a future refactor that moves the `add_documents` catch inside `_refresh_vector` (which is the natural place for it) would silently break the second assertion's intent without changing production behavior.

Suggested fix

Either:

  1. Add a comment to the test block explaining the two-layer coupling (`# 'vector refresh failed' originates from save()'s outer catch, not _refresh_vector — see PR feat: v2.0.0-rc3 — fix audit findings (finalizer, state_overrides, idempotency + 6 important) #41 review`).
  2. Move the `add_documents` catch into `_refresh_vector` itself (cleaner), and update the test accordingly.

(1) is the minimal-touch path.

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions