test: prefer id lookups over aggregate find#31
Merged
Conversation
📝 WalkthroughWalkthroughTwo test modules are refactored: snapshot tests migrate to bulk fetch semantics with simplified assertion patterns, and aggregate repository tests are replaced by an integration test validating metadata propagation through the outbox and publisher pipeline. ChangesTest Refactoring and Outbox Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/snapshots/main.rs (1)
219-221: ⚡ Quick winAssert Alice’s fields from
get_allresults, not a separateget.These checks currently validate
repo.get("t1"), which bypasses theget_allpath being tested. Pullt1fromtodosand assert on that record to keep this test scoped to bulk-fetch behavior.Suggested diff
- let alice = repo.get("t1").unwrap().unwrap(); - assert_eq!(alice.snapshot().user_id, "alice"); - assert_eq!(alice.snapshot().task, "Buy milk"); + let alice = todos + .iter() + .find(|todo| todo.snapshot().id == "t1") + .expect("t1 should be present in get_all result"); + assert_eq!(alice.snapshot().user_id, "alice"); + assert_eq!(alice.snapshot().task, "Buy milk");🤖 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/snapshots/main.rs` around lines 219 - 221, The test is asserting Alice's fields using repo.get("t1") which bypasses the get_all path under test; instead, locate the record with id "t1" inside the collection returned by repo.get_all (the todos variable), assign that entry to alice (or a similarly named variable), and assert on alice.snapshot().user_id and alice.snapshot().task so the assertions validate the bulk-fetch (get_all) results rather than a separate repo.get call.
🤖 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.
Inline comments:
In `@tests/todos/aggregate.rs`:
- Around line 93-99: The add_todo method currently appends the todo id to
completed_ids or pending_ids without ensuring mutual exclusivity or preventing
duplicates; update add_todo (working with TodoSnapshot) to first remove the
todo.id from both completed_ids and pending_ids (or at least from the opposite
bucket), then push the id into the correct bucket (completed_ids if
todo.completed is true, otherwise pending_ids) so a re-indexed todo never exists
in both lists or duplicated.
---
Nitpick comments:
In `@tests/snapshots/main.rs`:
- Around line 219-221: The test is asserting Alice's fields using repo.get("t1")
which bypasses the get_all path under test; instead, locate the record with id
"t1" inside the collection returned by repo.get_all (the todos variable), assign
that entry to alice (or a similarly named variable), and assert on
alice.snapshot().user_id and alice.snapshot().task so the assertions validate
the bulk-fetch (get_all) results rather than a separate repo.get call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cf42d9f-1198-4789-ba65-eb840911de66
📒 Files selected for processing (3)
tests/snapshots/main.rstests/todos/aggregate.rstests/todos/main.rs
Comment on lines
+93
to
+99
| pub fn add_todo(&mut self, todo: &TodoSnapshot) { | ||
| if todo.completed { | ||
| self.completed_ids.push(todo.id.clone()); | ||
| } else { | ||
| self.pending_ids.push(todo.id.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep status buckets mutually exclusive when updating an existing todo ID.
Line 93 currently only appends, so a todo that is re-indexed after status changes can end up in both lists (or duplicated), breaking the status partition.
Proposed fix
pub fn add_todo(&mut self, todo: &TodoSnapshot) {
+ self.completed_ids.retain(|id| id != &todo.id);
+ self.pending_ids.retain(|id| id != &todo.id);
if todo.completed {
self.completed_ids.push(todo.id.clone());
} else {
self.pending_ids.push(todo.id.clone());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn add_todo(&mut self, todo: &TodoSnapshot) { | |
| if todo.completed { | |
| self.completed_ids.push(todo.id.clone()); | |
| } else { | |
| self.pending_ids.push(todo.id.clone()); | |
| } | |
| } | |
| pub fn add_todo(&mut self, todo: &TodoSnapshot) { | |
| self.completed_ids.retain(|id| id != &todo.id); | |
| self.pending_ids.retain(|id| id != &todo.id); | |
| if todo.completed { | |
| self.completed_ids.push(todo.id.clone()); | |
| } else { | |
| self.pending_ids.push(todo.id.clone()); | |
| } | |
| } |
🤖 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/todos/aggregate.rs` around lines 93 - 99, The add_todo method currently
appends the todo id to completed_ids or pending_ids without ensuring mutual
exclusivity or preventing duplicates; update add_todo (working with
TodoSnapshot) to first remove the todo.id from both completed_ids and
pending_ids (or at least from the opposite bucket), then push the id into the
correct bucket (completed_ids if todo.completed is true, otherwise pending_ids)
so a re-indexed todo never exists in both lists or duplicated.
patrickleet
force-pushed
the
test/use-id-and-readmodel-lookups
branch
from
0cbad33 to
7f88db3
Compare
patrickleet
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/snapshots/main.rs (1)
215-221: ⚡ Quick winAssert
get_allcontent directly instead of re-fetchingt1.Line 219 re-queries
repo.get("t1"), so the test does not fully validate thatget_allreturned the expected entity payloads.Proposed test-focused adjustment
- let todos = repo.get_all(&["t1", "t2"]).unwrap(); - assert_eq!(todos.len(), 2); - assert!(todos.iter().all(|todo| todo.snapshot().completed)); - - let alice = repo.get("t1").unwrap().unwrap(); - assert_eq!(alice.snapshot().user_id, "alice"); - assert_eq!(alice.snapshot().task, "Buy milk"); + let todos = repo.get_all(&["t1", "t2"]).unwrap(); + assert_eq!(todos.len(), 2); + assert!(todos.iter().all(|todo| todo.snapshot().completed)); + assert!(todos.iter().any(|todo| { + let s = todo.snapshot(); + s.id == "t1" && s.user_id == "alice" && s.task == "Buy milk" + })); + assert!(todos.iter().any(|todo| todo.snapshot().id == "t2"));🤖 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/snapshots/main.rs` around lines 215 - 221, The test currently re-fetches repo.get("t1") instead of validating the payload returned by repo.get_all; change the assertions to inspect the todos variable directly: locate the returned entity for id "t1" from the Vec returned by repo.get_all (e.g. find by id or use ordering since get_all was called with ["t1","t2"]), then assert on that entity's snapshot().user_id and snapshot().task (and completed) instead of calling repo.get; update references to repo.get_all, todos, and snapshot() in the test accordingly.
🤖 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/snapshots/main.rs`:
- Around line 215-221: The test currently re-fetches repo.get("t1") instead of
validating the payload returned by repo.get_all; change the assertions to
inspect the todos variable directly: locate the returned entity for id "t1" from
the Vec returned by repo.get_all (e.g. find by id or use ordering since get_all
was called with ["t1","t2"]), then assert on that entity's snapshot().user_id
and snapshot().task (and completed) instead of calling repo.get; update
references to repo.get_all, todos, and snapshot() in the test accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f1261f1-b8c3-436d-8b05-8bc98b31694d
📒 Files selected for processing (2)
tests/snapshots/main.rstests/todos/main.rs
💤 Files with no reviewable changes (1)
- tests/todos/main.rs
This was referenced May 21, 2026
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.
Summary
Verification
Note: full all-features test still emits the pre-existing Reservation dead-code warning in tests/sagas/order/inventory.rs.
Summary by CodeRabbit