Test updating between app versions and nightly test runs

[niklasberglund]

This PR adds tests for verifying that settings are successfully migrated when updating from an older app version to the current app version. Also nightly test runs have been configured.

For running settings migration tests there's a new workflow iOS settings migration tests(ios-end-to-end-tests-settings-migration.yml). It installs an old app version, runs test changing settings, installs current app version and runs a test verifying that the settings are still changed. Since custom DNS cannot be set when DNS blockers are activated there has to be two runs where settings are changed and it's verified that they're still changed on new app version - one run for "use custom DNS" setting and one run for all other settings.

All tests are scheduled to run nightly(ios-end-to-end-tests.yml and ios-end-to-end-tests-settings-migration.yml)

The easiest way to test these changes is to trigger the workflow for settings migration(ios-end-to-end-tests-settings-migration.yml) on my mullvadvpn-app fork which will execute the tests on my computer and phone, but requires some coordination. The tests themselves can be tested locally by:

1. Check out commit db8e804996fbbbd6fe500e62785a7e81c8a68a5c
2. Run MullvadVPNUITestsChangeDNSSettings test plan
3. Check out latest commit in PR branch
4. Run MullvadVPNUITestsVerifyDNSSettingsChanged test plan
5. Check out commit db8e804996fbbbd6fe500e62785a7e81c8a68a5c
6. Run MullvadVPNUITestsChangeSettings
7. Check out latest commit in PR branch
8. Run MullvadVPNUITestsVerifySettingsChanged test plan

Note that if you want to run the tests locally there's a new configuration parameter in UITests.xcconfig which needs to be added:

SHOULD_BE_REACHABLE_DOMAIN = mullvad.net

---

This change is 

[linear]

IOS-424 Test updating between previous stable and current stable versions

[buggmagnet]

Reviewed 20 of 26 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 21 of 26 files reviewed, 6 unresolved discussions (waiting on @niklasberglund)

---

.github/workflows/ios-end-to-end-tests.yml line 31 at r2 (raw file):

    steps:
      - name: Update Rust toolchain
        run: rustup update

This probably will not work since the changes made last friday.
Here's what we want to do instead

run: |
  rustup default stable
  rustup update stable

---

.github/workflows/ios-end-to-end-tests-settings-migration.yml line 22 at r2 (raw file):

    steps:
      - name: Update Rust toolchain
        run: rustup update

run: |
  rustup default stable
  rustup update stable

Code quote:

run: rustup update

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 620 at r2 (raw file):

		8529693A2B4F0238007EAD4C /* TermsOfServicePage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 852969392B4F0238007EAD4C /* TermsOfServicePage.swift */; };
		8529693C2B4F0257007EAD4C /* Alert.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8529693B2B4F0257007EAD4C /* Alert.swift */; };
		852A26462BA9C9CB006EB9C8 /* DNSSettingsPage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 852A26452BA9C9CB006EB9C8 /* DNSSettingsPage.swift */; };

Don't forget to "Sort By Name" on the "Pages" folder

---

ios/MullvadVPNUITests/README.md line 17 at r2 (raw file):

 - `sudo installer -pkg yeetd-normal.pkg -target yeetd`
6. Install ios-deploy
  - `brew install ios-deploy`

Are we actually using this ?

---

