Skip to content

test: cover highlight repository failures #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
davidfedor wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test: cover highlight repository failures #127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 149 additions & 1 deletion Tests/YouVersionPlatformCoreTests/BibleHighlightsRepositoryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import Testing
// MARK: - Mock API

@MainActor
class MockBibleHighlightsAPI: BibleHighlightsAPIProtocol {
final class MockBibleHighlightsAPI: BibleHighlightsAPIProtocol {
var createHighlightCallCount = 0
var getHighlightsCallCount = 0
var updateHighlightCallCount = 0
Expand Down Expand Up @@ -70,6 +70,20 @@ class MockBibleHighlightsAPI: BibleHighlightsAPIProtocol {
}
}

@MainActor
final class ThrowingSaveOperationsBibleHighlightsRepository: BibleHighlightsRepository {
private let saveOperationsError: Error

init(api: BibleHighlightsAPIProtocol, saveOperationsError: Error) {
self.saveOperationsError = saveOperationsError
super.init(api: api)
}

override func saveOperations(_ operations: [PendingHighlightOperation]) async throws -> [UUID: Bool] {
throw saveOperationsError
}
}

// MARK: - Tests

@MainActor
Expand All @@ -83,6 +97,18 @@ struct BibleHighlightsRepositoryTests {
return mockAPI
}

func reference(verse: Int = 1) -> BibleReference {
BibleReference(versionId: 1, bookUSFM: "GEN", chapter: 1, verse: verse)
}

func operation(
color: String? = "#FF0000",
operationType: HighlightOperationType = .add,
verse: Int = 1
) -> PendingHighlightOperation {
PendingHighlightOperation(references: [reference(verse: verse)], color: color, operationType: operationType)
}
Comment on lines +104 to +110

@greptile-apps greptile-apps Bot May 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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!

Fix in Claude Code Fix in Cursor


// MARK: - Test Highlights Fetching

@Test
Expand Down Expand Up @@ -166,6 +192,22 @@ struct BibleHighlightsRepositoryTests {
#expect(result["1_GEN_1"]?.isEmpty == true)
}

@Test
func testHighlightsSkipsInvalidPassageId() async throws {
let mockAPI = setUp()
let repository = BibleHighlightsRepository(api: mockAPI)
mockAPI.mockGetHighlightsResult = [
HighlightResponse(bibleId: 1, passageId: "GEN.1", color: "FF0000"),
HighlightResponse(bibleId: 1, passageId: "GEN.ONE.1", color: "00FF00"),
HighlightResponse(bibleId: 1, passageId: "GEN.1.ONE", color: "0000FF")
]

let result = try await repository.highlights(for: [BibleReference(versionId: 1, bookUSFM: "GEN", chapter: 1)])

#expect(mockAPI.getHighlightsCallCount == 1)
#expect(result["1_GEN_1"]?.isEmpty == true)
}

// MARK: - Test Operation Processing

@Test
Expand Down Expand Up @@ -281,6 +323,58 @@ struct BibleHighlightsRepositoryTests {
#expect(result[operation.id] == false)
}

@Test
func testSaveOperationsRemoveHandlesFailedResult() async throws {
let mockAPI = setUp()
let repository = BibleHighlightsRepository(api: mockAPI)
mockAPI.mockDeleteHighlightResult = false
let operation = operation(color: nil, operationType: .remove)

let result = try await repository.saveOperations([operation])

#expect(mockAPI.deleteHighlightCallCount == 1)
#expect(result[operation.id] == false)
}

@Test
func testSaveOperationsRemoveHandlesError() async throws {
let mockAPI = setUp()
let repository = BibleHighlightsRepository(api: mockAPI)
mockAPI.shouldThrowError = true
let operation = operation(color: nil, operationType: .remove)

let result = try await repository.saveOperations([operation])

#expect(mockAPI.deleteHighlightCallCount == 1)
#expect(result[operation.id] == false)
}

@Test
func testSaveOperationsUpdateHandlesFailedResult() async throws {
let mockAPI = setUp()
let repository = BibleHighlightsRepository(api: mockAPI)
mockAPI.mockUpdateHighlightResult = false
let operation = operation(operationType: .update)

let result = try await repository.saveOperations([operation])

#expect(mockAPI.updateHighlightCallCount == 1)
#expect(result[operation.id] == false)
}

@Test
func testSaveOperationsUpdateHandlesError() async throws {
let mockAPI = setUp()
let repository = BibleHighlightsRepository(api: mockAPI)
mockAPI.shouldThrowError = true
let operation = operation(operationType: .update)

let result = try await repository.saveOperations([operation])

#expect(mockAPI.updateHighlightCallCount == 1)
#expect(result[operation.id] == false)
}

// MARK: - Test Queue Management

@Test
Expand Down Expand Up @@ -383,6 +477,60 @@ struct BibleHighlightsRepositoryTests {
#expect(repository.failedOperationCount == 1)
}

@Test
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)
}
Comment on lines +481 to +532

@greptile-apps greptile-apps Bot May 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

Fix in Claude Code Fix in Cursor


// MARK: - Test Operation Results

@Test
Expand Down
Loading