Test account creation and deletion

[niklasberglund]

Implemented create and delete account tests. To test run createAccount and deleteAccount under AccountTests. This PR is building upon changes from #5824 so it will be easier to review after it has been merged.

---

This change is 

[linear]

IOS-444 Test account creation

[niklasberglund]

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

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 7534 at r3 (raw file):

				"PROVISIONING_PROFILE_SPECIFIER[sdk=iphoneos*]" = "MullvadVPN app integration tests";
				SECURITY_GROUP_IDENTIFIER = "";
				SWIFT_ACTIVE_COMPILATION_CONDITIONS = "DEBUG MULLVAD_ENVIRONMENT_STAGING";

The MULLVAD_ENVIRONMENT_STAGINGand MULLVAD_ENVIRONMENT_PRODUCTION flags are no longer used in this PR. They are used by the PR for problem report test though. Thinking they might as well remain here to avoid merge conflict or should be removed here?

[rablador]

Reviewed 4 of 30 files at r1, 29 of 29 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @niklasberglund)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 7534 at r3 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

The MULLVAD_ENVIRONMENT_STAGINGand MULLVAD_ENVIRONMENT_PRODUCTION flags are no longer used in this PR. They are used by the PR for problem report test though. Thinking they might as well remain here to avoid merge conflict or should be removed here?

They should be removed if this PR doesn't use them.

---

ios/MullvadVPNUITests/README.md line 27 at r3 (raw file):

## GitHub setup
1. Ask GitHub admin for new runner token and set it up according to the steps presented, pass `--labels ios-test` to `config.sh` when running it.

What are the presented steps?

---

ios/MullvadVPNUITests/README.md line 33 at r3 (raw file):

  - `IOS_NO_TIME_ACCOUNT_NUMBER`
  - `IOS_TEST_DEVICE_IDENTIFIER_UUID` - unique identifier for the test device. Create new identifier with `uuidgen`.
  - `IOS_TEST_DEVICE_UDID` - the iOS device's UDID.

What are these strange characters? Hard return?

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 39 at r3 (raw file):

    }

    public static func getAPIIPAddress() throws -> String {

Nit: I know that we're currently not consistent about this, but I generally like camel casing abbreviations as well, eg. getApiIpAddress. Not sure if we should change anything though, since this class uses mostly uppercase abbreviations.

---

ios/MullvadVPNUITests/Pages/AccountDeletionPage.swift line 26 at r3 (raw file):

    @discardableResult func tapDeleteAccountButton() -> Self {
        guard let pageAccessibilityIdentifier = self.pageAccessibilityIdentifier else {

How come we do a specific check only for this identifier?

---

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

        }

        if iOSDevicePinCode.isEmpty == false {

Should the test fail here instead?

---

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

    }

    func discardChangeLogIfShown() {

Nit: dismiss[ChangeLog...]

[rablador]

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

[niklasberglund]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 7534 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

They should be removed if this PR doesn't use them.

Removed 👌

---

ios/MullvadVPNUITests/README.md line 27 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

What are the presented steps?

Right, it's not very clear here. When creating a new runner in GitHub you're guided to run a setup script with a wizard asking you for labels and name of runner. A GitHub admin should give the information from the screenshot and from there the process is quite straightforward. Should it be clarified more in the README?

---

ios/MullvadVPNUITests/README.md line 33 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

What are these strange characters? Hard return?

Good question. I don't know, but VSCode really wanted to add them after this list. It resulted in a new line and small indentation(two spaces). Have removed them now.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 39 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: I know that we're currently not consistent about this, but I generally like camel casing abbreviations as well, eg. getApiIpAddress. Not sure if we should change anything though, since this class uses mostly uppercase abbreviations.

I wasn't sure whether there's a convention for the codebase. Seen both. Here it's a bit awkward because MullvadAPIWrapper wraps MullvadApi 😄 Personally I like upper-casing abbreviations since Apple do it and it seems to be the most common convention in Swift?

---

ios/MullvadVPNUITests/Pages/AccountDeletionPage.swift line 26 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

How come we do a specific check only for this identifier?

Not sure if this answers your question but pageAccessibilityIdentifier is the only optional used in this method, and the chaining of app.otherElements[pageAccessibilityIdentifier].buttons[AccessibilityIdentifier.deleteButton] instead of justapp.otherElements[AccessibilityIdentifier.deleteButton] is necessary because there's another button in the view hierarchy named deleteButton - a button that isn't visible to the user but exists in the view hiearchy. It would probably be better to make sure the accessibility identifiers are unique though!

---

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

Previously, rablador (Jon Petersson) wrote…

Should the test fail here instead?

It is valid to not use any pin actually, and required for the devices used with the GitHub runner unfortunately. So that tests can run without a human manually entering the pin code every time tests run. If the device don't have any pin code this enter pin code step will be skipped and the VPN permission is still allowed as intended.

---

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

Previously, rablador (Jon Petersson) wrote…

Nit: dismiss[ChangeLog...]

Much better 👍 Renamed to dismissChangeLogIfShown

[rablador]

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

---

ios/MullvadVPNUITests/README.md line 27 at r3 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Right, it's not very clear here. When creating a new runner in GitHub you're guided to run a setup script with a wizard asking you for labels and name of runner. A GitHub admin should give the information from the screenshot and from there the process is quite straightforward. Should it be clarified more in the README?

I think it's enough to just point out in the doc comment that these steps are on a certain page on github.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 39 at r3 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

I wasn't sure whether there's a convention for the codebase. Seen both. Here it's a bit awkward because MullvadAPIWrapper wraps MullvadApi 😄 Personally I like upper-casing abbreviations since Apple do it and it seems to be the most common convention in Swift?

Let's keep it as it is now then 👍

---

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

Previously, niklasberglund (Niklas Berglund) wrote…

It is valid to not use any pin actually, and required for the devices used with the GitHub runner unfortunately. So that tests can run without a human manually entering the pin code every time tests run. If the device don't have any pin code this enter pin code step will be skipped and the VPN permission is still allowed as intended.

Ok!

[mojganii]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @niklasberglund)

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 13 at r5 (raw file):

enum MullvadAPIError: Error {
    case incorrectConfigurationFormat

it seems we are using this case for inavlidURL.isn't it a better name?

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 14 at r5 (raw file):

enum MullvadAPIError: Error {
    case incorrectConfigurationFormat
    case requestError

the error case is not well-defined which shows when it happens.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 35 at r5 (raw file):

    }

    public static func getAPIHostname() -> String {

do really we need to have another function that returns the static variable?

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 60 at r5 (raw file):

        for _ in 0 ..< 44 {
            bytes.append(UInt8.random(in: 0 ..< 255))

you can make it shorter.

Code snippet:

let bytes = (0..<44).map({_ in UInt8.random(in: UInt8.min ..< UInt8.max)})

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 72 at r5 (raw file):

        } catch {
            XCTFail("Failed to create account using app API")
            return String()

cannot we use try & catch?

---

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

        }

        if iOSDevicePinCode.isEmpty == false {

nit :

Code snippet:

if !iOSDevicePinCode.isEmpty {}

[niklasberglund]

Reviewable status: 30 of 33 files reviewed, 9 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPNUITests/README.md line 27 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think it's enough to just point out in the doc comment that these steps are on a certain page on github.

Reworded it a bit

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 13 at r5 (raw file):

Previously, mojganii wrote…

it seems we are using this case for inavlidURL.isn't it a better name?

That would point more to what went wrong. I intended to keep them generic and reusable but it's not reused for any other "incorrect configuration format error" so it makes sense to rename it to be more specific 👍 How about invalidEndpointFormatError?

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 14 at r5 (raw file):

Previously, mojganii wrote…

the error case is not well-defined which shows when it happens.

requestError is thrown when there's a client or server-side error so it's very generic. It could be broken down more but I think it should very rarely happen and might be over-engineering to identify different types of errors?

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 35 at r5 (raw file):

Previously, mojganii wrote…

do really we need to have another function that returns the static variable?

No, I made this a static method for consistency since others are. But it doesn't need to be. Changed to static let.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 60 at r5 (raw file):

Previously, mojganii wrote…

you can make it shorter.

Personally I prefer readability over shorter code and find the one-liner a bit complex. Is there any agreed upon convention on shorter vs longer code among the group? Google's style guide don't mention it what I could find.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 72 at r5 (raw file):

Previously, mojganii wrote…

cannot we use try & catch?

Avoiding catching errors in the test case layer to keep it easy to read so that it reads like a description of what the test does. The test should not attempt to recover from a failed API request. There could be error handling here instead like retries.

---

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

Previously, mojganii wrote…

nit :

I usually avoid ! for negating conditions and instead do == false because it's easier to read and avoid mistakes but don't have any strong opinion. If using ! is usually used in the code base I'll follow this convention instead 👍 Have updated this line to use !.

[rablador]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mojganii and @niklasberglund)

---

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

Previously, niklasberglund (Niklas Berglund) wrote…

I usually avoid ! for negating conditions and instead do == false because it's easier to read and avoid mistakes but don't have any strong opinion. If using ! is usually used in the code base I'll follow this convention instead 👍 Have updated this line to use !.

We have settled for both being ok for the time being. Although I personally like ! more. 🙃

[niklasberglund]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mojganii and @rablador)

---

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

Previously, rablador (Jon Petersson) wrote…

We have settled for both being ok for the time being. Although I personally like ! more. 🙃

No matter my preference I'd be happy to follow the group's preference though 😊

[rablador]

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

---

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

Previously, niklasberglund (Niklas Berglund) wrote…

No matter my preference I'd be happy to follow the group's preference though 😊

It's a split opinion, so do whichever at the moment ;)

[buggmagnet]

Reviewed 23 of 33 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @mojganii, @niklasberglund, and @rablador)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3109 at r6 (raw file):

				7A88DCDD2A8FABBE00D2FF0E /* RoutingTests */,
				58CE5E61224146200008646E /* Products */,
				584F991F2902CBDD001F858D /* Frameworks */,

It looks like there are too many files in this folder.

Can you delete the two files that are highlighted in the screenshot ?

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3110 at r6 (raw file):

				58CE5E61224146200008646E /* Products */,
				584F991F2902CBDD001F858D /* Frameworks */,
				85EC620B2B838B3A005AFFB5 /* Recovered References */,

We should delete this folder

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 35 at r5 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

No, I made this a static method for consistency since others are. But it doesn't need to be. Changed to static let.

nit
@mojganii There's a good distinction between a function and a getter.
One indicates that something is read-only whilst the other might not be.

If you don't have access to the source code, there's no way to know whether whether a variable can be written to, whereas a function hints strongly that it cannot be written to.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 60 at r5 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Personally I prefer readability over shorter code and find the one-liner a bit complex. Is there any agreed upon convention on shorter vs longer code among the group? Google's style guide don't mention it what I could find.

Readability trumps all but correctness.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 72 at r5 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Avoiding catching errors in the test case layer to keep it easy to read so that it reads like a description of what the test does. The test should not attempt to recover from a failed API request. There could be error handling here instead like retries.

In most cases the API should just work, since it's all locally controlled dependencies.
The UI Tests won't work without the API, so it's fine to work with the assumption that it always does the correct thing, and as written here, make the current test fail in case the API fails.

---

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

Previously, rablador (Jon Petersson) wrote…

It's a split opinion, so do whichever at the moment ;)

I need to go on a crusade here... I've been (hopelessly) trying to advocate for condition == false and condition == true as it's IMHO way easier to read than !condition

Please, let's stop having discussions on this. There is no team consensus, so everyone has to accept the status quo that it's up to the developer's personal preference to write statements how they see fit.

[rablador]

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

[rablador]

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @mojganii and @niklasberglund)

[niklasberglund]

Reviewable status: 30 of 36 files reviewed, 8 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3109 at r6 (raw file):

Previously, buggmagnet wrote…

It looks like there are too many files in this folder.

Can you delete the two files that are highlighted in the screenshot ?

Deleted 👌

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3110 at r6 (raw file):

Previously, buggmagnet wrote…

We should delete this folder

Have fixed the file references under it and deleted the folder

[rablador]

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

[mojganii]

Reviewable status: 34 of 36 files reviewed, 4 unresolved discussions (waiting on @buggmagnet, @niklasberglund, and @rablador)

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 14 at r5 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

requestError is thrown when there's a client or server-side error so it's very generic. It could be broken down more but I think it should very rarely happen and might be over-engineering to identify different types of errors?

Fine!

[rablador]

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

[buggmagnet]

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

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3550 at r9 (raw file):

			isa = PBXGroup;
			children = (
				85B267602B849ADB0098E3CD /* mullvad-api.h */,

Please move this file and the MullvadApi.swift file inside the MullvadVPNUITests folder

---

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 0 at r9 (raw file):
This file shouldn't be deleted, please run the following command from the ios folder
git checkout main -- MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/

[niklasberglund]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet)

---

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

Previously, buggmagnet wrote…

This file shouldn't be deleted, please run the following command from the ios folder
git checkout main -- MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/

Again! Have restored it

[niklasberglund]

Reviewable status: 35 of 36 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3550 at r9 (raw file):

Previously, buggmagnet wrote…

Please move this file and the MullvadApi.swift file inside the MullvadVPNUITests folder

It looks like MullvadApi.swift already is in the MullvadVPNUITests folder or am I missing something 🤔

Maybe its a good idea to movemullvad-api.h but would it make sense to treat as a separate issue? Its not introduced by this PR and is part of the build step Emils set up so it might require some modifications in multiple files. If it is to be consumed by more targets in the future maybe it makes sense to keep it outside of the targets' sub folders.

[buggmagnet]

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

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3550 at r9 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

It looks like MullvadApi.swift already is in the MullvadVPNUITests folder or am I missing something 🤔

Maybe its a good idea to movemullvad-api.h but would it make sense to treat as a separate issue? Its not introduced by this PR and is part of the build step Emils set up so it might require some modifications in multiple files. If it is to be consumed by more targets in the future maybe it makes sense to keep it outside of the targets' sub folders.

Fair enough, let's not do that here then.

[github-actions]

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