ios/MullvadVPNUITests/Pages/DNSSettingsPage.swift line 19 at r2 (raw file):

    }

    private func assertSwitchOn(accessibilityIdentifier: AccessibilityIdentifier) {

Can't we have this return self so that all the functions using it can avoid returning ?
It would look like this

    @discardableResult private func assertSwitchOn(accessibilityIdentifier: AccessibilityIdentifier) -> Self {
        let switchElement = app.cells[accessibilityIdentifier]
            .switches[AccessibilityIdentifier.customSwitch]

        guard let switchValue = switchElement.value as? String else {
            XCTFail("Failed to read switch state")
            return self
        }

        XCTAssertEqual(switchValue, "1")
        return self
    }

    @discardableResult func verifyBlockAdsSwitchOn() -> Self {
        assertSwitchOn(accessibilityIdentifier: .blockAdvertising)
    }

---

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

    @discardableResult func verifyCustomWireGuardPortSelected(portNumber: String) -> Self {
        let cell = app.cells[AccessibilityIdentifier.wireGuardCustomPort]
        XCTAssert(cell.isSelected)

XCTAssertTrue(cell.isSelected)

Code quote:

XCTAssert(cell.isSelected)

[pinkisemils]

Reviewable status: 21 of 26 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

---

.github/workflows/ios-end-to-end-tests.yml line 31 at r2 (raw file):

Previously, buggmagnet wrote…

This probably will not work since the changes made last friday.
Here's what we want to do instead

run: |
  rustup default stable
  rustup update stable

It should really be

      - name: Configure Rust
        uses: actions-rs/toolchain@v1
        with:
          toolchain: stable
          override: true
          target: aarch64-apple-ios-sim

[pinkisemils]

Reviewable status: 21 of 26 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

---

.github/workflows/ios-end-to-end-tests-settings-migration.yml line 77 at r2 (raw file):

      - name: Change all other settings on old app version
        uses: ./.github/actions/ios-end-to-end-tests
        if: always()

What does this line do?

[niklasberglund]

Reviewable status: 17 of 26 files reviewed, 6 unresolved discussions (waiting on @buggmagnet)

---

.github/workflows/ios-end-to-end-tests.yml line 31 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

It should really be

      - name: Configure Rust
        uses: actions-rs/toolchain@v1
        with:
          toolchain: stable
          override: true
          target: aarch64-apple-ios-sim

Should it always be the latest Rust toolchain or should 1.77.0 be specified? In other workflows a version is specified.

      - name: Configure Rust
        uses: actions-rs/toolchain@v1
        with:
          toolchain: 1.77.0
          override: true
          target: aarch64-apple-ios-sim

---

.github/workflows/ios-end-to-end-tests-settings-migration.yml line 77 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

What does this line do?

By default a step won't run if any previous step has failed. I believe the default condition is success(), so by specifying always() it will run even if any previous step failed.

So I specified that all four tests(changing settings and validating settings) should run even if anything has failed. The reason I think they should always run is because for example maybe something fails when setting custom DNS server, but then it's still meaningful to run the tests for all other settings.

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 620 at r2 (raw file):

Previously, buggmagnet wrote…

Don't forget to "Sort By Name" on the "Pages" folder

Sorted now

---

ios/MullvadVPNUITests/README.md line 17 at r2 (raw file):

Previously, buggmagnet wrote…

Are we actually using this ?

ios-deploy is added as a dependency for uninstalling the app in ios-end-to-end-tests-settings-migration.yml. By the way let me know if the team have another preferred tool that can uninstall apps.

---

ios/MullvadVPNUITests/Pages/DNSSettingsPage.swift line 19 at r2 (raw file):

Previously, buggmagnet wrote…

Can't we have this return self so that all the functions using it can avoid returning ?
It would look like this

    @discardableResult private func assertSwitchOn(accessibilityIdentifier: AccessibilityIdentifier) -> Self {
        let switchElement = app.cells[accessibilityIdentifier]
            .switches[AccessibilityIdentifier.customSwitch]

        guard let switchValue = switchElement.value as? String else {
            XCTFail("Failed to read switch state")
            return self
        }

        XCTAssertEqual(switchValue, "1")
        return self
    }

    @discardableResult func verifyBlockAdsSwitchOn() -> Self {
        assertSwitchOn(accessibilityIdentifier: .blockAdvertising)
    }

Ah yes that's better. Updated to use this approach.

---

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

Previously, buggmagnet wrote…

XCTAssertTrue(cell.isSelected)

Updated to use XCTAssertTrue

[pinkisemils]

Reviewable status: 17 of 26 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

---

.github/workflows/ios-end-to-end-tests.yml line 31 at r2 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Should it always be the latest Rust toolchain or should 1.77.0 be specified? In other workflows a version is specified.

      - name: Configure Rust
        uses: actions-rs/toolchain@v1
        with:
          toolchain: 1.77.0
          override: true
          target: aarch64-apple-ios-sim

The toolchain version should still be stable, as discussed offline.

---

.github/workflows/ios-end-to-end-tests-settings-migration.yml line 77 at r2 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

By default a step won't run if any previous step has failed. I believe the default condition is success(), so by specifying always() it will run even if any previous step failed.

So I specified that all four tests(changing settings and validating settings) should run even if anything has failed. The reason I think they should always run is because for example maybe something fails when setting custom DNS server, but then it's still meaningful to run the tests for all other settings.

Sounds good!

[niklasberglund]

Reviewable status: 16 of 26 files reviewed, 6 unresolved discussions (waiting on @buggmagnet)

---

.github/workflows/ios-end-to-end-tests.yml line 31 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

The toolchain version should still be stable, as discussed offline.

See updated step. I don't understand what override is for though. The documentation says "Set installed toolchain as an override for the current directory" and the default value is false. Do you know what it does?

[pinkisemils]

Reviewable status: 16 of 26 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

---

.github/workflows/ios-end-to-end-tests.yml line 31 at r2 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

See updated step. I don't understand what override is for though. The documentation says "Set installed toolchain as an override for the current directory" and the default value is false. Do you know what it does?

It ensures that any other config is overridden. It's just a precaution to ensure that the toolchain that is installed by default doesn't get used.

[niklasberglund]

Reviewable status: 16 of 26 files reviewed, 6 unresolved discussions (waiting on @buggmagnet)

---

.github/workflows/ios-end-to-end-tests.yml line 31 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

It ensures that any other config is overridden. It's just a precaution to ensure that the toolchain that is installed by default doesn't get used.

Ah thanks, makes sense its enabled then 👍

---

.github/workflows/ios-end-to-end-tests-settings-migration.yml line 22 at r2 (raw file):

Previously, buggmagnet wrote…

run: |
  rustup default stable
  rustup update stable

See discussion and changes above

[rablador]

Reviewed 21 of 26 files at r1, 3 of 4 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

---

ios/MullvadVPN/View controllers/VPNSettings/CustomDNSViewController.swift line 51 at r4 (raw file):

        )

        let editDoneButton = editButtonItem

You have explained why this is necessary before, but perhaps it would be good to add a comment in the code as well.

---

ios/MullvadVPNUITests/Networking/Networking.swift line 66 at r4 (raw file):

    }

    /// Get configured ad serving domain as URL object

Is this only used for testing? Perhaps it could live in a private Networking extension there instead?

[buggmagnet]

Reviewed 5 of 26 files at r1, 3 of 4 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @niklasberglund)

[rablador]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

[niklasberglund]

Reviewable status: 23 of 26 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/View controllers/VPNSettings/CustomDNSViewController.swift line 51 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

You have explained why this is necessary before, but perhaps it would be good to add a comment in the code as well.

I'm not buying my explanation so had another look at this and it is possible to use a more straigh forward approach so modified it

---

ios/MullvadVPNUITests/Networking/Networking.swift line 66 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Is this only used for testing? Perhaps it could live in a private Networking extension there instead?

Removed this function - it is unused and was removed as part of another PR but don't know what happened, maybe a merge conflict brought it back.

[rablador]

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

[rablador]

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