Add unit tests and refactor music service classes
This commit is contained in:
anthorob committed
1 parent
b8786c8dc9
commit
1b981abdf7
13 files changed
+1085
-133
No files matched your search
@@ -0,0 +1,95 @@
|
||||
---
|
||||
sessionId: session-260905-173932-36te
|
||||
---
|
||||
|
||||
# Requirements
|
||||
|
||||
### Overview & Goals
|
||||
Improve the `MusicManager` class for better maintainability, testability, and separation of concerns.
|
||||
|
||||
### Scope
|
||||
**In Scope:**
|
||||
- Refactor `MusicManager` to reduce responsibilities
|
||||
- Extract vote-skip logic into a dedicated service
|
||||
- Fix synchronization strategy
|
||||
- Improve error handling
|
||||
- Eliminate magic numbers
|
||||
|
||||
**Out of Scope:**
|
||||
- Changing `QueueManager` or `TrackScheduler` internals (beyond reducing circular refs)
|
||||
- Changing external behavior or command interfaces
|
||||
|
||||
# Technical Design
|
||||
|
||||
### Current Issues
|
||||
|
||||
1. **Vote-skip business logic in MusicManager** (lines 113–145): The `voteSkip()` method contains complex voting logic (threshold calculation, member counting, skip decisions) that doesn't belong in a player manager. Magic number `2.1` is used as a vote divisor with no explanation.
|
||||
|
||||
2. **Circular dependencies**: `MusicManager` → `TrackScheduler` → `QueueManager` → `TrackScheduler` → `MusicManager`. This makes testing and reasoning about the code difficult.
|
||||
|
||||
3. **Overly broad `synchronized` methods**: Nearly every public method is `synchronized` on `this`, even simple getters like `getCurrentlyPlaying()` and `isCurrentlyPlaying()`. This creates unnecessary contention. Methods like `getQueueSize()` aren't synchronized at all, creating inconsistency.
|
||||
|
||||
4. **Poor error handling in `connect()`** (line 80): Throws a bare `RuntimeException` when `audioChannel` is null instead of using `IllegalArgumentException` or returning a `CommandExecutionResult`.
|
||||
|
||||
5. **`baseVolume` parsed from String without error handling** (line 44): `Integer.parseInt()` can throw `NumberFormatException` with no fallback.
|
||||
|
||||
### Proposed Changes
|
||||
|
||||
#### 1. Extract `VoteSkipService`
|
||||
- New class: `me.bobert.netneutrality.music.VoteSkipService`
|
||||
- Moves all vote-skip logic out of `MusicManager`
|
||||
- Replace magic `2.1` with a named constant `VOTE_THRESHOLD_DIVISOR` or configurable guild setting
|
||||
- `MusicManager.voteSkip()` delegates to the new service
|
||||
|
||||
#### 2. Reduce circular dependencies
|
||||
- `TrackScheduler` and `QueueManager` should not need to call back into `MusicManager` for `disconnect()`. Instead, use a callback/listener interface (e.g., `MusicManagerCallback`) that `TrackScheduler` holds, breaking the direct circular reference.
|
||||
|
||||
#### 3. Fix synchronization
|
||||
- Remove `synchronized` from simple delegating methods (`getCurrentlyPlaying()`, `isCurrentlyPlaying()`, `resume()`, `stopTrack()`) — `AudioPlayer` is already thread-safe.
|
||||
- Keep `synchronized` only on methods that coordinate multiple state changes (`connect()`, `disconnect()`, `play()`).
|
||||
- Make `getQueueSize()` consistent with the chosen strategy.
|
||||
|
||||
#### 4. Improve error handling
|
||||
- `connect()`: Replace `RuntimeException` with `IllegalArgumentException("audioChannel must not be null")`
|
||||
- `baseVolume`: Add try-catch with a sensible default (e.g., 50) and a warning log.
|
||||
|
||||
#### 5. Extract constants
|
||||
- `VOTE_THRESHOLD_DIVISOR = 2.1` (or make configurable)
|
||||
- `MIN_MEMBERS_FOR_VOTE = 3`
|
||||
- `DEFAULT_VOLUME = 50`
|
||||
|
||||
### File Structure
|
||||
| File | Action |
|
||||
|------|--------|
|
||||
| `music/MusicManager.java` | Modify — remove vote logic, fix sync, fix error handling |
|
||||
| `music/VoteSkipService.java` | **New** — extracted vote-skip logic |
|
||||
| `music/MusicManagerCallback.java` | **New** — interface to break circular dependency |
|
||||
| `music/TrackScheduler.java` | Modify — use callback interface instead of direct `MusicManager` reference where possible |
|
||||
| `music/QueueManager.java` | Modify — remove `getMusicManager()` call chain for disconnect |
|
||||
|
||||
# Delivery Steps
|
||||
|
||||
### ✓ Step 1: Extract VoteSkipService and constants
|
||||
Vote-skip logic is isolated in a new service class and magic numbers are replaced with named constants.
|
||||
|
||||
- Create `VoteSkipService.java` in `me.bobert.netneutrality.music`
|
||||
- Move the vote-skip logic from `MusicManager.voteSkip()` into `VoteSkipService.voteSkip(CommandContext, QueueManager, AudioChannel)`
|
||||
- Define constants: `VOTE_THRESHOLD_DIVISOR`, `MIN_MEMBERS_FOR_VOTE`
|
||||
- Update `MusicManager.voteSkip()` to delegate to the new service
|
||||
- Add `DEFAULT_VOLUME` constant and wrap `baseVolume` parsing in try-catch with fallback
|
||||
|
||||
### ✓ Step 2: Fix synchronization and error handling
|
||||
Synchronization is applied only where needed and error handling is improved.
|
||||
|
||||
- Remove `synchronized` from simple delegating methods: `getCurrentlyPlaying()`, `isCurrentlyPlaying()`, `resume()`, `stopTrack()`, `setVolume()`
|
||||
- Keep `synchronized` on `connect()`, `disconnect()`, `play()`, `isNotConnectedTo()`
|
||||
- Replace `RuntimeException` in `connect()` with `IllegalArgumentException`
|
||||
- Ensure `getQueueSize()` follows the same synchronization strategy as other methods
|
||||
|
||||
### ✓ Step 3: Reduce circular dependencies between MusicManager, TrackScheduler, and QueueManager
|
||||
The circular reference chain is broken using a callback interface.
|
||||
|
||||
- Create `MusicManagerCallback.java` interface with method `void onQueueEmpty()` (or similar) to replace direct `MusicManager.disconnect()` calls from `QueueManager`
|
||||
- Have `MusicManager` implement the callback interface
|
||||
- Update `TrackScheduler` constructor to accept the callback interface instead of the full `MusicManager` where only disconnect/connection behavior is needed
|
||||
- Update `QueueManager.clearQueue()` to use the callback instead of `scheduler.getMusicManager().disconnect()`
|
||||
@@ -0,0 +1,132 @@
|
||||
---
|
||||
sessionId: session-260906-073726-sbxb
|
||||
---
|
||||
|
||||
# Requirements
|
||||
|
||||
### Overview & Goals
|
||||
Improve the code quality, correctness, and maintainability of `QueueManager`, `TrackScheduler`, `TrackInfo`, and `VoteSkipService` — the core music queue and playback scheduling subsystem.
|
||||
|
||||
### Scope
|
||||
**In Scope:**
|
||||
- Fix thread-safety issues (check-then-act race conditions in `QueueManager`)
|
||||
- Simplify confusing queue insertion logic in `QueueManager.queue()`
|
||||
- Separate concerns: extract command-result formatting from queue data operations
|
||||
- Fix `TrackInfo` using wrong logger (`QueueManager.class` instead of `TrackInfo.class`)
|
||||
- Clean up `TrackScheduler.onTrackStuck()` complexity
|
||||
- Add unit tests for `QueueManager`, `TrackScheduler`, `TrackInfo`, and `VoteSkipService`
|
||||
- Expose skip-vote state through `TrackInfo` more cleanly (encapsulate the `Set<String>` skips)
|
||||
|
||||
**Out of Scope:**
|
||||
- Changes to `MusicManager`, `MusicServiceImpl`, or audio source/loading classes
|
||||
- New user-facing features or commands
|
||||
- Changes to `MusicManagerCallback` interface
|
||||
|
||||
# Technical Design
|
||||
|
||||
### Current Implementation
|
||||
|
||||
**`QueueManager`** (`src/main/java/me/bobert/netneutrality/music/QueueManager.java`, 286 lines):
|
||||
- Manages a `LinkedBlockingDeque<TrackInfo>` with manual `synchronized` blocks
|
||||
- Multiple methods check state outside the synchronized block, then re-check inside (e.g., `clearQueue()` L81 vs L86, `skip()` L197 vs L203, `shuffle()` L229 vs L233, `removeFromQueue()` L254 vs L259) — the outer checks are racy and misleading
|
||||
- `queue()` method (L135-158) has confusing branching: `addFirst` + `add` can result in the same track being added twice when `player.getPlayingTrack() == null && !queue.isEmpty()`
|
||||
- `clearQueue()` calls `callback.onQueueEmpty()` before checking if queue is empty (L79)
|
||||
- `removeTracksFromUser()` iterates the queue outside synchronized (L97), then again inside (L102)
|
||||
- `skip(int)` silently resets `nbTrack` to 1 if it exceeds queue size (L209) — surprising behavior
|
||||
- Mixes queue data operations with `CommandExecutionResult` formatting
|
||||
|
||||
**`TrackScheduler`** (`src/main/java/me/bobert/netneutrality/music/TrackScheduler.java`, 175 lines):
|
||||
- `onTrackStuck()` (L129-173) has deeply nested logic with inline async recovery — hard to follow and test
|
||||
- Circular dependency: creates `QueueManager` in constructor, which takes `this` back
|
||||
- `context` field is accessed from multiple threads but only `attachContext`/`detachContext` are synchronized — `getContext()`, `getDj()` are not
|
||||
|
||||
**`TrackInfo`** (`src/main/java/me/bobert/netneutrality/music/TrackInfo.java`, 83 lines):
|
||||
- Logger incorrectly uses `QueueManager.class` (L26) instead of `TrackInfo.class`
|
||||
- `getSkips()` returns mutable `Set<String>` — breaks encapsulation
|
||||
- Duplicate copyright block (L1-5 and L7-11)
|
||||
|
||||
**`VoteSkipService`** (`src/main/java/me/bobert/netneutrality/music/VoteSkipService.java`, 57 lines):
|
||||
- Relatively clean, but directly calls `queueManager.skip()` mixing vote logic with queue mutation
|
||||
- `info.addSkip()` uses `context.getGuildUser()` but `info.getSkips().size()` check uses raw set — no race protection
|
||||
|
||||
### Proposed Changes
|
||||
|
||||
#### QueueManager
|
||||
1. **Remove all outer unsynchronized checks** — move all logic inside a single `synchronized` block per method to eliminate check-then-act races
|
||||
2. **Rewrite `queue()` method** — simplify to: add to queue, if nothing playing start the head track. Remove the confusing `addFirst`/`add` dual-path
|
||||
3. **Fix `clearQueue()`** — move `callback.onQueueEmpty()` call to after the queue is confirmed non-empty and cleared
|
||||
4. **Fix `skip(int)`** — when `nbTrack > queue.size()`, skip all remaining tracks instead of silently resetting to 1
|
||||
5. **Fix `removeTracksFromUser()`** — do all filtering inside synchronized block
|
||||
6. **Return queue operation results as data** (e.g., number removed, track name) and let callers build `CommandExecutionResult` — or at minimum, document that this class returns command results
|
||||
|
||||
#### TrackScheduler
|
||||
1. **Extract stuck-track recovery** into a dedicated `StuckTrackRecoveryHandler` class to reduce `onTrackStuck()` complexity
|
||||
2. **Make `context` access thread-safe** — synchronize `getContext()` and `getDj()`, or use `volatile`
|
||||
3. **Add `lastStuckTrack` reset** on track start or successful recovery to prevent stale state
|
||||
|
||||
#### TrackInfo
|
||||
1. **Fix logger** — change to `TrackInfo.class`
|
||||
2. **Remove duplicate copyright block**
|
||||
3. **Encapsulate skips** — return `Collections.unmodifiableSet(skips)` from `getSkips()`, add `getSkipCount()` method
|
||||
|
||||
#### VoteSkipService
|
||||
1. **Synchronize vote-skip check** — ensure `addSkip` + threshold check is atomic (or accept eventual consistency and document it)
|
||||
|
||||
### File Structure
|
||||
All changes in `src/main/java/me/bobert/netneutrality/music/`:
|
||||
- **Modified:** `QueueManager.java`, `TrackScheduler.java`, `TrackInfo.java`, `VoteSkipService.java`
|
||||
- **New:** `StuckTrackRecoveryHandler.java`
|
||||
- **New tests:** `src/test/java/me/bobert/netneutrality/feature/music/QueueManagerTest.java`, `TrackSchedulerTest.java`, `TrackInfoTest.java`, `VoteSkipServiceTest.java`
|
||||
|
||||
# Testing
|
||||
|
||||
### Validation Approach
|
||||
Add unit tests for all four classes using JUnit 5 and Mockito (already in project dependencies via the existing test classes).
|
||||
|
||||
### Key Scenarios
|
||||
- **QueueManager:** queue/dequeue ordering, skip single/multiple, shuffle preserves current track, removeFromQueue bounds, clearQueue idempotency, removeTracksFromUser correctness
|
||||
- **TrackInfo:** skip vote tracking, duplicate skip prevention, unmodifiable skips set, equals/hashCode
|
||||
- **VoteSkipService:** threshold calculation, skip when below min members, vote counting, duplicate vote handling
|
||||
- **TrackScheduler:** onTrackEnd triggers correct queue action per end reason, onTrackStuck recovery flow
|
||||
|
||||
### Edge Cases
|
||||
- Queue with single track: skip, shuffle, remove
|
||||
- Skip count exceeding queue size
|
||||
- Empty queue operations
|
||||
- Concurrent vote-skip from same user
|
||||
|
||||
# Delivery Steps
|
||||
|
||||
### ✓ Step 1: Fix TrackInfo issues
|
||||
TrackInfo has correct logger, clean copyright, and encapsulated skip state.
|
||||
|
||||
- Fix logger from `QueueManager.class` to `TrackInfo.class` (line 26)
|
||||
- Remove duplicate copyright block (lines 7-11)
|
||||
- Change `getSkips()` to return `Collections.unmodifiableSet(skips)`
|
||||
- Add `getSkipCount()` convenience method
|
||||
- Add `TrackInfoTest.java` covering skip tracking, duplicate prevention, equals/hashCode, and unmodifiable set enforcement
|
||||
|
||||
### ✓ Step 2: Fix QueueManager thread-safety and queue logic
|
||||
QueueManager has correct synchronization and simplified queue insertion.
|
||||
|
||||
- Remove all outer unsynchronized pre-checks in `clearQueue()`, `skip()`, `shuffle()`, `removeFromQueue()`, `removeTracksFromUser()` — consolidate into single synchronized blocks
|
||||
- Rewrite `queue(AudioTrack, Member)` to eliminate the confusing `addFirst`/`add` dual-path: simply add to tail, then start playback if nothing is playing
|
||||
- Fix `clearQueue()` to call `callback.onQueueEmpty()` only after confirming queue was non-empty
|
||||
- Fix `skip(int)` to skip all remaining when `nbTrack > queue.size()` instead of silently resetting to 1
|
||||
- Fix `removeTracksFromUser()` to do all filtering inside synchronized block
|
||||
- Add `QueueManagerTest.java` covering queue ordering, skip, shuffle, remove, clear, and edge cases
|
||||
|
||||
### ✓ Step 3: Extract stuck-track recovery and fix TrackScheduler thread-safety
|
||||
TrackScheduler is simplified with extracted recovery logic and thread-safe context access.
|
||||
|
||||
- Create `StuckTrackRecoveryHandler.java` encapsulating the retry-or-skip logic currently in `onTrackStuck()` (lines 129-173)
|
||||
- Simplify `onTrackStuck()` to delegate to the new handler
|
||||
- Reset `lastStuckTrack` in `onTrackStart()` to prevent stale state
|
||||
- Synchronize `getContext()` and `getDj()` methods (or make `context` volatile)
|
||||
- Add `TrackSchedulerTest.java` covering onTrackEnd behavior per end reason and stuck-track recovery delegation
|
||||
|
||||
### ✓ Step 4: Improve VoteSkipService robustness and add tests
|
||||
VoteSkipService has documented concurrency behavior and full test coverage.
|
||||
|
||||
- Add documentation comment about the vote-then-check atomicity assumption
|
||||
- Add `VoteSkipServiceTest.java` covering: threshold math, auto-skip below min members, vote accumulation, duplicate vote from same user, skip trigger when threshold exceeded
|
||||
Reference in new issue
Block a user