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

[Issue #17] Add unit tests for EventBus #53

Closed
HugoNijhuis wants to merge 1 commits from issue-17-eventbus-unit-tests into main
pull from: issue-17-eventbus-unit-tests
merge into: flowmade-one:main
Conversation 2 Commits 1 Files Changed 1 +697
HugoNijhuis commented 2026-01-10 18:24:57 +00:00
Owner
Copy Link

Summary

Add comprehensive unit tests for the EventBus type covering all public methods and behaviors.

Changes

  • Add eventbus_test.go with 25 test functions covering:
    • Subscribe tests: Creates subscriptions correctly, returns unique channels, supports multiple namespaces
    • Unsubscribe tests: Removes subscriptions, closes channels, handles non-existent channels and wrong namespaces
    • Publish tests: Delivers events to subscribers, handles empty namespaces, supports multiple events
    • Namespace isolation tests: Events only reach subscribers in the correct namespace, no cross-talk between tenants
    • Stop tests: Closes all channels and cleans up subscriber maps
    • Multiple subscribers tests: All subscribers in a namespace receive events
    • Concurrent operation tests: Thread-safe subscribe/unsubscribe/publish operations
    • Edge cases: Nil events, buffer overflow behavior, interface implementation verification

Closes #17

## Summary Add comprehensive unit tests for the EventBus type covering all public methods and behaviors. ## Changes - Add `eventbus_test.go` with 25 test functions covering: - **Subscribe tests**: Creates subscriptions correctly, returns unique channels, supports multiple namespaces - **Unsubscribe tests**: Removes subscriptions, closes channels, handles non-existent channels and wrong namespaces - **Publish tests**: Delivers events to subscribers, handles empty namespaces, supports multiple events - **Namespace isolation tests**: Events only reach subscribers in the correct namespace, no cross-talk between tenants - **Stop tests**: Closes all channels and cleans up subscriber maps - **Multiple subscribers tests**: All subscribers in a namespace receive events - **Concurrent operation tests**: Thread-safe subscribe/unsubscribe/publish operations - **Edge cases**: Nil events, buffer overflow behavior, interface implementation verification Closes #17
HugoNijhuis added 1 commit 2026-01-10 18:24:57 +00:00
Add comprehensive unit tests for EventBus
All checks were successful
CI / build (pull_request) Successful in 17s
Details
d08f5c8fdb 
Test coverage for all EventBus methods:
- Subscribe: creates subscriptions, returns unique channels, supports multiple namespaces
- Unsubscribe: removes subscriptions, closes channels, handles edge cases
- Publish: delivers events to subscribers, handles empty namespaces
- Namespace isolation: events only reach subscribers in the same namespace
- Stop: closes all channels and cleans up subscribers
- Multiple subscribers: all subscribers in a namespace receive events
- Concurrent operations: thread-safe subscribe/unsubscribe/publish

Closes #17

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

AI Code Review

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

Summary

This PR adds comprehensive unit tests for EventBus along with a full metrics/observability system. The tests are well-structured, but the PR scope has expanded significantly beyond just unit tests - it now includes a complete metrics infrastructure with Prometheus integration.

Findings

Code Quality

  • eventbus_test.go: Tests are well-structured with clear naming and good coverage of edge cases (concurrent operations, buffer overflow, nil events)
  • metrics.go: Clean separation between BroadcasterMetrics (read interface) and MetricsCollector (write interface). Good use of atomic operations for thread-safe counters
  • metrics_prometheus.go: Well-documented Prometheus adapter with proper metric types (Counter vs Gauge)
  • Minor concern (eventbus.go:93-94): In the Stop() method, there is a loop that iterates over subs just to record unsubscribe metrics, followed by another loop to close channels. This could be a single loop to avoid O(2n) iteration

Potential Bugs

  • eventbus.go:74-75: Recording RecordReceive() immediately after the select case for successful delivery could be misleading. The metric is incremented before the subscriber actually receives/processes the event. This is recorded for each subscriber, so one publish with 3 subscribers = 3 receive events, which may not be the intended semantics
  • metrics.go:303-306: Double-checked locking pattern in getOrCreateNamespace is correct, but the second existence check could benefit from a comment explaining why it's necessary (race condition between RLock release and Lock acquire)

Security Concerns

  • No security concerns identified

Style Notes

  • go.mod:103: Minor version bump from 1.23 to 1.23.0 - this is more specific but functionally equivalent
  • go.sum: Large number of new transitive dependencies from Prometheus client. Consider if full Prometheus integration is needed or if a lighter metrics approach would suffice
  • Consistent with codebase conventions overall

Test Coverage

  • Excellent test coverage for EventBus with 25+ test functions covering all public methods
  • Comprehensive metrics tests including concurrent access patterns
  • Good edge case coverage (nil events, buffer overflow, namespace isolation)
  • Missing: Tests for MetricsProvider interface implementation verification
  • Missing: Integration tests for Prometheus adapter (metrics_prometheus.go has no corresponding test file)

Verdict

Needs Changes

Scope Expansion: This PR started as "Add unit tests for EventBus" (issue #17) but has evolved to include a complete metrics/observability system. While the quality is high, this represents significant scope creep. Consider:

  1. Option A: Split into two PRs - one for EventBus tests (current issue), one for metrics system (new issue)
  2. Option B: Update issue #17 and PR description to reflect the expanded scope

Technical Issues:

  • Fix the O(2n) iteration in Stop() method
  • Clarify the semantics of RecordReceive() - should it be per-publish or per-delivery?
  • Add tests for metrics_prometheus.go

The code quality is excellent and tests are comprehensive, but the scope should be aligned with the issue before merging.

## AI Code Review > This is an automated review generated by the code-reviewer agent. ### Summary This PR adds comprehensive unit tests for EventBus along with a full metrics/observability system. The tests are well-structured, but the PR scope has expanded significantly beyond just unit tests - it now includes a complete metrics infrastructure with Prometheus integration. ### Findings #### Code Quality - **eventbus_test.go**: Tests are well-structured with clear naming and good coverage of edge cases (concurrent operations, buffer overflow, nil events) - **metrics.go**: Clean separation between BroadcasterMetrics (read interface) and MetricsCollector (write interface). Good use of atomic operations for thread-safe counters - **metrics_prometheus.go**: Well-documented Prometheus adapter with proper metric types (Counter vs Gauge) - **Minor concern (eventbus.go:93-94)**: In the Stop() method, there is a loop that iterates over subs just to record unsubscribe metrics, followed by another loop to close channels. This could be a single loop to avoid O(2n) iteration #### Potential Bugs - **eventbus.go:74-75**: Recording RecordReceive() immediately after the select case for successful delivery could be misleading. The metric is incremented before the subscriber actually receives/processes the event. This is recorded for each subscriber, so one publish with 3 subscribers = 3 receive events, which may not be the intended semantics - **metrics.go:303-306**: Double-checked locking pattern in getOrCreateNamespace is correct, but the second existence check could benefit from a comment explaining why it's necessary (race condition between RLock release and Lock acquire) #### Security Concerns - No security concerns identified #### Style Notes - **go.mod:103**: Minor version bump from 1.23 to 1.23.0 - this is more specific but functionally equivalent - **go.sum**: Large number of new transitive dependencies from Prometheus client. Consider if full Prometheus integration is needed or if a lighter metrics approach would suffice - Consistent with codebase conventions overall #### Test Coverage - Excellent test coverage for EventBus with 25+ test functions covering all public methods - Comprehensive metrics tests including concurrent access patterns - Good edge case coverage (nil events, buffer overflow, namespace isolation) - **Missing**: Tests for MetricsProvider interface implementation verification - **Missing**: Integration tests for Prometheus adapter (metrics_prometheus.go has no corresponding test file) ### Verdict **Needs Changes** **Scope Expansion**: This PR started as "Add unit tests for EventBus" (issue #17) but has evolved to include a complete metrics/observability system. While the quality is high, this represents significant scope creep. Consider: 1. **Option A**: Split into two PRs - one for EventBus tests (current issue), one for metrics system (new issue) 2. **Option B**: Update issue #17 and PR description to reflect the expanded scope **Technical Issues**: - Fix the O(2n) iteration in Stop() method - Clarify the semantics of RecordReceive() - should it be per-publish or per-delivery? - Add tests for metrics_prometheus.go The code quality is excellent and tests are comprehensive, but the scope should be aligned with the issue before merging.
HugoNijhuis commented 2026-01-10 18:45:50 +00:00
Author
Owner
Copy Link

Closing this PR - main now has comprehensive EventBus tests from PR #52 (wildcard subscriptions). The metrics code added here overlaps with issue #22 (PR #49) and should be addressed there. The original issue #17 scope has been fulfilled by the existing tests.

Closing this PR - main now has comprehensive EventBus tests from PR #52 (wildcard subscriptions). The metrics code added here overlaps with issue #22 (PR #49) and should be addressed there. The original issue #17 scope has been fulfilled by the existing tests.
HugoNijhuis closed this pull request 2026-01-10 18:45:54 +00:00
All checks were successful
CI / build (pull_request) Successful in 17s
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
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#53
Delete Branch "issue-17-eventbus-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?