test(e2e): scenario 5 — v1.4 migration roundtrip (#334 step 3) - #366
Merged
Conversation
The snapshot is built from the v1.4.0 PyPI release seeded with three distinctive lock statements (quokka/aardvark/wibble). build-v14-snapshot.sh regenerates the fixture deterministically from a fresh aelfrice==1.4.0 install — re-run when the v1.4 install path changes or to verify reproducibility. Unignore tests/e2e/fixtures/*.db so the binary fixture checks in (avoids requiring PyPI access at test time).
Loads the v1.4 fixture DB through the installed aelf binary and asserts:
1. aelf search returns each seeded statement's distinctive token
after the implicit migration on first connect.
2. The migration adds the post-v1.4 tables that current code
requires (belief_corroborations, ingest_log, belief_versions).
3. The original belief rows are preserved verbatim through the
additive schema changes.
Catches the class of regression where unit tests stay green against
fresh-init DBs but a migration silently drops or corrupts existing
rows on real on-disk shapes. Advances #334 acceptance gate from
3/4 to 4/4 seed scenarios.
Reviewer's GuideAdds an end-to-end migration scenario that validates upgrading a real v1.4 SQLite database to the current schema, using a checked-in binary fixture and a deterministic builder script, and verifies search behavior and table/row integrity via subprocess-invoked File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
yoshi280
added
attn:review
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR adds support for end-to-end testing of v1.4→current database migrations: a fixture-building script that creates a v1.4 snapshot database from a clean install, a Changesv1.4 Migration E2E Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The subprocess calls build a minimal
envwith onlyAELFRICE_DBandPATH, which discards the rest of the parent environment; consider starting fromos.environ.copy()and overlayingAELFRICE_DB/PATHso the tests don’t accidentally depend on missing variables on some systems. - The repeated
FIXTURE.exists()checks and skip messages could be centralized into a small helper or fixture so the regeneration instructions and path logic stay single-sourced and easier to update if the fixture moves.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The subprocess calls build a minimal `env` with only `AELFRICE_DB` and `PATH`, which discards the rest of the parent environment; consider starting from `os.environ.copy()` and overlaying `AELFRICE_DB`/`PATH` so the tests don’t accidentally depend on missing variables on some systems.
- The repeated `FIXTURE.exists()` checks and skip messages could be centralized into a small helper or fixture so the regeneration instructions and path logic stay single-sourced and easier to update if the fixture moves.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
[claim:review:Gylf:2026-05-03T03:40:03Z] |
Collaborator
Author
|
[release:review:Gylf:2026-05-03T03:40:58Z] |
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
2 participants
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.
Summary
Step 3 of #334 rollout: lands scenario #5 (v1.4 → current migration), advancing the umbrella's seed-scenario gate from 3/4 to 4/4.
tests/e2e/fixtures/v14-snapshot.dbis a 124KB SQLite snapshot built from a cleanaelfrice==1.4.0install with three seededaelf lockstatements (quokka/aardvark/wibble).build-v14-snapshot.shregenerates it idempotently from the v1.4.0 PyPI release..gitignoreunignorestests/e2e/fixtures/*.dbso the binary fixture checks in — keeps the e2e job offline.tests/e2e/test_migration_v14_to_current.py— 3 tests:test_v14_snapshot_seeds_are_searchable_after_migration—aelf search <token>returns each seeded statement; pre-asserts the fixture genuinely lacks the post-v1.4 tables so the test doesn't degrade silently.test_v14_migration_grows_tables_and_preserves_belief_rows— the migration addsbelief_corroborations+ingest_log+belief_versions, and the original belief rows survive verbatim.test_search_after_migration_returns_seeded_belief— same shape but goes through the suite'saelf_runfixture (env-overlay path).The bug class: unit suites that test fresh-init DBs are structurally blind to a migration that drops or corrupts existing rows on a real on-disk shape. This test fails when that drift happens.
Test plan
uv run pytest tests/e2e/ -q— all 9 cases pass locally (6 from steps 1-2 + 3 new)uvx ruff check tests/e2e/test_migration_v14_to_current.py— cleanaelfbinary refs, public API)aelfrice==1.4.0e2eworkflow runs on this PR via thee2elabel opt-in (reviewer adds label)Closes #334's seed-gate (4/4 scenarios). Failure-surfacing job +
docs/testing-strategy.mdremain (separate follow-up PRs).Summary by Sourcery
Add an end-to-end migration scenario ensuring v1.4 SQLite databases upgrade correctly to the current schema without losing data.
Tests:
Chores:
Summary by CodeRabbit
Tests
Chores