[Local Catalog] Fix flaky and slow POS catalog sync tests (#16227)

Author: jaclync

Modules/Sources/Yosemite/Tools/POS/POSCatalogFullSyncService.swift

@@ -28,7 +28,6 @@ public struct POSCatalog {
 // periphery:ignore
 public final class POSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol {
     private let syncRemote: POSCatalogSyncRemoteProtocol
-    private let batchSize: Int
     private let persistenceService: POSCatalogPersistenceServiceProtocol
     private let batchedLoader: BatchedRequestLoader
 
@@ -49,11 +48,15 @@ public final class POSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol
         self.init(syncRemote: syncRemote, batchSize: batchSize, persistenceService: persistenceService)
     }
 
-    init(syncRemote: POSCatalogSyncRemoteProtocol, batchSize: Int, persistenceService: POSCatalogPersistenceServiceProtocol) {
+    init(
+        syncRemote: POSCatalogSyncRemoteProtocol,
+        batchSize: Int,
+        retryDelay: TimeInterval = 2.0,
+        persistenceService: POSCatalogPersistenceServiceProtocol
+    ) {
         self.syncRemote = syncRemote
-        self.batchSize = batchSize
         self.persistenceService = persistenceService
-        self.batchedLoader = BatchedRequestLoader(batchSize: batchSize)
+        self.batchedLoader = BatchedRequestLoader(batchSize: batchSize, retryDelay: retryDelay)
     }
 
     // MARK: - Protocol Conformance

Modules/Sources/Yosemite/Tools/POS/POSCatalogIncrementalSyncService.swift

@@ -20,7 +20,6 @@ public protocol POSCatalogIncrementalSyncServiceProtocol {
 // periphery:ignore
 public final class POSCatalogIncrementalSyncService: POSCatalogIncrementalSyncServiceProtocol {
     private let syncRemote: POSCatalogSyncRemoteProtocol
-    private let batchSize: Int
     private let persistenceService: POSCatalogPersistenceServiceProtocol
     private let batchedLoader: BatchedRequestLoader
 
@@ -41,11 +40,15 @@ public final class POSCatalogIncrementalSyncService: POSCatalogIncrementalSyncSe
         self.init(syncRemote: syncRemote, batchSize: batchSize, persistenceService: persistenceService)
     }
 
-    init(syncRemote: POSCatalogSyncRemoteProtocol, batchSize: Int, persistenceService: POSCatalogPersistenceServiceProtocol) {
+    init(
+        syncRemote: POSCatalogSyncRemoteProtocol,
+        batchSize: Int,
+        retryDelay: TimeInterval = 2.0,
+        persistenceService: POSCatalogPersistenceServiceProtocol
+    ) {
         self.syncRemote = syncRemote
-        self.batchSize = batchSize
         self.persistenceService = persistenceService
-        self.batchedLoader = BatchedRequestLoader(batchSize: batchSize)
+        self.batchedLoader = BatchedRequestLoader(batchSize: batchSize, retryDelay: retryDelay)
     }
 
     // MARK: - Protocol Conformance

Modules/Tests/YosemiteTests/Mocks/MockPOSCatalogSyncRemote.swift

@@ -8,10 +8,10 @@ final class MockPOSCatalogSyncRemote: POSCatalogSyncRemoteProtocol {
     private(set) var incrementalProductResults: [Int: Result<PagedItems<POSProduct>, Error>] = [:]
     private(set) var incrementalVariationResults: [Int: Result<PagedItems<POSProductVariation>, Error>] = [:]
 
-    private(set) var loadProductsCallCount = 0
-    private(set) var loadProductVariationsCallCount = 0
-    private(set) var loadIncrementalProductsCallCount = 0
-    private(set) var loadIncrementalProductVariationsCallCount = 0
+    let loadProductsCallCount = Counter()
+    let loadProductVariationsCallCount = Counter()
+    let loadIncrementalProductsCallCount = Counter()
+    let loadIncrementalProductVariationsCallCount = Counter()
 
     private(set) var lastIncrementalProductsModifiedAfter: Date?
     private(set) var lastIncrementalVariationsModifiedAfter: Date?
@@ -67,7 +67,7 @@ final class MockPOSCatalogSyncRemote: POSCatalogSyncRemoteProtocol {
     // MARK: - Protocol Methods - Incremental Sync
 
     func loadProducts(modifiedAfter: Date, siteID: Int64, pageNumber: Int) async throws -> PagedItems<POSProduct> {
-        loadIncrementalProductsCallCount += 1
+        await loadIncrementalProductsCallCount.increment()
         lastIncrementalProductsModifiedAfter = modifiedAfter
 
         if let result = incrementalProductResults[pageNumber] {
@@ -82,7 +82,7 @@ final class MockPOSCatalogSyncRemote: POSCatalogSyncRemoteProtocol {
     }
 
     func loadProductVariations(modifiedAfter: Date, siteID: Int64, pageNumber: Int) async throws -> PagedItems<POSProductVariation> {
-        loadIncrementalProductVariationsCallCount += 1
+        await loadIncrementalProductVariationsCallCount.increment()
         lastIncrementalVariationsModifiedAfter = modifiedAfter
 
         if let result = incrementalVariationResults[pageNumber] {
@@ -99,7 +99,7 @@ final class MockPOSCatalogSyncRemote: POSCatalogSyncRemoteProtocol {
     // MARK: - Protocol Methods - Full Sync
 
     func loadProducts(siteID: Int64, pageNumber: Int) async throws -> PagedItems<POSProduct> {
-        loadProductsCallCount += 1
+        await loadProductsCallCount.increment()
 
         if let result = productResults[pageNumber] {
             switch result {
@@ -113,7 +113,7 @@ final class MockPOSCatalogSyncRemote: POSCatalogSyncRemoteProtocol {
     }
 
     func loadProductVariations(siteID: Int64, pageNumber: Int) async throws -> PagedItems<POSProductVariation> {
-        loadProductVariationsCallCount += 1
+        await loadProductVariationsCallCount.increment()
 
         if let result = variationResults[pageNumber] {
             switch result {

Modules/Tests/YosemiteTests/Tools/POS/BatchedRequestLoaderTests.swift

@@ -11,10 +11,10 @@ struct BatchedRequestLoaderTests {
         // Given
         let sut = BatchedRequestLoader(batchSize: 2)
         let expectedItems = ["item1", "item2", "item3"]
-        var callCount = 0
+        let callCount = Counter()
 
         let makeRequest: (Int) async throws -> PagedItems<String> = { pageNumber in
-            callCount += 1
+            await callCount.increment()
             #expect(pageNumber == 1 || pageNumber == 2)
             if pageNumber == 1 {
                 return PagedItems(items: expectedItems, hasMorePages: false, totalItems: 3)
@@ -28,7 +28,7 @@ struct BatchedRequestLoaderTests {
 
         // Then
         #expect(result == expectedItems)
-        #expect(callCount == 2) // batchSize = 2, so it fetches pages 1 and 2
+        #expect(await callCount.value == 2) // batchSize = 2, so it fetches pages 1 and 2
     }
 
     // MARK: - Retry Logic Tests
@@ -43,13 +43,23 @@ struct BatchedRequestLoaderTests {
             errorEvaluator: mockErrorEvaluator
         )
 
-        var attemptsByPage: [Int: Int] = [:]
+        let attemptCountForPage1 = Counter()
+        let attemptCountForPage2 = Counter()
+        let attemptCountForPage3 = Counter()
         let makeRequest: (Int) async throws -> PagedItems<String> = { pageNumber in
-            attemptsByPage[pageNumber, default: 0] += 1
-
-            // Page 2 fails once, then succeeds
-            if pageNumber == 2 && attemptsByPage[pageNumber] == 1 {
-                throw URLError(.timedOut)
+            switch pageNumber {
+            case 1:
+                await attemptCountForPage1.increment()
+            case 2:
+                await attemptCountForPage2.increment()
+                // Page 2 fails once, then succeeds
+                if await attemptCountForPage2.value == 1 {
+                    throw URLError(.timedOut)
+                }
+            case 3:
+                await attemptCountForPage3.increment()
+            default:
+                throw NSError(domain: "Invalid page number", code: 0)
             }
 
             return PagedItems(items: ["page \(pageNumber) success"], hasMorePages: false, totalItems: 1)
@@ -60,9 +70,9 @@ struct BatchedRequestLoaderTests {
 
         // Then
         #expect(result == ["page 1 success", "page 2 success", "page 3 success"])
-        #expect(attemptsByPage[1] == 1) // Page 1 succeeded immediately
-        #expect(attemptsByPage[2] == 2) // Page 2 failed once, then succeeded
-        #expect(attemptsByPage[3] == 1) // Page 3 succeeded immediately
+        #expect(await attemptCountForPage1.value == 1) // Page 1 succeeded immediately
+        #expect(await attemptCountForPage2.value == 2) // Page 2 failed once, then succeeded
+        #expect(await attemptCountForPage3.value == 1) // Page 3 succeeded immediately
     }
 
     @Test func loadAll_does_not_retry_on_non_retryable_error() async throws {
@@ -75,17 +85,17 @@ struct BatchedRequestLoaderTests {
             errorEvaluator: mockErrorEvaluator
         )
 
-        var attemptCount = 0
+        let attemptCount = Counter()
         let makeRequest: (Int) async throws -> PagedItems<String> = { pageNumber in
-            attemptCount += 1
+            await attemptCount.increment()
             throw NetworkError.unacceptableStatusCode(statusCode: 401, response: nil)
         }
 
         // When/Then
         await #expect(throws: NetworkError.self) {
             try await sut.loadAll(makeRequest: makeRequest)
         }
-        #expect(attemptCount == 2) // No retries for non-retryable error for both pages
+        #expect(await attemptCount.value == 2) // No retries for non-retryable error for both pages
     }
 
     @Test func loadAll_fails_after_max_retries() async throws {
@@ -98,18 +108,18 @@ struct BatchedRequestLoaderTests {
             errorEvaluator: mockErrorEvaluator
         )
 
-        var attemptCount = 0
+        let attemptCount = Counter()
         let expectedError = URLError(.networkConnectionLost)
         let makeRequest: (Int) async throws -> PagedItems<String> = { pageNumber in
-            attemptCount += 1
+            await attemptCount.increment()
             throw expectedError
         }
 
         // When/Then
         await #expect(throws: URLError.self) {
             try await sut.loadAll(makeRequest: makeRequest)
         }
-        #expect(attemptCount == 3) // maxRetries = 3
+        #expect(await attemptCount.value == 3) // maxRetries = 3
     }
 }
 

Modules/Tests/YosemiteTests/Tools/POS/ConcurrencyUtilities/Counter.swift

@@ -0,0 +1,8 @@
+/// A simple counter actor to track increments in a thread-safe manner.
+actor Counter {
+    var value = 0
+
+    func increment() {
+        value += 1
+    }
+}

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift

@@ -33,8 +33,8 @@ struct POSCatalogFullSyncServiceTests {
         // Then
         #expect(result.products.count == expectedProducts.count)
         #expect(result.variations.count == expectedVariations.count)
-        #expect(mockSyncRemote.loadProductsCallCount == 2) // 1 batch of 2 requests
-        #expect(mockSyncRemote.loadProductVariationsCallCount == 2) // 1 batch of 2 requests
+        #expect(await mockSyncRemote.loadProductsCallCount.value == 2) // 1 batch of 2 requests
+        #expect(await mockSyncRemote.loadProductVariationsCallCount.value == 2) // 1 batch of 2 requests
     }
 
     @Test func startFullSync_handles_paginated_products_correctly() async throws {
@@ -54,8 +54,8 @@ struct POSCatalogFullSyncServiceTests {
 
         // Then
         #expect(result.products.count == 3)
-        #expect(mockSyncRemote.loadProductsCallCount == 4) // 2 batches of 2 requests
-        #expect(mockSyncRemote.loadProductVariationsCallCount == 2) // 1 batch of 2 requests
+        #expect(await mockSyncRemote.loadProductsCallCount.value == 4) // 2 batches of 2 requests
+        #expect(await mockSyncRemote.loadProductVariationsCallCount.value == 2) // 1 batch of 2 requests
     }
 
     @Test func startFullSync_handles_paginated_variations_correctly() async throws {
@@ -75,8 +75,8 @@ struct POSCatalogFullSyncServiceTests {
 
         // Then
         #expect(result.variations.count == 4)
-        #expect(mockSyncRemote.loadProductsCallCount == 2) // 1 batch of 2 requests
-        #expect(mockSyncRemote.loadProductVariationsCallCount == 4) // 2 batches of 2 requests
+        #expect(await mockSyncRemote.loadProductsCallCount.value == 2) // 1 batch of 2 requests
+        #expect(await mockSyncRemote.loadProductVariationsCallCount.value == 4) // 2 batches of 2 requests
     }
 
     @Test func startFullSync_stops_pagination_when_no_new_items_returned_and_hasMorePages_is_inaccurate() async throws {
@@ -95,7 +95,7 @@ struct POSCatalogFullSyncServiceTests {
 
         // Then - Should stop after empty page
         #expect(result.products.count == 1)
-        #expect(mockSyncRemote.loadProductsCallCount == 4) // The results from the second batch are empty
+        #expect(await mockSyncRemote.loadProductsCallCount.value == 4) // The results from the second batch are empty
     }
 
     @Test func startFullSync_handles_batch_processing_correctly() async throws {
@@ -116,13 +116,14 @@ struct POSCatalogFullSyncServiceTests {
 
         // Then
         #expect(result.products.count == 5)
-        #expect(mockSyncRemote.loadProductsCallCount == 6)
+        #expect(await mockSyncRemote.loadProductsCallCount.value == 6)
     }
 
     @Test func startFullSync_propagates_network_errors() async throws {
         // Given
         let expectedError = NSError(domain: "network", code: 500, userInfo: [NSLocalizedDescriptionKey: "Network error"])
         mockSyncRemote.setProductResult(pageNumber: 1, result: .failure(expectedError))
+        let sut = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: 2, retryDelay: 0, persistenceService: mockPersistenceService)
 
         // When/Then
         await #expect(throws: expectedError) {
@@ -176,7 +177,7 @@ struct POSCatalogFullSyncServiceTests {
         _ = try await service.startFullSync(for: sampleSiteID)
 
         // Then
-        #expect(mockSyncRemote.loadProductsCallCount == 5)
-        #expect(mockSyncRemote.loadProductVariationsCallCount == 5)
+        #expect(await mockSyncRemote.loadProductsCallCount.value == 5)
+        #expect(await mockSyncRemote.loadProductVariationsCallCount.value == 5)
     }
 }

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogIncrementalSyncServiceTests.swift

@@ -31,8 +31,8 @@ struct POSCatalogIncrementalSyncServiceTests {
         try await sut.startIncrementalSync(for: sampleSiteID, lastFullSyncDate: lastFullSyncDate, lastIncrementalSyncDate: nil)
 
         // Then
-        #expect(mockSyncRemote.loadIncrementalProductsCallCount == 2)
-        #expect(mockSyncRemote.loadIncrementalProductVariationsCallCount == 2)
+        #expect(await mockSyncRemote.loadIncrementalProductsCallCount.value == 2)
+        #expect(await mockSyncRemote.loadIncrementalProductVariationsCallCount.value == 2)
         #expect(mockSyncRemote.lastIncrementalProductsModifiedAfter == lastFullSyncDate)
         #expect(mockSyncRemote.lastIncrementalVariationsModifiedAfter == lastFullSyncDate)
         #expect(mockPersistenceService.persistIncrementalCatalogDataCallCount == 1)
@@ -76,7 +76,7 @@ struct POSCatalogIncrementalSyncServiceTests {
         try await sut.startIncrementalSync(for: sampleSiteID, lastFullSyncDate: lastFullSyncDate, lastIncrementalSyncDate: nil)
 
         // Then
-        #expect(mockSyncRemote.loadIncrementalProductsCallCount == 4)
+        #expect(await mockSyncRemote.loadIncrementalProductsCallCount.value == 4)
         let persistedCatalog = try #require(mockPersistenceService.persistIncrementalCatalogDataLastPersistedCatalog)
         #expect(persistedCatalog.products.count == 3)
     }
@@ -97,7 +97,7 @@ struct POSCatalogIncrementalSyncServiceTests {
         try await sut.startIncrementalSync(for: sampleSiteID, lastFullSyncDate: lastFullSyncDate, lastIncrementalSyncDate: nil)
 
         // Then
-        #expect(mockSyncRemote.loadIncrementalProductVariationsCallCount == 2)
+        #expect(await mockSyncRemote.loadIncrementalProductVariationsCallCount.value == 2)
         let persistedCatalog = try #require(mockPersistenceService.persistIncrementalCatalogDataLastPersistedCatalog)
         #expect(persistedCatalog.variations.count == 2)
     }
@@ -108,6 +108,7 @@ struct POSCatalogIncrementalSyncServiceTests {
         // Given
         let lastFullSyncDate = Date(timeIntervalSince1970: 1000)
         let expectedError = NSError(domain: "test", code: 500, userInfo: nil)
+        let sut = POSCatalogIncrementalSyncService(syncRemote: mockSyncRemote, batchSize: 2, retryDelay: 0, persistenceService: mockPersistenceService)
 
         mockSyncRemote.setIncrementalProductResult(pageNumber: 1, result: .failure(expectedError))
         mockSyncRemote.setIncrementalVariationResult(pageNumber: 1, result: .success(PagedItems(items: [], hasMorePages: false, totalItems: 0)))