From 38b620a99c3667a2fa00bce7495643f83ba74614 Mon Sep 17 00:00:00 2001 From: Okhan Okbay Date: Tue, 19 Mar 2024 13:22:45 +0000 Subject: [PATCH 1/2] Improve error type definitions for more clarity --- .../CheckoutNetwork-Package.xcscheme | 93 +++++++++++++++++++ .../CheckoutNetworkFakeClient.xcscheme | 67 +++++++++++++ .../xcschemes/CheckoutNetworkTests.xcscheme | 54 +++++++++++ .../CheckoutClientInterface.swift | 6 +- .../CheckoutNetworkClient+AsyncWrappers.swift | 4 +- .../CheckoutNetworkClient+ErrorHandling.swift | 2 +- .../CheckoutNetworkClient.swift | 9 +- .../CheckoutNetworkError.swift | 10 +- .../AsyncWrapperTests.swift | 28 +++--- .../CheckoutNetworkClientTests.swift | 44 ++++----- .../Helpers/CheckoutNetworkClientSpy.swift | 2 +- .../Helpers/FakeError.swift | 12 --- 12 files changed, 271 insertions(+), 60 deletions(-) create mode 100644 .swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetwork-Package.xcscheme create mode 100644 .swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetworkFakeClient.xcscheme create mode 100644 .swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetworkTests.xcscheme delete mode 100644 Tests/CheckoutNetworkTests/Helpers/FakeError.swift diff --git a/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetwork-Package.xcscheme b/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetwork-Package.xcscheme new file mode 100644 index 0000000..0d3dc69 --- /dev/null +++ b/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetwork-Package.xcscheme @@ -0,0 +1,93 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetworkFakeClient.xcscheme b/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetworkFakeClient.xcscheme new file mode 100644 index 0000000..893fd80 --- /dev/null +++ b/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetworkFakeClient.xcscheme @@ -0,0 +1,67 @@ + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetworkTests.xcscheme b/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetworkTests.xcscheme new file mode 100644 index 0000000..f37b2eb --- /dev/null +++ b/.swiftpm/xcode/xcshareddata/xcschemes/CheckoutNetworkTests.xcscheme @@ -0,0 +1,54 @@ + + + + + + + + + + + + + + + + + + + + + diff --git a/Sources/CheckoutNetwork/CheckoutClientInterface.swift b/Sources/CheckoutNetwork/CheckoutClientInterface.swift index 527e8de..2f66029 100644 --- a/Sources/CheckoutNetwork/CheckoutClientInterface.swift +++ b/Sources/CheckoutNetwork/CheckoutClientInterface.swift @@ -11,10 +11,10 @@ import Foundation public protocol CheckoutClientInterface { /// Completion handler that will return a result containing a decodable object or an error - typealias CompletionHandler = ((Result) -> Void) - + typealias CompletionHandler = ((Result) -> Void) + /// Completion handler that will return errors if it fails or nothing if it completes as expected - typealias NoDataResponseCompletionHandler = ((Error?) -> Void) + typealias NoDataResponseCompletionHandler = ((CheckoutNetworkError?) -> Void) // MARK: Traditional Base Methods diff --git a/Sources/CheckoutNetwork/CheckoutNetworkClient+AsyncWrappers.swift b/Sources/CheckoutNetwork/CheckoutNetworkClient+AsyncWrappers.swift index 56b648e..d576399 100644 --- a/Sources/CheckoutNetwork/CheckoutNetworkClient+AsyncWrappers.swift +++ b/Sources/CheckoutNetwork/CheckoutNetworkClient+AsyncWrappers.swift @@ -11,7 +11,7 @@ public extension CheckoutNetworkClient { func runRequest(with configuration: RequestConfiguration) async throws -> T { return try await withCheckedThrowingContinuation { continuation in - runRequest(with: configuration) { (result: Result) in + runRequest(with: configuration) { (result: Result) in switch result { case .success(let response): continuation.resume(returning: response) @@ -24,7 +24,7 @@ public extension CheckoutNetworkClient { func runRequest(with configuration: RequestConfiguration) async throws { return try await withCheckedThrowingContinuation { continuation in - runRequest(with: configuration) { (error: Error?) in + runRequest(with: configuration) { (error: CheckoutNetworkError?) in guard let error = error else { continuation.resume(returning: Void()) diff --git a/Sources/CheckoutNetwork/CheckoutNetworkClient+ErrorHandling.swift b/Sources/CheckoutNetwork/CheckoutNetworkClient+ErrorHandling.swift index ce68c51..3170801 100644 --- a/Sources/CheckoutNetwork/CheckoutNetworkClient+ErrorHandling.swift +++ b/Sources/CheckoutNetwork/CheckoutNetworkClient+ErrorHandling.swift @@ -8,7 +8,7 @@ import Foundation extension CheckoutNetworkClient { - func getErrorFromResponse(_ response: URLResponse?, data: Data?) -> Error? { + func getErrorFromResponse(_ response: URLResponse?, data: Data?) -> CheckoutNetworkError? { guard let response = response as? HTTPURLResponse else { return CheckoutNetworkError.invalidURLResponse } diff --git a/Sources/CheckoutNetwork/CheckoutNetworkClient.swift b/Sources/CheckoutNetwork/CheckoutNetworkClient.swift index 5165837..63fa36d 100644 --- a/Sources/CheckoutNetwork/CheckoutNetworkClient.swift +++ b/Sources/CheckoutNetwork/CheckoutNetworkClient.swift @@ -32,12 +32,12 @@ public class CheckoutNetworkClient: CheckoutClientInterface { self?.tasks.removeValue(forKey: taskID) guard let self = self else { return } if let error = error { - completionHandler(.failure(self.convertDataTaskErrorsToCheckoutNetworkError(error: error))) + completionHandler(.failure(self.convertDataTaskErrorsToCheckoutNetworkError(error: error))) return } guard let data = data else { - completionHandler(.failure(CheckoutNetworkError.noDataResponseReceived)) + completionHandler(.failure(.noDataResponseReceived)) return } @@ -53,7 +53,7 @@ public class CheckoutNetworkClient: CheckoutClientInterface { completionHandler(.success(dataResponse)) } catch { - completionHandler(.failure(error)) + completionHandler(.failure(.decoding(errorDescription: error.localizedDescription))) } } self.tasks[taskID] = task @@ -69,8 +69,9 @@ public class CheckoutNetworkClient: CheckoutClientInterface { let task = session.dataTask(with: request) { [weak self] data, response, error in self?.tasks.removeValue(forKey: taskID) guard let self = self else { return } + if let error = error { - completionHandler(error) + completionHandler(self.convertDataTaskErrorsToCheckoutNetworkError(error: error)) return } if let responseError = self.getErrorFromResponse(response, data: data) { diff --git a/Sources/CheckoutNetwork/CheckoutNetworkError.swift b/Sources/CheckoutNetwork/CheckoutNetworkError.swift index 0d31c36..62b7c79 100644 --- a/Sources/CheckoutNetwork/CheckoutNetworkError.swift +++ b/Sources/CheckoutNetwork/CheckoutNetworkError.swift @@ -15,10 +15,15 @@ public enum CheckoutNetworkError: LocalizedError, Equatable { /// When a response could not be bound to an instance of HTTPURLResponse case invalidURLResponse + /// A decoding error has been received while decoding the API response + case decoding(errorDescription: String) + /// Network response was not in the 200 range case unexpectedHTTPResponse(code: Int) - /// Network call and completion appear valid but no data was returned making the parsing impossible. Use runRequest method with NoDataResponseCompletionHandler if no data is expected (HTTP 204 is a success case with no content being returned) + /// Network call and completion appear valid but no data was returned making the parsing impossible. + /// Use runRequest method with NoDataResponseCompletionHandler if no data is expected + /// Refresher: HTTP 204 is a success case with no content being returned case noDataResponseReceived /// Network response returned with HTTP Code 422 @@ -48,6 +53,9 @@ public enum CheckoutNetworkError: LocalizedError, Equatable { case .invalidURLResponse: return "Could not instantiate an HTTPURLResponse with the received response value" + case .decoding(let errorDescription): + return errorDescription + case .unexpectedHTTPResponse(let code): return "Received an unexpected HTTP response: \(code)" diff --git a/Tests/CheckoutNetworkTests/AsyncWrapperTests.swift b/Tests/CheckoutNetworkTests/AsyncWrapperTests.swift index 1c418ed..7b25d34 100644 --- a/Tests/CheckoutNetworkTests/AsyncWrapperTests.swift +++ b/Tests/CheckoutNetworkTests/AsyncWrapperTests.swift @@ -18,7 +18,7 @@ extension AsyncWrapperTests { fakeSession.calledDataTasksReturn = fakeDataTask let client = CheckoutNetworkClientSpy(session: fakeSession) let testConfig = try! RequestConfiguration(path: FakePath.testServices) - + let expectedResponseBody = FakeObject(id: "some response") client.expectedResponseBody = expectedResponseBody client.expectedError = nil @@ -27,25 +27,25 @@ extension AsyncWrapperTests { XCTAssertEqual(client.runRequestCallCount, 1) XCTAssertEqual(responseBody, expectedResponseBody) } - + func test_whenRunRequestReturnsError_ThenAsyncRunRequestPropagatesIt() async throws { let fakeSession = FakeSession() let fakeDataTask = FakeDataTask() fakeSession.calledDataTasksReturn = fakeDataTask let client = CheckoutNetworkClientSpy(session: fakeSession) let testConfig = try! RequestConfiguration(path: FakePath.testServices) - - let expectedError = FakeError.someError + + let expectedError = CheckoutNetworkError.other(underlyingError: .init(domain: "some_error", code: 1)) client.expectedResponseBody = nil client.expectedError = expectedError - + do { let _: FakeObject = try await client.runRequest(with: testConfig) XCTFail("An error was expected to be thrown") - } catch let error as FakeError { + } catch let error { XCTAssertEqual(client.configuration.request, testConfig.request) XCTAssertEqual(client.runRequestCallCount, 1) - XCTAssertEqual(error, expectedError) + XCTAssertEqual(error as? CheckoutNetworkError, expectedError) } } } @@ -59,7 +59,7 @@ extension AsyncWrapperTests { fakeSession.calledDataTasksReturn = fakeDataTask let client = CheckoutNetworkClientSpy(session: fakeSession) let testConfig = try! RequestConfiguration(path: FakePath.testServices) - + client.expectedResponseBody = nil client.expectedError = nil do { @@ -70,25 +70,25 @@ extension AsyncWrapperTests { XCTFail("Should have not thrown an error \(error)") } } - + func test_whenRunRequestWithNoDataReturnsError_ThenAsyncRunRequestPropagatesIt() async throws { let fakeSession = FakeSession() let fakeDataTask = FakeDataTask() fakeSession.calledDataTasksReturn = fakeDataTask let client = CheckoutNetworkClientSpy(session: fakeSession) let testConfig = try! RequestConfiguration(path: FakePath.testServices) - - let expectedError = FakeError.someError + + let expectedError = CheckoutNetworkError.other(underlyingError: .init(domain: "some_error", code: 1)) client.expectedResponseBody = nil client.expectedError = expectedError - + do { try await client.runRequest(with: testConfig) XCTFail("An error was expected to be thrown") - } catch let error as FakeError { + } catch let error { XCTAssertEqual(client.configuration.request, testConfig.request) XCTAssertEqual(client.runRequestCallCount, 1) - XCTAssertEqual(error, expectedError) + XCTAssertEqual(error as? CheckoutNetworkError, expectedError) } } } diff --git a/Tests/CheckoutNetworkTests/CheckoutNetworkClientTests.swift b/Tests/CheckoutNetworkTests/CheckoutNetworkClientTests.swift index 5a73681..ec68d93 100644 --- a/Tests/CheckoutNetworkTests/CheckoutNetworkClientTests.swift +++ b/Tests/CheckoutNetworkTests/CheckoutNetworkClientTests.swift @@ -31,7 +31,7 @@ final class CheckoutNetworkClientTests: XCTestCase { XCTAssertTrue(client.tasks.isEmpty) let testConfig = try! RequestConfiguration(path: FakePath.testServices) - client.runRequest(with: testConfig) { (result: Result) in } + client.runRequest(with: testConfig) { (result: Result) in } XCTAssertEqual(client.tasks.count, 1) XCTAssertTrue(client.tasks.values.first === fakeDataTask) @@ -47,7 +47,7 @@ final class CheckoutNetworkClientTests: XCTestCase { let numberOfTasksToCreate = 1000 (0..) in } + client.runRequest(with: testConfig) { (result: Result) in } } XCTAssertEqual(client.tasks.count, numberOfTasksToCreate) XCTAssertEqual(fakeSession.calledDataTasks.count, numberOfTasksToCreate) @@ -64,13 +64,13 @@ final class CheckoutNetworkClientTests: XCTestCase { let expectedError = NSError(domain: "fail", code: 12345) let expect = expectation(description: "Ensure completion handler is called") - client.runRequest(with: testConfig) { (result: Result) in + client.runRequest(with: testConfig) { (result: Result) in expect.fulfill() switch result { case .success(_): XCTFail("Test expects a specific error to be returned") case .failure(let failure): - XCTAssertEqual(failure as? CheckoutNetworkError, CheckoutNetworkError.other(underlyingError: expectedError)) + XCTAssertEqual(failure, CheckoutNetworkError.other(underlyingError: expectedError)) } } @@ -94,13 +94,13 @@ final class CheckoutNetworkClientTests: XCTestCase { headerFields: nil) let expect = expectation(description: "Ensure completion handler is called") - client.runRequest(with: testConfig) { (result: Result) in + client.runRequest(with: testConfig) { (result: Result) in expect.fulfill() switch result { case .success(_): XCTFail("Test expects a specific error to be returned") case .failure(let failure): - XCTAssertEqual(failure as? CheckoutNetworkError, CheckoutNetworkError.unexpectedHTTPResponse(code: testResponseCode)) + XCTAssertEqual(failure, CheckoutNetworkError.unexpectedHTTPResponse(code: testResponseCode)) } } @@ -120,13 +120,13 @@ final class CheckoutNetworkClientTests: XCTestCase { let expectedResponse = URLResponse() let expect = expectation(description: "Ensure completion handler is called") - client.runRequest(with: testConfig) { (result: Result) in + client.runRequest(with: testConfig) { (result: Result) in expect.fulfill() switch result { case .success(_): XCTFail("Test expects a specific error to be returned") case .failure(let failure): - XCTAssertEqual(failure as? CheckoutNetworkError, CheckoutNetworkError.invalidURLResponse) + XCTAssertEqual(failure, CheckoutNetworkError.invalidURLResponse) } } @@ -148,13 +148,13 @@ final class CheckoutNetworkClientTests: XCTestCase { headerFields: nil) let expect = expectation(description: "Ensure completion handler is called") - client.runRequest(with: testConfig) { (result: Result) in + client.runRequest(with: testConfig) { (result: Result) in expect.fulfill() switch result { case .success(_): XCTFail("Test expects a specific error to be returned") case .failure(let failure): - XCTAssertEqual(failure as? CheckoutNetworkError, CheckoutNetworkError.noDataResponseReceived) + XCTAssertEqual(failure, CheckoutNetworkError.noDataResponseReceived) } } @@ -177,7 +177,7 @@ final class CheckoutNetworkClientTests: XCTestCase { headerFields: nil) let expect = expectation(description: "Ensure completion handler is called") - client.runRequest(with: testConfig) { (result: Result) in + client.runRequest(with: testConfig) { (result: Result) in expect.fulfill() switch result { case .success(_): @@ -211,14 +211,13 @@ final class CheckoutNetworkClientTests: XCTestCase { headerFields: nil) let expect = expectation(description: "Ensure completion handler is called") - client.runRequest(with: testConfig) { (result: Result) in + client.runRequest(with: testConfig) { (result: Result) in expect.fulfill() switch result { case .success(_): XCTFail("Test expects a specific error to be returned") case .failure(let failure): - let decodingError = failure as? DecodingError - XCTAssertNotNil(decodingError) + XCTAssertEqual(failure, CheckoutNetworkError.decoding(errorDescription: "The data couldn’t be read because it is missing.")) } } @@ -242,7 +241,7 @@ final class CheckoutNetworkClientTests: XCTestCase { headerFields: nil) let expect = expectation(description: "Ensure completion handler is called") - client.runRequest(with: testConfig) { (result: Result) in + client.runRequest(with: testConfig) { (result: Result) in expect.fulfill() switch result { case .success(let receivedObject): @@ -267,17 +266,18 @@ final class CheckoutNetworkClientTests: XCTestCase { let expectedData = "nothing".data(using: .utf8) let expectedResponse = URLResponse() - let expectedError = NSError(domain: "fail", code: 12345) - + let underlyingError = NSError(domain: "fail", code: 12345) + let expectedError = CheckoutNetworkError.other(underlyingError: underlyingError) + let expect = expectation(description: "Ensure completion handler is called") - client.runRequest(with: testConfig) { + client.runRequest(with: testConfig) { (error: CheckoutNetworkError?) in expect.fulfill() - XCTAssertEqual($0 as? NSError, expectedError) + XCTAssertEqual(error, expectedError) } XCTAssertFalse(client.tasks.isEmpty) let requestCompletion = fakeSession.calledDataTasks.first!.completion - requestCompletion(expectedData, expectedResponse, expectedError) + requestCompletion(expectedData, expectedResponse, underlyingError) XCTAssertTrue(client.tasks.isEmpty) waitForExpectations(timeout: 1) } @@ -297,7 +297,7 @@ final class CheckoutNetworkClientTests: XCTestCase { let expect = expectation(description: "Ensure completion handler is called") client.runRequest(with: testConfig) { expect.fulfill() - XCTAssertEqual($0 as? CheckoutNetworkError, CheckoutNetworkError.unexpectedHTTPResponse(code: testResponseCode)) + XCTAssertEqual($0, CheckoutNetworkError.unexpectedHTTPResponse(code: testResponseCode)) } XCTAssertFalse(client.tasks.isEmpty) @@ -318,7 +318,7 @@ final class CheckoutNetworkClientTests: XCTestCase { let expect = expectation(description: "Ensure completion handler is called") client.runRequest(with: testConfig) { expect.fulfill() - XCTAssertEqual($0 as? CheckoutNetworkError, CheckoutNetworkError.invalidURLResponse) + XCTAssertEqual($0, CheckoutNetworkError.invalidURLResponse) } XCTAssertFalse(client.tasks.isEmpty) diff --git a/Tests/CheckoutNetworkTests/Helpers/CheckoutNetworkClientSpy.swift b/Tests/CheckoutNetworkTests/Helpers/CheckoutNetworkClientSpy.swift index 522be58..1fe8e8d 100644 --- a/Tests/CheckoutNetworkTests/Helpers/CheckoutNetworkClientSpy.swift +++ b/Tests/CheckoutNetworkTests/Helpers/CheckoutNetworkClientSpy.swift @@ -12,7 +12,7 @@ class CheckoutNetworkClientSpy: CheckoutNetworkClient { private(set) var configuration: RequestConfiguration! var expectedResponseBody: FakeObject? - var expectedError: Error? + var expectedError: CheckoutNetworkError? override func runRequest(with configuration: RequestConfiguration, completionHandler: @escaping CheckoutNetworkClient.CompletionHandler) where T : Decodable { runRequestCallCount += 1 diff --git a/Tests/CheckoutNetworkTests/Helpers/FakeError.swift b/Tests/CheckoutNetworkTests/Helpers/FakeError.swift deleted file mode 100644 index cd5ddb3..0000000 --- a/Tests/CheckoutNetworkTests/Helpers/FakeError.swift +++ /dev/null @@ -1,12 +0,0 @@ -// -// FakeError.swift -// -// -// Created by Okhan Okbay on 19/01/2024. -// - -import Foundation - -enum FakeError: Error { - case someError -} From 136a34f732a50cce4941d1c371271a2377b653d9 Mon Sep 17 00:00:00 2001 From: Okhan Okbay Date: Mon, 2 Dec 2024 22:24:14 +0000 Subject: [PATCH 2/2] Remove linter requirement for tests to run --- .github/workflows/verify-pr.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/verify-pr.yml b/.github/workflows/verify-pr.yml index 47e3607..90670f9 100644 --- a/.github/workflows/verify-pr.yml +++ b/.github/workflows/verify-pr.yml @@ -26,7 +26,6 @@ jobs: verify-pr: name: Verify PR runs-on: macos-14-xlarge - needs: lint steps: - name: Checkout repository