flowmade-one/aether
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[Issue #5] Add ShardManager unit tests #55

Merged
HugoNijhuis merged 1 commits from issue-5-shardmanager-unit-tests into main 2026-01-10 22:52:35 +00:00
Conversation 2 Commits 1 Files Changed 1 +713
HugoNijhuis commented 2026-01-10 22:47:29 +00:00
Owner
Copy Link

Summary

  • Add comprehensive unit tests for ShardManager shard management functionality
  • Tests cover GetShard, GetShardNodes, AssignShard, PlaceActor methods
  • Tests verify shard assignment behavior with node failures
  • Tests verify replication factor is properly tracked
  • Includes mock implementations for VMRegistry testing
  • Includes benchmark tests for performance-critical operations

Closes #5

Test Plan

  • All new tests pass (go test ./cluster/...)
  • Full test suite passes (go test ./...)
  • Tests cover all acceptance criteria from issue

Generated with Claude Code

## Summary - Add comprehensive unit tests for ShardManager shard management functionality - Tests cover GetShard, GetShardNodes, AssignShard, PlaceActor methods - Tests verify shard assignment behavior with node failures - Tests verify replication factor is properly tracked - Includes mock implementations for VMRegistry testing - Includes benchmark tests for performance-critical operations Closes #5 ## Test Plan - [x] All new tests pass (`go test ./cluster/...`) - [x] Full test suite passes (`go test ./...`) - [x] Tests cover all acceptance criteria from issue Generated with [Claude Code](https://claude.com/claude-code)
HugoNijhuis added 1 commit 2026-01-10 22:47:31 +00:00
Add ShardManager unit tests
All checks were successful
CI / build (pull_request) Successful in 19s
Details
53576d6f04 
Comprehensive unit tests for shard management functionality:
- GetShard returns correct shard for actor IDs consistently
- GetShardNodes returns nodes responsible for each shard
- AssignShard correctly updates shard assignments
- PlaceActor returns valid nodes from available set
- Shard assignment handles node failures gracefully
- Replication factor is properly tracked

Includes tests for edge cases (empty shards, nil registry, single node)
and benchmark tests for GetShard, AssignShard, and PlaceActor.

Closes #5

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
HugoNijhuis commented 2026-01-10 22:50:52 +00:00
Author
Owner
Copy Link

AI Code Review

This is an automated review generated by the code-reviewer agent.

Summary

This PR has a critical mismatch between its title/description and actual content. The PR claims to add ShardManager unit tests for Issue #5, but it actually contains JetStream integration tests (which appears to belong to Issue #10 based on the branch name issue-10-jetstream-integration-tests).

Findings

Blocking Issues

Wrong Issue Reference

  • PR title: "[Issue #5] Add ShardManager unit tests"
  • PR content: JetStream integration tests only
  • Issue #5 acceptance criteria: All about ShardManager (GetShard, GetShardNodes, AssignShard, PlaceActor)
  • Actual changes: CI configuration + store/jetstream_integration_test.go

The PR does not contain ANY ShardManager tests. All acceptance criteria for Issue #5 are unmet:

  • Test GetShard returns correct shard for actor
  • Test GetShardNodes returns nodes responsible for shard
  • Test AssignShard correctly updates assignments
  • Test PlaceActor uses hash ring correctly
  • Test shard assignment with node failures
  • Test replication factor is respected

Branch Mismatch
The branch name is issue-10-jetstream-integration-tests which suggests this work belongs to a different issue entirely.

Code Quality (for the JetStream tests that ARE present)

The JetStream integration tests are well-structured:

  • Comprehensive test coverage with 1400+ lines
  • Good use of test isolation with unique stream names
  • Proper cleanup with deferred stream deletion
  • Tests cover stream creation, events, snapshots, concurrency, namespace isolation
  • Includes benchmarks

Test Coverage (for JetStream tests)

The JetStream integration tests show excellent coverage:

  • Stream lifecycle and configuration
  • Event persistence and retrieval
  • Version conflict detection
  • Snapshot management
  • Namespace isolation
  • Concurrent operations
  • Connection persistence

However, these tests are for the wrong issue.

Verdict

Blocking Issues

This PR cannot be merged because:

  1. It references the wrong issue (#5 instead of #10)
  2. It contains no ShardManager tests despite claiming to implement Issue #5
  3. The branch name indicates this is for issue-10, not issue-5

Required Actions:

  1. Close this PR
  2. Create a new PR referencing the correct issue (likely #10 for JetStream integration tests)
  3. Create a separate PR for Issue #5 with actual ShardManager unit tests
## AI Code Review > This is an automated review generated by the code-reviewer agent. ### Summary This PR has a critical mismatch between its title/description and actual content. The PR claims to add ShardManager unit tests for Issue #5, but it actually contains JetStream integration tests (which appears to belong to Issue #10 based on the branch name issue-10-jetstream-integration-tests). ### Findings #### Blocking Issues **Wrong Issue Reference** - PR title: "[Issue #5] Add ShardManager unit tests" - PR content: JetStream integration tests only - Issue #5 acceptance criteria: All about ShardManager (GetShard, GetShardNodes, AssignShard, PlaceActor) - Actual changes: CI configuration + store/jetstream_integration_test.go The PR does not contain ANY ShardManager tests. All acceptance criteria for Issue #5 are unmet: - [ ] Test GetShard returns correct shard for actor - [ ] Test GetShardNodes returns nodes responsible for shard - [ ] Test AssignShard correctly updates assignments - [ ] Test PlaceActor uses hash ring correctly - [ ] Test shard assignment with node failures - [ ] Test replication factor is respected **Branch Mismatch** The branch name is issue-10-jetstream-integration-tests which suggests this work belongs to a different issue entirely. #### Code Quality (for the JetStream tests that ARE present) The JetStream integration tests are well-structured: - Comprehensive test coverage with 1400+ lines - Good use of test isolation with unique stream names - Proper cleanup with deferred stream deletion - Tests cover stream creation, events, snapshots, concurrency, namespace isolation - Includes benchmarks #### Test Coverage (for JetStream tests) The JetStream integration tests show excellent coverage: - Stream lifecycle and configuration - Event persistence and retrieval - Version conflict detection - Snapshot management - Namespace isolation - Concurrent operations - Connection persistence However, these tests are for the wrong issue. ### Verdict **Blocking Issues** This PR cannot be merged because: 1. It references the wrong issue (#5 instead of #10) 2. It contains no ShardManager tests despite claiming to implement Issue #5 3. The branch name indicates this is for issue-10, not issue-5 **Required Actions:** 1. Close this PR 2. Create a new PR referencing the correct issue (likely #10 for JetStream integration tests) 3. Create a separate PR for Issue #5 with actual ShardManager unit tests
HugoNijhuis commented 2026-01-10 22:52:31 +00:00
Author
Owner
Copy Link

Correction to Previous Review

The previous AI code review was incorrect due to a reviewer agent error (it was on the wrong git branch during analysis).

Actual PR Content:

  • Branch: issue-5-shardmanager-unit-tests ✓
  • Changes: cluster/shard_test.go with 713 lines of ShardManager tests
  • CI: Passing ✓

Acceptance Criteria Met:

  • Test GetShard returns correct shard for actor
  • Test GetShardNodes returns nodes responsible for shard
  • Test AssignShard correctly updates assignments
  • Test PlaceActor uses hash ring correctly
  • Test shard assignment with node failures
  • Test replication factor is respected

Verdict: LGTM - Ready to merge.

## Correction to Previous Review The previous AI code review was **incorrect** due to a reviewer agent error (it was on the wrong git branch during analysis). **Actual PR Content:** - Branch: `issue-5-shardmanager-unit-tests` ✓ - Changes: `cluster/shard_test.go` with 713 lines of ShardManager tests - CI: Passing ✓ **Acceptance Criteria Met:** - [x] Test GetShard returns correct shard for actor - [x] Test GetShardNodes returns nodes responsible for shard - [x] Test AssignShard correctly updates assignments - [x] Test PlaceActor uses hash ring correctly - [x] Test shard assignment with node failures - [x] Test replication factor is respected **Verdict: LGTM** - Ready to merge.
HugoNijhuis merged commit e66fa40b3a into main 2026-01-10 22:52:35 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
risk/high
risk/low
risk/medium
value/critical
value/high
value/low
value/medium
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
HugoNijhuis fluxcd-bot renovate-bot
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: flowmade-one/aether#55
Delete Branch "issue-5-shardmanager-unit-tests"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?