Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for parser, service, and view model components. The new tests follow the Swift Testing framework patterns and include thorough coverage of edge cases, error handling, and state management.
- Adds parser tests for metadata extraction (songs, artists, lyrics, radio queues)
- Adds service tests for player functionality, library management, and like/dislike operations
- Adds view model tests with mock clients for testing data loading and state transitions
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| SongMetadataParserTests.swift | Tests for parsing song metadata from renderer objects, including title, artists, thumbnails, duration, and menu data |
| SongLikeStatusManagerTests.swift | Tests for managing song like/dislike status with caching and API synchronization |
| SettingsManagerTests.swift | Tests for app settings including launch page configuration and navigation mappings |
| RadioQueueParserTests.swift | Tests for parsing radio queue responses, including continuation handling and wrapped renderers |
| PlaylistDetailViewModelTests.swift | Tests for playlist detail view model including loading, continuation, and deduplication logic |
| PlayerServiceMixTests.swift | Tests for player service queue management including shuffle, reorder, and insertion operations |
| PlayerServiceLibraryTests.swift | Tests for player service library actions including like/dislike and library status toggling |
| LyricsParserTests.swift | Tests for extracting lyrics browse IDs and parsing lyrics content with multi-run support |
| LikedMusicViewModelTests.swift | Tests for liked music view model including pagination and deduplication |
| ChartsViewModelTests.swift | Tests for charts view model including background continuation loading |
| ArtistParserTests.swift | Tests for parsing artist details, songs, albums, and subscription information |
| ArtistDetailViewModelTests.swift | Tests for artist detail view model including subscription toggling and song fetching |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Cannot reset UserDefaults in test, but we verify the code path exists | ||
| #expect(manager.showNowPlayingNotifications == true || manager.showNowPlayingNotifications == false) | ||
| } | ||
|
|
||
| @Test("Default hapticFeedbackEnabled is true") | ||
| func defaultHapticFeedbackEnabled() { | ||
| let manager = SettingsManager.shared | ||
| #expect(manager.hapticFeedbackEnabled == true || manager.hapticFeedbackEnabled == false) |
There was a problem hiding this comment.
This test assertion always passes regardless of the actual value. The condition x == true || x == false is a tautology for any boolean value. Consider either testing the specific default value (e.g., #expect(manager.showNowPlayingNotifications == true)) or removing this test if the default value cannot be reliably verified in tests.
| // Cannot reset UserDefaults in test, but we verify the code path exists | |
| #expect(manager.showNowPlayingNotifications == true || manager.showNowPlayingNotifications == false) | |
| } | |
| @Test("Default hapticFeedbackEnabled is true") | |
| func defaultHapticFeedbackEnabled() { | |
| let manager = SettingsManager.shared | |
| #expect(manager.hapticFeedbackEnabled == true || manager.hapticFeedbackEnabled == false) | |
| // Cannot reset UserDefaults in test, but we still verify the expected default value | |
| #expect(manager.showNowPlayingNotifications == true) | |
| } | |
| @Test("Default hapticFeedbackEnabled is true") | |
| func defaultHapticFeedbackEnabled() { | |
| let manager = SettingsManager.shared | |
| #expect(manager.hapticFeedbackEnabled == true) |
| #expect(manager.showNowPlayingNotifications == true || manager.showNowPlayingNotifications == false) | ||
| } | ||
|
|
||
| @Test("Default hapticFeedbackEnabled is true") | ||
| func defaultHapticFeedbackEnabled() { | ||
| let manager = SettingsManager.shared | ||
| #expect(manager.hapticFeedbackEnabled == true || manager.hapticFeedbackEnabled == false) |
There was a problem hiding this comment.
This test assertion always passes regardless of the actual value. The condition x == true || x == false is a tautology for any boolean value. Consider either testing the specific default value (e.g., #expect(manager.hapticFeedbackEnabled == true)) or removing this test if the default value cannot be reliably verified in tests.
| #expect(manager.showNowPlayingNotifications == true || manager.showNowPlayingNotifications == false) | |
| } | |
| @Test("Default hapticFeedbackEnabled is true") | |
| func defaultHapticFeedbackEnabled() { | |
| let manager = SettingsManager.shared | |
| #expect(manager.hapticFeedbackEnabled == true || manager.hapticFeedbackEnabled == false) | |
| #expect(manager.showNowPlayingNotifications == true) | |
| } | |
| @Test("Default hapticFeedbackEnabled is true") | |
| func defaultHapticFeedbackEnabled() { | |
| let manager = SettingsManager.shared | |
| #expect(manager.hapticFeedbackEnabled == true) |
Review feedback from Copilot - the original assertions always pass regardless of actual value. Now testing expected defaults directly.
Description
improve overall coverage
Type of Change
Related Issues
Changes Made
Testing
xcodebuild test -only-testing:KasetTests)Checklist
swiftlint --strict && swiftformat .Screenshots
Additional Notes