test: improve coverage for services, utils, and components (issue #69) - #80
Merged
CodeWithMa merged 8 commits intoAug 4, 2026
Merged
Conversation
|
@CodeWithMaBot is attempting to deploy a commit to the Ma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change expands tests for Angular components, IndexedDB-backed services, import/export workflows, watch-list state, and form utilities. Tests cover normal behavior, validation, error handling, persistence, and computed state. ChangesCoverage Test Expansion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/app/services/import-export.service.spec.ts (1)
35-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the serialized Blob payload in both download tests.
The tests verify filenames and DOM calls, but they can pass when
downloadJsonserializes the wrong object. Inspect theBlobpassed tocreateObjectURL, then assert its JSON content. Also assert thatexportDatacallsstorageService.getData.
src/app/services/import-export.service.spec.ts#L35-L46: assertstorageService.getDatawas called and validate the exported Blob content.src/app/services/import-export.service.spec.ts#L62-L71: retaincreateObjectURLfrommockDownload()and validate the recovery-backup Blob content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/services/import-export.service.spec.ts` around lines 35 - 46, The export tests in src/app/services/import-export.service.spec.ts at lines 35-46 (anchor) and 62-71 (sibling) verify DOM interactions and filenames but do not validate the actual Blob content passed to createObjectURL. At the anchor site, add an assertion that storageService.getData was called, then extract the Blob argument from the createObjectURL call and assert that its JSON content matches the expected export structure (the data returned by storageService.getData). At the sibling site, retain the createObjectURL reference from mockDownload() and similarly validate the Blob content to ensure the recovery-backup is serialized correctly. This prevents the tests from passing when downloadJson serializes the wrong object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/components/season-editor/season-editor.component.spec.ts`:
- Around line 44-55: Update the “handles non-contiguous season numbers” test
fixture to include seasons 1 and 3, then add one season and assert the new
season number is 4, ensuring the test distinguishes maximum-plus-one behavior
from length-plus-one behavior.
In `@src/app/components/settings/settings.component.spec.ts`:
- Around line 223-234: Both test cases are testing clearing/cleanup operations
but start with already-cleared state, so they pass without proving the cleanup
path executed. At
src/app/components/settings/settings.component.spec.ts#L223-L234 (anchor),
initialize target.value to a non-empty string before calling onFileSelected with
the empty-value cancellation target, then assert that the component clears it to
an empty string. At
src/app/components/settings/settings.component.spec.ts#L274-L283 (sibling), seed
the recoveryBackups signal with a valid backup entry and ensure
getRecoveryBackups() runs to populate it, then verify that the failure path
clears the signal instead of testing the already-empty state.
In `@src/app/services/group.service.spec.ts`:
- Around line 84-102: The deleteGroup test should verify that unrelated groups
and their items are preserved. Extend the test data with a second non-target
group, assign untouched to that group, and assert after
service.deleteGroup('films') that the second group still exists and
untouched.groupId remains its original group ID, while retaining the existing
assertions for the deleted group and moved items.
In `@src/app/services/watch-list.service.spec.ts`:
- Around line 75-81: Update the missing status-transition tests in
src/app/services/watch-list.service.spec.ts lines 237-243 to use a non-empty
fixture and assert storageService.getData() remains unchanged after all three
calls. The existing deleteItem missing-item test at
src/app/services/watch-list.service.spec.ts lines 75-81 already performs the
required unchanged-state assertion and needs no direct change.
In `@src/app/utils/form.utils.spec.ts`:
- Around line 19-23: Rename the affected test descriptions around
toPositiveNumber from “positive integers” to “positive numbers” so they
accurately reflect that decimal values such as 3.5 are accepted.
---
Nitpick comments:
In `@src/app/services/import-export.service.spec.ts`:
- Around line 35-46: The export tests in
src/app/services/import-export.service.spec.ts at lines 35-46 (anchor) and 62-71
(sibling) verify DOM interactions and filenames but do not validate the actual
Blob content passed to createObjectURL. At the anchor site, add an assertion
that storageService.getData was called, then extract the Blob argument from the
createObjectURL call and assert that its JSON content matches the expected
export structure (the data returned by storageService.getData). At the sibling
site, retain the createObjectURL reference from mockDownload() and similarly
validate the Blob content to ensure the recovery-backup is serialized correctly.
This prevents the tests from passing when downloadJson serializes the wrong
object.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91947da7-61c8-4a2e-9e01-047afaa9c6c5
📒 Files selected for processing (6)
src/app/components/season-editor/season-editor.component.spec.tssrc/app/components/settings/settings.component.spec.tssrc/app/services/group.service.spec.tssrc/app/services/import-export.service.spec.tssrc/app/services/watch-list.service.spec.tssrc/app/utils/form.utils.spec.ts
- Fix non-contiguous season test to use truly non-contiguous data (1, 3) - Verify unrelated groups preserved in deleteGroup test - Add existing item and assertion in status-transition no-op test - Initialize target.value with non-empty string in import cancellation test - Rename 'positive integers' to 'positive numbers' in form utils tests - Assert Blob content in import-export download tests
- watch-list.service.spec.ts: schemaVersion 3 -> CURRENT_SCHEMA_VERSION - group.service.spec.ts: schemaVersion 4 -> CURRENT_SCHEMA_VERSION - round-robin.service.spec.ts: schemaVersion 4 -> CURRENT_SCHEMA_VERSION
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/components/settings/settings.component.spec.ts (1)
274-285: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSeed the recovery-backup signal before testing failure cleanup.
The test still asserts an empty signal after
getRecoveryBackups()fails, but it does not prove that stale backups were cleared. Seed a valid backup before the failure, then assert that the signal becomes empty. The newerrorMessage()assertion checks a separate behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/components/settings/settings.component.spec.ts` around lines 274 - 285, To prove that stale backups are cleared when loading fails, first seed the recoveryBackups signal with valid backup data before creating the fixture and triggering the getRecoveryBackups failure. After the fixture is created and stable, assert that recoveryBackups() becomes empty, demonstrating the clearing behavior. Keep the existing errorMessage() assertion unchanged to verify that separate concern.
🧹 Nitpick comments (2)
src/app/services/import-export.service.spec.ts (2)
80-80: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
CURRENT_SCHEMA_VERSIONfor the recovery fixture.Line 80 hardcodes
schemaVersion: 1. If the schema changes, this test still passes becauseImportExportService.exportRecoveryBackuponly serializes the value returned byStorageService. UseCURRENT_SCHEMA_VERSION, or define this as an explicit legacy-backup fixture.Suggested fix
- const backupData = { schemaVersion: 1, data: 'test' }; + const backupData = { schemaVersion: CURRENT_SCHEMA_VERSION, data: 'test' };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/services/import-export.service.spec.ts` at line 80, Replace the hardcoded schemaVersion value of 1 in the backupData fixture with the CURRENT_SCHEMA_VERSION constant. This ensures that the test fixture remains synchronized with the actual schema version used in the ImportExportService.exportRecoveryBackup flow, so the test will properly fail if the schema version changes in the codebase.
15-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore the
Blob.prototype.textoverride after the suite.When the fallback branch runs, it assigns a method to a global prototype.
vi.restoreAllMocks()does not undo this assignment. Later tests can observe the test-onlyFileReaderimplementation. Save the original value and restore it, or delete the added property, inafterAll.Suggested cleanup
+let originalBlobText: Blob['text'] | undefined; + beforeAll(() => { + originalBlobText = Blob.prototype.text; if (!Blob.prototype.text) { Blob.prototype.text = function () { return new Promise<string>((resolve) => { @@ }); }); + +afterAll(() => { + if (originalBlobText) { + Blob.prototype.text = originalBlobText; + } else { + Reflect.deleteProperty(Blob.prototype, 'text'); + } +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/services/import-export.service.spec.ts` around lines 15 - 25, The beforeAll hook modifies the global Blob.prototype.text without cleanup, causing test pollution for subsequent suites. In the beforeAll hook, save the original value of Blob.prototype.text before the conditional assignment, then add an afterAll hook that restores the saved original value or deletes the property if it did not exist before the suite began.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/app/components/settings/settings.component.spec.ts`:
- Around line 274-285: To prove that stale backups are cleared when loading
fails, first seed the recoveryBackups signal with valid backup data before
creating the fixture and triggering the getRecoveryBackups failure. After the
fixture is created and stable, assert that recoveryBackups() becomes empty,
demonstrating the clearing behavior. Keep the existing errorMessage() assertion
unchanged to verify that separate concern.
---
Nitpick comments:
In `@src/app/services/import-export.service.spec.ts`:
- Line 80: Replace the hardcoded schemaVersion value of 1 in the backupData
fixture with the CURRENT_SCHEMA_VERSION constant. This ensures that the test
fixture remains synchronized with the actual schema version used in the
ImportExportService.exportRecoveryBackup flow, so the test will properly fail if
the schema version changes in the codebase.
- Around line 15-25: The beforeAll hook modifies the global Blob.prototype.text
without cleanup, causing test pollution for subsequent suites. In the beforeAll
hook, save the original value of Blob.prototype.text before the conditional
assignment, then add an afterAll hook that restores the saved original value or
deletes the property if it did not exist before the suite began.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f21f021-735a-49fe-a0ab-7939b314f9bb
📒 Files selected for processing (7)
src/app/components/season-editor/season-editor.component.spec.tssrc/app/components/settings/settings.component.spec.tssrc/app/services/group.service.spec.tssrc/app/services/import-export.service.spec.tssrc/app/services/round-robin.service.spec.tssrc/app/services/watch-list.service.spec.tssrc/app/utils/form.utils.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/utils/form.utils.spec.ts
- src/app/services/group.service.spec.ts
- src/app/services/watch-list.service.spec.ts
- src/app/components/season-editor/season-editor.component.spec.ts
- Seed explicit groups in createGroup ordering test to match test name - Replace global Blob.prototype.text polyfill with local readBlobText helper - Assert saveData not called in no-op tests instead of comparing snapshots
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #69
Summary by CodeRabbit