[Issue #4] Add SnapshotStore unit tests #31

HugoNijhuis · 2026-01-09T16:13:17Z

HugoNijhuis commented

2026-01-09 16:13:17 +00:00

Summary

Implement SaveSnapshot and GetLatestSnapshot methods on InMemoryEventStore to satisfy the SnapshotStore interface, along with comprehensive unit tests.

Changes

Extended InMemoryEventStore with snapshots map storage
Implemented SaveSnapshot method for persisting snapshots
Implemented GetLatestSnapshot method that returns snapshot with highest version
Added 16 unit tests covering all acceptance criteria

Test Coverage

SaveSnapshot persists snapshots correctly
GetLatestSnapshot returns most recent snapshot by version
Behavior when no snapshot exists (returns nil, nil)
Snapshot versioning respected across actors
Version ordering (higher version wins, not insertion order)
Data integrity for complex, nested, and special character states
Edge cases: zero version, large version, empty/nil state
Interface compliance verification

Closes #4

## Summary Implement SaveSnapshot and GetLatestSnapshot methods on InMemoryEventStore to satisfy the SnapshotStore interface, along with comprehensive unit tests. ## Changes - Extended InMemoryEventStore with snapshots map storage - Implemented SaveSnapshot method for persisting snapshots - Implemented GetLatestSnapshot method that returns snapshot with highest version - Added 16 unit tests covering all acceptance criteria ## Test Coverage - SaveSnapshot persists snapshots correctly - GetLatestSnapshot returns most recent snapshot by version - Behavior when no snapshot exists (returns nil, nil) - Snapshot versioning respected across actors - Version ordering (higher version wins, not insertion order) - Data integrity for complex, nested, and special character states - Edge cases: zero version, large version, empty/nil state - Interface compliance verification Closes #4

HugoNijhuis added 1 commit 2026-01-09 16:13:18 +00:00

Add comprehensive unit tests for SnapshotStore

CI / build (pull_request) Successful in 17s

Details

0b2b6a3125

Implement SaveSnapshot and GetLatestSnapshot methods on InMemoryEventStore
to satisfy the SnapshotStore interface, and add comprehensive tests covering:

- SaveSnapshot persists snapshots correctly
- GetLatestSnapshot returns most recent snapshot by version
- Behavior when no snapshot exists (returns nil, nil)
- Snapshot versioning is respected across actors
- Version ordering (higher version wins, not insertion order)
- Data integrity for complex, nested, and special character states
- Edge cases: zero version, large version, empty/nil state

Closes #4

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

HugoNijhuis commented

2026-01-09 16:28:22 +00:00

Code Review

Summary

Good implementation of SnapshotStore interface with comprehensive tests. A few issues to address before merging.

Issues

1. Missing thread safety (Bug) - store/memory.go:63-88

The snapshot methods lack mutex protection. Since PR #32 is adding mutex protection for event methods, please add the same protection to SaveSnapshot and GetLatestSnapshot for consistency.

2. No nil validation in SaveSnapshot - store/memory.go:63

Passing nil snapshot will cause a panic. Please add validation:

func (es *InMemoryEventStore) SaveSnapshot(snapshot *aether.ActorSnapshot) error {
    if snapshot == nil {
        return fmt.Errorf("snapshot cannot be nil")
    }
    // ...
}

3. Test key collision - store/memory_test.go:395

The key generation in TestSnapshotDataIntegrity_LargeState has collisions due to modulo arithmetic (i=0 and i=260 both produce "a0"). Consider using fmt.Sprintf("key-%d", i) instead.

Suggestions (optional)

Add a concurrent access test for snapshots (similar to PR #32's event concurrency test)

Once the thread safety and nil validation are addressed, this will be ready to merge.

## Code Review ### Summary Good implementation of SnapshotStore interface with comprehensive tests. A few issues to address before merging. ### Issues **1. Missing thread safety (Bug)** - `store/memory.go:63-88` The snapshot methods lack mutex protection. Since PR #32 is adding mutex protection for event methods, please add the same protection to `SaveSnapshot` and `GetLatestSnapshot` for consistency. **2. No nil validation in SaveSnapshot** - `store/memory.go:63` Passing nil snapshot will cause a panic. Please add validation: ```go func (es *InMemoryEventStore) SaveSnapshot(snapshot *aether.ActorSnapshot) error { if snapshot == nil { return fmt.Errorf("snapshot cannot be nil") } // ... } ``` **3. Test key collision** - `store/memory_test.go:395` The key generation in `TestSnapshotDataIntegrity_LargeState` has collisions due to modulo arithmetic (`i=0` and `i=260` both produce `"a0"`). Consider using `fmt.Sprintf("key-%d", i)` instead. ### Suggestions (optional) - Add a concurrent access test for snapshots (similar to PR #32's event concurrency test) Once the thread safety and nil validation are addressed, this will be ready to merge.

HugoNijhuis referenced this pull request

2026-01-09 16:29:28 +00:00

[Issue #3] Add unit tests for EventStore interface implementations #32

HugoNijhuis added 1 commit 2026-01-09 16:34:27 +00:00

Merge main and address review feedback

CI / build (pull_request) Successful in 17s

Details

185c645238

- Add mutex protection to SaveSnapshot and GetLatestSnapshot
- Add nil validation in SaveSnapshot
- Fix key collision in TestSnapshotDataIntegrity_LargeState (use fmt.Sprintf)
- Add concurrent access tests for snapshots

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

HugoNijhuis commented

2026-01-09 16:37:11 +00:00

Re-Review - Approved

All feedback from the previous review has been addressed:

Thread safety: Added mutex protection to SaveSnapshot (Lock) and GetLatestSnapshot (RLock)
Nil validation: SaveSnapshot now returns error for nil snapshot
Test key collision: Fixed to use fmt.Sprintf("key-%d", i)
Concurrent tests: Added TestConcurrentSaveSnapshot and TestConcurrentSaveAndGetSnapshot

LGTM - merging now.

## Re-Review - Approved All feedback from the previous review has been addressed: - **Thread safety**: Added mutex protection to `SaveSnapshot` (Lock) and `GetLatestSnapshot` (RLock) - **Nil validation**: `SaveSnapshot` now returns error for nil snapshot - **Test key collision**: Fixed to use `fmt.Sprintf("key-%d", i)` - **Concurrent tests**: Added `TestConcurrentSaveSnapshot` and `TestConcurrentSaveAndGetSnapshot` LGTM - merging now.

HugoNijhuis merged commit a269da4520 into main

2026-01-09 16:37:24 +00:00

HugoNijhuis referenced this issue from a commit

2026-01-09 16:37:24 +00:00

[Issue #4] Add SnapshotStore unit tests (#31)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: flowmade-one/aether#31