Added the beginning of unit tests fro StartTunnelOperation #5758

acb-mv · 2024-02-02T14:15:22Z

This is the first step in an ongoing process of adding unit tests for logic. This creates a unit test class for StartTunnelOperation and a (currently incomplete) mock of TunnelInteractor for use in it. The two tests currently implemented check that establishing tunnels fails if not logged in, and that in the disconnecting state, the next operation is set to reconnect. Additional logic will need to be tested, though this will involve teasing out intertwined logic and isolating it.

This change is

linear · 2024-02-02T14:15:25Z

IOS-480 Unit tests for StartTunnelOperation

buggmagnet

Great start !

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @acb-mv and @mojganii)

ios/build-rust-library.sh line 56 at r1 (raw file):

        "$HOME"/.cargo/bin/cargo build -p "$FFI_TARGET" --lib --target aarch64-apple-ios
      else
        "$HOME"/.cargo/bin/cargo build -p "$FFI_TARGET" --lib $RELFLAG --target aarch64-apple-ios-sim

it should be consistent with the other targets, why is only this line unquoted ?

ios/MullvadVPNTests/StartTunnelOperationTests.swift line 16 at r1 (raw file):

final class StartTunnelOperationTests: XCTestCase {

I know Xcode adds the final keyword automatically, but the swift compiler is smart enough to do it.
It's not a big deal here, but in genera, unless we want to make the intent explicit that we want a class to be final, we can omit the keyword

ios/MullvadVPNTests/StartTunnelOperationTests.swift line 23 at r1 (raw file):

        let operationQueue = AsyncOperationQueue()
        let settings = LatestTunnelSettings()
        let expectation = XCTestExpectation(description:"")

A couple of things to note here

Let's try to have meaningful description, when we look at failure logs, it will be much easier to see.
In the logs, this will appear as Failed due to unwaited expectation ''. which doesn't really help to see what went wrong.
However if it says instead : Failed due to unwaited expectation 'Failed login attempt'
It's much clearer to see what went wrong.

What is expected here ? The name expectation doesn't tell me anything.

Suggestion

let failedLogin = expectation(description: "Failed login attempt")

But most importantly, you should really call the expectation function, otherwise the created expectation won't count the number of times it's been fulfilled

    func testExpectations() throws {
        let exp1 = XCTestExpectation(description: "First expectation")
        exp1.fulfill()
        exp1.fulfill()
        exp1.fulfill()
        wait(for: [exp1]) // ✅ No failure here

        let exp2 = expectation(description: "Second expectation") // ❌ Failed due to unwaited expectation 'Second expectation'.

        exp2.fulfill()
        exp2.fulfill() // ❌ API violation - multiple calls made to -[XCTestExpectation fulfill] for Second expectation. (NSInternalInconsistencyException)
        exp2.fulfill()
        wait(for: [exp2])
    }

ios/MullvadVPNTests/StartTunnelOperationTests.swift line 36 at r1 (raw file):

        
        operationQueue.addOperation(operation)
        wait(for: [expectation], timeout: 10.0)

No need to wait for so long, 1 second should be more than enough no ?

buggmagnet

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @acb-mv and @mojganii)

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

		0697D6E728F01513007A9E99 /* TransportMonitor.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0697D6E628F01513007A9E99 /* TransportMonitor.swift */; };
		06AC116228F94C450037AF9A /* ApplicationConfiguration.swift in Sources */ = {isa = PBXBuildFile; fileRef = 58BFA5CB22A7CE1F00A6173D /* ApplicationConfiguration.swift */; };
		44DD7D242B6CFFD70005F67F /* StartTunnelOperationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 44DD7D232B6CFFD70005F67F /* StartTunnelOperationTests.swift */; };

Whenever we add new files, we try to keep them alphabetically ordered.
If you right click on the MullvadVPNTests folder, and select Sort by Name it should do the right thing

buggmagnet

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @acb-mv and @mojganii)

ios/MullvadVPNTests/StartTunnelOperationTests.swift line 23 at r2 (raw file):
To quote the swift API design guidelines

Avoid abbreviations. Abbreviations, especially non-standard ones, are effectively terms-of-art, because understanding depends on correctly translating them into their non-abbreviated forms.

ios/MullvadVPNTests/StartTunnelOperationTests.swift line 27 at r2 (raw file):

            dispatchQueue: testQueue,
            interactor: MockTunnelInteractor(isConfigurationLoaded: true, settings: settings, deviceState: .loggedOut)) { result in

Please don't forget to run swiftformat before pushing changes

acb-mv

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

ios/build-rust-library.sh line 56 at r1 (raw file):