From 42671e5ece8338588ca49271e27212c7805179f9 Mon Sep 17 00:00:00 2001 From: kean Date: Sat, 4 May 2024 19:39:18 -0400 Subject: [PATCH] Fix flaky tests --- Nuke.xcodeproj/project.pbxproj | 4 - Sources/Nuke/Pipeline/ImagePipeline.swift | 2 + Tests/NukeTests/DataLoaderTests.swift | 77 ---------- .../ImagePipelineCoalescingTests.swift | 144 ++++++++---------- .../ImagePipelineDataCacheTests.swift | 6 +- .../ImagePipelineLoadDataTests.swift | 22 ++- 6 files changed, 85 insertions(+), 170 deletions(-) delete mode 100644 Tests/NukeTests/DataLoaderTests.swift diff --git a/Nuke.xcodeproj/project.pbxproj b/Nuke.xcodeproj/project.pbxproj index ce8243059..26e13f156 100644 --- a/Nuke.xcodeproj/project.pbxproj +++ b/Nuke.xcodeproj/project.pbxproj @@ -26,7 +26,6 @@ 0C1C201E29ABBF19004B38FD /* Nuke.framework in Embed Frameworks */ = {isa = PBXBuildFile; fileRef = 0C9174901BAE99EE004A7905 /* Nuke.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; }; 0C1E620B1D6F817700AD5CF5 /* ImageRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0C1E620A1D6F817700AD5CF5 /* ImageRequestTests.swift */; }; 0C1ECA421D526461009063A9 /* ImageCacheTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0C7C06871BCA888800089D7F /* ImageCacheTests.swift */; }; - 0C211E502856328500F48AA6 /* DataLoaderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0C211E4F2856328500F48AA6 /* DataLoaderTests.swift */; }; 0C222DE3294E2DEA00012288 /* NukeUI.docc in Sources */ = {isa = PBXBuildFile; fileRef = 0C222DE2294E2DEA00012288 /* NukeUI.docc */; }; 0C222DE5294E2E0300012288 /* NukeExtensions.docc in Sources */ = {isa = PBXBuildFile; fileRef = 0C222DE4294E2E0200012288 /* NukeExtensions.docc */; }; 0C2A368B26437BF100F1D000 /* TaskLoadData.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0C2A368A26437BF100F1D000 /* TaskLoadData.swift */; }; @@ -362,7 +361,6 @@ 0C1E59682372EBBC00674B63 /* validate.sh */ = {isa = PBXFileReference; lastKnownFileType = text.script.sh; name = validate.sh; path = Scripts/validate.sh; sourceTree = ""; }; 0C1E596A2372EBBC00674B63 /* test.sh */ = {isa = PBXFileReference; lastKnownFileType = text.script.sh; name = test.sh; path = Scripts/test.sh; sourceTree = ""; }; 0C1E620A1D6F817700AD5CF5 /* ImageRequestTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ImageRequestTests.swift; sourceTree = ""; }; - 0C211E4F2856328500F48AA6 /* DataLoaderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DataLoaderTests.swift; sourceTree = ""; }; 0C222DE2294E2DEA00012288 /* NukeUI.docc */ = {isa = PBXFileReference; lastKnownFileType = folder.documentationcatalog; path = NukeUI.docc; sourceTree = ""; }; 0C222DE4294E2E0200012288 /* NukeExtensions.docc */ = {isa = PBXFileReference; lastKnownFileType = folder.documentationcatalog; path = NukeExtensions.docc; sourceTree = ""; }; 0C2A368A26437BF100F1D000 /* TaskLoadData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TaskLoadData.swift; sourceTree = ""; }; @@ -714,7 +712,6 @@ 0C64F73A24383043001983C6 /* ImageEncoderTests.swift */, 0CD195291D4348AC00E011BB /* ImagePrefetcherTests.swift */, 0CB26806208F25C2004C83F4 /* DataCacheTests.swift */, - 0C211E4F2856328500F48AA6 /* DataLoaderTests.swift */, 0CE5F681215638300046609F /* ResumableDataTests.swift */, 0C4AF1E91FE8551D002F86CB /* LinkedListTest.swift */, 0CB2EFD52110F52C00F7C63F /* RateLimiterTests.swift */, @@ -1659,7 +1656,6 @@ 0CE3992D1D4697CE00A87D47 /* ImagePipelineTests.swift in Sources */, 0C64F73B24383043001983C6 /* ImageEncoderTests.swift in Sources */, 0CF58FF726DAAC3800D2650D /* ImageDownsampleTests.swift in Sources */, - 0C211E502856328500F48AA6 /* DataLoaderTests.swift in Sources */, 0C967EB328688B3F0050E083 /* DocumentationTests.swift in Sources */, 0C91B0EC2438E287007F9100 /* ResizeTests.swift in Sources */, 0CA3BA63285C11EA0079A444 /* ImagePipelineTaskDelegateTests.swift in Sources */, diff --git a/Sources/Nuke/Pipeline/ImagePipeline.swift b/Sources/Nuke/Pipeline/ImagePipeline.swift index 7915ecc1f..829ccc070 100644 --- a/Sources/Nuke/Pipeline/ImagePipeline.swift +++ b/Sources/Nuke/Pipeline/ImagePipeline.swift @@ -53,6 +53,7 @@ public final class ImagePipeline: @unchecked Sendable { let rateLimiter: RateLimiter? let id = UUID() + var onTaskStarted: ((ImageTask) -> Void)? // Debug purposes deinit { lock.deinitialize(count: 1) @@ -309,6 +310,7 @@ public final class ImagePipeline: @unchecked Sendable { task?.process(.init($0)) } delegate.imageTaskDidStart(task, pipeline: self) + onTaskStarted?(task) } // MARK: - Image Task Events diff --git a/Tests/NukeTests/DataLoaderTests.swift b/Tests/NukeTests/DataLoaderTests.swift deleted file mode 100644 index ff3b7be25..000000000 --- a/Tests/NukeTests/DataLoaderTests.swift +++ /dev/null @@ -1,77 +0,0 @@ -// The MIT License (MIT) -// -// Copyright (c) 2015-2024 Alexander Grebenyuk (github.com/kean). - -import XCTest -@testable import Nuke - -#if !os(watchOS) && !os(tvOS) - -class DataLoaderTests: XCTestCase { - var sut: DataLoader! - private let delegate = MockSessionDelegate() - - override func setUp() { - super.setUp() - - sut = DataLoader() - } - - var url: URL { - Test.url(forResource: "fixture", extension: "jpeg") - } - - var request: ImageRequest { - ImageRequest(url: url) - } - - // MARK: - Loading - - func testDataIsLoaded() { - // WHEN - var recordedData = Data() - var recorededResponse: URLResponse? - let expectation = self.expectation(description: "DataLoaded") - _ = sut.loadData(with: URLRequest(url: url), didReceiveData: { data, response in - recordedData.append(data) - recorededResponse = response - }, completion: { error in - XCTAssertNil(error) - expectation.fulfill() - }) - wait() - - // THEN - XCTAssertEqual(recordedData.count, 22789) - XCTAssertEqual(recorededResponse?.url, url) - } - - // MARK: - Custom Delegate - - func testCustomDelegate() { - // GIVEN - sut.delegate = delegate - - // WHEN - let expectation = self.expectation(description: "MetricsCollected") - delegate.onCollectedMetrics = { _ in expectation.fulfill() } - - _ = sut.loadData(with: URLRequest(url: url), didReceiveData: { _, _ in }, completion: { _ in }) - wait(for: [expectation], timeout: 5) - - // THEN - XCTAssertEqual(delegate.recordedMetrics.count, 1) - } -} - -private final class MockSessionDelegate: NSObject, URLSessionTaskDelegate { - var recordedMetrics: [URLSessionTaskMetrics] = [] - var onCollectedMetrics: ((URLSessionTaskMetrics) -> Void)? - - func urlSession(_ session: URLSession, task: URLSessionTask, didFinishCollecting metrics: URLSessionTaskMetrics) { - recordedMetrics.append(metrics) - onCollectedMetrics?(metrics) - } -} - -#endif diff --git a/Tests/NukeTests/ImagePipelineTests/ImagePipelineCoalescingTests.swift b/Tests/NukeTests/ImagePipelineTests/ImagePipelineCoalescingTests.swift index 3001c5d22..19befce33 100644 --- a/Tests/NukeTests/ImagePipelineTests/ImagePipelineCoalescingTests.swift +++ b/Tests/NukeTests/ImagePipelineTests/ImagePipelineCoalescingTests.swift @@ -160,25 +160,25 @@ class ImagePipelineCoalescingTests: XCTestCase { // MARK: - Thumbnail func testDeduplicationGivenSameURLButDifferentThumbnailOptions() { - dataLoader.queue.isSuspended = true - // GIVEN requests with the same URLs but one accesses thumbnail let request1 = ImageRequest(url: Test.url, userInfo: [.thumbnailKey: ImageRequest.ThumbnailOptions(maxPixelSize: 400)]) let request2 = ImageRequest(url: Test.url) - // WHEN loading images for those requests - expect(pipeline).toLoadImage(with: request1) { result in - // THEN - guard let image = result.value?.image else { return XCTFail() } - XCTAssertEqual(image.sizeInPixels, CGSize(width: 400, height: 300)) - } - expect(pipeline).toLoadImage(with: request2) { result in - // THEN - guard let image = result.value?.image else { return XCTFail() } - XCTAssertEqual(image.sizeInPixels, CGSize(width: 640.0, height: 480.0)) - } + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { - dataLoader.queue.isSuspended = false + // WHEN loading images for those requests + expect(pipeline).toLoadImage(with: request1) { result in + // THEN + guard let image = result.value?.image else { return XCTFail() } + XCTAssertEqual(image.sizeInPixels, CGSize(width: 400, height: 300)) + } + expect(pipeline).toLoadImage(with: request2) { result in + // THEN + guard let image = result.value?.image else { return XCTFail() } + XCTAssertEqual(image.sizeInPixels, CGSize(width: 640.0, height: 480.0)) + } + + } wait { _ in // THEN the image data is fetched once @@ -187,27 +187,25 @@ class ImagePipelineCoalescingTests: XCTestCase { } func testDeduplicationGivenSameURLButDifferentThumbnailOptionsReversed() { - dataLoader.queue.isSuspended = true - // GIVEN requests with the same URLs but one accesses thumbnail // (in this test, order is reversed) let request1 = ImageRequest(url: Test.url) let request2 = ImageRequest(url: Test.url, userInfo: [.thumbnailKey: ImageRequest.ThumbnailOptions(maxPixelSize: 400)]) - // WHEN loading images for those requests - expect(pipeline).toLoadImage(with: request1) { result in - // THEN - guard let image = result.value?.image else { return XCTFail() } - XCTAssertEqual(image.sizeInPixels, CGSize(width: 640.0, height: 480.0)) - } - expect(pipeline).toLoadImage(with: request2) { result in - // THEN - guard let image = result.value?.image else { return XCTFail() } - XCTAssertEqual(image.sizeInPixels, CGSize(width: 400, height: 300)) + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { + // WHEN loading images for those requests + expect(pipeline).toLoadImage(with: request1) { result in + // THEN + guard let image = result.value?.image else { return XCTFail() } + XCTAssertEqual(image.sizeInPixels, CGSize(width: 640.0, height: 480.0)) + } + expect(pipeline).toLoadImage(with: request2) { result in + // THEN + guard let image = result.value?.image else { return XCTFail() } + XCTAssertEqual(image.sizeInPixels, CGSize(width: 400, height: 300)) + } } - dataLoader.queue.isSuspended = false - wait { _ in // THEN the image data is fetched once XCTAssertEqual(self.dataLoader.createdTaskCount, 1) @@ -217,8 +215,6 @@ class ImagePipelineCoalescingTests: XCTestCase { // MARK: - Processing func testProcessorsAreDeduplicated() { - dataLoader.queue.isSuspended = true - // Given // Make sure we don't start processing when some requests haven't // started yet. @@ -226,11 +222,11 @@ class ImagePipelineCoalescingTests: XCTestCase { let queueObserver = OperationQueueObserver(queue: pipeline.configuration.imageProcessingQueue) // When - expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [processors.make(id: "1")])) - expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [processors.make(id: "2")])) - expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [processors.make(id: "1")])) - - dataLoader.queue.isSuspended = false + suspendDataLoading(for: pipeline, expectedRequestCount: 3) { + expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [processors.make(id: "1")])) + expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [processors.make(id: "2")])) + expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [processors.make(id: "1")])) + } // When/Then wait { _ in @@ -488,18 +484,11 @@ class ImagePipelineCoalescingTests: XCTestCase { $0.isTaskCoalescingEnabled = false } - dataLoader.queue.isSuspended = true - // When/Then - let request1 = Test.request - let request2 = Test.request - - expect(pipeline).toLoadImage(with: request1) - expect(pipeline).toLoadImage(with: request2) - - pipeline.queue.sync {} - dataLoader.queue.isSuspended = false - + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { + expect(pipeline).toLoadImage(with: Test.request) + expect(pipeline).toLoadImage(with: Test.request) + } wait { _ in XCTAssertEqual(self.dataLoader.createdTaskCount, 2) } @@ -528,18 +517,18 @@ class ImagePipelineProcessingDeduplicationTests: XCTestCase { let request2 = ImageRequest(url: Test.url, processors: [processors.make(id: "1"), processors.make(id: "2")]) // When - dataLoader.queue.isSuspended = true - expect(pipeline).toLoadImage(with: request1) { result in - let image = result.value?.image - XCTAssertEqual(image?.nk_test_processorIDs ?? [], ["1"]) - } - expect(pipeline).toLoadImage(with: request2) { result in - let image = result.value?.image - XCTAssertEqual(image?.nk_test_processorIDs ?? [], ["1", "2"]) + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { + expect(pipeline).toLoadImage(with: request1) { result in + let image = result.value?.image + XCTAssertEqual(image?.nk_test_processorIDs ?? [], ["1"]) + } + expect(pipeline).toLoadImage(with: request2) { result in + let image = result.value?.image + XCTAssertEqual(image?.nk_test_processorIDs ?? [], ["1", "2"]) + } } // Then the processor "1" is only applied once - dataLoader.queue.isSuspended = false wait { _ in XCTAssertEqual(processors.numberOfProcessorsApplied, 2) } @@ -557,12 +546,12 @@ class ImagePipelineProcessingDeduplicationTests: XCTestCase { let request2 = ImageRequest(url: Test.url, processors: [processors.make(id: "1"), processors.make(id: "2"), processors.make(id: "3")]) // When - dataLoader.queue.isSuspended = true - expect(pipeline).toLoadImage(with: request1) - expect(pipeline).toLoadImage(with: request2) + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { + expect(pipeline).toLoadImage(with: request1) + expect(pipeline).toLoadImage(with: request2) + } // Then - dataLoader.queue.isSuspended = false wait { _ in XCTAssertNotNil(cache[request1]) XCTAssertNotNil(cache[request2]) @@ -634,18 +623,18 @@ class ImagePipelineProcessingDeduplicationTests: XCTestCase { let request2 = ImageRequest(url: Test.url, processors: [processors.make(id: "1"), processors.make(id: "2")]) // When - dataLoader.queue.isSuspended = true - expect(pipeline).toLoadImage(with: request1) { result in - let image = result.value?.image - XCTAssertEqual(image?.nk_test_processorIDs ?? [], ["1"]) - } - expect(pipeline).toLoadImage(with: request2) { result in - let image = result.value?.image - XCTAssertEqual(image?.nk_test_processorIDs ?? [], ["1", "2"]) + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { + expect(pipeline).toLoadImage(with: request1) { result in + let image = result.value?.image + XCTAssertEqual(image?.nk_test_processorIDs ?? [], ["1"]) + } + expect(pipeline).toLoadImage(with: request2) { result in + let image = result.value?.image + XCTAssertEqual(image?.nk_test_processorIDs ?? [], ["1", "2"]) + } } // Then the processor "1" is applied twice - dataLoader.queue.isSuspended = false wait { _ in XCTAssertEqual(processors.numberOfProcessorsApplied, 3) } @@ -658,16 +647,15 @@ class ImagePipelineProcessingDeduplicationTests: XCTestCase { pipeline = pipeline.reconfigured { $0.dataCache = dataCache } - dataLoader.queue.isSuspended = true // When func makeRequest(options: ImageRequest.Options) -> ImageRequest { ImageRequest(urlRequest: URLRequest(url: Test.url), options: options) } - expect(pipeline).toLoadImage(with: makeRequest(options: [])) - expect(pipeline).toLoadImage(with: makeRequest(options: [.reloadIgnoringCachedData])) - pipeline.queue.sync {} - dataLoader.queue.isSuspended = false + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { + expect(pipeline).toLoadImage(with: makeRequest(options: [])) + expect(pipeline).toLoadImage(with: makeRequest(options: [.reloadIgnoringCachedData])) + } // Then wait { _ in @@ -681,17 +669,17 @@ class ImagePipelineProcessingDeduplicationTests: XCTestCase { pipeline = pipeline.reconfigured { $0.dataCache = dataCache } - dataLoader.queue.isSuspended = true // When // - One request reloading cache data, another one not func makeRequest(options: ImageRequest.Options) -> ImageRequest { ImageRequest(urlRequest: URLRequest(url: Test.url), options: options) } - expect(pipeline).toLoadImage(with: makeRequest(options: [])) - expect(pipeline).toLoadImage(with: makeRequest(options: [.reloadIgnoringCachedData])) - pipeline.queue.sync {} - dataLoader.queue.isSuspended = false + + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { + expect(pipeline).toLoadImage(with: makeRequest(options: [])) + expect(pipeline).toLoadImage(with: makeRequest(options: [.reloadIgnoringCachedData])) + } // Then wait { _ in diff --git a/Tests/NukeTests/ImagePipelineTests/ImagePipelineDataCacheTests.swift b/Tests/NukeTests/ImagePipelineTests/ImagePipelineDataCacheTests.swift index 2ad079646..9226cd0d7 100644 --- a/Tests/NukeTests/ImagePipelineTests/ImagePipelineDataCacheTests.swift +++ b/Tests/NukeTests/ImagePipelineTests/ImagePipelineDataCacheTests.swift @@ -392,7 +392,7 @@ class ImagePipelineDataCachePolicyTests: XCTestCase { } // WHEN - pipeline.registerMultipleRequests { + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [MockImageProcessor(id: "p1")])) expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url)) } @@ -479,7 +479,7 @@ class ImagePipelineDataCachePolicyTests: XCTestCase { } // WHEN - pipeline.registerMultipleRequests { + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [MockImageProcessor(id: "p1")])) expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url)) } @@ -544,7 +544,7 @@ class ImagePipelineDataCachePolicyTests: XCTestCase { } // WHEN - pipeline.registerMultipleRequests { + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url, processors: [MockImageProcessor(id: "p1")])) expect(pipeline).toLoadImage(with: ImageRequest(url: Test.url)) } diff --git a/Tests/NukeTests/ImagePipelineTests/ImagePipelineLoadDataTests.swift b/Tests/NukeTests/ImagePipelineTests/ImagePipelineLoadDataTests.swift index 66c1ddac1..961351bca 100644 --- a/Tests/NukeTests/ImagePipelineTests/ImagePipelineLoadDataTests.swift +++ b/Tests/NukeTests/ImagePipelineTests/ImagePipelineLoadDataTests.swift @@ -211,7 +211,7 @@ extension ImagePipelineLoadDataTests { } // WHEN - pipeline.registerMultipleRequests { + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { expect(pipeline).toLoadData(with: ImageRequest(url: Test.url, processors: [MockImageProcessor(id: "p1")])) expect(pipeline).toLoadData(with: ImageRequest(url: Test.url)) } @@ -275,7 +275,7 @@ extension ImagePipelineLoadDataTests { } // WHEN - pipeline.registerMultipleRequests { + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { expect(pipeline).toLoadData(with: ImageRequest(url: Test.url, processors: [MockImageProcessor(id: "p1")])) expect(pipeline).toLoadData(with: ImageRequest(url: Test.url)) } @@ -401,7 +401,7 @@ extension ImagePipelineLoadDataTests { } // WHEN - pipeline.registerMultipleRequests { + suspendDataLoading(for: pipeline, expectedRequestCount: 2) { expect(pipeline).toLoadData(with: ImageRequest(url: Test.url, processors: [MockImageProcessor(id: "p1")])) expect(pipeline).toLoadData(with: ImageRequest(url: Test.url)) } @@ -417,11 +417,17 @@ extension ImagePipelineLoadDataTests { } } -extension ImagePipeline { - func registerMultipleRequests(_ closure: () -> Void) { - configuration.dataLoadingQueue.isSuspended = true +extension XCTestCase { + func suspendDataLoading(for pipeline: ImagePipeline, expectedRequestCount count: Int, _ closure: () -> Void) { + let dataLoader = pipeline.configuration.dataLoader as! MockDataLoader + dataLoader.isSuspended = true + let expectation = self.expectation(description: "registered") + expectation.expectedFulfillmentCount = count + pipeline.onTaskStarted = { _ in + expectation.fulfill() + } closure() - queue.sync {} // Important! - configuration.dataLoadingQueue.isSuspended = false + wait(for: [expectation], timeout: 5) + dataLoader.isSuspended = false } }