Add ad blocking test for iOS

[niklasberglund]

Implemented test covering the ad blocking functionality. This PR introduces the attributes MULLVAD_IOS_DEVICE_PIN_CODE and MULLVAD_AD_SERVING_DOMAIN which need to be added to your Configurations/UITests.xcconfig file.

The test assumes a blank state, so the app should be uninstalled before running the test. It can be tried out by running the test testAdBlockingViaDNS.

---

This change is 

[linear]

IOS-440 Ad blocking via DNS

[niklasberglund]

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

---

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

        if let urlError = requestError as? URLError {
            if urlError.code == .cannotFindHost {

I'm not entirely sure whether this is a good way to verify that the domain can or cannot be resolved. Does DNS lookup do a better job at verifying that the domain is resolvable? There's also a limitation that this will only work when the domain points to a host running a HTTP server.

[buggmagnet]

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

---

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 0 at r1 (raw file):
That usually happens when you switch branches from the CLI, sometimes Ccode gets conflicted and automatically deletes that file.
We actually need that file, please re-checkout that file from main like so
git checkout main -- MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

---

ios/MullvadVPNUITests/BaseUITestCase.swift line 15 at r1 (raw file):

    public static let defaultTimeout = 10.0

    // swiftlint:disable force_cast line_length

Superfluous Disable Command Violation: SwiftLint rule 'line_length' did not trigger a violation in the disabled region; remove the disable command (superfluous_disable_command)

---

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

    }

    /// Verify that  ad serving domain is reachable by making sure the host can be found when sending HTTP request to it

Verify that an ad serving...

---

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

        task.resume()

        _ = semaphore.wait(timeout: .distantFuture)

This is a surefire way to deadlock the test when things go wrong. Since we are running with XCTest, we can take advantage of its asynchronous waiting APIs instead

    private func canReachAdServingDomain() -> Bool {
        guard let url = URL(string: "http://\(adServingDomain)") else { return false }
        var requestError: Error?

        let urlErrorExpectation = expectation(description: "URLSession encountered an error")
        let task = URLSession.shared.dataTask(with: url) { _, _, error in
            requestError = error
            urlErrorExpectation.fulfill()
        }

        task.resume()

        wait(for: [urlErrorExpectation], timeout: 30)

        if let urlError = requestError as? URLError {
            if urlError.code == .cannotFindHost {
                return false
            }
        }

        return true
    }

---

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

Previously, niklasberglund (Niklas Berglund) wrote…

I'm not entirely sure whether this is a good way to verify that the domain can or cannot be resolved. Does DNS lookup do a better job at verifying that the domain is resolvable? There's also a limitation that this will only work when the domain points to a host running a HTTP server.

I dug a bit around, I think this error code is the correct one to use.
If we want to be extra sure, when that happens, we can assert that the response parameter in the callback (the 2nd, before the error) is nil, which will be the case when the DNS is blocked.

---

ios/MullvadVPNUITests/Pages/SettingsPage.swift line 20 at r1 (raw file):

    @discardableResult func tapVPNSettingsCell() -> Self {
        app.tables[AccessibilityIdentifier.settingsTableView.rawValue]

To avoid calling rawValue everywhere, we could use write extension that does it on our behalf like so

extension XCUIElementQuery {
    subscript(key: any RawRepresentable<String>) -> XCUIElement {
        self[key.rawValue]
    }
}

It makes reading a nicer experience

    @discardableResult func tapVPNSettingsCell() -> Self {
        app.tables[AccessibilityIdentifier.settingsTableView]
            .cells[AccessibilityIdentifier.preferencesCell]
            .tap()

        return self
    }

[rablador]

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

---

ios/MullvadVPNUITests/BaseUITestCase.swift line 15 at r1 (raw file):

Previously, buggmagnet wrote…

Superfluous Disable Command Violation: SwiftLint rule 'line_length' did not trigger a violation in the disabled region; remove the disable command (superfluous_disable_command)

Also, please line break it somehow. Eg.

Code snippet:

Bundle(for: BaseUITestCase.self)
    .infoDictionary?["MullvadNoTimeAccountNumber"] as! String

---

ios/MullvadVPNUITests/Pages/SettingsPage.swift line 20 at r1 (raw file):

Previously, buggmagnet wrote…

To avoid calling rawValue everywhere, we could use write extension that does it on our behalf like so

extension XCUIElementQuery {
    subscript(key: any RawRepresentable<String>) -> XCUIElement {
        self[key.rawValue]
    }
}

It makes reading a nicer experience

    @discardableResult func tapVPNSettingsCell() -> Self {
        app.tables[AccessibilityIdentifier.settingsTableView]
            .cells[AccessibilityIdentifier.preferencesCell]
            .tap()

        return self
    }

+1

[niklasberglund]

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

---

ios/MullvadVPNUITests/BaseUITestCase.swift line 15 at r1 (raw file):
Ah yes 👍 There was a line exceeding the limit before but it has been moved.

Also, please line break it somehow. Eg.

Oops, I see now that some were separated to multiple lines but some not. Totally agree that they should all follow the same format. Not sure all separated into multiple lines is clearer, see first code snippet below where the lines have been separated into two, and second code snippet which disables line_length and let these lines be long. How do you think?

Code snippet (i):

    // 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
    let iOSDevicePinCode = Bundle(for: BaseUITestCase.self)
        .infoDictionary?["MullvadIOSDevicePinCode"] as! String
    let adServingDomain = Bundle(for: BaseUITestCase.self)
        .infoDictionary?["MullvadAdServingDomain"] as! String
    // swiftlint:enable force_cast

Code snippet (ii):

    // swiftlint:disable force_cast line_length
    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
    let iOSDevicePinCode = Bundle(for: BaseUITestCase.self).infoDictionary?["MullvadIOSDevicePinCode"] as! String
    let adServingDomain = Bundle(for: BaseUITestCase.self).infoDictionary?["MullvadAdServingDomain"] as! String
    // swiftlint:enable force_cast line_length

---

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

Previously, buggmagnet wrote…

Verify that an ad serving...

Fixed

---

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

Previously, buggmagnet wrote…

This is a surefire way to deadlock the test when things go wrong. Since we are running with XCTest, we can take advantage of its asynchronous waiting APIs instead

    private func canReachAdServingDomain() -> Bool {
        guard let url = URL(string: "http://\(adServingDomain)") else { return false }
        var requestError: Error?

        let urlErrorExpectation = expectation(description: "URLSession encountered an error")
        let task = URLSession.shared.dataTask(with: url) { _, _, error in
            requestError = error
            urlErrorExpectation.fulfill()
        }

        task.resume()

        wait(for: [urlErrorExpectation], timeout: 30)

        if let urlError = requestError as? URLError {
            if urlError.code == .cannotFindHost {
                return false
            }
        }

        return true
    }

Ah yes, .distantFuture is not a good choice for timeout. Is wait preferred over semaphore? It's in a layer under the testing code, and using semaphore feels more natural than expectation to me here because the expectation is not on some specific data but used to wait for response.

---

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

Previously, buggmagnet wrote…

I dug a bit around, I think this error code is the correct one to use.
If we want to be extra sure, when that happens, we can assert that the response parameter in the callback (the 2nd, before the error) is nil, which will be the case when the DNS is blocked.

Thanks for verifying! See the updated code where both .cannotFindHost error and nil value for response is taken into account.

---

ios/MullvadVPNUITests/Pages/SettingsPage.swift line 20 at r1 (raw file):

Previously, buggmagnet wrote…

To avoid calling rawValue everywhere, we could use write extension that does it on our behalf like so

extension XCUIElementQuery {
    subscript(key: any RawRepresentable<String>) -> XCUIElement {
        self[key.rawValue]
    }
}

It makes reading a nicer experience

    @discardableResult func tapVPNSettingsCell() -> Self {
        app.tables[AccessibilityIdentifier.settingsTableView]
            .cells[AccessibilityIdentifier.preferencesCell]
            .tap()

        return self
    }

Reads much better 👍 Have added the extension and removed all .rawValue.

---

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

Previously, buggmagnet wrote…

That usually happens when you switch branches from the CLI, sometimes Ccode gets conflicted and automatically deletes that file.
We actually need that file, please re-checkout that file from main like so
git checkout main -- MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

Oops! Added it again

[rablador]

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

---

ios/MullvadVPNUITests/BaseUITestCase.swift line 15 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Ah yes 👍 There was a line exceeding the limit before but it has been moved.

Also, please line break it somehow. Eg.

Oops, I see now that some were separated to multiple lines but some not. Totally agree that they should all follow the same format. Not sure all separated into multiple lines is clearer, see first code snippet below where the lines have been separated into two, and second code snippet which disables line_length and let these lines be long. How do you think?

I think the first snippet is fine. We usually follow the swiftlint rather than disable it, with some exceptions where needed of course.

[niklasberglund]

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

---

ios/MullvadVPNUITests/BaseUITestCase.swift line 15 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think the first snippet is fine. We usually follow the swiftlint rather than disable it, with some exceptions where needed of course.

Sounds good. Have changed it to follow the format of the first snippet.

[rablador]

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

[buggmagnet]

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

---

ios/MullvadVPNUITests/BaseUITestCase.swift line 15 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Sounds good. Have changed it to follow the format of the first snippet.

Yeah I would say usually as long as swiftlint + swiftformat don't complain about it, then I won't either, we've put those tools in place because we used to have endless arguments about how code should look like.

---

ios/MullvadVPNUITests/RelayTests.swift line 81 at r1 (raw file):
There are a couple of problems with that piece of code

• The return value of semaphore.wait(timeout: .now() + 10), is not checked, so we don't know whether the code timed out, or did run to completion.
  By using an expectation, we get the desired outcome, if that network call didn't run as expected, we fail the test (which would / should fail anyway)

• If the test were using the swift async runtime, it would be a violation of the contract that "a thread must always be allowed to progress" which semaphore prevents, which would lead to either undefined behaviour, runtime crashes, or deadlocks.

the expectation is not on some specific data but used to wait for response.

The expectation is asserting essentially that the completion was calledback, right now, if it doesn't happen, this function is relying on the call site to indicate failure.

[niklasberglund]

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

---

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

Previously, buggmagnet wrote…

There are a couple of problems with that piece of code

• The return value of semaphore.wait(timeout: .now() + 10), is not checked, so we don't know whether the code timed out, or did run to completion.
  By using an expectation, we get the desired outcome, if that network call didn't run as expected, we fail the test (which would / should fail anyway)

• If the test were using the swift async runtime, it would be a violation of the contract that "a thread must always be allowed to progress" which semaphore prevents, which would lead to either undefined behaviour, runtime crashes, or deadlocks.

the expectation is not on some specific data but used to wait for response.

The expectation is asserting essentially that the completion was calledback, right now, if it doesn't happen, this function is relying on the call site to indicate failure.

Ah yes, if the timeout is reached resultIndicatesDNSBlock will be false since that's the initial value, which will be incorrect in that case. Could rewrite a bit to fix but it's easier to understand the code if checking whether timeout was reached 👍

I'm not opposed to changing the syntax. Isn't the difference between wait and DispatchSemaphore semantical, or do they also work in different ways? For an application it would be bad to write synchronous code but for tests I'd say it's preferred, want to avoid asynchronous code in test case abstraction layer.

[buggmagnet]

Reviewed 1 of 1 files at r4.
Reviewable status: 19 of 21 files reviewed, all discussions resolved (waiting on @rablador)