diff --git a/Sources/Crashes/CrashCollection.swift b/Sources/Crashes/CrashCollection.swift index 71d242abd..e553fc218 100644 --- a/Sources/Crashes/CrashCollection.swift +++ b/Sources/Crashes/CrashCollection.swift @@ -20,6 +20,7 @@ import Foundation import MetricKit import Persistence import os.log +import Common public enum CrashCollectionPlatform { case iOS, macOS, macOSAppStore @@ -40,7 +41,8 @@ public enum CrashCollectionPlatform { @available(iOS 13, macOS 12, *) public final class CrashCollection { - public init(crashReportSender: CrashReportSending, crashCollectionStorage: KeyValueStoring = UserDefaults()) { + public init(crashReportSender: CrashReportSending, + crashCollectionStorage: KeyValueStoring = UserDefaults()) { self.crashHandler = CrashHandler() self.crashSender = crashReportSender self.crashCollectionStorage = crashCollectionStorage @@ -232,12 +234,10 @@ public class CRCIDManager { Logger.general.debug("Crash Collection - Received matching value for CRCID: \(receivedCRCID), no update necessary") } } else { - // TODO: Pixel: crashreporting_crcid-missing Logger.general.debug("Crash Collection - No value for CRCID header: \(CRCIDManager.crcidKey), clearing local crcid value if present") crcid = nil } case .failure(let failure): - // TODO: Pixel: crashreporting_submission-failed Logger.general.debug("Crash Collection - Sending Crash Report: failed (\(failure))") } } diff --git a/Sources/Crashes/CrashReportSender.swift b/Sources/Crashes/CrashReportSender.swift index e284460dc..176996c8d 100644 --- a/Sources/Crashes/CrashReportSender.swift +++ b/Sources/Crashes/CrashReportSender.swift @@ -18,15 +18,20 @@ import Foundation import MetricKit +import Common public protocol CrashReportSending { - init(platform: CrashCollectionPlatform) + var pixelEvents: EventMapping? { get } // TODO: This should _not_ be optional + + init(platform: CrashCollectionPlatform, pixelEvents: EventMapping?) + func send(_ crashReportData: Data, crcid: String?) async -> (result: Result, response: HTTPURLResponse?) func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result, _ response: HTTPURLResponse?) -> Void) } -enum CrashReportSenderError: Error { - case invalidResponse +public enum CrashReportSenderError: Error { + case noCRCID + case submissionFailed(HTTPURLResponse?) } // By conforming to a protocol, we can sub in mocks more easily @@ -36,11 +41,13 @@ public final class CrashReportSender: CrashReportSending { static let httpHeaderCRCID = "crcid" public let platform: CrashCollectionPlatform + public var pixelEvents: EventMapping? private let session = URLSession(configuration: .ephemeral) - public init(platform: CrashCollectionPlatform) { + public init(platform: CrashCollectionPlatform, pixelEvents: EventMapping?) { self.platform = platform + self.pixelEvents = pixelEvents } public func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result, _ response: HTTPURLResponse?) -> Void) { @@ -60,6 +67,10 @@ public final class CrashReportSender: CrashReportSending { Logger.general.debug("CrashReportSender: Received HTTP response code: \(response.statusCode)") if response.statusCode == 200 { response.allHeaderFields.forEach { print("\($0.key): \($0.value)") } // TODO: Why do we straight-up print these, rather than debug logging? + if response.allHeaderFields[CrashReportSender.httpHeaderCRCID] == nil { + let crashReportError = CrashReportSenderError.noCRCID + self.pixelEvents?.fire(crashReportError) + } } else { assertionFailure("CrashReportSender: Failed to send the crash report: \(response.statusCode)") } @@ -67,12 +78,18 @@ public final class CrashReportSender: CrashReportSending { if let data { completion(.success(data), response) } else if let error { + let crashReportError = CrashReportSenderError.submissionFailed(response) + self.pixelEvents?.fire(crashReportError) completion(.failure(error), response) } else { - completion(.failure(CrashReportSenderError.invalidResponse), response) + let crashReportError = CrashReportSenderError.submissionFailed(response) + self.pixelEvents?.fire(crashReportError) + completion(.failure(crashReportError), response) } } else { - completion(.failure(CrashReportSenderError.invalidResponse), nil) + let crashReportError = CrashReportSenderError.submissionFailed(nil) + self.pixelEvents?.fire(crashReportError) + completion(.failure(crashReportError), nil) } } task.resume() diff --git a/Tests/CrashesTests/CrashCollectionTests.swift b/Tests/CrashesTests/CrashCollectionTests.swift index 3bd5b2dfc..a4c4706d4 100644 --- a/Tests/CrashesTests/CrashCollectionTests.swift +++ b/Tests/CrashesTests/CrashCollectionTests.swift @@ -21,11 +21,14 @@ import MetricKit import XCTest import Persistence import TestUtils +import Common class CrashCollectionTests: XCTestCase { func testFirstCrashFlagSent() { - let crashCollection = CrashCollection(crashReportSender: CrashReportSender(platform: .iOS), crashCollectionStorage: MockKeyValueStore()) + let crashReportSender = CrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: MockKeyValueStore()) // 2 pixels with first = true attached XCTAssertTrue(crashCollection.isFirstCrash) crashCollection.start { pixelParameters, _, _ in @@ -42,7 +45,9 @@ class CrashCollectionTests: XCTestCase { } func testSubsequentPixelsDontSendFirstFlag() { - let crashCollection = CrashCollection(crashReportSender: CrashReportSender(platform: .iOS), crashCollectionStorage: MockKeyValueStore()) + let crashReportSender = CrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: MockKeyValueStore()) // 2 pixels with no first parameter crashCollection.isFirstCrash = false crashCollection.start { pixelParameters, _, _ in @@ -62,9 +67,10 @@ class CrashCollectionTests: XCTestCase { let responseCRCIDValue = "CRCID Value" let store = MockKeyValueStore() - let crashReportSender = MockCrashReportSender(platform: .iOS) + let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil) crashReportSender.responseCRCID = responseCRCIDValue - let crashCollection = CrashCollection(crashReportSender: crashReportSender, crashCollectionStorage: store) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: store) let expectation = self.expectation(description: "Crash collection response") // Set up closures on our CrashCollection object @@ -93,8 +99,9 @@ class CrashCollectionTests: XCTestCase { func testCRCIDIsClearedWhenServerReturnsSuccessWithNoCRCID() { let store = MockKeyValueStore() - let crashReportSender = MockCrashReportSender(platform: .iOS) - let crashCollection = CrashCollection(crashReportSender: crashReportSender, crashCollectionStorage: store) + let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: store) let expectation = self.expectation(description: "Crash collection response") // Set up closures on our CrashCollection object @@ -123,8 +130,9 @@ class CrashCollectionTests: XCTestCase { func testCRCIDIsRetainedWhenServerErrorIsReceived() { let store = MockKeyValueStore() - let crashReportSender = MockCrashReportSender(platform: .iOS) - let crashCollection = CrashCollection(crashReportSender: crashReportSender, crashCollectionStorage: store) + let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: store) let expectation = self.expectation(description: "Crash collection response") // Set up closures on our CrashCollection object @@ -151,7 +159,41 @@ class CrashCollectionTests: XCTestCase { XCTAssertEqual(store.object(forKey: CRCIDManager.crcidKey) as? String, crcid) } - + + // TODO: This test doesn't actually test CrashCollection, it's testing the MockCrashReportSender + // Useful for debugging, but not actually a good test, and should be removed. + func testInvalidResponsePixelIsSent() { + let store = MockKeyValueStore() + let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: store) + let expectation = self.expectation(description: "Crash collection response") + + // Set up closures on our CrashCollection object + crashCollection.start(process: {_ in + return ["fake-crash-data".data(using: .utf8)!] // Not relevant to this test + }) { pixelParameters, payloads, uploadReports in + uploadReports() + } didFinishHandlingResponse: { + expectation.fulfill() + } + + // Execute crash collection (which will call our mocked CrashReportSender as well) + let crcid = "Initial CRCID Value" + store.set(crcid, forKey: CRCIDManager.crcidKey) + crashReportSender.responseStatusCode = 500 + crashCollection.crashHandler.didReceive([ + MockPayload(mockCrashes: [ + MXCrashDiagnostic(), + MXCrashDiagnostic() + ]) + ]) + + self.wait(for: [expectation, crashReportSender.submissionFailedExpectation], timeout: 3) + + XCTAssertEqual(store.object(forKey: CRCIDManager.crcidKey) as? String, crcid) + } + func testCRCIDIsSentToServer() { // TODO: Requires ability to inspect outbound HTTP request } @@ -184,9 +226,27 @@ class MockCrashReportSender: CrashReportSending { let platform: CrashCollectionPlatform var responseCRCID: String? var responseStatusCode = 200 - - required init(platform: CrashCollectionPlatform) { + + // Pixel handling + var noCRCIDExpectation = XCTestExpectation(description: "No CRCID Expectation") + var submissionFailedExpectation = XCTestExpectation(description: "Submission Failed Expectation") + + var pixelEvents: EventMapping? + + required init(platform: CrashCollectionPlatform, pixelEvents: EventMapping?) { self.platform = platform + + self.pixelEvents = .init { event, _, _, _ in + switch event { + case CrashReportSenderError.noCRCID: + self.noCRCIDExpectation.fulfill() + return + + case CrashReportSenderError.submissionFailed(_): + self.submissionFailedExpectation.fulfill() + return + } + } } func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result, _ response: HTTPURLResponse?) -> Void) { @@ -204,11 +264,12 @@ class MockCrashReportSender: CrashReportSending { } if responseStatusCode == 200 { - completion(.success(nil), response) + completion(.success(nil), response) // Success with nil data } else { - completion(.failure(CrashReportSenderError.invalidResponse), response) + let crashReportError = CrashReportSenderError.submissionFailed(response) + self.pixelEvents?.fire(crashReportError) + completion(.failure(CrashReportSenderError.submissionFailed(response)), response) } - } func send(_ crashReportData: Data, crcid: String?) async -> (result: Result, response: HTTPURLResponse?) {