Michael Freno
1b5fb6b635
P1: Add distribution cert + provisioning profile import for TestFlight P3: Remove --recursive from swift format lint (redundant, causes error) P3: Revert vercel-action v25 → v30
102 lines
3.9 KiB
Markdown
102 lines
3.9 KiB
Markdown
# Code Review: FRE-4830 - Unit Tests for Phase 3 Services
|
|
|
|
**Reviewer:** Code Reviewer (f274248f-c47e-4f79-98ad-45919d951aa0)
|
|
**Engineer:** Senior Engineer (opencode_local)
|
|
**Status:** Changes requested (P0 blocker found)
|
|
|
|
## Files Reviewed
|
|
|
|
- `iOS/Lendair/LendairTests/IdVerificationServiceTests.swift`
|
|
- `iOS/Lendair/LendairTests/PaymentServiceTests.swift`
|
|
- `iOS/Lendair/LendairTests/UserServiceTests.swift`
|
|
|
|
---
|
|
|
|
## P0 — Blocking: mockTRPC computed property makes authToken setup ineffective
|
|
|
|
**Location:** `UserServiceTests.swift:75-81` (and all test files with `mockTRPC` computed property)
|
|
|
|
**Problem:** `mockTRPC` is defined as a computed property:
|
|
|
|
```swift
|
|
var mockTRPC: TRPCService! {
|
|
return TRPCService(baseURL: URL(string: "http://localhost:3000")!, session: mockSession)
|
|
}
|
|
```
|
|
|
|
Every access creates a **new** `TRPCService` instance. This means:
|
|
|
|
1. **`setUp()` line 80 is a no-op:**
|
|
```swift
|
|
mockTRPC.authToken = "test-jwt-token" // creates Instance A, sets token, discards Instance A
|
|
```
|
|
The token is never set on the instance that tests actually use.
|
|
|
|
2. **All success-path tests will throw `UserError.notAuthenticated`:**
|
|
```swift
|
|
let service = UserService(trpc: mockTRPC) // creates Instance B (nil authToken)
|
|
let result = try await service.updateUserProfile(input: input) // throws because authToken is nil
|
|
```
|
|
Since `UserService.updateUserProfile()` guards `trpc.authToken != nil`, these tests will fail.
|
|
|
|
3. **Same pattern in `AuthServiceTests.swift`** — any assertion reading `mockTRPC.authToken` (e.g., `XCTAssertEqual(mockTRPC.authToken, token, ...)`) checks a brand-new instance, not the one passed to `AuthService`. This bug likely exists there too.
|
|
|
|
**Fix:** Replace computed property with initialization in `setUp()`:
|
|
|
|
```swift
|
|
var mockTRPC: TRPCService!
|
|
var mockSession: URLSession!
|
|
|
|
override func setUp() {
|
|
super.setUp()
|
|
MockURLProtocol.responseData = nil
|
|
MockURLProtocol.responseError = nil
|
|
MockURLProtocol.httpStatusCode = 200
|
|
let config = URLSessionConfiguration.test
|
|
config.protocolClasses = [MockURLProtocol.self]
|
|
mockSession = URLSession(configuration: config)
|
|
mockTRPC = TRPCService(baseURL: URL(string: "http://localhost:3000")!, session: mockSession)
|
|
mockTRPC.authToken = "test-jwt-token"
|
|
}
|
|
```
|
|
|
|
This applies to **all three test files** (IdVerificationServiceTests, PaymentServiceTests, UserServiceTests) plus AuthServiceTests if it exists.
|
|
|
|
---
|
|
|
|
## Minor: NASA 2+ Assertions Standard
|
|
|
|
Several tests have only 1 assertion, violating the NASA standard declared in the file headers:
|
|
|
|
**IdVerificationServiceTests:**
|
|
- `testCreateSessionWithPassportType` (line 88)
|
|
- `testCreateSessionWithNationalIdType` (line 101)
|
|
- `testGetStatusReturnsPending` (line 162)
|
|
- `testGetStatusReturnsNotStarted` (line 175)
|
|
- `testGetStatusReturnsFailed` (line 188)
|
|
|
|
**PaymentServiceTests:**
|
|
- `testCreateDepositIntentWithMinimumAmount` (line 89)
|
|
|
|
**UserServiceTests:**
|
|
- `testUpdateUserProfileWithPhoneReturnsUpdatedUser` (line 111)
|
|
- `testUpdateUserProfileWithNilFieldsReturnsUpdatedUser` (line 139)
|
|
- `testUpdateProfileInputEncodesName` (line 221)
|
|
- `testUpdateProfileInputEncodesPhone` (line 229)
|
|
|
|
---
|
|
|
|
## Minor: Inconsistency in DEBUG assertion test pattern
|
|
|
|
**PaymentServiceTests** uses bare `try await` for assertion tests, while **LoanServiceTests** uses `do-catch`. The fix commit's bare `try await` approach is cleaner (assert crashes via `abort()`, not throw), but inconsistent with the reference pattern. Recommend either aligning all files to one pattern or noting that this is acceptable given the semantic difference.
|
|
|
|
---
|
|
|
|
## Summary
|
|
|
|
| Severity | Finding | Action |
|
|
|----------|---------|--------|
|
|
| P0 | mockTRPC computed property makes authToken setup a no-op | Fix fixture initialization pattern |
|
|
| P3 | Single-assertion tests (10 tests) | Add second assertions |
|
|
| P3 | Inconsistent DEBUG test pattern | Align or document as intentional |
|