test: cover highlight repository failures#127
Open
davidfedor wants to merge 1 commit into
Open
Conversation
Contributor
✅ Commit Lint: passedAll commit messages in this PR conform to Conventional Commits. 📦 Release preview: no releaseThese commits would not trigger a new release on semantic-release dry-run output |
Contributor
Code Coverage ReportCoverage after merging codex/move-preview-member-to-extension into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
davidfedor
marked this pull request as ready for review
May 18, 2026 19:13
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+481
to
+532
| func testProcessQueueRequeuesFailedRemoveOperation() async { | ||
| let mockAPI = setUp() | ||
| let repository = BibleHighlightsRepository(api: mockAPI) | ||
| mockAPI.mockDeleteHighlightResult = false | ||
| let operation = operation(color: nil, operationType: .remove) | ||
|
|
||
| repository.queueOperation(operation) | ||
| await repository.processQueue() | ||
|
|
||
| let result = repository.getOperationResult(for: operation.id) | ||
| #expect(mockAPI.deleteHighlightCallCount >= 1) | ||
| #expect(repository.pendingOperationCount == 1) | ||
| #expect(repository.failedOperationCount == 1) | ||
| #expect(result?.success == false) | ||
| #expect(result?.error != nil) | ||
| } | ||
|
|
||
| @Test | ||
| func testProcessQueueRequeuesFailedUpdateOperation() async { | ||
| let mockAPI = setUp() | ||
| let repository = BibleHighlightsRepository(api: mockAPI) | ||
| mockAPI.mockUpdateHighlightResult = false | ||
| let operation = operation(operationType: .update) | ||
|
|
||
| repository.queueOperation(operation) | ||
| await repository.processQueue() | ||
|
|
||
| let result = repository.getOperationResult(for: operation.id) | ||
| #expect(mockAPI.updateHighlightCallCount >= 1) | ||
| #expect(repository.pendingOperationCount == 1) | ||
| #expect(repository.failedOperationCount == 1) | ||
| #expect(result?.success == false) | ||
| #expect(result?.error != nil) | ||
| } | ||
|
|
||
| @Test | ||
| func testProcessQueueRequeuesOperationWhenSaveOperationsThrows() async { | ||
| let mockAPI = setUp() | ||
| let error = NSError(domain: "TestError", code: 1) | ||
| let repository = ThrowingSaveOperationsBibleHighlightsRepository(api: mockAPI, saveOperationsError: error) | ||
| let operation = operation() | ||
|
|
||
| repository.queueOperation(operation) | ||
| await repository.processQueue() | ||
|
|
||
| let result = repository.getOperationResult(for: operation.id) | ||
| #expect(repository.pendingOperationCount == 1) | ||
| #expect(repository.failedOperationCount == 1) | ||
| #expect(result?.success == false) | ||
| #expect(result?.retryCount == 1) | ||
| #expect(result?.error != nil) | ||
| } |
Contributor
There was a problem hiding this comment.
Three of the new tests — testProcessQueueRequeuesFailedRemoveOperation (line 481), testProcessQueueRequeuesFailedUpdateOperation (line 499), and testProcessQueueRequeuesOperationWhenSaveOperationsThrows (line 517) — each call await repository.processQueue() with a mock that always fails. The production processQueue re-queues the failing operations and then does try await Task.sleep(nanoseconds: 2_000_000_000) before the recursive call. Because the recursive call returns immediately (the guard !processingQueue short-circuits), the function does complete, but only after the 2-second sleep. That adds at least ~6 seconds wall-clock time to the test suite per run. The pre-existing tests testProcessQueueHandlesFailure and testProcessQueueHandlesError have the same characteristic, so this is not a new problem you introduced — it's just worth noting so the team can decide whether to abstract the retry delay in the repository (e.g., inject a clock or delay strategy) in a follow-up.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Tests/YouVersionPlatformCoreTests/BibleHighlightsRepositoryTests.swift
Line: 481-532
Comment:
**Retry sleep makes these tests ~2 seconds each**
Three of the new tests — `testProcessQueueRequeuesFailedRemoveOperation` (line 481), `testProcessQueueRequeuesFailedUpdateOperation` (line 499), and `testProcessQueueRequeuesOperationWhenSaveOperationsThrows` (line 517) — each call `await repository.processQueue()` with a mock that always fails. The production `processQueue` re-queues the failing operations and then does `try await Task.sleep(nanoseconds: 2_000_000_000)` before the recursive call. Because the recursive call returns immediately (the `guard !processingQueue` short-circuits), the function does complete, but only after the 2-second sleep. That adds at least ~6 seconds wall-clock time to the test suite per run. The pre-existing tests `testProcessQueueHandlesFailure` and `testProcessQueueHandlesError` have the same characteristic, so this is not a new problem you introduced — it's just worth noting so the team can decide whether to abstract the retry delay in the repository (e.g., inject a clock or delay strategy) in a follow-up.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+104
to
+110
| func operation( | ||
| color: String? = "#FF0000", | ||
| operationType: HighlightOperationType = .add, | ||
| verse: Int = 1 | ||
| ) -> PendingHighlightOperation { | ||
| PendingHighlightOperation(references: [reference(verse: verse)], color: color, operationType: operationType) | ||
| } |
Contributor
There was a problem hiding this comment.
operation shadows the operation() helper method
In several tests (e.g. lines 331, 344, 357, 370, 485, 503) a let operation = operation(...) declaration is used, where the local variable name is identical to the helper method being called. Swift resolves this correctly because the call appears on the right-hand side before the binding takes effect, but the shadowing makes the code harder to scan — especially since operation is the same identifier in both positions. Renaming the helper to something like makeOperation() (consistent with the common Swift make… factory convention) or renaming the local bindings to op would remove the ambiguity entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Tests/YouVersionPlatformCoreTests/BibleHighlightsRepositoryTests.swift
Line: 104-110
Comment:
**Local variable `operation` shadows the `operation()` helper method**
In several tests (e.g. lines 331, 344, 357, 370, 485, 503) a `let operation = operation(...)` declaration is used, where the local variable name is identical to the helper method being called. Swift resolves this correctly because the call appears on the right-hand side before the binding takes effect, but the shadowing makes the code harder to scan — especially since `operation` is the same identifier in both positions. Renaming the helper to something like `makeOperation()` (consistent with the common Swift `make…` factory convention) or renaming the local bindings to `op` would remove the ambiguity entirely.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Member
Author
|
@greptile please re-review |
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
1 participant
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.
Motivation
saveOperationsthrows.Description
Tests/YouVersionPlatformCoreTests/BibleHighlightsRepositoryTests.swiftto simplify creating commonBibleReference/PendingHighlightOperationfixtures and simulate asaveOperationsfailure.MockBibleHighlightsAPIasfinaland kept it as the mock implementation for API calls used in tests.passageIdvalues soconvertToBibleHighlight()failure paths are hit viahighlights(for:).saveOperationsandprocessQueuecovering remove/update failure results, thrown API errors, and the re-queue/retry behavior when operations fail orsaveOperationsthrows.Testing
swift testand the full test suite completed successfully with all tests passing.LINUX_SOURCEKIT_LIB_PATH=/root/.local/share/swiftly/toolchains/6.1.3/usr/lib /tmp/swiftlint-install/swiftlint --strictand it reported no violations.Codex Task
Greptile Summary
This PR expands test coverage for
BibleHighlightsRepositoryby exercising previously untested failure paths: malformedpassageIdparsing inhighlights(for:),false-return and error-throw branches for remove/update insaveOperations, and the re-queue/retry behavior inprocessQueuewhen operations fail orsaveOperationsitself throws.ThrowingSaveOperationsBibleHighlightsRepositorytest subclass that always throws fromsaveOperations, alongsidereference()andoperation()factory helpers that reduce boilerplate in the new tests.MockBibleHighlightsAPIasfinal, which is a clean improvement that lets the compiler dispatch calls statically.@Testfunctions covering the remove/update failure and error results for bothsaveOperationsandprocessQueue.Confidence Score: 5/5
Test-only additions with no changes to production code; safe to merge.
All changes are confined to the test file. The new tests correctly isolate each failure branch, use a fresh mock per test, and assert both the queue state and individual operation results. No production logic is modified.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[processQueue called] --> B{operations in queue?} B -- No --> Z[return] B -- Yes --> C[call saveOperations] C -- success map returned --> D{any result == false?} D -- No --> E[mark all operations complete] E --> Z D -- Yes --> F[re-queue failed ops\nincrement retryCount] F --> G[sleep 2s then recursive call] G --> H{guard: already processing?} H -- Yes --> Z H -- No --> B C -- throws --> I[catch error\nmark ops failed\nre-queue with retryCount+1] I --> GPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "test: cover highlight repository failure..." | Re-trigger Greptile