From 581ac35e51961684c32b377da84ef16c9489cf5b Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Wed, 10 Apr 2024 16:39:40 +0200 Subject: [PATCH 01/11] Adds IPC attempt pixels, unit tests for NetworkProtectionIPCTunnelController --- DuckDuckGo.xcodeproj/project.pbxproj | 20 ++ DuckDuckGo/LoginItems/LoginItemsManager.swift | 20 +- ...NetworkProtectionIPCTunnelController.swift | 178 ++++++++++++++++-- ...rkProtectionIPCTunnelControllerTests.swift | 126 +++++++++++++ .../NetworkProtectionTestingSupport.swift | 141 ++++++++++++++ 5 files changed, 472 insertions(+), 13 deletions(-) create mode 100644 UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift create mode 100644 UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index 6204a3116e..fb0d5386e6 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -2311,6 +2311,10 @@ 7BBD45B12A691AB500C83CA9 /* NetworkProtectionDebugUtilities.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BBD45B02A691AB500C83CA9 /* NetworkProtectionDebugUtilities.swift */; }; 7BBD45B22A691AB500C83CA9 /* NetworkProtectionDebugUtilities.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BBD45B02A691AB500C83CA9 /* NetworkProtectionDebugUtilities.swift */; }; 7BBE2B7B2B61663C00697445 /* NetworkProtectionProxy in Frameworks */ = {isa = PBXBuildFile; productRef = 7BBE2B7A2B61663C00697445 /* NetworkProtectionProxy */; }; + 7BBE650D2BC67BA0008F4EE9 /* NetworkProtectionIPCTunnelControllerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BBE650C2BC67BA0008F4EE9 /* NetworkProtectionIPCTunnelControllerTests.swift */; }; + 7BBE650E2BC67BA0008F4EE9 /* NetworkProtectionIPCTunnelControllerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BBE650C2BC67BA0008F4EE9 /* NetworkProtectionIPCTunnelControllerTests.swift */; }; + 7BBE65102BC67EED008F4EE9 /* NetworkProtectionTestingSupport.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BBE650F2BC67EED008F4EE9 /* NetworkProtectionTestingSupport.swift */; }; + 7BBE65112BC67EED008F4EE9 /* NetworkProtectionTestingSupport.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BBE650F2BC67EED008F4EE9 /* NetworkProtectionTestingSupport.swift */; }; 7BD01C192AD8319C0088B32E /* IPCServiceManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BD01C182AD8319C0088B32E /* IPCServiceManager.swift */; }; 7BD1688E2AD4A4C400D24876 /* NetworkExtensionController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BD1688D2AD4A4C400D24876 /* NetworkExtensionController.swift */; }; 7BD3AF5D2A8E7AF1006F9F56 /* KeychainType+ClientDefault.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BD3AF5C2A8E7AF1006F9F56 /* KeychainType+ClientDefault.swift */; }; @@ -4135,6 +4139,8 @@ 7BB108582A43375D000AB95F /* PFMoveApplication.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = PFMoveApplication.m; sourceTree = ""; }; 7BBA7CE52BAB03C1007579A3 /* DefaultSubscriptionFeatureAvailability+DefaultInitializer.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "DefaultSubscriptionFeatureAvailability+DefaultInitializer.swift"; sourceTree = ""; }; 7BBD45B02A691AB500C83CA9 /* NetworkProtectionDebugUtilities.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NetworkProtectionDebugUtilities.swift; sourceTree = ""; }; + 7BBE650C2BC67BA0008F4EE9 /* NetworkProtectionIPCTunnelControllerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NetworkProtectionIPCTunnelControllerTests.swift; sourceTree = ""; }; + 7BBE650F2BC67EED008F4EE9 /* NetworkProtectionTestingSupport.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NetworkProtectionTestingSupport.swift; sourceTree = ""; }; 7BD01C182AD8319C0088B32E /* IPCServiceManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = IPCServiceManager.swift; sourceTree = ""; }; 7BD1688D2AD4A4C400D24876 /* NetworkExtensionController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = NetworkExtensionController.swift; sourceTree = ""; }; 7BD3AF5C2A8E7AF1006F9F56 /* KeychainType+ClientDefault.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "KeychainType+ClientDefault.swift"; sourceTree = ""; }; @@ -6470,10 +6476,12 @@ 4BCF15E32ABB987F0083F6DF /* NetworkProtection */ = { isa = PBXGroup; children = ( + 7BBE65122BC67EF6008F4EE9 /* Support */, 4BCF15E62ABB98A20083F6DF /* Resources */, 4BCF15E42ABB98990083F6DF /* NetworkProtectionRemoteMessageTests.swift */, 4BD57C032AC112DF00B580EE /* NetworkProtectionRemoteMessagingTests.swift */, 7B09CBA72BA4BE7000CF245B /* NetworkProtectionPixelEventTests.swift */, + 7BBE650C2BC67BA0008F4EE9 /* NetworkProtectionIPCTunnelControllerTests.swift */, ); path = NetworkProtection; sourceTree = ""; @@ -6677,6 +6685,14 @@ path = LetsMove1.25; sourceTree = ""; }; + 7BBE65122BC67EF6008F4EE9 /* Support */ = { + isa = PBXGroup; + children = ( + 7BBE650F2BC67EED008F4EE9 /* NetworkProtectionTestingSupport.swift */, + ); + path = Support; + sourceTree = ""; + }; 7BDA36E72B7E037200AD5388 /* VPNProxyExtension */ = { isa = PBXGroup; children = ( @@ -11218,8 +11234,10 @@ B630E80129C887ED00363609 /* NSErrorAdditionalInfo.swift in Sources */, 3706FE31293F661700E42796 /* TabCollectionViewModelDelegateMock.swift in Sources */, 3706FE32293F661700E42796 /* BookmarksHTMLReaderTests.swift in Sources */, + 7BBE650E2BC67BA0008F4EE9 /* NetworkProtectionIPCTunnelControllerTests.swift in Sources */, 3706FE33293F661700E42796 /* FireTests.swift in Sources */, B60C6F8229B1B4AD007BFAA8 /* TestRunHelper.swift in Sources */, + 7BBE65112BC67EED008F4EE9 /* NetworkProtectionTestingSupport.swift in Sources */, 567DA94029E8045D008AC5EE /* MockEmailStorage.swift in Sources */, 317295D32AF058D3002C3206 /* MockWaitlistTermsAndConditionsActionHandler.swift in Sources */, 3706FE34293F661700E42796 /* PermissionStoreTests.swift in Sources */, @@ -13353,6 +13371,7 @@ 1D3B1AC22936B816006F4388 /* BWMessageIdGeneratorTests.swift in Sources */, B6C2C9F62760B659005B7F0A /* TestDataModel.xcdatamodeld in Sources */, 1DA6D1022A1FFA3700540406 /* HTTPCookieTests.swift in Sources */, + 7BBE65102BC67EED008F4EE9 /* NetworkProtectionTestingSupport.swift in Sources */, 1D9FDEC02B9B5FEA0040B78C /* AccessibilityPreferencesTests.swift in Sources */, B68172AE269EB43F006D1092 /* GeolocationServiceTests.swift in Sources */, B6AE74342609AFCE005B9B1A /* ProgressEstimationTests.swift in Sources */, @@ -13433,6 +13452,7 @@ 37CD54B927F1F8AC00F1F7B9 /* AppearancePreferencesTests.swift in Sources */, EEF53E182950CED5002D78F4 /* JSAlertViewModelTests.swift in Sources */, 376C4DB928A1A48A00CC0F5B /* FirePopoverViewModelTests.swift in Sources */, + 7BBE650D2BC67BA0008F4EE9 /* NetworkProtectionIPCTunnelControllerTests.swift in Sources */, AAEC74B62642CC6A00C2EFBC /* HistoryStoringMock.swift in Sources */, AA652CB125DD825B009059CC /* LocalBookmarkStoreTests.swift in Sources */, B630794226731F5400DCEE41 /* WKDownloadMock.swift in Sources */, diff --git a/DuckDuckGo/LoginItems/LoginItemsManager.swift b/DuckDuckGo/LoginItems/LoginItemsManager.swift index e0a44a4fb4..0cce2dc5c6 100644 --- a/DuckDuckGo/LoginItems/LoginItemsManager.swift +++ b/DuckDuckGo/LoginItems/LoginItemsManager.swift @@ -20,9 +20,13 @@ import Common import Foundation import LoginItems +protocol LoginItemsManaging { + func throwingEnableLoginItems(_ items: Set, log: OSLog) throws +} + /// Class to manage the login items for the VPN and DBP /// -final class LoginItemsManager { +final class LoginItemsManager: LoginItemsManaging { private enum Action: String { case enable case disable @@ -42,6 +46,20 @@ final class LoginItemsManager { } } + /// Throwing version of enableLoginItems + /// + func throwingEnableLoginItems(_ items: Set, log: OSLog) throws { + for item in items { + do { + try item.enable() + os_log("🟢 Enabled successfully %{public}@", log: log, String(describing: item)) + } catch let error as NSError { + handleError(for: item, action: .enable, error: error) + throw error + } + } + } + func restartLoginItems(_ items: Set, log: OSLog) { for item in items { do { diff --git a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift index 5e2027a3dc..55af1d1065 100644 --- a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift +++ b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift @@ -20,15 +20,18 @@ import Common import Foundation import NetworkProtection import NetworkProtectionIPC +import PixelKit -final class NetworkProtectionIPCTunnelController: TunnelController { +/// VPN tunnel controller through IPC. +/// +final class NetworkProtectionIPCTunnelController { private let featureVisibility: NetworkProtectionFeatureVisibility - private let loginItemsManager: LoginItemsManager + private let loginItemsManager: LoginItemsManaging private let ipcClient: NetworkProtectionIPCClient init(featureVisibility: NetworkProtectionFeatureVisibility = DefaultNetworkProtectionVisibility(), - loginItemsManager: LoginItemsManager = LoginItemsManager(), + loginItemsManager: LoginItemsManaging = LoginItemsManager(), ipcClient: NetworkProtectionIPCClient) { self.featureVisibility = featureVisibility @@ -37,7 +40,9 @@ final class NetworkProtectionIPCTunnelController: TunnelController { } @MainActor - func start() async { + func start(attemptHandler: VPNStartAttemptHandling = DefaultVPNStartAttemptHandler()) async { + attemptHandler.begin() + do { guard try await enableLoginItems() else { os_log("🔴 IPC Controller refusing to start the VPN menu app. Not authorized.", log: .networkProtection) @@ -45,13 +50,17 @@ final class NetworkProtectionIPCTunnelController: TunnelController { } ipcClient.start() + attemptHandler.success() } catch { os_log("🔴 IPC Controller found en error when starting the VPN: \(error)", log: .networkProtection) + attemptHandler.failure(error) } } @MainActor - func stop() async { + func stop(attemptHandler: VPNStopAttemptHandling) async { + attemptHandler.begin() + do { guard try await enableLoginItems() else { os_log("🔴 IPC Controller refusing to start the VPN. Not authorized.", log: .networkProtection) @@ -59,11 +68,40 @@ final class NetworkProtectionIPCTunnelController: TunnelController { } ipcClient.stop() + attemptHandler.success() } catch { os_log("🔴 IPC Controller found en error when starting the VPN: \(error)", log: .networkProtection) + attemptHandler.failure(error) } } + // MARK: - Login Items Manager + + private func enableLoginItems() async throws -> Bool { + guard try await featureVisibility.canStartVPN() else { + // We shouldn't enable the menu app is the VPN feature is disabled. + return false + } + + try loginItemsManager.throwingEnableLoginItems(LoginItemsManager.networkProtectionLoginItems, log: .networkProtection) + return true + } +} + +// MARK: - TunnelController Conformance + +extension NetworkProtectionIPCTunnelController: TunnelController { + + @MainActor + func start() async { + await start(attemptHandler: DefaultVPNStartAttemptHandler()) + } + + @MainActor + func stop() async { + await stop(attemptHandler: DefaultVPNStopAttemptHandler()) + } + /// Queries VPN to know if it's connected. /// /// - Returns: `true` if the VPN is connected, connecting or reasserting, and `false` otherwise. @@ -77,16 +115,132 @@ final class NetworkProtectionIPCTunnelController: TunnelController { return false } } +} - // MARK: - Login Items Manager +// MARK: - Start Attempts - private func enableLoginItems() async throws -> Bool { - guard try await featureVisibility.canStartVPN() else { - // We shouldn't enable the menu app is the VPN feature is disabled. - return false +protocol VPNStartAttemptHandling { + func begin() + func success() + func failure(_ error: Error) +} + +extension NetworkProtectionIPCTunnelController { + + private enum StartAttempt: PixelKitEventV2 { + case begin + case success + case failure(_ error: Error) + + var name: String { + switch self { + case .begin: + return "netp_browser_start_attempt" + + case .success: + return "netp_browser_start_success" + + case .failure: + return "netp_browser_start_failure" + } } - loginItemsManager.enableLoginItems(LoginItemsManager.networkProtectionLoginItems, log: .networkProtection) - return true + var parameters: [String: String]? { + return nil + } + + var error: Error? { + switch self { + case .begin, + .success: + return nil + case .failure(let error): + return error + } + } + } + + private class DefaultVPNStartAttemptHandler: VPNStartAttemptHandling { + private let pixelKit: PixelKit? + + init(pixelKit: PixelKit? = .shared) { + self.pixelKit = pixelKit + } + + func begin() { + pixelKit?.fire(StartAttempt.begin) + } + + func success() { + pixelKit?.fire(StartAttempt.success, frequency: .dailyAndContinuous) + } + + func failure(_ error: Error) { + pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndContinuous) + } + } +} + +// MARK: - Stop Attempts + +protocol VPNStopAttemptHandling { + func begin() + func success() + func failure(_ error: Error) +} + +extension NetworkProtectionIPCTunnelController { + + private enum StopAttempt: PixelKitEventV2 { + case begin + case success + case failure(_ error: Error) + + var name: String { + switch self { + case .begin: + return "netp_browser_stop_attempt" + + case .success: + return "netp_browser_stop_success" + + case .failure: + return "netp_browser_stop_failure" + } + } + + var parameters: [String: String]? { + return nil + } + + var error: Error? { + switch self { + case .begin, + .success: + return nil + case .failure(let error): + return error + } + } + } + + private class DefaultVPNStopAttemptHandler: VPNStopAttemptHandling { + private let pixelKit: PixelKit? + + init(pixelKit: PixelKit? = .shared) { + self.pixelKit = pixelKit + } + + func begin() { + pixelKit?.fire(StopAttempt.begin) + } + + func success() { + pixelKit?.fire(StopAttempt.success, frequency: .dailyAndContinuous) + } + + func failure(_ error: Error) { + pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndContinuous) + } } } diff --git a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift new file mode 100644 index 0000000000..91221d4f2a --- /dev/null +++ b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift @@ -0,0 +1,126 @@ +// +// NetworkProtectionIPCTunnelControllerTests.swift +// +// Copyright © 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Combine +import Foundation +import NetworkProtection +import XCTest +@testable import DuckDuckGo_Privacy_Browser + +final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { + + final class AttemptHandler: VPNStartAttemptHandling, VPNStopAttemptHandling { + + enum ExpectedResult { + case success + case failure(_ error: Error) + } + + private var beginCallCount = 0 + private var successCallCount = 0 + private var failureCallCount = 0 + private var error: Error? + private let expectedResult: ExpectedResult + + init(expectedResult: ExpectedResult) { + self.expectedResult = expectedResult + } + + func begin() { + beginCallCount += 1 + } + + func success() { + successCallCount += 1 + } + + func failure(_ error: Error) { + failureCallCount += 1 + self.error = error + } + + var expectationsMet: Bool { + switch expectedResult { + case .success: + return beginCallCount == 1 && successCallCount == 1 && failureCallCount == 0 + case .failure(let error): + guard let expectedNSError = self.error as? NSError else { + return false + } + + let nsError = error as NSError + + return beginCallCount == 1 && successCallCount == 0 && failureCallCount == 1 && nsError == expectedNSError + } + } + } + + // MARK: - Tunnel Start Tests + + func testStartTunnelSuccess() async { + let controller = NetworkProtectionIPCTunnelController( + featureVisibility: MockFeatureVisibility(), + loginItemsManager: MockLoginItemsManager(mockResult: .success), + ipcClient: MockIPCClient()) + let attemptHandler = AttemptHandler(expectedResult: .success) + + await controller.start(attemptHandler: attemptHandler) + + XCTAssertTrue(attemptHandler.expectationsMet) + } + + func testStartTunnelLoginItemFailure() async { + let error = NSError(domain: "test", code: 1) + let controller = NetworkProtectionIPCTunnelController( + featureVisibility: MockFeatureVisibility(), + loginItemsManager: MockLoginItemsManager(mockResult: .failure(error)), + ipcClient: MockIPCClient()) + let attemptHandler = AttemptHandler(expectedResult: .failure(error)) + + await controller.start(attemptHandler: attemptHandler) + + XCTAssertTrue(attemptHandler.expectationsMet) + } + + // MARK: - Tunnel Stop Tests + + func testStopTunnelSuccess() async { + let controller = NetworkProtectionIPCTunnelController( + featureVisibility: MockFeatureVisibility(), + loginItemsManager: MockLoginItemsManager(mockResult: .success), + ipcClient: MockIPCClient()) + let attemptHandler = AttemptHandler(expectedResult: .success) + + await controller.stop(attemptHandler: attemptHandler) + + XCTAssertTrue(attemptHandler.expectationsMet) + } + + func testStopTunnelLoginItemFailure() async { + let error = NSError(domain: "test", code: 1) + let controller = NetworkProtectionIPCTunnelController( + featureVisibility: MockFeatureVisibility(), + loginItemsManager: MockLoginItemsManager(mockResult: .failure(error)), + ipcClient: MockIPCClient()) + let attemptHandler = AttemptHandler(expectedResult: .failure(error)) + + await controller.stop(attemptHandler: attemptHandler) + + XCTAssertTrue(attemptHandler.expectationsMet) + } +} diff --git a/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift b/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift new file mode 100644 index 0000000000..6723546952 --- /dev/null +++ b/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift @@ -0,0 +1,141 @@ +// +// NetworkProtectionTestingSupport.swift +// +// Copyright © 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Combine +import Common +import Foundation +import LoginItems +import NetworkProtection +import NetworkProtectionUI +@testable import DuckDuckGo_Privacy_Browser + +struct MockFeatureVisibility: NetworkProtectionFeatureVisibility { + let isEligibleForThankYouMessage: Bool + let isInstalled: Bool + let canStartVPNValue: Bool + let isVPNVisibleValue: Bool + let isNetworkProtectionBetaVisibleValue: Bool + let shouldUninstallAutomaticallyValue: Bool + let disableIfUserHasNoAccessValue: Bool + + func canStartVPN() async throws -> Bool { + canStartVPNValue + } + + func isVPNVisible() -> Bool { + isVPNVisibleValue + } + + func isNetworkProtectionBetaVisible() -> Bool { + isNetworkProtectionBetaVisibleValue + } + + func shouldUninstallAutomatically() -> Bool { + shouldUninstallAutomaticallyValue + } + + func disableForAllUsers() async { + // Intentional no-op + } + + func disableForWaitlistUsers() { + // Intentional no-op + } + + func disableIfUserHasNoAccess() async -> Bool { + disableIfUserHasNoAccessValue + } + + let onboardStatusPublisher: AnyPublisher + + init(isEligibleForThankYouMessage: Bool = false, + isInstalled: Bool = false, + canStartVPNValue: Bool = true, + isVPNVisibleValue: Bool = true, + isNetworkProtectionBetaVisibleValue: Bool = false, + shouldUninstallAutomaticallyValue: Bool = false, + disableIfUserHasNoAccessValue: Bool = false, + onboardStatusPublisher: AnyPublisher = Just(.default).eraseToAnyPublisher()) { + + self.isEligibleForThankYouMessage = isEligibleForThankYouMessage + self.isInstalled = isInstalled + self.canStartVPNValue = canStartVPNValue + self.isVPNVisibleValue = isVPNVisibleValue + self.isNetworkProtectionBetaVisibleValue = isNetworkProtectionBetaVisibleValue + self.shouldUninstallAutomaticallyValue = shouldUninstallAutomaticallyValue + self.disableIfUserHasNoAccessValue = disableIfUserHasNoAccessValue + self.onboardStatusPublisher = onboardStatusPublisher + } +} + +struct MockConnectionStatusObserver: ConnectionStatusObserver { + var publisher: AnyPublisher = Just(.disconnected).eraseToAnyPublisher() + + var recentValue: NetworkProtection.ConnectionStatus = .disconnected +} + +struct MockServerInfoObserver: ConnectionServerInfoObserver { + var publisher: AnyPublisher = Just(.unknown).eraseToAnyPublisher() + + var recentValue: NetworkProtection.NetworkProtectionStatusServerInfo = .unknown +} + +struct MockConnectionErrorObserver: ConnectionErrorObserver { + var publisher: AnyPublisher = Just(nil).eraseToAnyPublisher() + + var recentValue: String? = nil +} + +struct MockIPCClient: NetworkProtectionIPCClient { + var ipcStatusObserver: NetworkProtection.ConnectionStatusObserver = MockConnectionStatusObserver() + + var ipcServerInfoObserver: NetworkProtection.ConnectionServerInfoObserver = MockServerInfoObserver() + + var ipcConnectionErrorObserver: NetworkProtection.ConnectionErrorObserver = MockConnectionErrorObserver() + + func start() { + // Intentional no-op + } + + func stop() { + // Intentional no-op + } +} + +struct MockLoginItemsManager: LoginItemsManaging { + + enum MockResult { + case success + case failure(_ error: Error) + } + + private let mockResult: MockResult + + init(mockResult: MockResult) { + self.mockResult = mockResult + } + + func throwingEnableLoginItems(_ items: Set, log: Common.OSLog) throws { + switch mockResult { + case .success: + return + case .failure(let error): + throw error + } + } +} From b6671a8459bd606d51d0479fb66831fd1e566638 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Mon, 15 Apr 2024 12:25:50 +0200 Subject: [PATCH 02/11] Adds IPC start pixels for the VPN --- ...NetworkProtectionIPCTunnelController.swift | 121 +++++------------- .../Sources/PixelKit/PixelKitEventV2.swift | 39 ++++++ .../PixelKitMock.swift | 66 ++++++++++ ...rkProtectionIPCTunnelControllerTests.swift | 52 +++++--- 4 files changed, 171 insertions(+), 107 deletions(-) create mode 100644 LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift diff --git a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift index 55af1d1065..b585032774 100644 --- a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift +++ b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift @@ -29,19 +29,39 @@ final class NetworkProtectionIPCTunnelController { private let featureVisibility: NetworkProtectionFeatureVisibility private let loginItemsManager: LoginItemsManaging private let ipcClient: NetworkProtectionIPCClient + private let pixelKit: PixelFiring? init(featureVisibility: NetworkProtectionFeatureVisibility = DefaultNetworkProtectionVisibility(), loginItemsManager: LoginItemsManaging = LoginItemsManager(), - ipcClient: NetworkProtectionIPCClient) { + ipcClient: NetworkProtectionIPCClient, + pixelKit: PixelFiring? = PixelKit.shared) { self.featureVisibility = featureVisibility self.loginItemsManager = loginItemsManager self.ipcClient = ipcClient + self.pixelKit = pixelKit } + // MARK: - Login Items Manager + + private func enableLoginItems() async throws -> Bool { + guard try await featureVisibility.canStartVPN() else { + // We shouldn't enable the menu app is the VPN feature is disabled. + return false + } + + try loginItemsManager.throwingEnableLoginItems(LoginItemsManager.networkProtectionLoginItems, log: .networkProtection) + return true + } +} + +// MARK: - TunnelController Conformance + +extension NetworkProtectionIPCTunnelController: TunnelController { + @MainActor - func start(attemptHandler: VPNStartAttemptHandling = DefaultVPNStartAttemptHandler()) async { - attemptHandler.begin() + func start() async { + try? await pixelKit?.fire(StartAttempt.begin) do { guard try await enableLoginItems() else { @@ -50,16 +70,16 @@ final class NetworkProtectionIPCTunnelController { } ipcClient.start() - attemptHandler.success() + try? await pixelKit?.fire(StartAttempt.success, frequency: .dailyAndContinuous) } catch { os_log("🔴 IPC Controller found en error when starting the VPN: \(error)", log: .networkProtection) - attemptHandler.failure(error) + try? await pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndContinuous) } } @MainActor - func stop(attemptHandler: VPNStopAttemptHandling) async { - attemptHandler.begin() + func stop() async { + try? await pixelKit?.fire(StopAttempt.begin) do { guard try await enableLoginItems() else { @@ -68,40 +88,13 @@ final class NetworkProtectionIPCTunnelController { } ipcClient.stop() - attemptHandler.success() + try? await pixelKit?.fire(StopAttempt.success, frequency: .dailyAndContinuous) } catch { os_log("🔴 IPC Controller found en error when starting the VPN: \(error)", log: .networkProtection) - attemptHandler.failure(error) + try? await pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndContinuous) } } - // MARK: - Login Items Manager - - private func enableLoginItems() async throws -> Bool { - guard try await featureVisibility.canStartVPN() else { - // We shouldn't enable the menu app is the VPN feature is disabled. - return false - } - - try loginItemsManager.throwingEnableLoginItems(LoginItemsManager.networkProtectionLoginItems, log: .networkProtection) - return true - } -} - -// MARK: - TunnelController Conformance - -extension NetworkProtectionIPCTunnelController: TunnelController { - - @MainActor - func start() async { - await start(attemptHandler: DefaultVPNStartAttemptHandler()) - } - - @MainActor - func stop() async { - await stop(attemptHandler: DefaultVPNStopAttemptHandler()) - } - /// Queries VPN to know if it's connected. /// /// - Returns: `true` if the VPN is connected, connecting or reasserting, and `false` otherwise. @@ -119,15 +112,9 @@ extension NetworkProtectionIPCTunnelController: TunnelController { // MARK: - Start Attempts -protocol VPNStartAttemptHandling { - func begin() - func success() - func failure(_ error: Error) -} - extension NetworkProtectionIPCTunnelController { - private enum StartAttempt: PixelKitEventV2 { + enum StartAttempt: PixelKitEventV2 { case begin case success case failure(_ error: Error) @@ -159,39 +146,13 @@ extension NetworkProtectionIPCTunnelController { } } } - - private class DefaultVPNStartAttemptHandler: VPNStartAttemptHandling { - private let pixelKit: PixelKit? - - init(pixelKit: PixelKit? = .shared) { - self.pixelKit = pixelKit - } - - func begin() { - pixelKit?.fire(StartAttempt.begin) - } - - func success() { - pixelKit?.fire(StartAttempt.success, frequency: .dailyAndContinuous) - } - - func failure(_ error: Error) { - pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndContinuous) - } - } } // MARK: - Stop Attempts -protocol VPNStopAttemptHandling { - func begin() - func success() - func failure(_ error: Error) -} - extension NetworkProtectionIPCTunnelController { - private enum StopAttempt: PixelKitEventV2 { + enum StopAttempt: PixelKitEventV2 { case begin case success case failure(_ error: Error) @@ -223,24 +184,4 @@ extension NetworkProtectionIPCTunnelController { } } } - - private class DefaultVPNStopAttemptHandler: VPNStopAttemptHandling { - private let pixelKit: PixelKit? - - init(pixelKit: PixelKit? = .shared) { - self.pixelKit = pixelKit - } - - func begin() { - pixelKit?.fire(StopAttempt.begin) - } - - func success() { - pixelKit?.fire(StopAttempt.success, frequency: .dailyAndContinuous) - } - - func failure(_ error: Error) { - pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndContinuous) - } - } } diff --git a/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift b/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift index 19525d0d0b..d6b2269afd 100644 --- a/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift +++ b/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift @@ -30,3 +30,42 @@ import Foundation public protocol PixelKitEventV2: PixelKitEvent { var error: Error? { get } } + +/// Protocol to support mocking pixel firing. +/// +/// We're adding support for `PixelKitEventV2` events strategically because adding support for earlier pixels +/// would be more complicated and time consuming. The idea of V2 events is that fire calls should not include a lot +/// of parameters. Parameters should be provided by the `PixelKitEventV2` protocol (extending it if necessary) +/// and the call to `fire` should process those properties to serialize in the requests. +/// +public protocol PixelFiring { + @discardableResult + func fire(_ event: PixelKitEventV2) async throws -> Bool + + @discardableResult + func fire(_ event: PixelKitEventV2, + frequency: PixelKit.Frequency) async throws -> Bool +} + + +extension PixelKit: PixelFiring { + @discardableResult + public func fire(_ event: PixelKitEventV2) async throws -> Bool { + try await fire(event, frequency: .standard) + } + + @discardableResult + public func fire(_ event: PixelKitEventV2, + frequency: PixelKit.Frequency) async throws -> Bool { + try await withCheckedThrowingContinuation { continuation in + fire(event, frequency: frequency) { fired, error in + if let error { + continuation.resume(throwing: error) + return + } + + continuation.resume(returning: fired) + } + } + } +} diff --git a/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift b/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift new file mode 100644 index 0000000000..108a4a63ad --- /dev/null +++ b/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift @@ -0,0 +1,66 @@ +// +// PixelFireMock.swift +// +// Copyright © 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Foundation +import PixelKit + +public final class PixelKitMock: PixelFiring { + + /// An array of fire calls, in order, that this mock expects + /// + private let expectedFireCalls: [ExpectedFireCall] + + /// The actual fire calls + /// + private var actualFireCalls = [ExpectedFireCall]() + + public init(expecting expectedFireCalls: [ExpectedFireCall]) { + self.expectedFireCalls = expectedFireCalls + } + + public func fire(_ event: PixelKitEventV2) async throws -> Bool { + try await fire(event, frequency: .standard) + } + + public func fire(_ event: PixelKitEventV2, frequency: PixelKit.Frequency) async throws -> Bool { + let fireCall = ExpectedFireCall(pixel: event, frequency: frequency) + actualFireCalls.append(fireCall) + return true + } + + public var expectationsMet: Bool { + expectedFireCalls == actualFireCalls + } +} + +public struct ExpectedFireCall: Equatable { + let pixel: PixelKitEventV2 + let frequency: PixelKit.Frequency + + public init(pixel: PixelKitEventV2, frequency: PixelKit.Frequency) { + self.pixel = pixel + self.frequency = frequency + } + + public static func == (lhs: ExpectedFireCall, rhs: ExpectedFireCall) -> Bool { + lhs.pixel.name == rhs.pixel.name + && lhs.pixel.parameters == rhs.pixel.parameters + && (lhs.pixel.error as? NSError) == (rhs.pixel.error as? NSError) + && lhs.frequency == rhs.frequency + } +} diff --git a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift index 91221d4f2a..978cca109f 100644 --- a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift +++ b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift @@ -19,12 +19,14 @@ import Combine import Foundation import NetworkProtection +import PixelKit +import PixelKitTestingUtilities import XCTest @testable import DuckDuckGo_Privacy_Browser final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { - final class AttemptHandler: VPNStartAttemptHandling, VPNStopAttemptHandling { + final class AttemptHandler { enum ExpectedResult { case success @@ -73,54 +75,70 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { // MARK: - Tunnel Start Tests func testStartTunnelSuccess() async { + let pixelKit = PixelKitMock(expecting: [ + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.success, frequency: .dailyAndContinuous) + ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), loginItemsManager: MockLoginItemsManager(mockResult: .success), - ipcClient: MockIPCClient()) - let attemptHandler = AttemptHandler(expectedResult: .success) + ipcClient: MockIPCClient(), + pixelKit: pixelKit) - await controller.start(attemptHandler: attemptHandler) + await controller.start() - XCTAssertTrue(attemptHandler.expectationsMet) + XCTAssertTrue(pixelKit.expectationsMet) } func testStartTunnelLoginItemFailure() async { let error = NSError(domain: "test", code: 1) + let pixelKit = PixelKitMock(expecting: [ + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(error), frequency: .dailyAndContinuous) + ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), loginItemsManager: MockLoginItemsManager(mockResult: .failure(error)), - ipcClient: MockIPCClient()) - let attemptHandler = AttemptHandler(expectedResult: .failure(error)) + ipcClient: MockIPCClient(), + pixelKit: pixelKit) - await controller.start(attemptHandler: attemptHandler) + await controller.start() - XCTAssertTrue(attemptHandler.expectationsMet) + XCTAssertTrue(pixelKit.expectationsMet) } // MARK: - Tunnel Stop Tests func testStopTunnelSuccess() async { + let pixelKit = PixelKitMock(expecting: [ + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.success, frequency: .dailyAndContinuous) + ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), loginItemsManager: MockLoginItemsManager(mockResult: .success), - ipcClient: MockIPCClient()) - let attemptHandler = AttemptHandler(expectedResult: .success) + ipcClient: MockIPCClient(), + pixelKit: pixelKit) - await controller.stop(attemptHandler: attemptHandler) + await controller.stop() - XCTAssertTrue(attemptHandler.expectationsMet) + XCTAssertTrue(pixelKit.expectationsMet) } func testStopTunnelLoginItemFailure() async { let error = NSError(domain: "test", code: 1) + let pixelKit = PixelKitMock(expecting: [ + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(error), frequency: .dailyAndContinuous) + ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), loginItemsManager: MockLoginItemsManager(mockResult: .failure(error)), - ipcClient: MockIPCClient()) - let attemptHandler = AttemptHandler(expectedResult: .failure(error)) + ipcClient: MockIPCClient(), + pixelKit: pixelKit) - await controller.stop(attemptHandler: attemptHandler) + await controller.stop() - XCTAssertTrue(attemptHandler.expectationsMet) + XCTAssertTrue(pixelKit.expectationsMet) } } From 9d0a49bc2459e8e137b4070658f657b828b8dd70 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Mon, 15 Apr 2024 12:46:51 +0200 Subject: [PATCH 03/11] Fixes some small mistakes --- LocalPackages/PixelKit/Sources/PixelKit/PixelKit.swift | 6 ++++-- .../PixelKit/Sources/PixelKit/PixelKitEventV2.swift | 1 - .../Sources/PixelKitTestingUtilities/PixelKitMock.swift | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/LocalPackages/PixelKit/Sources/PixelKit/PixelKit.swift b/LocalPackages/PixelKit/Sources/PixelKit/PixelKit.swift index f55ec17ba4..f4e6e89731 100644 --- a/LocalPackages/PixelKit/Sources/PixelKit/PixelKit.swift +++ b/LocalPackages/PixelKit/Sources/PixelKit/PixelKit.swift @@ -277,12 +277,14 @@ public final class PixelKit { let newError: Error? - if let event = event as? PixelKitEventV2 { + if let event = event as? PixelKitEventV2, + let error = event.error { + // For v2 events we only consider the error specified in the event // and purposedly ignore the parameter in this call. // This is to encourage moving the error over to the protocol error // instead of still relying on the parameter of this call. - newError = event.error + newError = error } else { newError = error } diff --git a/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift b/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift index d6b2269afd..73da26d596 100644 --- a/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift +++ b/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift @@ -47,7 +47,6 @@ public protocol PixelFiring { frequency: PixelKit.Frequency) async throws -> Bool } - extension PixelKit: PixelFiring { @discardableResult public func fire(_ event: PixelKitEventV2) async throws -> Bool { diff --git a/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift b/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift index 108a4a63ad..df442748db 100644 --- a/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift +++ b/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift @@ -1,5 +1,5 @@ // -// PixelFireMock.swift +// PixelKitMock.swift // // Copyright © 2024 DuckDuckGo. All rights reserved. // From 7f893a1aba3d9f2dd480ec226d8453d30f7ab38e Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Mon, 15 Apr 2024 12:54:43 +0200 Subject: [PATCH 04/11] Removes obsolete code --- ...rkProtectionIPCTunnelControllerTests.swift | 46 ------------------- 1 file changed, 46 deletions(-) diff --git a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift index 978cca109f..387631488e 100644 --- a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift +++ b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift @@ -26,52 +26,6 @@ import XCTest final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { - final class AttemptHandler { - - enum ExpectedResult { - case success - case failure(_ error: Error) - } - - private var beginCallCount = 0 - private var successCallCount = 0 - private var failureCallCount = 0 - private var error: Error? - private let expectedResult: ExpectedResult - - init(expectedResult: ExpectedResult) { - self.expectedResult = expectedResult - } - - func begin() { - beginCallCount += 1 - } - - func success() { - successCallCount += 1 - } - - func failure(_ error: Error) { - failureCallCount += 1 - self.error = error - } - - var expectationsMet: Bool { - switch expectedResult { - case .success: - return beginCallCount == 1 && successCallCount == 1 && failureCallCount == 0 - case .failure(let error): - guard let expectedNSError = self.error as? NSError else { - return false - } - - let nsError = error as NSError - - return beginCallCount == 1 && successCallCount == 0 && failureCallCount == 1 && nsError == expectedNSError - } - } - } - // MARK: - Tunnel Start Tests func testStartTunnelSuccess() async { From 66772ec75d89d31cc76c1d88f8dcafb31f472785 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Mon, 15 Apr 2024 13:14:28 +0200 Subject: [PATCH 05/11] Rolls back some bad changes --- ...NetworkProtectionIPCTunnelController.swift | 12 +++++----- .../Sources/PixelKit/PixelKitEventV2.swift | 24 +++++-------------- .../PixelKitMock.swift | 7 +++--- 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift index b585032774..a76933d6b5 100644 --- a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift +++ b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift @@ -61,7 +61,7 @@ extension NetworkProtectionIPCTunnelController: TunnelController { @MainActor func start() async { - try? await pixelKit?.fire(StartAttempt.begin) + pixelKit?.fire(StartAttempt.begin) do { guard try await enableLoginItems() else { @@ -70,16 +70,16 @@ extension NetworkProtectionIPCTunnelController: TunnelController { } ipcClient.start() - try? await pixelKit?.fire(StartAttempt.success, frequency: .dailyAndContinuous) + pixelKit?.fire(StartAttempt.success, frequency: .dailyAndContinuous) } catch { os_log("🔴 IPC Controller found en error when starting the VPN: \(error)", log: .networkProtection) - try? await pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndContinuous) + pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndContinuous) } } @MainActor func stop() async { - try? await pixelKit?.fire(StopAttempt.begin) + pixelKit?.fire(StopAttempt.begin) do { guard try await enableLoginItems() else { @@ -88,10 +88,10 @@ extension NetworkProtectionIPCTunnelController: TunnelController { } ipcClient.stop() - try? await pixelKit?.fire(StopAttempt.success, frequency: .dailyAndContinuous) + pixelKit?.fire(StopAttempt.success, frequency: .dailyAndContinuous) } catch { os_log("🔴 IPC Controller found en error when starting the VPN: \(error)", log: .networkProtection) - try? await pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndContinuous) + pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndContinuous) } } diff --git a/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift b/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift index 73da26d596..02a220eae8 100644 --- a/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift +++ b/LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift @@ -39,32 +39,20 @@ public protocol PixelKitEventV2: PixelKitEvent { /// and the call to `fire` should process those properties to serialize in the requests. /// public protocol PixelFiring { - @discardableResult - func fire(_ event: PixelKitEventV2) async throws -> Bool + func fire(_ event: PixelKitEventV2) - @discardableResult func fire(_ event: PixelKitEventV2, - frequency: PixelKit.Frequency) async throws -> Bool + frequency: PixelKit.Frequency) } extension PixelKit: PixelFiring { - @discardableResult - public func fire(_ event: PixelKitEventV2) async throws -> Bool { - try await fire(event, frequency: .standard) + public func fire(_ event: PixelKitEventV2) { + fire(event, frequency: .standard) } - @discardableResult public func fire(_ event: PixelKitEventV2, - frequency: PixelKit.Frequency) async throws -> Bool { - try await withCheckedThrowingContinuation { continuation in - fire(event, frequency: frequency) { fired, error in - if let error { - continuation.resume(throwing: error) - return - } + frequency: PixelKit.Frequency) { - continuation.resume(returning: fired) - } - } + fire(event, frequency: frequency, onComplete: { _, _ in }) } } diff --git a/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift b/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift index df442748db..d2e5cdc617 100644 --- a/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift +++ b/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift @@ -33,14 +33,13 @@ public final class PixelKitMock: PixelFiring { self.expectedFireCalls = expectedFireCalls } - public func fire(_ event: PixelKitEventV2) async throws -> Bool { - try await fire(event, frequency: .standard) + public func fire(_ event: PixelKitEventV2) { + fire(event, frequency: .standard) } - public func fire(_ event: PixelKitEventV2, frequency: PixelKit.Frequency) async throws -> Bool { + public func fire(_ event: PixelKitEventV2, frequency: PixelKit.Frequency) { let fireCall = ExpectedFireCall(pixel: event, frequency: frequency) actualFireCalls.append(fireCall) - return true } public var expectationsMet: Bool { From 6f9652e40b6e3d1ddb993529a159630484037c01 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Wed, 17 Apr 2024 14:48:30 +0200 Subject: [PATCH 06/11] Fixes some swiftlint errors --- .../NetworkProtectionTestingSupport.swift | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift b/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift index 6723546952..8aeec1c4ff 100644 --- a/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift +++ b/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift @@ -36,31 +36,31 @@ struct MockFeatureVisibility: NetworkProtectionFeatureVisibility { func canStartVPN() async throws -> Bool { canStartVPNValue } - + func isVPNVisible() -> Bool { isVPNVisibleValue } - + func isNetworkProtectionBetaVisible() -> Bool { isNetworkProtectionBetaVisibleValue } - + func shouldUninstallAutomatically() -> Bool { shouldUninstallAutomaticallyValue } - + func disableForAllUsers() async { // Intentional no-op } - + func disableForWaitlistUsers() { // Intentional no-op } - + func disableIfUserHasNoAccess() async -> Bool { disableIfUserHasNoAccessValue } - + let onboardStatusPublisher: AnyPublisher init(isEligibleForThankYouMessage: Bool = false, @@ -98,7 +98,7 @@ struct MockServerInfoObserver: ConnectionServerInfoObserver { struct MockConnectionErrorObserver: ConnectionErrorObserver { var publisher: AnyPublisher = Just(nil).eraseToAnyPublisher() - var recentValue: String? = nil + var recentValue: String? } struct MockIPCClient: NetworkProtectionIPCClient { @@ -118,7 +118,7 @@ struct MockIPCClient: NetworkProtectionIPCClient { } struct MockLoginItemsManager: LoginItemsManaging { - + enum MockResult { case success case failure(_ error: Error) From 8c17a9491db85a331454eebd6270f5c5b781d014 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Thu, 18 Apr 2024 10:16:10 +0200 Subject: [PATCH 07/11] Makes several improvements to IPC error handling --- .../View/NetPPopoverManagerMock.swift | 9 +- ...etworkProtectionNavBarPopoverManager.swift | 4 +- ...NetworkProtectionIPCTunnelController.swift | 88 ++++++++++++++----- .../NetworkProtectionFeatureDisabler.swift | 4 +- .../TunnelControllerIPCService.swift | 14 ++- .../TunnelControllerIPCClient.swift | 18 ++-- .../TunnelControllerIPCServer.swift | 24 +++-- ...rkProtectionIPCTunnelControllerTests.swift | 8 +- 8 files changed, 117 insertions(+), 52 deletions(-) diff --git a/DuckDuckGo/NavigationBar/View/NetPPopoverManagerMock.swift b/DuckDuckGo/NavigationBar/View/NetPPopoverManagerMock.swift index 0af23bdcd5..211dcc460d 100644 --- a/DuckDuckGo/NavigationBar/View/NetPPopoverManagerMock.swift +++ b/DuckDuckGo/NavigationBar/View/NetPPopoverManagerMock.swift @@ -18,6 +18,7 @@ #if DEBUG +import AppKit import Combine import Foundation import NetworkProtection @@ -63,9 +64,13 @@ final class IPCClientMock: NetworkProtectionIPCClient { } var ipcControllerErrorMessageObserver: any NetworkProtection.ControllerErrorMesssageObserver = ControllerErrorMesssageObserverMock() - func start() {} + func start(completion: @escaping (Error?) -> Void) { + completion(nil) + } - func stop() {} + func stop(completion: @escaping (Error?) -> Void) { + completion(nil) + } } diff --git a/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarPopoverManager.swift b/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarPopoverManager.swift index 7bb6fa1363..5d635f7ded 100644 --- a/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarPopoverManager.swift +++ b/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarPopoverManager.swift @@ -33,8 +33,8 @@ protocol NetworkProtectionIPCClient { var ipcServerInfoObserver: ConnectionServerInfoObserver { get } var ipcConnectionErrorObserver: ConnectionErrorObserver { get } - func start() - func stop() + func start(completion: @escaping (Error?) -> Void) + func stop(completion: @escaping (Error?) -> Void) } extension TunnelControllerIPCClient: NetworkProtectionIPCClient { diff --git a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift index a76933d6b5..09843e888c 100644 --- a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift +++ b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift @@ -26,6 +26,27 @@ import PixelKit /// final class NetworkProtectionIPCTunnelController { + enum RequestError: CustomNSError { + case notAuthorizedToEnableLoginItem + case internalLoginItemError(_ error: Error) + + var errorCode: Int { + switch self { + case .notAuthorizedToEnableLoginItem: return 0 + case .internalLoginItemError: return 1 + } + } + + var errorUserInfo: [String: Any] { + switch self { + case .notAuthorizedToEnableLoginItem: + return [:] + case .internalLoginItemError(let error): + return [NSUnderlyingErrorKey: error as NSError] + } + } + } + private let featureVisibility: NetworkProtectionFeatureVisibility private let loginItemsManager: LoginItemsManaging private let ipcClient: NetworkProtectionIPCClient @@ -44,14 +65,16 @@ final class NetworkProtectionIPCTunnelController { // MARK: - Login Items Manager - private func enableLoginItems() async throws -> Bool { + private func enableLoginItems() async throws { guard try await featureVisibility.canStartVPN() else { - // We shouldn't enable the menu app is the VPN feature is disabled. - return false + throw RequestError.notAuthorizedToEnableLoginItem } - try loginItemsManager.throwingEnableLoginItems(LoginItemsManager.networkProtectionLoginItems, log: .networkProtection) - return true + do { + try loginItemsManager.throwingEnableLoginItems(LoginItemsManager.networkProtectionLoginItems, log: .networkProtection) + } catch { + throw RequestError.internalLoginItemError(error) + } } } @@ -63,17 +86,23 @@ extension NetworkProtectionIPCTunnelController: TunnelController { func start() async { pixelKit?.fire(StartAttempt.begin) + func handleFailure(_ error: Error) { + log(error) + pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndCount) + } + do { - guard try await enableLoginItems() else { - os_log("🔴 IPC Controller refusing to start the VPN menu app. Not authorized.", log: .networkProtection) - return + try await enableLoginItems() + + ipcClient.start { [pixelKit] error in + if let error { + handleFailure(error) + } else { + pixelKit?.fire(StartAttempt.success, frequency: .dailyAndCount) + } } - - ipcClient.start() - pixelKit?.fire(StartAttempt.success, frequency: .dailyAndContinuous) } catch { - os_log("🔴 IPC Controller found en error when starting the VPN: \(error)", log: .networkProtection) - pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndContinuous) + handleFailure(error) } } @@ -81,17 +110,23 @@ extension NetworkProtectionIPCTunnelController: TunnelController { func stop() async { pixelKit?.fire(StopAttempt.begin) + func handleFailure(_ error: Error) { + log(error) + pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndCount) + } + do { - guard try await enableLoginItems() else { - os_log("🔴 IPC Controller refusing to start the VPN. Not authorized.", log: .networkProtection) - return + try await enableLoginItems() + + ipcClient.stop { [pixelKit] error in + if let error { + handleFailure(error) + } else { + pixelKit?.fire(StopAttempt.success, frequency: .dailyAndCount) + } } - - ipcClient.stop() - pixelKit?.fire(StopAttempt.success, frequency: .dailyAndContinuous) } catch { - os_log("🔴 IPC Controller found en error when starting the VPN: \(error)", log: .networkProtection) - pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndContinuous) + handleFailure(error) } } @@ -108,6 +143,17 @@ extension NetworkProtectionIPCTunnelController: TunnelController { return false } } + + private func log(_ error: Error) { + switch error { + case RequestError.notAuthorizedToEnableLoginItem: + os_log("🔴 IPC Controller not authorized to enable the login item", log: .networkProtection) + case RequestError.internalLoginItemError(let error): + os_log("🔴 IPC Controller found an error while enabling the login item: \(error)", log: .networkProtection) + default: + os_log("🔴 IPC Controller found an unknown error: \(error)", log: .networkProtection) + } + } } // MARK: - Start Attempts diff --git a/DuckDuckGo/Waitlist/NetworkProtectionFeatureDisabler.swift b/DuckDuckGo/Waitlist/NetworkProtectionFeatureDisabler.swift index c04ed9c691..e6d085bc91 100644 --- a/DuckDuckGo/Waitlist/NetworkProtectionFeatureDisabler.swift +++ b/DuckDuckGo/Waitlist/NetworkProtectionFeatureDisabler.swift @@ -128,7 +128,9 @@ final class NetworkProtectionFeatureDisabler: NetworkProtectionFeatureDisabling } func stop() { - ipcClient.stop() + ipcClient.stop { _ in + // Intentional no-op + } } private func enableLoginItems() { diff --git a/DuckDuckGoVPN/TunnelControllerIPCService.swift b/DuckDuckGoVPN/TunnelControllerIPCService.swift index c8a9ce456d..e8df373d16 100644 --- a/DuckDuckGoVPN/TunnelControllerIPCService.swift +++ b/DuckDuckGoVPN/TunnelControllerIPCService.swift @@ -95,16 +95,26 @@ extension TunnelControllerIPCService: IPCServerInterface { server.statusChanged(statusReporter.statusObserver.recentValue) } - func start() { + func start(completion: @escaping (Error?) -> Void) { Task { await tunnelController.start() } + + // For IPC requests, completion means the IPC request was processed, and NOT + // that the requested operation was executed fully. Failure to complete the + // operation will be handled entirely within the tunnel controller. + completion(nil) } - func stop() { + func stop(completion: @escaping (Error?) -> Void) { Task { await tunnelController.stop() } + + // For IPC requests, completion means the IPC request was processed, and NOT + // that the requested operation was executed fully. Failure to complete the + // operation will be handled entirely within the tunnel controller. + completion(nil) } func resetAll(uninstallSystemExtension: Bool) async { diff --git a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/TunnelControllerIPCClient.swift b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/TunnelControllerIPCClient.swift index 769939f65e..f5158c239b 100644 --- a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/TunnelControllerIPCClient.swift +++ b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/TunnelControllerIPCClient.swift @@ -144,22 +144,16 @@ extension TunnelControllerIPCClient: IPCServerInterface { }) } - public func start() { + public func start(completion: @escaping (Error?) -> Void) { xpc.execute(call: { server in - server.start() - }, xpcReplyErrorHandler: { _ in - // Intentional no-op as there's no completion block - // If you add a completion block, please remember to call it here too! - }) + server.start(completion: completion) + }, xpcReplyErrorHandler: completion) } - public func stop() { + public func stop(completion: @escaping (Error?) -> Void) { xpc.execute(call: { server in - server.stop() - }, xpcReplyErrorHandler: { _ in - // Intentional no-op as there's no completion block - // If you add a completion block, please remember to call it here too! - }) + server.stop(completion: completion) + }, xpcReplyErrorHandler: completion) } public func debugCommand(_ command: DebugCommand) async throws { diff --git a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/TunnelControllerIPCServer.swift b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/TunnelControllerIPCServer.swift index 3fe62c74f3..f2a7adc439 100644 --- a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/TunnelControllerIPCServer.swift +++ b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/TunnelControllerIPCServer.swift @@ -31,11 +31,19 @@ public protocol IPCServerInterface: AnyObject { /// Start the VPN tunnel. /// - func start() + /// - Parameters: + /// - completion: the completion closure. This will be called as soon as the IPC request has been processed, and won't + /// signal successful completion of the request. + /// + func start(completion: @escaping (Error?) -> Void) /// Stop the VPN tunnel. /// - func stop() + /// - Parameters: + /// - completion: the completion closure. This will be called as soon as the IPC request has been processed, and won't + /// signal successful completion of the request. + /// + func stop(completion: @escaping (Error?) -> Void) /// Debug commands /// @@ -57,11 +65,11 @@ protocol XPCServerInterface { /// Start the VPN tunnel. /// - func start() + func start(completion: @escaping (Error?) -> Void) /// Stop the VPN tunnel. /// - func stop() + func stop(completion: @escaping (Error?) -> Void) /// Debug commands /// @@ -144,12 +152,12 @@ extension TunnelControllerIPCServer: XPCServerInterface { serverDelegate?.register() } - func start() { - serverDelegate?.start() + func start(completion: @escaping (Error?) -> Void) { + serverDelegate?.start(completion: completion) } - func stop() { - serverDelegate?.stop() + func stop(completion: @escaping (Error?) -> Void) { + serverDelegate?.stop(completion: completion) } func debugCommand(_ payload: Data, completion: @escaping (Error?) -> Void) { diff --git a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift index 387631488e..178ba61b3c 100644 --- a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift +++ b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift @@ -31,7 +31,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { func testStartTunnelSuccess() async { let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.success, frequency: .dailyAndContinuous) + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.success, frequency: .dailyAndCount) ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), @@ -48,7 +48,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { let error = NSError(domain: "test", code: 1) let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(error), frequency: .dailyAndContinuous) + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(error), frequency: .dailyAndCount) ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), @@ -66,7 +66,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { func testStopTunnelSuccess() async { let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.success, frequency: .dailyAndContinuous) + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.success, frequency: .dailyAndCount) ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), @@ -83,7 +83,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { let error = NSError(domain: "test", code: 1) let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(error), frequency: .dailyAndContinuous) + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(error), frequency: .dailyAndCount) ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), From 74e421e73d8ebc0a585f5a4987aa1802cc01aa08 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Thu, 18 Apr 2024 10:33:03 +0200 Subject: [PATCH 08/11] Fixes the unit tests for NetworkProtectionIPCTunnelController and adds more tests --- ...NetworkProtectionIPCTunnelController.swift | 2 +- .../PixelKitMock.swift | 5 +- ...rkProtectionIPCTunnelControllerTests.swift | 52 ++++++++++++++++--- .../NetworkProtectionTestingSupport.swift | 17 +++--- 4 files changed, 61 insertions(+), 15 deletions(-) diff --git a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift index 09843e888c..7e05352066 100644 --- a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift +++ b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift @@ -112,7 +112,7 @@ extension NetworkProtectionIPCTunnelController: TunnelController { func handleFailure(_ error: Error) { log(error) - pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndCount) + pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndCount) } do { diff --git a/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift b/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift index d2e5cdc617..dd9bf5390f 100644 --- a/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift +++ b/LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelKitMock.swift @@ -18,6 +18,7 @@ import Foundation import PixelKit +import XCTest public final class PixelKitMock: PixelFiring { @@ -42,8 +43,8 @@ public final class PixelKitMock: PixelFiring { actualFireCalls.append(fireCall) } - public var expectationsMet: Bool { - expectedFireCalls == actualFireCalls + public func verifyExpectations(file: StaticString, line: UInt) { + XCTAssertEqual(expectedFireCalls, actualFireCalls, file: file, line: line) } } diff --git a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift index 178ba61b3c..b2376ee397 100644 --- a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift +++ b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift @@ -41,15 +41,18 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { await controller.start() - XCTAssertTrue(pixelKit.expectationsMet) + pixelKit.verifyExpectations(file: #file, line: #line) } func testStartTunnelLoginItemFailure() async { let error = NSError(domain: "test", code: 1) + let expectedError = NetworkProtectionIPCTunnelController.RequestError.internalLoginItemError(error) + let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(error), frequency: .dailyAndCount) + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(expectedError), frequency: .dailyAndCount) ]) + let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), loginItemsManager: MockLoginItemsManager(mockResult: .failure(error)), @@ -58,7 +61,24 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { await controller.start() - XCTAssertTrue(pixelKit.expectationsMet) + pixelKit.verifyExpectations(file: #file, line: #line) + } + + func testStartTunnelIPCFailure() async { + let error = NSError(domain: "test", code: 1) + let pixelKit = PixelKitMock(expecting: [ + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(error), frequency: .dailyAndCount) + ]) + let controller = NetworkProtectionIPCTunnelController( + featureVisibility: MockFeatureVisibility(), + loginItemsManager: MockLoginItemsManager(mockResult: .success), + ipcClient: MockIPCClient(error: error), + pixelKit: pixelKit) + + await controller.start() + + pixelKit.verifyExpectations(file: #file, line: #line) } // MARK: - Tunnel Stop Tests @@ -76,15 +96,18 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { await controller.stop() - XCTAssertTrue(pixelKit.expectationsMet) + pixelKit.verifyExpectations(file: #file, line: #line) } func testStopTunnelLoginItemFailure() async { let error = NSError(domain: "test", code: 1) + let expectedError = NetworkProtectionIPCTunnelController.RequestError.internalLoginItemError(error) + let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(error), frequency: .dailyAndCount) + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(expectedError), frequency: .dailyAndCount) ]) + let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), loginItemsManager: MockLoginItemsManager(mockResult: .failure(error)), @@ -93,6 +116,23 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { await controller.stop() - XCTAssertTrue(pixelKit.expectationsMet) + pixelKit.verifyExpectations(file: #file, line: #line) + } + + func testStopTunnelIPCFailure() async { + let error = NSError(domain: "test", code: 1) + let pixelKit = PixelKitMock(expecting: [ + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(error), frequency: .dailyAndCount) + ]) + let controller = NetworkProtectionIPCTunnelController( + featureVisibility: MockFeatureVisibility(), + loginItemsManager: MockLoginItemsManager(mockResult: .success), + ipcClient: MockIPCClient(error: error), + pixelKit: pixelKit) + + await controller.stop() + + pixelKit.verifyExpectations(file: #file, line: #line) } } diff --git a/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift b/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift index 8aeec1c4ff..e298ad553d 100644 --- a/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift +++ b/UnitTests/NetworkProtection/Support/NetworkProtectionTestingSupport.swift @@ -102,18 +102,23 @@ struct MockConnectionErrorObserver: ConnectionErrorObserver { } struct MockIPCClient: NetworkProtectionIPCClient { - var ipcStatusObserver: NetworkProtection.ConnectionStatusObserver = MockConnectionStatusObserver() - var ipcServerInfoObserver: NetworkProtection.ConnectionServerInfoObserver = MockServerInfoObserver() + private let error: Error? + var ipcStatusObserver: NetworkProtection.ConnectionStatusObserver = MockConnectionStatusObserver() + var ipcServerInfoObserver: NetworkProtection.ConnectionServerInfoObserver = MockServerInfoObserver() var ipcConnectionErrorObserver: NetworkProtection.ConnectionErrorObserver = MockConnectionErrorObserver() - func start() { - // Intentional no-op + init(error: Error? = nil) { + self.error = error } - func stop() { - // Intentional no-op + func start(completion: @escaping (Error?) -> Void) { + completion(error) + } + + func stop(completion: @escaping (Error?) -> Void) { + completion(error) } } From 79235e2b06672b0e4edd9014c47e88ac0b68b499 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Thu, 18 Apr 2024 10:57:10 +0200 Subject: [PATCH 09/11] Rolls back an unintentional change --- Configuration/BuildNumber.xcconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Configuration/BuildNumber.xcconfig b/Configuration/BuildNumber.xcconfig index 1bb6651810..b71e1d4a29 100644 --- a/Configuration/BuildNumber.xcconfig +++ b/Configuration/BuildNumber.xcconfig @@ -1 +1 @@ -CURRENT_PROJECT_VERSION = 166 +CURRENT_PROJECT_VERSION = 165 From efb8598b4ce24c98879348c1a920a20df364c4fe Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Thu, 18 Apr 2024 11:06:00 +0200 Subject: [PATCH 10/11] Fixes some build issues due to rebasing --- .../NetworkProtectionIPCTunnelController.swift | 8 ++++---- .../NetworkProtectionIPCTunnelControllerTests.swift | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift index 7e05352066..88a5fbfc94 100644 --- a/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift +++ b/DuckDuckGo/NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift @@ -88,7 +88,7 @@ extension NetworkProtectionIPCTunnelController: TunnelController { func handleFailure(_ error: Error) { log(error) - pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndCount) + pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndContinuous) } do { @@ -98,7 +98,7 @@ extension NetworkProtectionIPCTunnelController: TunnelController { if let error { handleFailure(error) } else { - pixelKit?.fire(StartAttempt.success, frequency: .dailyAndCount) + pixelKit?.fire(StartAttempt.success, frequency: .dailyAndContinuous) } } } catch { @@ -112,7 +112,7 @@ extension NetworkProtectionIPCTunnelController: TunnelController { func handleFailure(_ error: Error) { log(error) - pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndCount) + pixelKit?.fire(StopAttempt.failure(error), frequency: .dailyAndContinuous) } do { @@ -122,7 +122,7 @@ extension NetworkProtectionIPCTunnelController: TunnelController { if let error { handleFailure(error) } else { - pixelKit?.fire(StopAttempt.success, frequency: .dailyAndCount) + pixelKit?.fire(StopAttempt.success, frequency: .dailyAndContinuous) } } } catch { diff --git a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift index b2376ee397..0f7837391a 100644 --- a/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift +++ b/UnitTests/NetworkProtection/NetworkProtectionIPCTunnelControllerTests.swift @@ -31,7 +31,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { func testStartTunnelSuccess() async { let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.success, frequency: .dailyAndCount) + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.success, frequency: .dailyAndContinuous) ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), @@ -50,7 +50,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(expectedError), frequency: .dailyAndCount) + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(expectedError), frequency: .dailyAndContinuous) ]) let controller = NetworkProtectionIPCTunnelController( @@ -68,7 +68,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { let error = NSError(domain: "test", code: 1) let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(error), frequency: .dailyAndCount) + .init(pixel: NetworkProtectionIPCTunnelController.StartAttempt.failure(error), frequency: .dailyAndContinuous) ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), @@ -86,7 +86,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { func testStopTunnelSuccess() async { let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.success, frequency: .dailyAndCount) + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.success, frequency: .dailyAndContinuous) ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), @@ -105,7 +105,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(expectedError), frequency: .dailyAndCount) + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(expectedError), frequency: .dailyAndContinuous) ]) let controller = NetworkProtectionIPCTunnelController( @@ -123,7 +123,7 @@ final class NetworkProtectionIPCTunnelControllerTests: XCTestCase { let error = NSError(domain: "test", code: 1) let pixelKit = PixelKitMock(expecting: [ .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.begin, frequency: .standard), - .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(error), frequency: .dailyAndCount) + .init(pixel: NetworkProtectionIPCTunnelController.StopAttempt.failure(error), frequency: .dailyAndContinuous) ]) let controller = NetworkProtectionIPCTunnelController( featureVisibility: MockFeatureVisibility(), From 5ab50fe4c507c0b54d5439178388e74e2c083a42 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Thu, 18 Apr 2024 11:07:07 +0200 Subject: [PATCH 11/11] Rolls back an unintentional change --- Configuration/BuildNumber.xcconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Configuration/BuildNumber.xcconfig b/Configuration/BuildNumber.xcconfig index b71e1d4a29..1bb6651810 100644 --- a/Configuration/BuildNumber.xcconfig +++ b/Configuration/BuildNumber.xcconfig @@ -1 +1 @@ -CURRENT_PROJECT_VERSION = 165 +CURRENT_PROJECT_VERSION = 166