Fix RetryPolicy's Handling of AFError (#3103)

* Add support for testing through SPM.

* Test on GitHub using SPM.

* Make SPM step independent.

* Make completion more deterministic.

* Remove reference to completion once run.

* Fix RetryPolicy by properly handling AFError.

* Improve test reliability.

* Update and add additional tests.

* Add delay comparison to RetryResult equality.

* Make tests more reliable.

Source/RetryPolicy.swift

@@ -324,11 +324,15 @@ open class RetryPolicy: RequestInterceptor {
     private func shouldRetry(response: HTTPURLResponse?, error: Error) -> Bool {
         if let statusCode = response?.statusCode, retryableHTTPStatusCodes.contains(statusCode) {
             return true
-        } else if let errorCode = (error as? URLError)?.code, retryableURLErrorCodes.contains(errorCode) {
-            return true
+        } else {
+            let errorCode = (error as? URLError)?.code
+            let afErrorCode = (error.asAFError?.underlyingError as? URLError)?.code
+            if let code = errorCode ?? afErrorCode, retryableURLErrorCodes.contains(code) {
+                return true
+            } else {
+                return false
+            }
         }
-
-        return false
     }
 }
 

Tests/RequestInterceptorTests.swift

@@ -500,16 +500,74 @@ final class InterceptorTestCase: BaseTestCase {
     }
 }
 
