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
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions