Fixing Flaky Test in UnsentDataHandlerTests (#207)

* Fixing flaky test

* Increased timeout

* Fixed issue with the logic of the modified test

* Movied test expectation before the actual condition happens so there're no race conditions

* Fixed EmbraceConfig tests: they werent mocking the URL properly

* Waiting for the condition that is failing (which seems logical as happens async)

* Waiting for the cleanup of the cache

* Fixed test that started to fail

Tests/EmbraceConfigTests/EmbraceConfigTests.swift

@@ -9,6 +9,10 @@ import TestSupport
 class EmbraceConfigTests: XCTestCase {
     static var urlSessionConfig: URLSessionConfiguration!
 
+    private var apiBaseUrl: String {
+        "https://embrace.\(testName).com/config"
+    }
+
     override func setUpWithError() throws {
         EmbraceConfigTests.urlSessionConfig = URLSessionConfiguration.ephemeral
         EmbraceConfigTests.urlSessionConfig.protocolClasses = [EmbraceHTTPMock.self]
@@ -20,7 +24,7 @@ class EmbraceConfigTests: XCTestCase {
         minimumUpdateInterval: TimeInterval = 0
     ) -> EmbraceConfig.Options {
         return EmbraceConfig.Options(
-            apiBaseUrl: "https://embrace.\(testName).com/config",
+            apiBaseUrl: apiBaseUrl,
             queue: DispatchQueue(label: "com.test.embrace.queue", attributes: .concurrent),
             appId: TestConstants.appId,
             deviceId: deviceId,
@@ -33,22 +37,57 @@ class EmbraceConfigTests: XCTestCase {
         )
     }
 
+    func mockSuccessfulResponse() throws {
+        var url = try XCTUnwrap(URL(string: "\(apiBaseUrl)/v2/config"))
+
+        if #available(iOS 16.0, *) {
+            url.append(queryItems: [
+                .init(name: "appId", value: TestConstants.appId),
+                .init(name: "osVersion", value: TestConstants.osVersion),
+                .init(name: "appVersion", value: TestConstants.appVersion),
+                .init(name: "deviceId", value: TestConstants.deviceId),
+                .init(name: "sdkVersion", value: TestConstants.sdkVersion)
+            ])
+        } else {
+            XCTFail("This will fail on versions prior to iOS 16.0")
+        }
+
+        let path = Bundle.module.path(
+            forResource: "remote_config",
+            ofType: "json",
+            inDirectory: "Mocks"
+        )!
+        let data = try Data(contentsOf: URL(fileURLWithPath: path))
+        EmbraceHTTPMock.mock(url: url, response: .withData(data, statusCode: 200))
+    }
+
     func test_frequentUpdatesIgnored() throws {
         // given a config with 1 hour minimum update interval
         let options = testOptions(
             deviceId: TestConstants.deviceId,
             minimumUpdateInterval: 60 * 60
         )
+
+        // Given the response is successful (necessary to save the `lastUpdateTime` value)
+        try mockSuccessfulResponse()
+
+        // Given an EmbraceConfig (executes fetch on init)
         let config = EmbraceConfig(options: options)
 
+        // Wait until the fetch from init has finished
+        wait(timeout: .longTimeout) {
+            config.updating == false
+        }
+
         // when trying to update too soon
         config.updateIfNeeded()
 
         // then the update call is ignored
-        wait(delay: 2)
         let url = try XCTUnwrap(config.fetcher.buildURL())
-        XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(url).count, 1)
-        XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 1)
+        wait(timeout: .longTimeout) {
+            return EmbraceHTTPMock.requestsForUrl(url).count == 1 &&
+            EmbraceHTTPMock.totalRequestCount() == 1
+        }
     }
 
     func test_frequentUpdatesNotIgnored() throws {
@@ -57,40 +96,59 @@ class EmbraceConfigTests: XCTestCase {
             deviceId: TestConstants.deviceId,
             minimumUpdateInterval: 1
         )
+
+        // Given the response is successful (necessary to save the `lastUpdateTime` value)
+        try mockSuccessfulResponse()
+
+        // Given an EmbraceConfig (executes fetch on init)
         let config = EmbraceConfig(options: options)
 
-        // when trying to update after 1 second
+        // Wait until the fetch from init has finished
         wait(timeout: .longTimeout) {
             config.updating == false
         }
+
+        // When invoking updateIfNeeded after waiting the "minimumUpdateInterval" amount assigned above
+        wait(delay: 1)
         config.updateIfNeeded()
 
         // then the update call is not ignored
-        wait(delay: 1)
         let url = try XCTUnwrap(config.fetcher.buildURL())
-        XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(url).count, 2)
-        XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 2)
+        wait(timeout: .longTimeout) {
+            return EmbraceHTTPMock.requestsForUrl(url).count == 2 &&
+            EmbraceHTTPMock.totalRequestCount() == 2
+        }
     }
 
     func test_forcedUpdateNotIgnored() throws {
         let options = testOptions(
             deviceId: TestConstants.deviceId,
             minimumUpdateInterval: 60 * 60
         )
+
+        // Given the response is successful (necessary to save the `lastUpdateTime` value)
+        try mockSuccessfulResponse()
+
+        // Given an EmbraceConfig (executes fetch on init)
         let config = EmbraceConfig(options: options)
-        // when forcing an update
+
+        // Wait until the fetch from init has finished
         wait(timeout: .longTimeout) {
             config.updating == false
         }
+
+        // When forcing an update
         config.update()
 
         // then the update call is not ignored
         wait(timeout: .longTimeout) {
             config.updating == false
         }
         let url = try XCTUnwrap(config.fetcher.buildURL())
-        XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(url).count, 2)
-        XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 2)
+        wait(timeout: .longTimeout) {
+            return EmbraceHTTPMock.requestsForUrl(url).count == 2 &&
+            EmbraceHTTPMock.totalRequestCount() == 2
+        }
     }
 
     func test_invalidDeviceId() {

Tests/EmbraceConfigTests/RemoteConfigFetcherTests.swift

@@ -83,7 +83,7 @@ class RemoteConfigFetcherTests: XCTestCase {
             expectation.fulfill()
         }
 
-        wait(for: [expectation], timeout: .defaultTimeout)
+        wait(for: [expectation], timeout: .longTimeout)
 
         XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 1)
         XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(url).count, 1)
