Migrate end to end tests to staging and fix related issues

[niklasberglund]

In this branch the tests are running against staging production instead of production. The following Linear tickets have been addressed:

• Create client for partner API and use partner API for adding time to temporary accounts(https://linear.app/mullvad/issue/IOS-611/create-a-client-for-partner-api)
• Store test artifacts on GitHub to ease debugging(https://linear.app/mullvad/issue/IOS-614/store-test-artifacts-on-github)
• Store logs from debug builds as artifacts(https://linear.app/mullvad/issue/IOS-623/store-logs-from-debug-builds)
• Generate new temporary accounts for tests instead of using persistent accounts which might expire or have too many devices(https://linear.app/mullvad/issue/IOS-613/add-ability-to-generate-an-account-per-test)
• Run end to end tests in staging environment(https://linear.app/mullvad/issue/IOS-612/run-end-to-end-tests-in-staging-environment)
• Retry login when failing(https://linear.app/mullvad/issue/IOS-622/attempt-login-three-times-before-failing-test)

Emils also fixed an issue with the API client did not work with staging.

When running tests against staging instead of production there was some unexpected issues which has been addressed too, such as timing issues caused by staging responding faster than production and differences in number location and relay availability.

Regressions in main has also been adressed in this PR, and an input parameter was added for when the workflow is manually triggered. You can now optionally specify a test case/suite to run. The format must be MullvadVPNUITests/test-suite-name/test-case-name for example MullvadVPNUITests/RelayTests/testAppConnection to run testAppConnection or MullvadVPNUITests/RelayTests to run all test cases under the suite RelayTests.

To try out the changes in this PR you might want to run all end to end tests. Do so either by triggering the workflow and specifying branch create-a-client-for-partner-api-ios-611 https://github.com/mullvad/mullvadvpn-app/actions/workflows/ios-end-to-end-tests.yml or by running them all locally from Xcode.

If running the tests locally you must first make sure your UITests.xcconfig has the two new configuration properties introduced:

// URL for Mullvad provided JSON data with information about the connection. https://am.i.mullvad.net/json for production, https://am.i.stagemole.eu/json for staging.
AM_I_JSON_URL = https:/${}/am.i.stagemole.eu/json

// Specify whether app logs should be extracted and attached to test report for failing tests
ATTACH_APP_LOGS_ON_FAILURE = 0

⚠️ Note that there's a backend bug causing connecting to relay to sometimes fail when doing so immediately after creating an account and adding time to it using partner API. testAppConnection and maybe also testCustomDNS are expected to sometimes fail with XCTAssertTrue failed - Failed to verify that the Internet can be acccessed ⚠️

---

This change is 

[linear]

IOS-611 Create a client for partner API

[rablador]

Reviewed 5 of 6 files at r1, 55 of 55 files at r2, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @niklasberglund)

---

ios/MullvadVPN/Views/StatusImageView.swift line 44 at r2 (raw file):

        // swiftlint:disable:next unused_setter_value
        set {
            print("This accessibilityValue property is get only")

I think we can go with a fatalError() here instead since it would be a programmer error to use the setter.

---

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

        removeFirewallRulesInTearDown = true

        addTeardownBlock {

Maybe put the contents in this teardown to a shared function istead?

---

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

    /// Throttle what's in the callback. This is used for throttling requests to the app API. All requests should be throttled or else we might be rate limited. 5 requests per second allowed.
    private func throttle(callback: @escaping () -> Void) {

Combine lib has a throttle function: throttle(for:scheduler:latest:). You can find an example of it in PacketTunnelPathObserver.

---

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

                requestError = MullvadAPIError.requestError
                requestCompletedExpectation.fulfill()
            }

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

---

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

            requestError = MullvadAPIError.requestError
            requestCompletedExpectation.fulfill()
        }

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

---

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

                addDeviceError = MullvadAPIError.requestError
                requestCompletedExpectation.fulfill()
            }

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

---

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

                requestError = MullvadAPIError.requestError
                requestCompletedExpectation.fulfill()
            }

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

---

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

                requestError = MullvadAPIError.requestError
                requestCompletedExpectation.fulfill()
            }

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

---

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

                  error == nil else {
                XCTFail("Error: \(error?.localizedDescription ?? "Unknown error")")
                return

We should probably fulfull the expectation here as well to be consistent with the rest of the function.

---

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

    }

    @discardableResult public func verifySuccessIconShown() -> Self {

I see why this works now, but is it reliable long-term? Hard check at 0.2 secs seems prone to timing issues depending on devide and/or platform in the future.

---

ios/MullvadVPNUITests/Pages/LoginPage.swift line 72 at r2 (raw file):

    /// Wait for success icon to be shown. Returns true if shown, false if timing out without the icon being shown
    func waitForSuccessIcon() throws -> Self {

This function is not used anywhere.

---

ios/MullvadVPNUITests/Pages/SelectLocationPage.swift line 74 at r2 (raw file):

            .matching(identifier: AccessibilityIdentifier.openCustomListsMenuButton.rawValue).allElementsBoundByIndex

        for ellipsisButton in customListEllipsisButtons where ellipsisButton.isHittable {

This is a workaround for the weird behavior of the select location view headers. Would be good to have a comment on why we do this.

[niklasberglund]

Reviewable status: 4 of 60 files reviewed, 11 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/Views/StatusImageView.swift line 44 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think we can go with a fatalError() here instead since it would be a programmer error to use the setter.

Good point, sounds like good use of fatalError()

---

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

Previously, rablador (Jon Petersson) wrote…

Maybe put the contents in this teardown to a shared function istead?

Done

---

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

Previously, rablador (Jon Petersson) wrote…

Combine lib has a throttle function: throttle(for:scheduler:latest:). You can find an example of it in PacketTunnelPathObserver.

We had an AFK talk about not using Combine framework's throttle because it's more oriented towards publishing data changes

---

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

Previously, rablador (Jon Petersson) wrote…

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

Ah yes. Moved it outside.

---

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

Previously, rablador (Jon Petersson) wrote…

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

Moved

---

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

Previously, rablador (Jon Petersson) wrote…

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

Moved

---

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

Previously, rablador (Jon Petersson) wrote…

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

Moved

---

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

Previously, rablador (Jon Petersson) wrote…

requestCompletedExpectation.fulfill() is same for both try and catch and can be moved outside.

Moved

---

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

Previously, rablador (Jon Petersson) wrote…

We should probably fulfull the expectation here as well to be consistent with the rest of the function.

I like it. Fulfilling the expectation now, and asserting that there is no error.

---

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

Previously, rablador (Jon Petersson) wrote…

I see why this works now, but is it reliable long-term? Hard check at 0.2 secs seems prone to timing issues depending on devide and/or platform in the future.

waitForElement is unreliable because the polling is slower. So polling every 0.2 seconds is an improvement, but it's not bullet proof for any future changes. It might work fine with a shorter polling interval too, but if the success icon is shown for less than 0.2 seconds it might start feeling like flickering.

I can't think of a more reliable way to do it, I think it's inevitable that if the app allows the icon to show for a very short time then the polling interval must be adjusted. How do you think?

---

ios/MullvadVPNUITests/Pages/LoginPage.swift line 72 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

This function is not used anywhere.

Removed

---

ios/MullvadVPNUITests/Pages/SelectLocationPage.swift line 74 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

This is a workaround for the weird behavior of the select location view headers. Would be good to have a comment on why we do this.

Good idea. Added comment

[rablador]

Reviewed 52 of 55 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @niklasberglund)

---

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

Previously, niklasberglund (Niklas Berglund) wrote…

We had an AFK talk about not using Combine framework's throttle because it's more oriented towards publishing data changes

Yeah, let's drop this one.

---

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

Previously, niklasberglund (Niklas Berglund) wrote…

waitForElement is unreliable because the polling is slower. So polling every 0.2 seconds is an improvement, but it's not bullet proof for any future changes. It might work fine with a shorter polling interval too, but if the success icon is shown for less than 0.2 seconds it might start feeling like flickering.

I can't think of a more reliable way to do it, I think it's inevitable that if the app allows the icon to show for a very short time then the polling interval must be adjusted. How do you think?

Let's keep it this way.

---

dist-assets/binaries line 1 at r5 (raw file):

Subproject commit 0d9d7c09e14a3b661575d536c2af6f27977d8e45

Why is this changed?

[niklasberglund]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

---

dist-assets/binaries line 1 at r5 (raw file):

Previously, rablador (Jon Petersson) wrote…

Why is this changed?

This branch has a commit from @pinkisemils which included changes to dist-assets under a submodule and it created merge conflicts. I'm not sure about the details but Emils asked me to modify the commit and drop those changes.

[rablador]

Reviewable status: complete! all files reviewed, all discussions resolved

[rablador]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @niklasberglund)

---

ios/MullvadVPNUITests/RelayTests.swift line 247 at r6 (raw file):

    }

    /// Connect to a realy in the default country and city, get name and IP address of the relay the app successfully connects to. Assumes user is logged on and at tunnel control page.

Nit: relay

[rablador]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @niklasberglund)

[buggmagnet]

Reviewed 4 of 6 files at r1, 50 of 55 files at r4, 4 of 4 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @niklasberglund)

---

ios/MullvadVPNUITests/README.md line 8 at r7 (raw file):

3. Make sure device is configured in GitHub secrets(see *GitHub setup* below)
4. Make sure the test device is connected to the WiFi `app-team-ios-tests`
5. Make sure iCloud syncing of keychain is off on the device so that the device isn't getting WiFi passwords from another device causing it to sometimes connect to another WiFi.

nit:

This comment made me laugh and cry at the same time.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 41 at r7 (raw file):

    /// Throttle what's in the callback. This is used for throttling requests to the app API. All requests should be throttled or else we might be rate limited. 5 requests per second allowed.
    private func throttle(callback: @escaping () -> Void) {

This is only making sure each request is done at least .25 seconds after the previous one, it's not making sure only 5 requests per seconds are allowed.
If we want to make that happen, we should either use a token bucket, or have a custom rate limiter.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 112 at r7 (raw file):

        let requestCompletedExpectation = XCTestExpectation(description: "Delete account request completed")

        do {

Shouldn't we throttle this too ?

---

ios/MullvadVPNUITests/Test base classes/BaseUITestCase.swift line 58 at r7 (raw file):

    /// Return account with time after done using it This is neccessary because if a temporary account was created we want to delete it.
    func returnAccountWithTime(accountNumber: String) {

From that function name alone, I definitely did not expect the account to be deleted.
Why not call it deleteAccountWithTime instead ?

---

ios/MullvadVPNUITests/Test base classes/LoggedInWithTimeUITestCase.swift line 45 at r7 (raw file):

        }

        app.terminate() // Terminate app to make sure we have Internet connectivity

Why would terminating the app ensure there is internet connectivity ?

[rablador]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @niklasberglund)

[niklasberglund]

Reviewable status: 57 of 60 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPNUITests/RelayTests.swift line 247 at r6 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: relay

Corrected

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 41 at r7 (raw file):

Previously, buggmagnet wrote…

This is only making sure each request is done at least .25 seconds after the previous one, it's not making sure only 5 requests per seconds are allowed.
If we want to make that happen, we should either use a token bucket, or have a custom rate limiter.

5 requests per second is the maximum allowed by the API, the throttling makes sure we are well within this limit. I think it is a feature that we're not near the 5 times per second limit? 😊 Clarified the comment a bit

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 112 at r7 (raw file):

Previously, buggmagnet wrote…

Shouldn't we throttle this too ?

Added throttling to this too 👍

---

ios/MullvadVPNUITests/Test base classes/BaseUITestCase.swift line 58 at r7 (raw file):

Previously, buggmagnet wrote…

From that function name alone, I definitely did not expect the account to be deleted.
Why not call it deleteAccountWithTime instead ?

It's a good point. I think the issue might be in the documentation. The account is "borrowed" - if a partner API token has been configured it is a temporary account which will be deleted here. If it is a reused account(HAS_TIME_ACCOUNT_NUMBER) nothing will be done.

See improved documentation for returnAccountWithTime() and getAccountWithTime(). How do you think?

[niklasberglund]

Reviewable status: 57 of 60 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPNUITests/Test base classes/LoggedInWithTimeUITestCase.swift line 45 at r7 (raw file):

Previously, buggmagnet wrote…

Why would terminating the app ensure there is internet connectivity ?

Beats me! 😄 This line didn't make much sense - neither code nor comment. It is not needed since the app is not launched in this teardown, and terminating the app wouldn't cause a disconnect. I removed the entire line.

[buggmagnet]

Reviewed 1 of 2 files at r8, 2 of 3 files at r9, 2 of 3 files at r10, all commit messages.
Reviewable status: 59 of 60 files reviewed, 4 unresolved discussions (waiting on @niklasberglund and @rablador)

[niklasberglund]

Reviewable status: 58 of 60 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 41 at r7 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

5 requests per second is the maximum allowed by the API, the throttling makes sure we are well within this limit. I think it is a feature that we're not near the 5 times per second limit? 😊 Clarified the comment a bit

@buggmagnet see comment added below regarding "throttle" implementation ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift:54

[rablador]

Reviewed 1 of 3 files at r9, 3 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

---

ios/MullvadVPNUITests/Test base classes/BaseUITestCase.swift line 58 at r7 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

It's a good point. I think the issue might be in the documentation. The account is "borrowed" - if a partner API token has been configured it is a temporary account which will be deleted here. If it is a reused account(HAS_TIME_ACCOUNT_NUMBER) nothing will be done.

See improved documentation for returnAccountWithTime() and getAccountWithTime(). How do you think?

I think the wording is the problem, with "return" meaning the return of a borrowed account rather than return as in "get".

[buggmagnet]

Reviewed 1 of 3 files at r10, 1 of 1 files at r11.
Reviewable status: 59 of 60 files reviewed, 1 unresolved discussion (waiting on @niklasberglund and @rablador)

[buggmagnet]

Reviewed 1 of 10 files at r13, all commit messages.
Reviewable status: 53 of 63 files reviewed, 1 unresolved discussion (waiting on @niklasberglund and @rablador)

[buggmagnet]

Reviewable status: 53 of 63 files reviewed, 2 unresolved discussions (waiting on @niklasberglund and @rablador)

---

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 0 at r13 (raw file):
Let's undelete this

[niklasberglund]

Reviewable status: 49 of 63 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line at r13 (raw file):

Previously, buggmagnet wrote…

Let's undelete this

Undeleted

---

ios/MullvadVPNUITests/Test base classes/BaseUITestCase.swift line 58 at r7 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think the wording is the problem, with "return" meaning the return of a borrowed account rather than return as in "get".

See suggested rename returnAccountWithTime => deleteTemporaryAccountWithTime(and same for accoun without time)

[buggmagnet]

Reviewed 1 of 1 files at r12, 7 of 10 files at r13, 6 of 6 files at r14, 3 of 3 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @niklasberglund)

---

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 60 at r15 (raw file):

    /// Delete temporary account with time if a temporary account was used
    func deleteTemporaryAccountWithTime(accountNumber: String) {

Pairing up with Niklas, we are deciding to rename this deleteAccount(accountNumber: String) .

We want to create a strucutre for both accounts, and accounts created by the partner API, both using an Account protocol, that the BaseUITestCase will know whether to call the partner API to delete or not.

This will come in a future PR.