From 062f2799cb2db581ec3cbec50b3c07d30baf273c Mon Sep 17 00:00:00 2001 From: Maciek Grzybowski Date: Mon, 10 Jan 2022 10:07:44 +0100 Subject: [PATCH 1/7] RUMM-1890 Fix flakiness in `CrashContextTests` when running below iOS 13.0 --- Tests/DatadogTests/Helpers/XCTestCase.swift | 23 +++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/Tests/DatadogTests/Helpers/XCTestCase.swift b/Tests/DatadogTests/Helpers/XCTestCase.swift index 3dd1e52f6b..d93388292e 100644 --- a/Tests/DatadogTests/Helpers/XCTestCase.swift +++ b/Tests/DatadogTests/Helpers/XCTestCase.swift @@ -146,12 +146,23 @@ extension XCTestCase { ) throws { let prettyEncoder = JSONEncoder() prettyEncoder.outputFormatting = [.prettyPrinted, .sortedKeys] - let encodedValue1 = try prettyEncoder.encode(value1) - let encodedValue2 = try prettyEncoder.encode(value2) - - let value1JSONString = encodedValue1.utf8String - let value2JSONString = encodedValue2.utf8String - XCTAssertEqual(value1JSONString, value2JSONString, file: file, line: line) + do { + let encodedValue1: Data + let encodedValue2: Data + if #available(iOS 13.0, *) { + encodedValue1 = try prettyEncoder.encode(value1) + encodedValue2 = try prettyEncoder.encode(value2) + } else { + encodedValue1 = try prettyEncoder.encode(EncodingContainer(value1)) + encodedValue2 = try prettyEncoder.encode(EncodingContainer(value2)) + } + let value1JSONString = encodedValue1.utf8String + let value2JSONString = encodedValue2.utf8String + XCTAssertEqual(value1JSONString, value2JSONString, file: file, line: line) + } catch let exception { + XCTFail("Failed to encode one of the values in `AssertEncodedRepresentationsEqual`", file: file, line: line) + throw exception + } } func AssertURLSessionTasksIdentical( From 583766c6d14772371504fd2d1b98952a08c3191f Mon Sep 17 00:00:00 2001 From: Maciek Grzybowski Date: Mon, 10 Jan 2022 10:24:47 +0100 Subject: [PATCH 2/7] RUMM-1890 Fix flakiness in `DataUploadWorkerTests` notably: `testWhenDataIsBeingUploaded_itPrintsUploadProgressInformationAndSendsErrorsThroughInternalMonitoring` The flakiness was caused by `userLogger` reference leaked in some other tests which use `DateCorrector` (e.g. all tests in `DatadogTests`). NTP sync completion block was executed no matter of `self` existence, making it send logs to current (global) `userLogger` arbitrarily. This was causing some other tests asserting `userLogger` output to receive false data, coming not from their execution. --- Sources/Datadog/Core/System/Time/DateCorrector.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Sources/Datadog/Core/System/Time/DateCorrector.swift b/Sources/Datadog/Core/System/Time/DateCorrector.swift index a3f3dc9e4a..4bb1c49d8a 100644 --- a/Sources/Datadog/Core/System/Time/DateCorrector.swift +++ b/Sources/Datadog/Core/System/Time/DateCorrector.swift @@ -38,7 +38,11 @@ internal class DateCorrector: DateCorrectorType { self.serverDateProvider = serverDateProvider serverDateProvider.synchronize( with: DateCorrector.datadogNTPServers.randomElement()!, // swiftlint:disable:this force_unwrapping - completion: { offset in + completion: { [weak self] offset in + guard let _ = self else { + return + } + if let offset = offset { let difference = (offset * 1_000).rounded() / 1_000 userLogger.info( From b97b9ea678cbda81ad94dacda32ae9b1b057aa53 Mon Sep 17 00:00:00 2001 From: Maciek Grzybowski Date: Mon, 10 Jan 2022 13:36:40 +0100 Subject: [PATCH 3/7] RUMM-1890 Fix flakiness in `VitalRefreshRateReaderTests` by running each measure to fixed number of samples, instead of using time-based condition. --- .../VitalRefreshRateReaderTests.swift | 69 +++++++++++-------- 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/Tests/DatadogTests/Datadog/RUM/RUMVitals/VitalRefreshRateReaderTests.swift b/Tests/DatadogTests/Datadog/RUM/RUMVitals/VitalRefreshRateReaderTests.swift index 834ea7add5..359d236647 100644 --- a/Tests/DatadogTests/Datadog/RUM/RUMVitals/VitalRefreshRateReaderTests.swift +++ b/Tests/DatadogTests/Datadog/RUM/RUMVitals/VitalRefreshRateReaderTests.swift @@ -11,43 +11,54 @@ import UIKit class VitalRefreshRateReaderTests: XCTestCase { private let mockNotificationCenter = NotificationCenter() - func testHighAndLowRefreshRates() { + func testWhenMainThreadOverheadGoesUp_itMeasuresLowerRefreshRate() throws { let reader = VitalRefreshRateReader(notificationCenter: mockNotificationCenter) - let registrar_view1 = VitalPublisher(initialValue: VitalInfo()) - let registrar_view2 = VitalPublisher(initialValue: VitalInfo()) - - // View1 has simple UI, high FPS expected - reader.register(registrar_view1) - - // Wait without blocking UI thread - let expectation1 = expectation(description: "async expectation for first observer") - DispatchQueue.global().asyncAfter(deadline: .now() + 0.5) { - expectation1.fulfill() + let targetSamplesCount = 30 + + /// Runs given work on the main thread until `condition` is met, then calls `completion`. + func run(mainThreadWork: @escaping () -> Void, until condition: @escaping () -> Bool, completion: @escaping () -> Void) { + if !condition() { + mainThreadWork() + DispatchQueue.main.async { // schedule to next runloop + run(mainThreadWork: mainThreadWork, until: condition, completion: completion) + } + } else { + completion() + } } - waitForExpectations(timeout: 1.0) { _ in - XCTAssertGreaterThan(registrar_view1.currentValue.sampleCount, 0) - XCTAssertGreaterThan(UIScreen.main.maximumFramesPerSecond, Int(registrar_view1.currentValue.maxValue!)) - XCTAssertGreaterThan(registrar_view1.currentValue.minValue!, 0.0) + /// Records `targetSamplesCount` samples into `measure` by running given work on the main thread. + func record(_ measure: VitalPublisher, mainThreadWork: @escaping () -> Void) { + let completion = expectation(description: "Complete measurement") + reader.register(measure) + + run( + mainThreadWork: mainThreadWork, + until: { measure.currentValue.sampleCount == targetSamplesCount }, + completion: { + reader.unregister(measure) + completion.fulfill() + } + ) + + wait(for: [completion], timeout: 10) } - reader.unregister(registrar_view1) + // Given + let lowOverhead = { /* no-op */ } // no overhead in succeeding runloop runs + let lowOverheadMeasure = VitalPublisher(initialValue: VitalInfo()) - // View2 has complex UI, lower FPS expected - reader.register(registrar_view2) + let highOverhead = { Thread.sleep(forTimeInterval: 0.02) } // 0.02 overhead in succeeding runloop runs + let highOverheadMeasure = VitalPublisher(initialValue: VitalInfo()) - // Block UI thread - Thread.sleep(forTimeInterval: 1.0) - - // Wait after blocking UI thread so that reader will read refresh rate before assertions - let expectation2 = expectation(description: "async expectation for second observer") - DispatchQueue.global().asyncAfter(deadline: .now() + 0.1) { - expectation2.fulfill() - } - waitForExpectations(timeout: 0.5) { _ in } + // When + record(lowOverheadMeasure, mainThreadWork: lowOverhead) + record(highOverheadMeasure, mainThreadWork: highOverhead) - XCTAssertGreaterThan(registrar_view2.currentValue.sampleCount, 0) - XCTAssertGreaterThan(registrar_view1.currentValue.meanValue!, registrar_view2.currentValue.meanValue!) + // Then + let expectedHighFPS = try XCTUnwrap(lowOverheadMeasure.currentValue.meanValue) + let expectedLowFPS = try XCTUnwrap(highOverheadMeasure.currentValue.meanValue) + XCTAssertGreaterThan(expectedHighFPS, expectedLowFPS, "It must measure higher FPS for lower main thread overhead") } func testAppStateHandling() { From ab4d65d7c48082ab1592fb69d3c8177329896a19 Mon Sep 17 00:00:00 2001 From: Maciek Grzybowski Date: Mon, 10 Jan 2022 13:40:08 +0100 Subject: [PATCH 4/7] RUMM-1890 Add more verbosity to failure in `CrashContextProviderTests` precisely in `testWhenCurrentValueIsObtainedFromNetworkConnectionInfoProvider_thenCrashContextProviderNotifiesNewContext`. --- .../CrashContext/CrashContextProviderTests.swift | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Tests/DatadogTests/Datadog/CrashReporting/CrashContext/CrashContextProviderTests.swift b/Tests/DatadogTests/Datadog/CrashReporting/CrashContext/CrashContextProviderTests.swift index a13ae902ec..b54da95ab9 100644 --- a/Tests/DatadogTests/Datadog/CrashReporting/CrashContext/CrashContextProviderTests.swift +++ b/Tests/DatadogTests/Datadog/CrashReporting/CrashContext/CrashContextProviderTests.swift @@ -159,7 +159,7 @@ class CrashContextProviderTests: XCTestCase { // MARK: - `NetworkConnectionInfo` Integration - func testWhenCurrentValueIsObtainedFromNetworkConnectionInfoProvider_thenCrashContextProviderNotifiesNewContext() { + func testWhenCurrentValueIsObtainedFromNetworkConnectionInfoProvider_thenCrashContextProviderNotifiesNewContext() throws { let expectation = self.expectation(description: "Notify new crash context") let initialNetworkConnectionInfo: NetworkConnectionInfo = .mockRandom() let wrappedProvider = NetworkConnectionInfoProviderMock(networkConnectionInfo: initialNetworkConnectionInfo) @@ -188,8 +188,9 @@ class CrashContextProviderTests: XCTestCase { // Then waitForExpectations(timeout: 1, handler: nil) - XCTAssertEqual(initialContext.lastNetworkConnectionInfo, initialNetworkConnectionInfo) - XCTAssertEqual(updatedContext?.lastNetworkConnectionInfo, currentNetworkConnectionInfo) + let updatedNetworkConnectionInfo = try XCTUnwrap(updatedContext?.lastNetworkConnectionInfo) + XCTAssertEqual(initialContext.lastNetworkConnectionInfo, initialNetworkConnectionInfo, "It must store initial network info") + XCTAssertEqual(updatedNetworkConnectionInfo, currentNetworkConnectionInfo, "It must store updated network info") } // MARK: - `CarrierInfo` Integration From cc5058dbf7df138624db23d7e1cbf42e621c2380 Mon Sep 17 00:00:00 2001 From: Maciek Grzybowski Date: Mon, 10 Jan 2022 13:47:16 +0100 Subject: [PATCH 5/7] RUMM-1890 Fix flakiness in `testItStoresWeakReferenceToUIViewController` by ensuring autorelease VC deallocation with `autoreleasepool {}` --- .../RUM/RUMMonitor/Scopes/RUMViewIdentityTests.swift | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMViewIdentityTests.swift b/Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMViewIdentityTests.swift index c60bd9da59..7909c3dc5d 100644 --- a/Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMViewIdentityTests.swift +++ b/Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMViewIdentityTests.swift @@ -89,12 +89,15 @@ class RUMViewIdentityTests: XCTestCase { // MARK: - Memory management func testItStoresWeakReferenceToUIViewController() throws { - var vc: UIViewController? = UIViewController() + var identity: RUMViewIdentity! // swiftlint:disable:this implicitly_unwrapped_optional - let identity = try XCTUnwrap(vc?.asRUMViewIdentity()) + try autoreleasepool { + var vc: UIViewController? = UIViewController() + identity = try XCTUnwrap(vc?.asRUMViewIdentity()) + XCTAssertNotNil(identity.identifiable, "Reference should be available while `vc` is alive.") + vc = nil + } - XCTAssertNotNil(identity.identifiable, "Reference should be available while `vc` is alive.") - vc = nil XCTAssertNil(identity.identifiable, "Reference should not be available after `vc` was deallocated.") } } From 36d0b478511fb624ba268e64868d4264344fa65d Mon Sep 17 00:00:00 2001 From: Maciek Grzybowski Date: Mon, 10 Jan 2022 14:02:15 +0100 Subject: [PATCH 6/7] RUMM-1890 Fix flakiness in `testWhenSamplingRateIs50_onlyHalfOfTheEventsAreSent` by increasing number of samples to 400 (with 200 it was failing 4/500 repetitions, with 300 1/500, with 400 it's 100% success). --- .../RUM/RUMMonitor/Scopes/RUMApplicationScopeTests.swift | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMApplicationScopeTests.swift b/Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMApplicationScopeTests.swift index 550d146f18..cc56f2c301 100644 --- a/Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMApplicationScopeTests.swift +++ b/Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMApplicationScopeTests.swift @@ -151,7 +151,7 @@ class RUMApplicationScopeTests: XCTestCase { backgroundEventTrackingEnabled: .mockAny() ) - let simulatedSessionsCount = 200 + let simulatedSessionsCount = 400 (0...self).count let trackedSessionsCount = Double(viewEventsCount) / 2 // each Session should send 2 View updates - XCTAssertGreaterThan(trackedSessionsCount, 100 * 0.8) // -20% - XCTAssertLessThan(trackedSessionsCount, 100 * 1.2) // +20% + let halfSessionsCount = 0.5 * Double(simulatedSessionsCount) + XCTAssertGreaterThan(trackedSessionsCount, halfSessionsCount * 0.8) // -20% + XCTAssertLessThan(trackedSessionsCount, halfSessionsCount * 1.2) // +20% } } From 70d8729ae7e9551714212cb6fcaa6129777e0283 Mon Sep 17 00:00:00 2001 From: Maciek Grzybowski Date: Tue, 11 Jan 2022 08:50:29 +0100 Subject: [PATCH 7/7] RUMM-1890 CR feedback --- Sources/Datadog/Core/System/Time/DateCorrector.swift | 6 +----- .../Datadog/Core/System/Time/ServerDateProvider.swift | 10 +++++++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Sources/Datadog/Core/System/Time/DateCorrector.swift b/Sources/Datadog/Core/System/Time/DateCorrector.swift index 4bb1c49d8a..a3f3dc9e4a 100644 --- a/Sources/Datadog/Core/System/Time/DateCorrector.swift +++ b/Sources/Datadog/Core/System/Time/DateCorrector.swift @@ -38,11 +38,7 @@ internal class DateCorrector: DateCorrectorType { self.serverDateProvider = serverDateProvider serverDateProvider.synchronize( with: DateCorrector.datadogNTPServers.randomElement()!, // swiftlint:disable:this force_unwrapping - completion: { [weak self] offset in - guard let _ = self else { - return - } - + completion: { offset in if let offset = offset { let difference = (offset * 1_000).rounded() / 1_000 userLogger.info( diff --git a/Sources/Datadog/Core/System/Time/ServerDateProvider.swift b/Sources/Datadog/Core/System/Time/ServerDateProvider.swift index 33b90628f4..5e4963bba9 100644 --- a/Sources/Datadog/Core/System/Time/ServerDateProvider.swift +++ b/Sources/Datadog/Core/System/Time/ServerDateProvider.swift @@ -33,17 +33,21 @@ internal class NTPServerDateProvider: ServerDateProvider { self?.publisher.publishAsync(offset) }, completion: { [weak self] now, offset in + guard let self = self else { + return + } + // Kronos only notifies for the first and last samples. // In case, the last sample does not return an offset, we calculate the offset // from the returned `now` parameter. The `now` parameter in this callback // is `Clock.now`, so it is possible to have `now` but not `offset`. if let offset = offset { - self?.publisher.publishAsync(offset) + self.publisher.publishAsync(offset) } else if let now = now { - self?.publisher.publishAsync(now.timeIntervalSinceNow) + self.publisher.publishAsync(now.timeIntervalSinceNow) } - completion(self?.publisher.currentValue) + completion(self.publisher.currentValue) } )