Add WireGuard tests for iOS app

[niklasberglund]

In this PR the following tests have been implemented:

• testWireGuardOverTCPManually - https://linear.app/mullvad/issue/IOS-431/test-wireguard-over-tcp-manually
• testWireGuardOverTCPAutomatically - https://linear.app/mullvad/issue/IOS-429/test-wireguard-over-tcp-works-automatically
• testWireGuardPortSettings - https://linear.app/mullvad/issue/IOS-434/test-wireguard-port-settings

To test it out you can run the three tests, they are all under RelayTests. Another configuration property SHOULD_BE_REACHABLE_DOMAIN = mullvad.net has been introduced so make sure to set it in your UITests.xcconfig file. You must be connected to the app-team-ios-tests wifi to be able to run tests interacting with the firewall running on the NUC.

---

This change is 

[linear]

IOS-431 Test WireGuard over TCP manually

[niklasberglund]

Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion

---

ios/MullvadVPNUITests/RelayTests.swift line 16 at r1 (raw file):

        super.setUp()

        FirewallAPIClient().removeRules()

Removing rules in both setUp and tearDown is a compromise. Because the firewall API cannot be reached when connected to VPN, and it's difficult and extends the time tests need to run if trying to make sure to always disconnect before invoking removeRules() from tearDown. In a perfect world tearDown would always make sure to disconnect from relay if connected.

[niklasberglund]

Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion

---

ios/MullvadVPNUITests/RelayTests.swift line 16 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Removing rules in both setUp and tearDown is a compromise. Because the firewall API cannot be reached when connected to VPN, and it's difficult and extends the time tests need to run if trying to make sure to always disconnect before invoking removeRules() from tearDown. In a perfect world tearDown would always make sure to disconnect from relay if connected.

Also testWireGuardOverTCPAutomatically is the only test in this suite creating a firewall rule but removeRules() runs for all. So there could be logic for only running it when needed. I was thinking it doesn't hurt to run it even when it isn't needed and it doesn't add much time but would be great to hear your thoughts.

[buggmagnet]

Reviewed 18 of 19 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @niklasberglund)

---

ios/MullvadVPNUITests/RelayTests.swift line 16 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Also testWireGuardOverTCPAutomatically is the only test in this suite creating a firewall rule but removeRules() runs for all. So there could be logic for only running it when needed. I was thinking it doesn't hurt to run it even when it isn't needed and it doesn't add much time but would be great to hear your thoughts.

