test(db): cover accounts CRUD + settings_db round-trips#67
Merged
Conversation
Two modules with zero coverage flagged by the coverage audit (2026-05-11). Both are foundational — accounts handles OAuth-linked identities + Product Truth (security-adjacent), settings_db backs the active_provider / active_model persistence the AI flow depends on. Tests use the same fresh in-memory pool pattern as history and groups. ## db::accounts (+10 tests) - upsert_inserts_new_account_and_returns_it - upsert_on_existing_account_updates_in_place - list_returns_all_accounts_ordered_by_created_at - get_by_id_returns_account_and_404s_on_missing - update_product_truth_round_trips_and_clears - update_branding_sets_both_colors_atomically (partial-clear case) - update_display_handle_overrides_and_clears (migration 016 regression guard for LinkedIn full-name vs brand handle) - update_visual_profile_round_trips_and_clears - delete_removes_account_and_nulls_post_references (manual cascade enforced in app code because SQLite forbids FK on ALTER ADD) - delete_is_idempotent_on_missing_account (race between publish flow and disconnect must not panic) ## db::settings_db (+5 tests) - get_returns_none_for_unknown_key - set_then_get_round_trips - set_on_existing_key_upserts_the_value (refresh-model flow) - migrations_seed_default_provider_and_model (consumer-POV smoke for the migrations chain 001/005/006/008/010) - set_accepts_empty_string_value (intentional — keeps the function pure key-value, no business-rule pollution) Total: Rust 221/221 (was 206), clippy + fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
🧙 Sourcery a terminé la revue de votre pull request ! Conseils et commandesInteragir avec Sourcery
Personnaliser votre expérienceAccédez à votre dashboard pour :
Obtenir de l’aide
Original review guide in English🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Salut - j’ai laissé quelques retours généraux :
- Le helper
fresh_poolest maintenant dupliqué dans plusieurs modules de tests de la base de données ; envisage d’extraire une utilité de test partagée (par ex.db::test_utils::fresh_pool()) pour centraliser la configuration du pool en mémoire initialisé par les migrations. - Le test
list_returns_all_accounts_ordered_by_created_atsuppose actuellement que l’ordre d’insertion est égal à l’ordre decreated_at; si l’implémentation change un jour les timestamps (par ex. avec des insertions anté-datées), cela pourrait devenir fragile, donc tu pourrais vouloir vérifier l’ordre via des timestamps explicites ou clarifier que l’ordre d’insertion fait partie du contrat.
Prompt pour agents IA
Veuillez traiter les commentaires de cette revue de code :
## Commentaires généraux
- Le helper `fresh_pool` est maintenant dupliqué dans plusieurs modules de tests de la base de données ; envisage d’extraire une utilité de test partagée (par ex. `db::test_utils::fresh_pool()`) pour centraliser la configuration du pool en mémoire initialisé par les migrations.
- Le test `list_returns_all_accounts_ordered_by_created_at` suppose actuellement que l’ordre d’insertion est égal à l’ordre de `created_at` ; si l’implémentation change un jour les timestamps (par ex. avec des insertions anté-datées), cela pourrait devenir fragile, donc vous pourriez vouloir vérifier l’ordre via des timestamps explicites ou clarifier que l’ordre d’insertion fait partie du contrat.Sourcery est gratuit pour l’open source - si vous appréciez nos revues, pensez à les partager ✨
Original comment in English
Hey - I've left some high level feedback:
- The
fresh_poolhelper is now duplicated across multiple DB test modules; consider extracting a shared test utility (e.g.,db::test_utils::fresh_pool()) to centralize the migration-bootstrapped in-memory pool setup. - The
list_returns_all_accounts_ordered_by_created_attest currently assumes insertion order equalscreated_atorder; if the implementation ever changes timestamps (e.g., with backdated inserts), this could become fragile, so you might want to assert ordering via explicit timestamps or clarify that insertion order is a contract.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `fresh_pool` helper is now duplicated across multiple DB test modules; consider extracting a shared test utility (e.g., `db::test_utils::fresh_pool()`) to centralize the migration-bootstrapped in-memory pool setup.
- The `list_returns_all_accounts_ordered_by_created_at` test currently assumes insertion order equals `created_at` order; if the implementation ever changes timestamps (e.g., with backdated inserts), this could become fragile, so you might want to assert ordering via explicit timestamps or clarify that insertion order is a contract.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Continuing the coverage audit started on this branch. `sidecar.rs` has 3 public functions but they're all process-spawn / async IO — true integration territory. What IS testable cleanly without a real Python install is the SERDE contract of the IPC types, and that's exactly the layer where silent cross-language drift hits the user the hardest. - sidecar_request_serialises_with_all_fields — every Python-side field present in the wire JSON. Renaming a Rust field without updating the sidecar would silently break generation. - sidecar_request_serialises_api_key_none_as_null — SECURITY: Ollama / local flows pass None; serde must emit `null`, not omit. Skipping would let the sidecar fall back to an env-var read on the Python side. - sidecar_data_deserialises_with_usage_field — cost tracker contract for the `usage` token counts. - sidecar_data_deserialises_without_usage_for_legacy_sidecars — `#[serde(default)] Option<TokenUsage>` keeps stale sidecars parseable instead of breaking every BYOK user on update lag. - token_usage_defaults_to_zero_when_field_missing — verify the `#[serde(default)]` on TokenUsage fields so a future refactor that drops the attribute is caught. Total: Rust 226/226 (was 221), clippy + fmt clean. Process-spawn coverage stays in the integration tier — out of scope for `cargo test --lib` and tracked for a future end-to-end suite once we have Python available in CI. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Coverage audit follow-up
Spotted 8 Rust modules with public functions but no tests. This PR fills the two highest-ROI gaps:
db::accounts— security-adjacent OAuth identity store, 8 public fns, 0 tests before this PRdb::settings_db— backsactive_provider/active_modelpersistence the AI flow depends on, 2 public fns, 0 testsRemaining gaps are tracked in the v0.4.x backlog — they'll get covered as we touch those modules.
What's locked in
accounts (+10 tests):
post_history.account_idbecause SQLite forbids FK onALTER TABLE ADD COLUMN(migration 013 hotfix history). Regression guard against silently dropping the cascade in a future refactor.settings_db (+5 tests):
Test counts
Rust 221/221 (was 206), clippy + fmt clean. No frontend or sidecar changes.
Why these two first
db::accountscarries Product Truth + brand colours + display handle — content that lands in every generated post and every rendered image. Bugs there mutate the AI output silently.db::settings_dbis the persistence layer for the model/provider selection — a broken set/get would mean the user re-picks Claude Sonnet on every launch. Both are foundational without being deeply business-logic, so unit tests carry their weight directly.🤖 Generated with Claude Code
Résumé par Sourcery
Ajout de tests de base de données axés sur la régression pour la gestion des comptes et la persistance des paramètres.
Tests :
db::accounts, y compris les suppressions en cascade et les suppressions idempotentes.settings_dbcouvrant le comportement de get/set, les valeurs par défaut fournies par les migrations et l’acceptation des valeurs de chaîne vide.Original summary in English
Summary by Sourcery
Add regression-focused database tests for account management and settings persistence.
Tests: