Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add login UI tests #5678

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Add login UI tests #5678

merged 1 commit into from
Jan 16, 2024

Conversation

niklasberglund
Copy link
Collaborator

@niklasberglund niklasberglund commented Jan 11, 2024

The first basic login tests. Will implement more login-related tests later on to cover more of the login flow(more tests documented in https://linear.app/mullvad/document/tests-09cbefd4a4a6). Using Page Object Pattern(AKA Page Object Model) where code looking for elements, tapping them etc is abstracted away into page classes. So the test cases are very high level and easy to read, making it easy to verify that the tests are testing what they should and it's easier to maintain and create new tests reusing existing page classes.

A new .xcconfig file template has been added under Configurations so you need to copy the template for example by cd Configurations ; cp UITests.xcconfig.template UITests.xcconfig or Xcode might complain. To run the tests the variables on the configuration need to be populated with valid account numbers.

The tests are in the MullvadVPNUITests test plan and they can be launched for example from the test navigator in Xcode. For now there's an assumption that the app has been uninstalled before running tests. There will be a solution for this in upcoming PR.
image


This change is Reviewable

@niklasberglund niklasberglund added the iOS Issues related to iOS label Jan 11, 2024
Copy link

linear bot commented Jan 11, 2024

IOS-428 Test app login

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


ios/MullvadVPN.xcodeproj/project.pbxproj line 1713 at r1 (raw file):

		7AF9BE942A40461100DBFEDB /* RelayFilterView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RelayFilterView.swift; sourceTree = "<group>"; };
		7AF9BE962A41C71F00DBFEDB /* RelayFilterChipView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RelayFilterChipView.swift; sourceTree = "<group>"; };
		852969252B4D9C1F007EAD4C /* MullvadVPNUITests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = MullvadVPNUITests.xctest; sourceTree = BUILT_PRODUCTS_DIR; };

We usually sort every file by name alphabetically
If you right click on the Pages group, you can select "Sort by name" and it should do it automatically for you


ios/MullvadVPNUITests/AccountTests.swift line 11 at r1 (raw file):

import XCTest

final class AccountTests: XCTestCase {

We don't need to declare classes as final, the compiler can infer it for us, and will automatically do the right thing (and write it on our behalf) most of the time.

It's the same line of reasoning as to why we want to avoid writing self most of the time if we can avoid it.
So that when we see it, we can think "This is intentional, and I need to think carefully about why it was put there in the first place"


ios/MullvadVPNUITests/AccountTests.swift line 12 at r1 (raw file):

final class AccountTests: XCTestCase {
    let noTimeAccountNumber = Bundle(for: AccountTests.self).infoDictionary?["MullvadNoTimeAccountNumber"] as! String

Given that a lot (if not all ?) of the UI tests will likely start from a clean slate, I think we can generalize a bit how to access those properties.

Something as simple as the below example should suffice for now

class BaseUITest: XCTestCase {
    // swiftlint:disable force_cast
    let noTimeAccountNumber = Bundle(for: AccountTests.self).infoDictionary?["MullvadNoTimeAccountNumber"] as! String
    let hasTimeAccountNumber = Bundle(for: AccountTests.self).infoDictionary?["MullvadHasTimeAccountNumber"] as! String
    let fiveWireGuardKeysAccountNumber = Bundle(for: AccountTests.self)
        .infoDictionary?["MullvadFiveWireGuardKeysAccountNumber"] as! String
    // swiftlint:enable force_cast
}

class AccountTests: BaseUITest {

ios/MullvadVPNUITests/AccountTests.swift line 25 at r1 (raw file):

    func testLogin() throws {
        let app = XCUIApplication()
        app.launch()

If we ever want to run the UI automation suite manually, this will most likely fail if we had a previous build running, or a previous UI test running.
It's not super important for now, but for the future, we should uninstall the app in each class of UITests we run before launching the app.
I'm thinking something along the lines of (Or this can probably also be in the BaseUITest class)

class SomeUITest: BaseUITest {

    class override func setUp() {
        uninstallApplication()
        let app = XCUIApplication()
        app.launch()
    }
}

Alternatively, we can force a logout at the end of each test, and clear the cached account.


ios/MullvadVPNUITests/AccountTests.swift line 28 at r1 (raw file):

        TermsOfServicePage(app)
            .tapAgree()

This is good, but I think we can do even better.

Here we don't really know what "agree" is, what are we agreeing to ?
Here's a suggestion:

TermsOfServicePage(app)
    .AgreeToTermsAndConditions()

IMHO UI Tests should read like a good book, it's easier to understand the intent if the test tells me what is supposed to be achieved rather than what it is trying to do.
The distinction is subtle, but important.

Here it's just a button to tap, so that's easy.
But let's imagine that it would have been a flow like "enter password, wait 2 seconds, go accept 2FA token, turn around 3 times in a counter clockwise rotation on your chair"
tapAgree wouldn't quite cut it.

This way, the underlying implementation can change, but the intent stays the same.


ios/MullvadVPNUITests/AccountTests.swift line 53 at r1 (raw file):

        LoginPage(app)
            .tapAccountNumberTextField()
            .enterText("1234123412341234")

We try to avoid magic numbers as much as possible, also technically, "1234123412341234" is a valid account number.

suggestion

let incorrectAccountNumber = "0000000000000000"
LoginPage(app)
    .tapAccountNumberTextField()
    .enterText(incorrectAccountNumber)

ios/MullvadVPNUITests/AccountTests.swift line 55 at r1 (raw file):

            .enterText("1234123412341234")
            .tapAccountNumberSubmitButton()
            .verifyFailIconShown()

nit
I don't think we should verify that an image exists instead to verify that.
Instead we should verify that the text Login Failed is shown.
We should also be mindful that this will need eventually to read localized text.
There is a subtext that is showing why the login failed, which we will want to verify in the future as well.

I'm on the fence on this, on one hand, this is good enough for now.
On the other hand, we will have to change that in the future when we want to validate localization too.


ios/MullvadVPNUITests/Pages/LoginPage.swift line 13 at r1 (raw file):

class LoginPage: Page {
    @discardableResult override init(_ app: XCUIApplication) {

nit
We could probably use the new macro features introduced in swift 5.9 to automatically mark all functions / variables @discardableResult in this class 🤔


ios/MullvadVPNUITests/Pages/Page.swift line 25 at r1 (raw file):

    }

    public func waitForPageToBeShown() {

It would be nicer to be able to override the timeouts instead of using hardcoded values.
suggestion

public func waitForPageToBeShown(_ timeout: TimeInterval = 10) {
    if let pageAccessibilityIdentifier = self.pageAccessibilityIdentifier {
        XCTAssertTrue(
            self.app.otherElements[pageAccessibilityIdentifier.rawValue]
                .waitForExistence(timeout: timeout)
        )
    }
}

ios/MullvadVPNUITests/Pages/Page.swift line 27 at r1 (raw file):

    public func waitForPageToBeShown() {
        if let pageAccessibilityIdentifier = self.pageAccessibilityIdentifier {
            XCTAssert(self.app.otherElements[pageAccessibilityIdentifier.rawValue].waitForExistence(timeout: 10))

Let's be explicit in the intent and use XCTAssertTrue here

Copy link
Collaborator Author

@niklasberglund niklasberglund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 13 of 17 files reviewed, 10 unresolved discussions (waiting on @buggmagnet and @pinkisemils)


ios/MullvadVPN.xcodeproj/project.pbxproj line 1713 at r1 (raw file):

Previously, buggmagnet wrote…

We usually sort every file by name alphabetically
If you right click on the Pages group, you can select "Sort by name" and it should do it automatically for you

Good, done 👍


ios/MullvadVPNUITests/AccountTests.swift line 11 at r1 (raw file):

Previously, buggmagnet wrote…

We don't need to declare classes as final, the compiler can infer it for us, and will automatically do the right thing (and write it on our behalf) most of the time.

It's the same line of reasoning as to why we want to avoid writing self most of the time if we can avoid it.
So that when we see it, we can think "This is intentional, and I need to think carefully about why it was put there in the first place"

Makes sense. Removed final


ios/MullvadVPNUITests/AccountTests.swift line 12 at r1 (raw file):

Previously, buggmagnet wrote…

Given that a lot (if not all ?) of the UI tests will likely start from a clean slate, I think we can generalize a bit how to access those properties.

Something as simple as the below example should suffice for now

class BaseUITest: XCTestCase {
    // swiftlint:disable force_cast
    let noTimeAccountNumber = Bundle(for: AccountTests.self).infoDictionary?["MullvadNoTimeAccountNumber"] as! String
    let hasTimeAccountNumber = Bundle(for: AccountTests.self).infoDictionary?["MullvadHasTimeAccountNumber"] as! String
    let fiveWireGuardKeysAccountNumber = Bundle(for: AccountTests.self)
        .infoDictionary?["MullvadFiveWireGuardKeysAccountNumber"] as! String
    // swiftlint:enable force_cast
}

class AccountTests: BaseUITest {

Yes, good idea 👍 Created a base class


ios/MullvadVPNUITests/AccountTests.swift line 25 at r1 (raw file):

Previously, buggmagnet wrote…

If we ever want to run the UI automation suite manually, this will most likely fail if we had a previous build running, or a previous UI test running.
It's not super important for now, but for the future, we should uninstall the app in each class of UITests we run before launching the app.
I'm thinking something along the lines of (Or this can probably also be in the BaseUITest class)

class SomeUITest: BaseUITest {

    class override func setUp() {
        uninstallApplication()
        let app = XCUIApplication()
        app.launch()
    }
}

Alternatively, we can force a logout at the end of each test, and clear the cached account.

Yes! This is something I haven't implemented a solution for yet and been thinking of as outside of the scope of this first PR, but maybe it shouldn't be. The tests in this PR assume the app isn't installed on the phone. I should've added that to the description. Added it now but let me know if it's something that should be part of this PR even.

Been looking into how to start XCUITest tests with a clear state but it doesn't seem trivial or like there's a very good programmatic solution from what I've found so far. Pondering creating a separate ticket for this. How does that sound?


ios/MullvadVPNUITests/AccountTests.swift line 28 at r1 (raw file):

Previously, buggmagnet wrote…

This is good, but I think we can do even better.

Here we don't really know what "agree" is, what are we agreeing to ?
Here's a suggestion:

TermsOfServicePage(app)
    .AgreeToTermsAndConditions()

IMHO UI Tests should read like a good book, it's easier to understand the intent if the test tells me what is supposed to be achieved rather than what it is trying to do.
The distinction is subtle, but important.

Here it's just a button to tap, so that's easy.
But let's imagine that it would have been a flow like "enter password, wait 2 seconds, go accept 2FA token, turn around 3 times in a counter clockwise rotation on your chair"
tapAgree wouldn't quite cut it.

This way, the underlying implementation can change, but the intent stays the same.

The tapAgree method isn't following the convention other methods follow and should be named tapAgreeButton. Does that make it more clear?

All those two lines do is verify TermsOfServiceContentView shows and then taps the accept button to accept the terms.


ios/MullvadVPNUITests/AccountTests.swift line 53 at r1 (raw file):

Previously, buggmagnet wrote…

We try to avoid magic numbers as much as possible, also technically, "1234123412341234" is a valid account number.

suggestion

let incorrectAccountNumber = "0000000000000000"
LoginPage(app)
    .tapAccountNumberTextField()
    .enterText(incorrectAccountNumber)

True. Is account number0000000000000000 guaranteed to be invalid so using that instead solves the issue or should it also be broken out of the code to the configuration file?


ios/MullvadVPNUITests/AccountTests.swift line 55 at r1 (raw file):

Previously, buggmagnet wrote…

nit
I don't think we should verify that an image exists instead to verify that.
Instead we should verify that the text Login Failed is shown.
We should also be mindful that this will need eventually to read localized text.
There is a subtext that is showing why the login failed, which we will want to verify in the future as well.

I'm on the fence on this, on one hand, this is good enough for now.
On the other hand, we will have to change that in the future when we want to validate localization too.

At first I also wanted to look for the label, but why I finally went with checking the image name instead of checking the label is because it is a status label that is reused so it's the same label object for all login states. And trying to avoid checking for specific text strings because it's so easy to break checks against specific strings. There is also an additional check on the line underneath, waitForPageToBeShown() which verifies that we're still on the login page so the user haven't been sent to the tunnel control page you get to after being logged in.

Code snippet:

private extension LoginState {
    var localizedTitle: String {
        switch self {
        case .default:
            return NSLocalizedString(
                "HEADING_TITLE_DEFAULT",
                tableName: "Login",
                value: "Login",
                comment: ""
            )

        case .authenticating:
            return NSLocalizedString(
                "HEADING_TITLE_AUTHENTICATING",
                tableName: "Login",
                value: "Logging in...",
                comment: ""
            )

        case .failure:
            return NSLocalizedString(
                "HEADING_TITLE_FAILURE",
                tableName: "Login",
                value: "Login failed",
                comment: ""
            )

        case .success:
            return NSLocalizedString(
                "HEADING_TITLE_SUCCESS",
                tableName: "Login",
                value: "Logged in",
                comment: ""
            )
        }
    }
    ...

ios/MullvadVPNUITests/Pages/LoginPage.swift line 13 at r1 (raw file):

Previously, buggmagnet wrote…

nit
We could probably use the new macro features introduced in swift 5.9 to automatically mark all functions / variables @discardableResult in this class 🤔

That would be neat! The app target is using Swift 5.0. I'm not sure how much 5.0 and 5.9 differs, would it be okay to use different Swift versions for these targets?


ios/MullvadVPNUITests/Pages/Page.swift line 25 at r1 (raw file):

Previously, buggmagnet wrote…

It would be nicer to be able to override the timeouts instead of using hardcoded values.
suggestion

public func waitForPageToBeShown(_ timeout: TimeInterval = 10) {
    if let pageAccessibilityIdentifier = self.pageAccessibilityIdentifier {
        XCTAssertTrue(
            self.app.otherElements[pageAccessibilityIdentifier.rawValue]
                .waitForExistence(timeout: timeout)
        )
    }
}

Should the time it takes to navigate to a page differ though? For example 10 seconds is a super high value actually not meant to catch performance issues really but a timeout that is reached when there's an issue with the test usually.


ios/MullvadVPNUITests/Pages/Page.swift line 27 at r1 (raw file):

Previously, buggmagnet wrote…

Let's be explicit in the intent and use XCTAssertTrue here

Good idea, changed 👍

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 13 of 17 files reviewed, 10 unresolved discussions (waiting on @buggmagnet and @pinkisemils)


ios/MullvadVPNUITests/AccountTests.swift line 25 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Yes! This is something I haven't implemented a solution for yet and been thinking of as outside of the scope of this first PR, but maybe it shouldn't be. The tests in this PR assume the app isn't installed on the phone. I should've added that to the description. Added it now but let me know if it's something that should be part of this PR even.

Been looking into how to start XCUITest tests with a clear state but it doesn't seem trivial or like there's a very good programmatic solution from what I've found so far. Pondering creating a separate ticket for this. How does that sound?

I agree with @buggmagnet

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 12 of 16 files at r1, 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @niklasberglund)


ios/MullvadVPNUITests/AccountTests.swift line 25 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Yes! This is something I haven't implemented a solution for yet and been thinking of as outside of the scope of this first PR, but maybe it shouldn't be. The tests in this PR assume the app isn't installed on the phone. I should've added that to the description. Added it now but let me know if it's something that should be part of this PR even.

Been looking into how to start XCUITest tests with a clear state but it doesn't seem trivial or like there's a very good programmatic solution from what I've found so far. Pondering creating a separate ticket for this. How does that sound?

Sounds good, I'll create a separate ticket for that 👍


ios/MullvadVPNUITests/AccountTests.swift line 28 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

The tapAgree method isn't following the convention other methods follow and should be named tapAgreeButton. Does that make it more clear?

All those two lines do is verify TermsOfServiceContentView shows and then taps the accept button to accept the terms.

nit:
Turning this into a nitpick cause it's not super important at this stage.

Yes it would make it more clear, but this isn't exactly what I was trying to say.
Please take with a grain of salt what I'm proposing here because it might be early optimization, but what I'm suggesting is that the test looks like this

/// Validates that the user can login with a valid account number
func testLoginWithValidAccountNumber() throws {
    let app = XCUIApplication()
    app.launch()

    TermsOfServicePage(app)
        .agreeToTermsAndServiceConditions()

    LoginPage(app)
        .verifyLoginWith(
            app,
            validAccountNumber: self.noTimeAccountNumber
        )
}

The idea is that the test doesn't require looking at the functions to see what it does, and what it tries to validate, it's very self explanatory.
There's a TOS page, which will need to agree to conditions, then there's a login page where you enter an account number.

IMHO we shouldn't need to read more than that to see what the test is about.

That being said, it's okay if you prefer to just rename tapAgree to tapAgreeButton and move on.


ios/MullvadVPNUITests/AccountTests.swift line 53 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

True. Is account number0000000000000000 guaranteed to be invalid so using that instead solves the issue or should it also be broken out of the code to the configuration file?

Let's assume it is for now, I will check around and come back to you with a better answer than "I'm almost sure it is, but not quite"


ios/MullvadVPNUITests/AccountTests.swift line 55 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

At first I also wanted to look for the label, but why I finally went with checking the image name instead of checking the label is because it is a status label that is reused so it's the same label object for all login states. And trying to avoid checking for specific text strings because it's so easy to break checks against specific strings. There is also an additional check on the line underneath, waitForPageToBeShown() which verifies that we're still on the login page so the user haven't been sent to the tunnel control page you get to after being logged in.

Fair enough, let's keep it like this for now then 👍


ios/MullvadVPNUITests/BaseTestCase.swift line 14 at r3 (raw file):

class BaseTestCase: XCTestCase {
    // swiftlint:disable force_cast
    let noTimeAccountNumber = Bundle(for: AccountTests.self).infoDictionary?["MullvadNoTimeAccountNumber"] as! String

I know I made the original suggestion but I forgot one important detail. We should rename this BaseUITestCase to avoid clashing with Unit Tests (if we ever get to have a BaseUnitTestCase), also the bundle class refered to should be this one, it's likely to out-live AccountTests if we ever get rid of them.

class BaseUITestCase: XCTestCase {
    // swiftlint:disable force_cast
    let noTimeAccountNumber = Bundle(for: BaseUITestCase.self).infoDictionary?["MullvadNoTimeAccountNumber"] as! String
    let hasTimeAccountNumber = Bundle(for: BaseUITestCase.self).infoDictionary?["MullvadHasTimeAccountNumber"] as! String
    let fiveWireGuardKeysAccountNumber = Bundle(for: BaseUITestCase.self)
        .infoDictionary?["MullvadFiveWireGuardKeysAccountNumber"] as! String
    // swiftlint:enable force_cast
}

ios/MullvadVPNUITests/Pages/LoginPage.swift line 13 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

That would be neat! The app target is using Swift 5.0. I'm not sure how much 5.0 and 5.9 differs, would it be okay to use different Swift versions for these targets?

Xcode 15 ships with the swift 5.9 compiler. Which means we can already use the features from that version.
If you run the following command, you will likely get the same input (or at least, the same version)

[~/Code/mullvadvpn-app/ios (test-app-login-ios-428)]% xcrun swift --version
swift-driver version: 1.87.3 Apple Swift version 5.9.2 (swiftlang-5.9.2.2.56 clang-1500.1.0.2.5)
Target: arm64-apple-macosx13.0

What you see (probably) in "Swift Language Version" means little nowadays since Swift reached ABI stability in version 5.0. (It was more important when swift didn't have ABI stability, and when the syntax was still evolving a lot)

What really matters is which tool we use, and that means we choose to tie which swift version we can use, with which Xcode version we use (I'm taking a shortcut here).

Let's not bother too much with this for now, we can take time to do this later if it proves to be too much boilerplate


ios/MullvadVPNUITests/Pages/Page.swift line 25 at r1 (raw file):

Should the time it takes to navigate to a page differ though?
Most likely not. But since we're trying to avoid magic values in general, it's better to have this at least as a constant defined somewhere.
What do you think ?

Copy link
Collaborator Author

@niklasberglund niklasberglund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 73 files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @pinkisemils)


ios/MullvadVPNUITests/AccountTests.swift line 28 at r1 (raw file):
Ah, I see your point. In the past I've made the same suggestion as you but I was convinced to not go with this higher abstraction. There's some big benefits of automated test cases reading the same as manual test cases which list all the steps, such as making it easier to maintain test cases and be sure that all functionality is covered. With a higher abstraction level the steps are less visible, you'd have to jump around in the code to figure out exactly what user actions the tests are doing. And some flows such as login will be slightly different in different test cases also - for instance there are two submit buttons.

That being said, it's okay if you prefer to just rename tapAgree to tapAgreeButton and move on.

Renamed it to tapAgreeButton. If I haven't convinced you yet I'll try to do so with time! 😊


ios/MullvadVPNUITests/AccountTests.swift line 53 at r1 (raw file):

Previously, buggmagnet wrote…

Let's assume it is for now, I will check around and come back to you with a better answer than "I'm almost sure it is, but not quite"

Maybe it doesn't answer our question but for the Android end to end tests there's a configurable invalid_test_account_token https://github.com/mullvad/mullvadvpn-app/tree/main/android/test/e2e

Have changed the invalid account number to number0000000000000000``


ios/MullvadVPNUITests/BaseTestCase.swift line 14 at r3 (raw file):

Previously, buggmagnet wrote…

I know I made the original suggestion but I forgot one important detail. We should rename this BaseUITestCase to avoid clashing with Unit Tests (if we ever get to have a BaseUnitTestCase), also the bundle class refered to should be this one, it's likely to out-live AccountTests if we ever get rid of them.

class BaseUITestCase: XCTestCase {
    // swiftlint:disable force_cast
    let noTimeAccountNumber = Bundle(for: BaseUITestCase.self).infoDictionary?["MullvadNoTimeAccountNumber"] as! String
    let hasTimeAccountNumber = Bundle(for: BaseUITestCase.self).infoDictionary?["MullvadHasTimeAccountNumber"] as! String
    let fiveWireGuardKeysAccountNumber = Bundle(for: BaseUITestCase.self)
        .infoDictionary?["MullvadFiveWireGuardKeysAccountNumber"] as! String
    // swiftlint:enable force_cast
}

Good point! Renamed it to BaseUITestCase

Copy link
Collaborator Author

@niklasberglund niklasberglund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 73 files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @pinkisemils)


ios/MullvadVPNUITests/Pages/LoginPage.swift line 13 at r1 (raw file):

Previously, buggmagnet wrote…

Xcode 15 ships with the swift 5.9 compiler. Which means we can already use the features from that version.
If you run the following command, you will likely get the same input (or at least, the same version)

[~/Code/mullvadvpn-app/ios (test-app-login-ios-428)]% xcrun swift --version
swift-driver version: 1.87.3 Apple Swift version 5.9.2 (swiftlang-5.9.2.2.56 clang-1500.1.0.2.5)
Target: arm64-apple-macosx13.0

What you see (probably) in "Swift Language Version" means little nowadays since Swift reached ABI stability in version 5.0. (It was more important when swift didn't have ABI stability, and when the syntax was still evolving a lot)

What really matters is which tool we use, and that means we choose to tie which swift version we can use, with which Xcode version we use (I'm taking a shortcut here).

Let's not bother too much with this for now, we can take time to do this later if it proves to be too much boilerplate

Sounds good, let's see later then


ios/MullvadVPNUITests/Pages/Page.swift line 25 at r1 (raw file):

Previously, buggmagnet wrote…

Should the time it takes to navigate to a page differ though?
Most likely not. But since we're trying to avoid magic values in general, it's better to have this at least as a constant defined somewhere.
What do you think ?

Agree. I created a constant for the timeout value.

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 17 files at r11, 2 of 59 files at r12, all commit messages.
Reviewable status: 5 of 73 files reviewed, 2 unresolved discussions (waiting on @mojganii, @niklasberglund, and @pinkisemils)


ios/MullvadVPNUITests/AccountTests.swift line 25 at r1 (raw file):

Previously, buggmagnet wrote…

Sounds good, I'll create a separate ticket for that 👍

@niklasberglund I created IOS-456 in regards to this


ios/MullvadVPNUITests/AccountTests.swift line 53 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Maybe it doesn't answer our question but for the Android end to end tests there's a configurable invalid_test_account_token https://github.com/mullvad/mullvadvpn-app/tree/main/android/test/e2e

Have changed the invalid account number to number0000000000000000``

nice !

@buggmagnet buggmagnet merged commit b700b79 into main Jan 16, 2024
4 of 5 checks passed
@buggmagnet buggmagnet deleted the test-app-login-ios-428 branch January 16, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants