Unit tests for base_store

[jaylfc]

CARD TITLE (intent, not commit subject): Unit tests for base_store

Autonomous build of board card tsk-k44oda.

Files:
tests/test_base_store.py | 226 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 225 insertions(+), 1 deletion(-)

Summary by CodeRabbit

• Tests
  • Added comprehensive end-to-end coverage for creating, reading, listing, updating, and deleting stored items.
  • Verified persistence, generated IDs, missing records, row counts, owner filtering, uniqueness constraints, rollback behavior, and ID-scoped operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

The test module adds an SQLite-backed CrudStore with asynchronous CRUD methods. Tests cover persistence, IDs, filtering, uniqueness errors, rollback behavior, affected-row counts, and ID-scoped operations.

Changes

CRUD store tests

Layer / File(s)	Summary
Store schema and CRUD operations tests/test_base_store.py	Adds the CrudStore schema and asynchronous methods for creating, reading, listing, updating, and deleting items.
CRUD behavior validation tests/test_base_store.py	Adds fixture setup and tests for persistence, IDs, missing rows, filtering, updates, deletes, and affected-row counts.
Constraint and scope validation tests/test_base_store.py	Adds tests for uniqueness errors, rollback, owner isolation, and ID-scoped updates and deletes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the addition of CRUD and edge-case unit tests for BaseStore.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch exec/tsk-k44oda

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gitar-bot]

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

_Gitar

[jaylfc]

Reviewed: additive expansion of the existing base_store tests (+13, 0 deleted) - CRUD, rowcounts, duplicate-key integrity, and owner-scoping isolation incl. the id-not-owner scoping negatives. On-card, merging on green. The highest-value base_store test is still missing though - a red-capable pin of the SCHEMA-runs-before-_post_init ordering contract (the boot-brick class that has bitten three stores); carding that separately rather than blocking this.

[qodo-code-review]

PR Summary by Qodo

Add CRUD-focused tests for BaseStore connection lifecycle

🧪 Tests 🕐 20-40 Minutes

AI Description

• Add an end-to-end CRUD test store to exercise BaseStore init/commit/rollback/close.
• Cover edge cases: missing rows, rowcount semantics, and per-owner uniqueness/scoping.
• Assert failed inserts don’t corrupt state and require rollback to continue cleanly.

Diagram

graph TD
  T["tests/test_base_store.py"] --> CS["CrudStore (test)"] --> BS["BaseStore"] --> AIO["aiosqlite"] --> DB[("tmp_path SQLite DB")]
  T --> BS

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Test against a real production Store subclass

• ➕ Increases confidence in real schemas/migrations beyond a synthetic table
• ➕ Avoids maintaining a test-only CRUD store implementation
• ➖ Harder to isolate failures to BaseStore vs domain logic
• ➖ May require more fixtures and data setup, making tests slower/more brittle

2. Parameterize CRUD scenarios to reduce repetition

• ➕ Less duplicated test code for create/read/update/delete cases
• ➕ Easier to add new edge cases consistently
• ➖ Can reduce readability when debugging failures
• ➖ Pytest param + async patterns can be noisier than explicit tests

Recommendation: Current approach (a minimal test-only CrudStore) is a good balance: it isolates BaseStore lifecycle guarantees while still doing real end-to-end SQLite I/O. Consider light parameterization only if this suite continues to grow and duplication becomes a maintenance issue.

Files changed (1) +225 / -1

Tests (1) +225 / -1

test_base_store.pyAdd end-to-end CRUD test store and edge-case coverage for BaseStore +225/-1

Add end-to-end CRUD test store and edge-case coverage for BaseStore

• Introduces a minimal CrudStore subclass with a real schema to exercise BaseStore’s connection lifecycle end-to-end. Adds CRUD tests for persistence, rowcount semantics, missing-row behavior, owner-scoped listing, uniqueness constraint failures (IntegrityError), and explicit rollback handling after failed writes.

tests/test_base_store.py

[jaylfc]

nemotron-ultra-kilo review

VERDICT: Minor style inconsistency; tests are comprehensive and correct.

• tests/test_base_store.py:1: Em dash (—) changed to double hyphen (--) in module docstring — inconsistent with surrounding codebase style (other docstrings use em dashes)

• tests/test_base_store.py:178: await store._db.rollback() accesses private _db attribute; consider adding a public rollback() method to BaseStore for testability