I've been thinking about this since the discussion we had yesterday.
If the firewall API is running on a local network, it should still be reachable when the VPN is turned on, since the excludeLocalNetworks on the VPN connection on iOS is on by default (i.e. local traffic won't be routed through the VPN)
We should double check this, and take a closer look why your original test attempts didn't work.

---

ios/MullvadVPNUITests/RelayTests.swift line 80 at r2 (raw file):

    func testWireGuardOverTCPAutomatically() throws {
        let wireGuardGot001RelayIPAddress = "185.213.154.66"

We can't really do this because the server IP might change in the future.
It's okay to leave it for now, but what we should probably be doing is either leveraging the JSON file that contains all the relays IPs that we automatically download when we build the app, or do something similar to avoid having a static IP defined.

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 83 at r2 (raw file):

        var request = URLRequest(url: removeRulesURL)
        request.httpMethod = "DELETE"
        request.setValue("application/json", forHTTPHeaderField: "Content-Type")

Why did we remove that line ? What was not working ?

---

ios/MullvadVPNUITests/Networking/Networking.swift line 89 at r2 (raw file):

    /// Get configured domain to use for Internet connectivity checks
    private static func getShouldBeReachableDomain() throws -> String {

nit
The name doesn't read super well, I think we can rename this alwaysReachableDomain and get the same intended idea.

With this addition, the checks become more readable too IMHO

XCTAssertTrue(try canConnectSocket(host: alwaysReachableDomain(), port: "80"))

What do you think ?

---

ios/MullvadVPNUITests/Networking/Networking.swift line 154 at r2 (raw file):

    }

    /// Verify that the device do not have Internet connectivity

nit
does not have

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 31 at r2 (raw file):

        var capturedTexts: [String] = []

        while -startTime.timeIntervalSinceNow < timeout {

I understand what you're trying to do, but it looks really convoluted.
It's not performance critical code, so we should do Date().timeIntervalSince(startTime) instead for readability's sake

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 103 at r2 (raw file):

    /// Verify that the app attempts to connect over UDP before switching to TCP. For testing blocked UDP traffic.
    @discardableResult func verifyConnectingOverTCPAfterUDPAttempts() -> Self {

nit
The logic in this function is a bit hard to follow, so the comments are very appreciated.
I'm wondering if we can make it easier to follow. I'll think about it.

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 106 at r2 (raw file):

        let connectionAttempts = waitForConnectionAttempts(3, timeout: 15)

        var i = 0

Can we rename this to something more meaningful ?

[rablador]

Reviewed 18 of 19 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 770 at r2 (raw file):

        let navigationController = CustomNavigationController()
        navigationController.view.accessibilityIdentifier = .settingsContainerView

How come this isn't set on the view controller inside SettingsCoordinator instead?

---

ios/MullvadVPN/View controllers/Settings/SettingsViewController.swift line 58 at r2 (raw file):

        )
        doneButton.accessibilityIdentifier = .settingsDoneButton
        navigationItem.rightBarButtonItem = doneButton

I know we discussed the issues with navigation bar buttons before, but do we really need to go through these hoops to make it work?

---

ios/MullvadVPNUITests/RelayTests.swift line 16 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Also testWireGuardOverTCPAutomatically is the only test in this suite creating a firewall rule but removeRules() runs for all. So there could be logic for only running it when needed. I was thinking it doesn't hurt to run it even when it isn't needed and it doesn't add much time but would be great to hear your thoughts.

I'd say run removeRules() only when you needed, unless it's needed often enough to warrant a general inclusion. In general, I think we shouldn't proactively add stuff that we don't actually use.

---

ios/MullvadVPNUITests/RelayTests.swift line 59 at r2 (raw file):

        VPNSettingsPage(app)
            .tapWireGuardObfuscationExpandButton()
            .tapWireGuardObfuscationValueOnCell()

Nit: I'd prefer omitting Value here, as I accidentally read it as "tap the wireguard value on the cell".

---

ios/MullvadVPNUITests/Networking/Networking.swift line 67 at r2 (raw file):

    /// Get configured ad serving domain as URL object
    private static func getAdServingDomainURL() -> URL? {

Old code, but I think function should be based on getAdServingDomain() below, eg.:

Code snippet:

guard let adServingDomain = getAdServingDomain(),
      let adServingDomainURL = URL(string: adServingDomain) else {
    XCTFail("Ad serving domain not configured")
    return nil
}

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 29 at r2 (raw file):

        let inAddressRow = app.otherElements[AccessibilityIdentifier.connectionPanelInAddressRow]
        var capturedTexts: [String] = []

capturedTexts doesn't seem to be used for anything.

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 42 at r2 (raw file):

            if let currentText = inAddressRow.value as? String {
                // Skip initial label value with IP address only - no port or protocol
                if currentText.contains(" ") == false {

Nit: More common to use guard when returning early.

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 106 at r2 (raw file):

Previously, buggmagnet wrote…

Can we rename this to something more meaningful ?

Perhaps the loops could be rewritten to include the index intead, eg.:

Code snippet:

 for (attemptCount, attempt) in connectionAttempts {
    if attemptCount < 2 {
        XCTAssertEqual(attempt.protocolName, "UDP")
    } else if attemptCount == 2 {
        XCTAssertEqual(attempt.protocolName, "TCP")
    } else {
        XCTFail("Unexpected connection attempt")
    }
}

---

ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 17 at r2 (raw file):

    }

    private func cellExpandButton(_ cellAccessiblityIdentifier: AccessibilityIdentifier) -> XCUIElement {

Nit: We look for AccessibilityIdentifier.collapseButton, but the function name is cellEXPANDButton 🙃

[niklasberglund]

Reviewable status: 14 of 20 files reviewed, 15 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 770 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

How come this isn't set on the view controller inside SettingsCoordinator instead?

I found it hard to follow the code here 😊 SettingsCoordinator sounds like a better place to set it but it's not a view. You mean it could be set inside SettingsCoordinator on presentedViewController?

---

ios/MullvadVPN/View controllers/Settings/SettingsViewController.swift line 58 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

I know we discussed the issues with navigation bar buttons before, but do we really need to go through these hoops to make it work?

This is to set accessibility identifier for the "Done" button in the upper right when in settings view

UIBarButtonItem don't have any initialiser accepting accessibility identifier so need to store it as a variable and set accessibilityIdentifier

---

ios/MullvadVPNUITests/RelayTests.swift line 16 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I'd say run removeRules() only when you needed, unless it's needed often enough to warrant a general inclusion. In general, I think we shouldn't proactively add stuff that we don't actually use.

@buggmagnet true, but I think it makes sense that it's failing because of the workaround we use to get around the local network restriction? 8.8.8.8 is pointing to a local IP address. So it is treated as an external IP address even though the machine is on the local network.

@rablador sounds good to me! Have updated it to run only before and after the test that actually create firewall rules.

---

ios/MullvadVPNUITests/RelayTests.swift line 59 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: I'd prefer omitting Value here, as I accidentally read it as "tap the wireguard value on the cell".

I like it. Have removed "Value" from the names.

---

ios/MullvadVPNUITests/RelayTests.swift line 80 at r2 (raw file):

Previously, buggmagnet wrote…

We can't really do this because the server IP might change in the future.
It's okay to leave it for now, but what we should probably be doing is either leveraging the JSON file that contains all the relays IPs that we automatically download when we build the app, or do something similar to avoid having a static IP defined.

See updated implementation where the test first connects to the relay to extract its IP address from the UI

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 83 at r2 (raw file):

Previously, buggmagnet wrote…

Why did we remove that line ? What was not working ?

It was added by mistake before. This request don't have any body so there's no need to specify a content type.

---

ios/MullvadVPNUITests/Networking/Networking.swift line 67 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Old code, but I think function should be based on getAdServingDomain() below, eg.:

Totally agree. I found that getAdServingDomainURL isn't even used though so removed it.

---

ios/MullvadVPNUITests/Networking/Networking.swift line 89 at r2 (raw file):

Previously, buggmagnet wrote…

nit
The name doesn't read super well, I think we can rename this alwaysReachableDomain and get the same intended idea.

With this addition, the checks become more readable too IMHO

XCTAssertTrue(try canConnectSocket(host: alwaysReachableDomain(), port: "80"))

What do you think ?

I like it 👍 See updated suggestion

---

ios/MullvadVPNUITests/Networking/Networking.swift line 154 at r2 (raw file):

Previously, buggmagnet wrote…

nit
does not have

Thanks, I've always been confused about do and does 😄 Will probably make this mistake again

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 29 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

capturedTexts doesn't seem to be used for anything.

True, it was left from an earlier implementation and I forgot to remove it. Removed now.

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 31 at r2 (raw file):

Previously, buggmagnet wrote…

I understand what you're trying to do, but it looks really convoluted.
It's not performance critical code, so we should do Date().timeIntervalSince(startTime) instead for readability's sake

Good idea 👍 Much easier to read

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 42 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: More common to use guard when returning early.

Updated to use guard

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 103 at r2 (raw file):

Previously, buggmagnet wrote…

nit
The logic in this function is a bit hard to follow, so the comments are very appreciated.
I'm wondering if we can make it easier to follow. I'll think about it.

I totally agree and was thinking about this too. One way to make it easier to follow could be breaking out the asserts to a separate function accepting an array of protocol names and their order and the attempts array. Something like assertAttemptOrder(connectionAttempts, ["UDP", "UDP", "TCP"]. Then in this function it would just be something like:

            if connectionAttempts.count == 3 { // Expected retries flow
                assertAttemptOrder(connectionAttempts, ["UDP", "UDP", "TCP"]
                }
            } else if connectionAttempts.count == 2 { // Most of the times this incorrect flow is shown
                assertAttemptOrder(connectionAttempts, ["UDP", "TCP"]
                }
            } else {
                XCTFail("Unexpected number of connection attempts")
            }

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 106 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Perhaps the loops could be rewritten to include the index intead, eg.:

I'm all for longer descriptive names 👍 Renamed i to attemptIndex. @rablador I like it. See updated syntax. Is this what you meant?

---

ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 17 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: We look for AccessibilityIdentifier.collapseButton, but the function name is cellEXPANDButton 🙃

Yea this bothers me too. Actually the button isn't a collapse button OR expand button but both 😊 I can't come up with a better convention than naming them "expand collapse button". There should be a better name?

[buggmagnet]

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @niklasberglund and @rablador)

---

ios/MullvadVPNUITests/Pages/TunnelControlPage.swift line 106 at r3 (raw file):

        // Should do three connection attempts but due to UI bug sometimes only two are displayed, sometimes all three
        if connectionAttempts.count == 3 { // Expected retries flow
            for (attemptIndex, attempt) in connectionAttempts.enumerated() {

That is much better now 👍

[rablador]

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

---

ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift line 770 at r2 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

I found it hard to follow the code here 😊 SettingsCoordinator sounds like a better place to set it but it's not a view. You mean it could be set inside SettingsCoordinator on presentedViewController?

Either that or SettingsViewController, which also resides there.

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.