@@ -109,7 +109,7 @@ class RemoteConfigFetcherTests: XCTestCase {
             expectation.fulfill()
         }
 
-        wait(for: [expectation], timeout: .defaultTimeout)
+        wait(for: [expectation], timeout: .longTimeout)
 
         XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 1)
         XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(url).count, 1)
@@ -130,7 +130,7 @@ class RemoteConfigFetcherTests: XCTestCase {
             expectation.fulfill()
         }
 
-        wait(for: [expectation], timeout: .defaultTimeout)
+        wait(for: [expectation], timeout: .longTimeout)
 
         XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 1)
         XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(url).count, 1)

Tests/EmbraceCoreTests/Session/UnsentDataHandlerTests.swift

@@ -11,7 +11,6 @@ import EmbraceStorage
 import TestSupport
 import GRDB
 
-// swiftlint:disable file_length
 class UnsentDataHandlerTests: XCTestCase {
     let filePathProvider = TemporaryFilepathProvider()
     var context: CrashReporterContext!
@@ -145,7 +144,7 @@ class UnsentDataHandlerTests: XCTestCase {
         let upload = try EmbraceUpload(options: uploadOptions, queue: queue)
 
         // given a crash reporter
-        let crashReporter = CrashReporterMock()
+        let crashReporter = CrashReporterMock(crashSessionId: TestConstants.sessionId.toString)
 
         // given a finished session in the storage
         try storage.addSession(
@@ -173,13 +172,15 @@ class UnsentDataHandlerTests: XCTestCase {
                 }
             }
         }
-        wait(delay: .longTimeout)
+        wait(for: [expectation1], timeout: .veryLongTimeout)
+        cancellable.cancel()
 
         // then a crash report was sent
-        XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(testBlobsUrl()).count, 1)
-
         // then a session request was sent
-        XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(testSessionUrl()).count, 1)
+        wait(timeout: .veryLongTimeout) {
+            EmbraceHTTPMock.requestsForUrl(self.testBlobsUrl()).count == 1 &&
+            EmbraceHTTPMock.requestsForUrl(self.testSessionUrl()).count == 1
+        }
 
         // then the total amount of requests is correct
         XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 2)
