Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR expands the StateMachine test suite with additional edge-case coverage and adds an in-file TODO backlog for potential future graph/collection/time-based APIs.
Changes:
- Added a TODO list of potential
StateMachineAPIs (graph traversal, collection utilities, bulk add, recency queries). - Added new unit tests covering disconnect idempotency, missing-memory behaviors, time range empty results, and update edge cases.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn test_disconnect_from_nonexistent_memory() { | ||
| let mut sm = StateMachine::new(); | ||
| sm.add(create_test_entry(1, "default", 1000)).unwrap(); | ||
|
|
||
| sm.disconnect(MemoryId(999), MemoryId(1)).unwrap(); | ||
| assert_eq!(sm.len(), 1); |
There was a problem hiding this comment.
This test encodes that disconnect is a no-op even when from does not exist. That’s inconsistent with connect/get_neighbors, which return MemoryNotFound for unknown IDs. Please decide the intended API semantics and align accordingly (either make disconnect return MemoryNotFound for missing from/to and update this test, or rename/expand the test and add documentation explicitly stating disconnect is idempotent even for missing nodes).
| fn test_disconnect_from_nonexistent_memory() { | |
| let mut sm = StateMachine::new(); | |
| sm.add(create_test_entry(1, "default", 1000)).unwrap(); | |
| sm.disconnect(MemoryId(999), MemoryId(1)).unwrap(); | |
| assert_eq!(sm.len(), 1); | |
| fn test_disconnect_is_idempotent_even_when_endpoint_memories_are_missing() { | |
| let mut sm = StateMachine::new(); | |
| sm.add(create_test_entry(1, "default", 1000)).unwrap(); | |
| // `disconnect` is intentionally a no-op when either endpoint is missing: | |
| // the postcondition is simply that the edge does not exist. | |
| sm.disconnect(MemoryId(999), MemoryId(1)).unwrap(); | |
| sm.disconnect(MemoryId(1), MemoryId(999)).unwrap(); | |
| assert_eq!(sm.len(), 1); | |
| let neighbors = sm.get_neighbors(MemoryId(1)).unwrap(); | |
| assert!(neighbors.is_empty()); |
| #[test] | ||
| fn test_multiple_edges_same_relation_rejected() { | ||
| let mut sm = StateMachine::new(); | ||
| sm.add(create_test_entry(1, "default", 1000)).unwrap(); | ||
| sm.add(create_test_entry(2, "default", 1000)).unwrap(); | ||
|
|
||
| sm.connect(MemoryId(1), MemoryId(2), "rel".to_string()).unwrap(); | ||
| sm.connect(MemoryId(1), MemoryId(2), "rel".to_string()).unwrap(); | ||
|
|
||
| let neighbors = sm.get_neighbors(MemoryId(1)).unwrap(); | ||
| assert_eq!(neighbors.len(), 1); | ||
| } |
There was a problem hiding this comment.
Test name is misleading: the second connect call is expected to succeed (it’s unwrapped) and the behavior under test is edge de-duplication, not rejection via an error. Rename the test (and/or assert the exact behavior you want, e.g., that the second call returns Ok and does not increase neighbor count).
| // pub fn has_path(&self, from: MemoryId, to: MemoryId) -> bool | ||
|
|
||
| // TODO: Delete all memories in a collection | ||
| // pub fn delete_collection(&mut self, collection: &str) -> Result<()> | ||
|
|
||
| // TODO: Get all unique collection names | ||
| // pub fn collections(&self) -> Vec<String> | ||
|
|
||
| // TODO: Get count of memories in a collection | ||
| // pub fn collection_len(&self, collection: &str) -> usize | ||
|
|
||
| // TODO: Get N most recent memories | ||
| // pub fn get_latest(&self, n: usize) -> Vec<&MemoryEntry> | ||
|
|
||
| // TODO: Get N oldest memories | ||
| // pub fn get_oldest(&self, n: usize) -> Vec<&MemoryEntry> | ||
|
|
||
| // TODO: Add multiple entries in bulk | ||
| // pub fn add_many(&mut self, entries: Vec<MemoryEntry>) -> Result<()> |
There was a problem hiding this comment.
These commented-out function “signatures” inside the impl can easily drift out of sync and aren’t valid Rust declarations if someone tries to “uncomment” them later (impl fns require bodies). Consider moving this backlog to an issue/tracking doc, or add real stub methods with a minimal body (e.g., returning an error / todo!) behind an explicit feature flag so the compiler keeps signatures honest.
| // pub fn has_path(&self, from: MemoryId, to: MemoryId) -> bool | |
| // TODO: Delete all memories in a collection | |
| // pub fn delete_collection(&mut self, collection: &str) -> Result<()> | |
| // TODO: Get all unique collection names | |
| // pub fn collections(&self) -> Vec<String> | |
| // TODO: Get count of memories in a collection | |
| // pub fn collection_len(&self, collection: &str) -> usize | |
| // TODO: Get N most recent memories | |
| // pub fn get_latest(&self, n: usize) -> Vec<&MemoryEntry> | |
| // TODO: Get N oldest memories | |
| // pub fn get_oldest(&self, n: usize) -> Vec<&MemoryEntry> | |
| // TODO: Add multiple entries in bulk | |
| // pub fn add_many(&mut self, entries: Vec<MemoryEntry>) -> Result<()> | |
| // TODO: Implement path existence checks between two memories. | |
| // TODO: Implement deleting all memories in a collection. | |
| // TODO: Implement returning all unique collection names. | |
| // TODO: Implement counting memories in a collection. | |
| // TODO: Implement returning the N most recent memories. | |
| // TODO: Implement returning the N oldest memories. | |
| // TODO: Implement bulk insertion of multiple entries. |
No description provided.