• tests/test_base_store.py:152-377: Very large test class (226 lines added) in single file — consider splitting into separate test module for CrudStore if this pattern grows
  VERDICT: Minor style inconsistency; tests are comprehensive and correct.

• tests/test_base_store.py:1: Em dash (—) changed to double hyphen (--) in module docstring — inconsistent with surrounding codebase style (other docstrings use em dashes)

• tests/test_base_store.py:178: await store._db.rollback() accesses private _db attribute; consider adding a public rollback() method to BaseStore for testability

• tests/test_base_store.py:152-377: Very large test class (226 lines added) in single file — consider splitting into separate test module for CrudStore if this pattern grows

Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[jaylfc]

nemotron-ultra-orB review

VERDICT: Tests are functionally correct but have fragile patterns and missing edge-case coverage.

• tests/test_base_store.py:158-160: _row_to_dict assumes fixed column order from SQL queries; any schema/query change breaks it silently. Use cursor.description or explicit column mapping.
• tests/test_base_store.py:282: await store._db.rollback() after IntegrityError is suspect — aiosqlite auto-rolls back on exception; explicit rollback may error or be no-op.
• tests/test_base_store.py:276-289: Test accesses private _db attribute, coupling to implementation. Use public API or add helper method.
• tests/test_base_store.py:171-175: UNIQUE(owner, key) with nullable owner allows duplicate (NULL, 'key') rows in SQLite (NULL != NULL). No test covers NULL owner behavior.
• tests/test_base_store.py:224: _make_store doesn't use self; should be @staticmethod or module-level function.
• tests/test_base_store.py: Missing tests for owner=None in create/get/update/delete, concurrent access, and special characters in key/value.

Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge.

[jaylfc]

nemotron-super review

VERDICT: Pass
No blocking issues found.

Automated first-pass review by the nemotron-super lane. The lead still reviews before merge.

[coderabbitai]

🧹 Nitpick comments (2)

tests/test_base_store.py (2)

165-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider zip(..., strict=True) for column/row alignment.

_row_to_dict zips _COLUMNS with row without strict=True. If a future SELECT statement's column list and _COLUMNS ever fall out of sync, zip silently truncates instead of raising an error.

♻️ Proposed fix

 def _row_to_dict(row) -> dict | None:
-    return dict(zip(_COLUMNS, row)) if row is not None else None
+    return dict(zip(_COLUMNS, row, strict=True)) if row is not None else None

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_base_store.py` around lines 165 - 170, Update _row_to_dict to use
strict zip semantics when pairing _COLUMNS with row, so mismatched column and
row lengths raise an error instead of silently truncating; preserve the existing
None-to-None behavior.

Source: Linters/SAST tools

---

172-183: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

UNIQUE(owner, key) does not prevent duplicate keys when owner is NULL.

SQLite treats NULL values as distinct for uniqueness purposes, so two rows with owner = NULL and the same key would both insert successfully without raising sqlite3.IntegrityError. None of the added tests exercise this case (test_duplicate_owner_key_raises_integrity and test_duplicate_key_allowed_across_owners only use non-null owners). Confirm whether this NULL-owner gap is intentional for this fixture, since a reader could otherwise assume (owner, key) is always enforced as unique.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_base_store.py` around lines 172 - 183, The CrudStore schema’s
UNIQUE(owner, key) constraint does not enforce uniqueness for NULL owners.
Update the fixture or its tests to explicitly define and verify the intended
NULL-owner behavior, ensuring readers are not misled about whether duplicate
keys with owner=NULL should be rejected.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_base_store.py`:
- Around line 165-170: Update _row_to_dict to use strict zip semantics when
pairing _COLUMNS with row, so mismatched column and row lengths raise an error
instead of silently truncating; preserve the existing None-to-None behavior.
- Around line 172-183: The CrudStore schema’s UNIQUE(owner, key) constraint does
not enforce uniqueness for NULL owners. Update the fixture or its tests to
explicitly define and verify the intended NULL-owner behavior, ensuring readers
are not misled about whether duplicate keys with owner=NULL should be rejected.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9f956f33-5200-4c8b-b81a-b812bff0824e

📥 Commits

Reviewing files that changed from the base of the PR and between 3242887 and da6b4fc.

📒 Files selected for processing (1)

• tests/test_base_store.py

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)

• tests/test_base_store.py

---

_{Reviewed by step-3.7-flash · Input: 139.2K · Output: 26K · Cached: 1.2M}