@@ -216,7 +217,20 @@ class UnsentDataHandlerTests: XCTestCase {
         let upload = try EmbraceUpload(options: uploadOptions, queue: queue)
 
         // given a crash reporter
-        let crashReporter = CrashReporterMock()
+        let crashReporter = CrashReporterMock(crashSessionId: TestConstants.sessionId.toString)
+
+        // then the crash report id is set on the session
+        let didSendCrashesExpectation = XCTestExpectation()
+        let observation = ValueObservation.tracking(SessionRecord.fetchAll)
+        let cancellable = observation.start(in: storage.dbQueue) { error in
+            XCTAssert(false, error.localizedDescription)
+        } onChange: { records in
+            if let record = records.first {
+                if record.crashReportId != nil {
+                    didSendCrashesExpectation.fulfill()
+                }
+            }
+        }
 
         // given a finished session in the storage
         try storage.addSession(
@@ -232,25 +246,15 @@ class UnsentDataHandlerTests: XCTestCase {
         // when failing to send unsent sessions
         UnsentDataHandler.sendUnsentData(storage: storage, upload: upload, crashReporter: crashReporter)
 
-        // then the crash report id is set on the session
-        let expectation1 = XCTestExpectation()
-        let observation = ValueObservation.tracking(SessionRecord.fetchAll)
-        let cancellable = observation.start(in: storage.dbQueue) { error in
-            XCTAssert(false, error.localizedDescription)
-        } onChange: { records in
-            if let record = records.first {
-                if record.crashReportId != nil {
-                    expectation1.fulfill()
-                }
-            }
-        }
-        wait(delay: .longTimeout)
+        wait(for: [didSendCrashesExpectation], timeout: .veryLongTimeout)
+        cancellable.cancel()
 
         // then a crash report request was attempted
-        XCTAssertGreaterThan(EmbraceHTTPMock.requestsForUrl(testBlobsUrl()).count, 0)
-
         // then a session request was attempted
-        XCTAssertGreaterThan(EmbraceHTTPMock.requestsForUrl(testSessionUrl()).count, 0)
+        wait(timeout: .veryLongTimeout) {
+            EmbraceHTTPMock.requestsForUrl(self.testBlobsUrl()).count > 0 &&
+            EmbraceHTTPMock.requestsForUrl(self.testSessionUrl()).count > 0
+        }
 
         // then the total amount of requests is correct
         XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 2)
@@ -271,9 +275,6 @@ class UnsentDataHandlerTests: XCTestCase {
         }
 
         wait(for: [expectation], timeout: .defaultTimeout)
-
-        // clean up
-        cancellable.cancel()
     }
 
     func test_withCrashReporter_unfinishedSession() throws {
@@ -288,7 +289,7 @@ class UnsentDataHandlerTests: XCTestCase {
         let upload = try EmbraceUpload(options: uploadOptions, queue: queue)
 
         // given a crash reporter
-        let crashReporter = CrashReporterMock()
+        let crashReporter = CrashReporterMock(crashSessionId: TestConstants.sessionId.toString)
 
         // given an unfinished session in the storage
         try storage.addSession(
@@ -315,13 +316,15 @@ class UnsentDataHandlerTests: XCTestCase {
                 }
             }
         }
-        wait(delay: .longTimeout)
+        wait(for: [expectation1], timeout: .veryLongTimeout)
+        cancellable.cancel()
 
         // then a crash report was sent
-        XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(testBlobsUrl()).count, 1)
-
         // then a session request was sent
-        XCTAssertEqual(EmbraceHTTPMock.requestsForUrl(testSessionUrl()).count, 1)
+        wait(timeout: .veryLongTimeout) {
+            EmbraceHTTPMock.requestsForUrl(self.testBlobsUrl()).count == 1 &&
+            EmbraceHTTPMock.requestsForUrl(self.testSessionUrl()).count == 1
+        }
 
         // then the total amount of requests is correct
         XCTAssertEqual(EmbraceHTTPMock.totalRequestCount(), 2)
@@ -331,10 +334,10 @@ class UnsentDataHandlerTests: XCTestCase {
         XCTAssertNil(session)
 
         // then the session and crash report upload data is no longer cached
-        let uploadData = try upload.cache.fetchAllUploadData()
-        XCTAssertEqual(uploadData.count, 0)
+        wait(timeout: .veryLongTimeout) {
+            try upload.cache.fetchAllUploadData().count == 0
+        }
 
-        // then the crash is not longer stored
         let expectation = XCTestExpectation()
         crashReporter.fetchUnsentCrashReports { reports in
             XCTAssertEqual(reports.count, 0)
@@ -636,4 +639,3 @@ private extension UnsentDataHandlerTests {
         return url
     }
 }
-// swiftlint:enable file_length

Tests/EmbraceCoreTests/TestSupport/TestDoubles/CrashReporterMock.swift

@@ -9,10 +9,14 @@ class CrashReporterMock: CrashReporter {
     var currentSessionId: String?
     var mockReports: [CrashReport]
 
-    init(currentSessionId: String? = nil, mockReports: [CrashReport]? = nil) {
+    init(
+        currentSessionId: String? = nil,
+        crashSessionId: String? = nil,
+        mockReports: [CrashReport]? = nil
+    ) {
         self.currentSessionId = currentSessionId
         self.mockReports = mockReports ?? [.init(ksCrashId: 123,
-                                                 sessionId: SessionIdentifier.random.toString,
+                                                 sessionId: crashSessionId ?? SessionIdentifier.random.toString,
                                                  timestamp: Date(),
                                                  dictionary: ["some key": ["some value"]])]
     }