-// MARK: -
+// MARK: - Functional Tests
+
+final class InterceptorRequestTests: BaseTestCase {
+    func testThatRetryPolicyRetriesRequestTimeout() {
+        // Given
+        let interceptor = InspectorInterceptor(RetryPolicy(retryLimit: 1, exponentialBackoffScale: 0.1))
+        let urlRequest = URLRequest.makeHTTPBinRequest(path: "delay/1", timeout: 0.01)
+        let expect = expectation(description: "request completed")
+
+        // When
+        let request = AF.request(urlRequest, interceptor: interceptor).response { _ in
+            expect.fulfill()
+        }
+
+        waitForExpectations(timeout: timeout)
+
+        // Then
+        XCTAssertEqual(request.tasks.count, 2, "There should be two tasks, one original, one retry.")
+        XCTAssertEqual(interceptor.retryCalledCount, 2, "retry() should be called twice.")
+        XCTAssertEqual(interceptor.retries, [.retryWithDelay(0.1), .doNotRetry], "RetryResults should retryWithDelay, doNotRetry")
+    }
+}
+
+// MARK: - Helpers
+
+/// Class which captures the output of any underlying `RequestInterceptor`.
+final class InspectorInterceptor<Interceptor: RequestInterceptor>: RequestInterceptor {
+    var onAdaptation: ((Result<URLRequest, Error>) -> Void)?
+    var onRetry: ((RetryResult) -> Void)?
+
+    private(set) var adaptations: [Result<URLRequest, Error>] = []
+    private(set) var retries: [RetryResult] = []
+
+    /// Number of times `retry` was called.
+    var retryCalledCount: Int { return retries.count }
+
+    let interceptor: Interceptor
+
+    init(_ interceptor: Interceptor) {
+        self.interceptor = interceptor
+    }
+
+    func adapt(_ urlRequest: URLRequest, for session: Session, completion: @escaping (Result<URLRequest, Error>) -> Void) {
+        interceptor.adapt(urlRequest, for: session) { result in
+            self.adaptations.append(result)
+            completion(result)
+            self.onAdaptation?(result)
+        }
+    }
+
+    func retry(_ request: Request, for session: Session, dueTo error: Error, completion: @escaping (RetryResult) -> Void) {
+        interceptor.retry(request, for: session, dueTo: error) { result in
+            self.retries.append(result)
+            completion(result)
+            self.onRetry?(result)
+        }
+    }
+}
 
 extension RetryResult: Equatable {
     public static func ==(lhs: RetryResult, rhs: RetryResult) -> Bool {
         switch (lhs, rhs) {
         case (.retry, .retry),
-             (.retryWithDelay, .retryWithDelay),
              (.doNotRetry, .doNotRetry),
              (.doNotRetryWithError, .doNotRetryWithError):
             return true
+        case let (.retryWithDelay(leftDelay), .retryWithDelay(rightDelay)):
+            return leftDelay == rightDelay
         default:
             return false
         }

Tests/RequestTests.swift

@@ -313,7 +313,7 @@ final class RequestResponseTestCase: BaseTestCase {
         let session = Session(eventMonitors: [eventMonitor])
 
         let expect = expectation(description: "request should receive appropriate lifetime events")
-        expect.expectedFulfillmentCount = 3
+        expect.expectedFulfillmentCount = 4
 
         eventMonitor.requestDidResumeTask = { _, _ in expect.fulfill() }
         eventMonitor.requestDidResume = { _ in expect.fulfill() }
@@ -325,7 +325,7 @@ final class RequestResponseTestCase: BaseTestCase {
         eventMonitor.requestDidCancelTask = { _, _ in expect.fulfill() }
 
         // When
-        let request = session.request(URLRequest.makeHTTPBinRequest())
+        let request = session.request(URLRequest.makeHTTPBinRequest()).response { _ in expect.fulfill() }
         for _ in 0..<100 {
             request.resume()
         }
@@ -371,7 +371,7 @@ final class RequestResponseTestCase: BaseTestCase {
         let session = Session(startRequestsImmediately: false, eventMonitors: [eventMonitor])
 
         let expect = expectation(description: "request should receive appropriate lifetime events")
-        expect.expectedFulfillmentCount = 3
+        expect.expectedFulfillmentCount = 4
 
         eventMonitor.requestDidResumeTask = { _, _ in expect.fulfill() }
         eventMonitor.requestDidResume = { _ in expect.fulfill() }
@@ -383,7 +383,7 @@ final class RequestResponseTestCase: BaseTestCase {
         eventMonitor.requestDidCancelTask = { _, _ in expect.fulfill() }
 
         // When
-        let request = session.request(URLRequest.makeHTTPBinRequest())
+        let request = session.request(URLRequest.makeHTTPBinRequest()).response { _ in expect.fulfill() }
         for _ in 0..<100 {
             request.resume()
         }

Tests/RetryPolicyTests.swift

@@ -29,7 +29,7 @@ import XCTest
 class BaseRetryPolicyTestCase: BaseTestCase {
     // MARK: Helper Types
 
-    class StubRequest: DataRequest {
+    final class StubRequest: DataRequest {
         let urlRequest: URLRequest
         override var request: URLRequest? { return urlRequest }
 
@@ -65,9 +65,14 @@ class BaseRetryPolicyTestCase: BaseTestCase {
     let session = Session(startRequestsImmediately: false)
 
     let url = URL(string: "https://api.alamofire.org")!
-    let connectionLostError = NSError(domain: URLError.errorDomain, code: URLError.networkConnectionLost.rawValue, userInfo: nil)
-    let resourceUnavailableError = NSError(domain: URLError.errorDomain, code: URLError.resourceUnavailable.rawValue, userInfo: nil)
-    let unknownError = NSError(domain: URLError.errorDomain, code: URLError.unknown.rawValue, userInfo: nil)
+
+    let connectionLost = URLError(.networkConnectionLost)
+    let resourceUnavailable = URLError(.resourceUnavailable)
+    let unknown = URLError(.unknown)
+
+    lazy var connectionLostError = AFError.sessionTaskFailed(error: connectionLost)
+    lazy var resourceUnavailableError = AFError.sessionTaskFailed(error: resourceUnavailable)
+    lazy var unknownError = AFError.sessionTaskFailed(error: unknown)
 
     let retryableStatusCodes: Set<Int> = [408, 500, 502, 503, 504]
 
@@ -128,7 +133,7 @@ class BaseRetryPolicyTestCase: BaseTestCase {
 
 // MARK: -
 
-class RetryPolicyTestCase: BaseRetryPolicyTestCase {
+final class RetryPolicyTestCase: BaseRetryPolicyTestCase {
     // MARK: Tests - Retry
 
     func testThatRetryPolicyRetriesRequestsBelowRetryLimit() {
@@ -235,7 +240,37 @@ class RetryPolicyTestCase: BaseRetryPolicyTestCase {
         // When
         for code in errorCodes {
             let request = self.request(method: .get)
-            let error = urlError(with: code)
+            let error = URLError(code)
+
+            let expectation = self.expectation(description: "retry policy should complete")
+
+            retryPolicy.retry(request, for: session, dueTo: error) { result in
+                results[code] = result
+                expectation.fulfill()
+            }
+
+            waitForExpectations(timeout: timeout, handler: nil)
+        }
+
+        // Then
+        XCTAssertEqual(results.count, errorCodes.count)
+
+        for (urlErrorCode, result) in results {
+            XCTAssertEqual(result.retryRequired, retryableErrorCodes.contains(urlErrorCode))
+            XCTAssertEqual(result.delay, result.retryRequired ? 0.5 : nil)
+            XCTAssertNil(result.error)
+        }
+    }
+
+    func testThatRetryPolicyRetriesRequestsWithRetryableAFErrors() {
+        // Given
+        let retryPolicy = RetryPolicy()
+        var results: [URLError.Code: RetryResult] = [:]
+
+        // When
+        for code in errorCodes {
+            let request = self.request(method: .get)
+            let error = AFError.sessionTaskFailed(error: URLError(code))
 
             let expectation = self.expectation(description: "retry policy should complete")
 
@@ -257,12 +292,14 @@ class RetryPolicyTestCase: BaseRetryPolicyTestCase {
         }
     }
 
-    func testThatRetryPolicyDoesNotRetryErrorsThatAreNotURLErrors() {
+    func testThatRetryPolicyDoesNotRetryErrorsThatAreNotRetryable() {
         // Given
         let retryPolicy = RetryPolicy()
         let request = self.request(method: .get)
 
-        let errors: [Error] = [resourceUnavailableError,
+        let errors: [Error] = [resourceUnavailable,
+                               unknown,
+                               resourceUnavailableError,
                                unknownError]
 
         var results: [RetryResult] = []
@@ -283,7 +320,7 @@ class RetryPolicyTestCase: BaseRetryPolicyTestCase {
         XCTAssertEqual(results.count, errors.count)
 
         for result in results {
-            XCTAssertEqual(result.retryRequired, false)
+            XCTAssertFalse(result.retryRequired)
             XCTAssertNil(result.delay)
             XCTAssertNil(result.error)
         }
@@ -349,15 +386,11 @@ class RetryPolicyTestCase: BaseRetryPolicyTestCase {
 
         return StubRequest(url, method: method, response: response, session: session)
     }
-
-    func urlError(with code: URLError.Code) -> URLError {
-        return NSError(domain: URLError.errorDomain, code: code.rawValue, userInfo: nil) as! URLError
-    }
 }
 
 // MARK: -
 
-class ConnectionLostRetryPolicyTestCase: BaseRetryPolicyTestCase {
+final class ConnectionLostRetryPolicyTestCase: BaseRetryPolicyTestCase {
     func testThatConnectionLostRetryPolicyCanBeInitializedWithDefaultValues() {
         // Given, When
         let retryPolicy = ConnectionLostRetryPolicy()

Tests/SessionTests.swift

@@ -1576,8 +1576,10 @@ final class SessionCancellationTestCase: BaseTestCase {
                 hasRetried = true
             }
         }
-        let monitor = ClosureEventMonitor()
-        let session = Session(interceptor: OnceRetrier(), eventMonitors: [monitor])
+
+        let queue = DispatchQueue(label: "com.alamofire.testQueue")
+        let monitor = ClosureEventMonitor(queue: queue)
+        let session = Session(rootQueue: queue, interceptor: OnceRetrier(), eventMonitors: [monitor])
         let request = URLRequest.makeHTTPBinRequest(path: "status/401")
         let completion = expectation(description: "all requests should finish")
         let cancellation = expectation(description: "cancel all requests should be called")