Tunnel async actor tests

[buggmagnet]

This PR enables testing for the following classes:

• PacketTunnelActor
• TunnelManager
• AppMessageHandler

---

This change is 

[rablador]

Reviewed 13 of 13 files at r1, 57 of 57 files at r2, 13 of 13 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 175 at r1 (raw file):

        // Wait for the connected state to happen so it doesn't get coalesced immediately after the call to `actor.stop`
        actor.start(options: StartOptions(launchSource: .app))
        await fulfillment(of: [connectedStateExpectation], timeout: 1)

We should probably use the actor.waitUntilConnected instead. Same for similar other places in the file.

[pronebird]

Reviewed 12 of 13 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/PacketTunnelCore/Actor/State.swift line 59 at r1 (raw file):

 */
public enum State: Equatable {
    public static func == (lhs: State, rhs: State) -> Bool {

This is a very specialized equality check. How about we move that extension into tests and not implement it in the actor state itself? I'd also use a method instead because the name itself only takes into account the enum variant, so it's rather a similarity check based on variant ignoring all associated data which can be misleading if we were to use it in other contexts.

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 175 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

We should probably use the actor.waitUntilConnected instead. Same for similar other places in the file.

waitUntilConnected() does not accept timeout, so it's perfectly normal to observe state directly.

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 191 at r1 (raw file):

        actor.stop()

        switch await actor.state {

Since internally commands are handled in a separate detached task, picking actor.state right away could yield the initial value. It would be a bit more reliable to wait for commands in the channel to be consumed.

Maybe we could add some internal method to wait for commands to be processed either inside of actor or directly on command channel. Also I don't think we need 3 calls in a row to prove the test case, one should be enough :-)

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 237 at r1 (raw file):

                        actor.setErrorState(reason: .readSettings)
                    }
                    task.cancel()

What's the purpose of this detached task, given that it is cancelled immediately and therefore will not advance past Task.sleep()?

---

ios/PacketTunnelCoreTests/Mocks/DefaultPathObserverFake.swift line 26 at r1 (raw file):

    private var stateLock = NSLock()

    private var defaultPathHandler: ((NetworkPath) -> Void)?

It's unsafe to access internal properties normally guarded behind the mutex lock. If you have a race within a test case, then tests could prove to be faulty.

I think it would be better to implement a stub accepting expectation and then fulfill it on stop() as I understand this is the requirement of one of tests.

[pronebird]

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

---

ios/PacketTunnelCoreTests/AppMessageHandlerTests.swift line 16 at r3 (raw file):

import XCTest

final class AppMessageHandlerTests: XCTestCase {

Looks like integration test. Maybe we should focus on testing inputs/outputs and signals as opposed to testing the reaction of underlying dependencies. I left a rather long comment on Slack to explain my point.

[buggmagnet]

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

---

ios/PacketTunnelCore/Actor/State.swift line 59 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

This is a very specialized equality check. How about we move that extension into tests and not implement it in the actor state itself? I'd also use a method instead because the name itself only takes into account the enum variant, so it's rather a similarity check based on variant ignoring all associated data which can be misleading if we were to use it in other contexts.

That's a good point.
All in all, equating two states would be really complicated given how State uses associated values, so I'm not sure we would ever want to do that outside of a test context.

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 191 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Since internally commands are handled in a separate detached task, picking actor.state right away could yield the initial value. It would be a bit more reliable to wait for commands in the channel to be consumed.

Maybe we could add some internal method to wait for commands to be processed either inside of actor or directly on command channel. Also I don't think we need 3 calls in a row to prove the test case, one should be enough :-)

I went back and forth on this test, the idea is mainly to test that the actor state shouldn't be anything else than initial before we start it.
I think I want to keep this test quite light, but maybe we could get rid of it entirely too, I haven't made up my mind.

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 237 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

What's the purpose of this detached task, given that it is cancelled immediately and therefore will not advance past Task.sleep()?

The idea is to make sure that the actor doesn't go into error state if whatever triggered it gets cancelled.
We wrote this test based on the old TaskQueue based implementation, so some of the assumptions here might be outdated.

It's still valid to test that the actor doesn't go into error state if it stops right after start, so I will refactor the test to guarantee that.

---

ios/PacketTunnelCoreTests/Mocks/DefaultPathObserverFake.swift line 26 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

It's unsafe to access internal properties normally guarded behind the mutex lock. If you have a race within a test case, then tests could prove to be faulty.

I think it would be better to implement a stub accepting expectation and then fulfill it on stop() as I understand this is the requirement of one of tests.

Fair enough, we can do that.

[rablador]

Reviewable status: 74 of 76 files reviewed, 6 unresolved discussions (waiting on @pronebird)

---

ios/PacketTunnelCoreTests/AppMessageHandlerTests.swift line 16 at r3 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Looks like integration test. Maybe we should focus on testing inputs/outputs and signals as opposed to testing the reaction of underlying dependencies. I left a rather long comment on Slack to explain my point.

Fair enough, I'll look at it.

[pronebird]

Reviewed 2 of 13 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadREST/RESTProxy.swift line 13 at r3 (raw file):

import Operations

public typealias ProxyCompletionHandler<Success> = (Result<Success, Swift.Error>) -> Void

Why not define it under REST namespace where everything else is currently defined to keep it contained?

---

ios/PacketTunnelCore/Actor/State.swift line 196 at r3 (raw file):

    /// Invalid public key.
    case invalidPublicKey

Nit: Perhaps name this case so that it better reflects that the invalid public key is in the context of relay.

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 260 at r4 (raw file):

        let initialStateExpectation = expectation(description: "Expect initial state")

        stateSink = await actor.$state.receive(on: DispatchQueue.main).sink { newState in

$state always emits initial value first, if I remember that right. So you'd have to wait a bit to observe if there are any changes occur to state after actor.reconnect() is called.

Maybe "inverse" expectation with a reasonable timeout can be used instead of XCTFail().

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 287 at r4 (raw file):

        await fulfillment(of: [disconnectedStateExpectation], timeout: 1)

        await expect(.initial, on: actor) {

1. You probably mean to expect .disconnected state and not .initial in here.
2. Since you assign a new sink within expect(), the first value it emits should be current value (.disconnected) and this test will finish before actor.reconnect() executes. So again I can only think of using a short timeout and inverse expectation to ensure that actor remains in the same state.

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 311 at r4 (raw file):

            connectedExpectation.fulfill()
        }
        actor.start(options: StartOptions(launchSource: .app))

Nit: Maybe we should provide a default value for launchSource = .app to avoid repeating it in every test

---

ios/PacketTunnelCoreTests/Mocks/DefaultPathObserverFake.swift line 26 at r1 (raw file):

Previously, buggmagnet wrote…

Fair enough, we can do that.

What if we make onStart and onStop immutable and fill them over init() to guarantee that they can be safely accessed without lock within the class? Or do we need to mutate them dynamically?

It feels odd to have object half of which is protected by mutex lock and the other half isn't. If the object is expected to be used from multiple threads then we should protect all properties that are accessed concurrently.

I know it's for tests only but as you said, we should give some love to tests, so that they serve us well for the years to come.

[rablador]

Reviewed 2 of 2 files at r4, 1 of 85 files at r5, 7 of 76 files at r6, 6 of 19 files at r8, 53 of 66 files at r11, 19 of 28 files at r12, 11 of 11 files at r13, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/PacketTunnelCore/Actor/State.swift line 196 at r3 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: Perhaps name this case so that it better reflects that the invalid public key is in the context of relay.

Done.

---

ios/PacketTunnelCoreTests/AppMessageHandlerTests.swift line 16 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

Fair enough, I'll look at it.

Done.

[rablador]

Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @pronebird)

[rablador]

Reviewed 2 of 2 files at r15, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @pronebird)

[pronebird]

Reviewed 5 of 76 files at r6, 52 of 66 files at r11, 6 of 28 files at r12, 9 of 11 files at r13, 1 of 2 files at r15, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadREST/RESTAccessTokenManager.swift line 14 at r15 (raw file):

import Operations

public protocol AccessTokenManagement {

Nit: it would be in spirit of how other protocols are named within the framework, i.e with "REST" prefix.

---

ios/MullvadREST/RESTAccountsProxy.swift line 12 at r15 (raw file):

import MullvadTypes

public protocol AccountHandling {

Nit: same here RESTAccountHandling would be a bit less generic.

---

ios/MullvadREST/RESTAuthenticationProxy.swift line 60 at r15 (raw file):

    }

    public struct AccessTokenData: Decodable {

Nit: note that AuthenticationProxy.getAccessToken() is internal.

---

ios/MullvadVPN/Classes/ObserverList.swift line 26 at r15 (raw file):

    }

    static func == (lhs: WeakBox<T>, rhs: WeakBox<T>) -> Bool {

Nit: I don't mind this change but it is completely out of scope of this PR and could have been done in isolation.

---

ios/MullvadVPN/TunnelManager/Tunnel.swift line 24 at r15 (raw file):

protocol TunnelProtocol: AnyObject, Equatable {
    func addObserver(_ observer: TunnelStatusObserver)

Do you mind to group observation methods a bit below around addBlockObserver()?

---

ios/MullvadVPN/TunnelManager/TunnelStore.swift line 98 at r15 (raw file):

    private func handleTunnelStatus(tunnel: any TunnelProtocol, status: NEVPNStatus) {
        guard let tunnel = tunnel as? Tunnel else { return }

This does not look right. This is defeats the purpose of having TunnelProtocol. Why do we need to cast?

---

ios/MullvadVPN/TunnelManager/UIApplication+Extensions.swift line 14 at r15 (raw file):

import UIKit

public protocol UIApplicationProtocol {

Nit: you could name it something like BackgroundTaskProviding to better describe why we need this object in tunnel manager.

---

ios/MullvadVPNTests/AccessTokenManager+Stubs.swift line 18 at r15 (raw file):

        completionHandler: @escaping ProxyCompletionHandler<REST.AccessTokenData>
    ) -> Cancellable {
        AnyCancellable()

FYI I am not sure in which context this stub is used but absence of call to completionHandler will paralyze.

[pronebird]

Reviewed 2 of 76 files at r6, 7 of 28 files at r12, 1 of 11 files at r13, 1 of 1 files at r14.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @buggmagnet)

---

ios/PacketTunnelCore/Actor/State+Extensions.swift line 40 at r15 (raw file):

    }

    var packetTunnelStatus: PacketTunnelStatus {

Where was this copied from?

[pronebird]

Reviewed 1 of 28 files at r12.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @buggmagnet)

[pronebird]

Reviewed 1 of 66 files at r11, 3 of 28 files at r12, 1 of 11 files at r13.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @buggmagnet)

[pronebird]

Reviewed 1 of 2 files at r15.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @buggmagnet)

---

ios/PacketTunnelCore/Actor/State+Extensions.swift line 2 at r15 (raw file):

//
//  State+.swift

Nit: does not reflect the name

[buggmagnet]

Reviewed 2 of 76 files at r6, 52 of 66 files at r11, 13 of 28 files at r12, 1 of 1 files at r14, 2 of 2 files at r15, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadREST/RESTAccessTokenManager.swift line 14 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: it would be in spirit of how other protocols are named within the framework, i.e with "REST" prefix.

Done

---

ios/MullvadREST/RESTAuthenticationProxy.swift line 60 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: note that AuthenticationProxy.getAccessToken() is internal.

Good point, I've made it public

---

ios/MullvadREST/RESTProxy.swift line 13 at r3 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Why not define it under REST namespace where everything else is currently defined to keep it contained?

Because it makes it really cumbersome to have to write
REST.ProxyCompletionHandler<REST.NewAccountData>

---

ios/MullvadVPN/Classes/ObserverList.swift line 26 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: I don't mind this change but it is completely out of scope of this PR and could have been done in isolation.

The reason I made this change is because I was hitting a compiler bug where it would choose this overload for an unrelated comparison and I couldn't get around it in TunnelStore.handleTunnelStatus

---

ios/MullvadVPN/TunnelManager/Tunnel.swift line 24 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Do you mind to group observation methods a bit below around addBlockObserver()?

Done.

---

ios/MullvadVPN/TunnelManager/TunnelStore.swift line 98 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

This does not look right. This is defeats the purpose of having TunnelProtocol. Why do we need to cast?

I replied to this in the comment about the WeakBox compiler issue I had to deal with.

Actually I realize that I had left an Equatable in the TunnelProtocol definition (which doesn't make sense)
and I didn't want to introduce a type eraser just for this case.

This blog post explains why better than I can

---

ios/MullvadVPN/TunnelManager/UIApplication+Extensions.swift line 14 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: you could name it something like BackgroundTaskProviding to better describe why we need this object in tunnel manager.

As you can see, I was not very inspired when naming things 😂🫠

I like your suggestion, I think however BackgroundTaskProvider has a better ring to it, thanks !

---

ios/MullvadVPNTests/AccessTokenManager+Stubs.swift line 18 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

FYI I am not sure in which context this stub is used but absence of call to completionHandler will paralyze.

Yes, as discussed offline, this PR makes TunnelManager testable, but doesn't implement any test.
We can make further modifications as needed when we will write tests for TunnelManager

---

ios/PacketTunnelCore/Actor/State+Extensions.swift line 2 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: does not reflect the name

Done

---

ios/PacketTunnelCore/Actor/State+Extensions.swift line 40 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Where was this copied from?

I don't remember introducing this. Maybe @rablador knows ?

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 260 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

$state always emits initial value first, if I remember that right. So you'd have to wait a bit to observe if there are any changes occur to state after actor.reconnect() is called.

Maybe "inverse" expectation with a reasonable timeout can be used instead of XCTFail().

Hmmmm that's true, I hadn't realized, thanks for catching this.
It looks like I need to rework quite a lot of assumptions I made in the PacketTunnelActorTests
I will come back to this soon

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 311 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: Maybe we should provide a default value for launchSource = .app to avoid repeating it in every test

Done

---

ios/PacketTunnelCoreTests/Mocks/DefaultPathObserverFake.swift line 26 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

What if we make onStart and onStop immutable and fill them over init() to guarantee that they can be safely accessed without lock within the class? Or do we need to mutate them dynamically?

It feels odd to have object half of which is protected by mutex lock and the other half isn't. If the object is expected to be used from multiple threads then we should protect all properties that are accessed concurrently.

I know it's for tests only but as you said, we should give some love to tests, so that they serve us well for the years to come.

Ah yes I forgot to put the onStart and onStop in the locking statement.
It's not horribly bad because it's unlikely that we'll use those in a multithreaded context, even in tests, but it's still bad practice.

I will put those in the lock.

[rablador]

Reviewed 25 of 25 files at r16, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/PacketTunnelCore/Actor/State+Extensions.swift line 40 at r15 (raw file):

Previously, buggmagnet wrote…

I don't remember introducing this. Maybe @rablador knows ?

There was a State+Extensions.swiftin PacketTunnel module as well. This code bit was copied from there and that file was then deleted.

[buggmagnet]

Reviewed 5 of 76 files at r6, 1 of 66 files at r11, 3 of 28 files at r12, 9 of 11 files at r13, 24 of 25 files at r16, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @pronebird)

[pronebird]

Reviewed 5 of 25 files at r16.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN/Classes/ObserverList.swift line 26 at r15 (raw file):

Previously, buggmagnet wrote…

The reason I made this change is because I was hitting a compiler bug where it would choose this overload for an unrelated comparison and I couldn't get around it in TunnelStore.handleTunnelStatus

I see. Thanks for explanation.

---

ios/MullvadVPN/TunnelManager/TunnelStore.swift line 98 at r15 (raw file):

Previously, buggmagnet wrote…

I replied to this in the comment about the WeakBox compiler issue I had to deal with.

Actually I realize that I had left an Equatable in the TunnelProtocol definition (which doesn't make sense)
and I didn't want to introduce a type eraser just for this case.

This blog post explains why better than I can

I still don't quite grasp the benefit of TunnelProtocol because:

• The internal implementation always downcasts to Tunnel otherwise it can't work with objects implementing TunnelProtocol.
• Internal factory method creates new Tunnel but then it erases the type by casting to any TunnelProtocol

Maybe what you meant to do was:

• Introduce associated type in TunnelStoreProtocol, i.e:

protocol TunnelStoreProtocol {
    associatedtype TunnelType: TunnelProtocol
    func createNewTunnel() -> TunnelType
    func getPersistentTunnels() -> [TunnelType]
}

• Declare TunnelStore with generic constraint, i.e:

class TunnelStore<TunnelType: TunnelProtocol>

Use TunnelType internally?

---

ios/MullvadVPN/TunnelManager/UIApplication+Extensions.swift line 14 at r15 (raw file):

Previously, buggmagnet wrote…

As you can see, I was not very inspired when naming things 😂🫠

I like your suggestion, I think however BackgroundTaskProvider has a better ring to it, thanks !

Ha! Ok that looks better now. 👍

---

ios/MullvadVPNTests/AccessTokenManager+Stubs.swift line 18 at r15 (raw file):

Previously, buggmagnet wrote…

Yes, as discussed offline, this PR makes TunnelManager testable, but doesn't implement any test.
We can make further modifications as needed when we will write tests for TunnelManager

Fair.

---

ios/PacketTunnelCore/Actor/State+Extensions.swift line 2 at r15 (raw file):

Previously, buggmagnet wrote…

Done

Mhm probably should be State+Extensions.swift

---

ios/PacketTunnelCore/Actor/State+Extensions.swift line 40 at r15 (raw file):

Previously, rablador (Jon Petersson) wrote…

There was a State+Extensions.swiftin PacketTunnel module as well. This code bit was copied from there and that file was then deleted.

Thanks. I guess since app message handler is a part of framework, the extensions should be available for it to function too.

[pronebird]

Reviewed 20 of 25 files at r16.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet)

[pronebird]

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

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 235 at r16 (raw file):

                switch newState {
                case .connecting:
                    actor.setErrorState(reason: .readSettings)

Given that we use a channel behind:

• actor.start() and actor.stop() should schedule two commands right away.
• error state is scheduled after "stop" because actor.setErrorState() is called from the sink, so to verify that it doesn't do anything, you could introduce an inverse expectation:

let ex = expectation(description: "Should not enter blocked state")
ex.isInverted = true

then plug it in error case:

case .error:
  ex.fulfill()

Finally wait for all of them:

 await fulfillment(of: [connectingStateExpectation, disconnectedStateExpectation, ex], timeout: 1)

I believe that fulfillment(of:) would wait for 1 second before concluding that "ex" hasn't been fulfilled which would be correct given that we don't want it to be fulfilled, hence it's inverted expectation.

[buggmagnet]

Reviewed 1 of 25 files at r16.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/TunnelManager/TunnelStore.swift line 98 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I still don't quite grasp the benefit of TunnelProtocol because:

• The internal implementation always downcasts to Tunnel otherwise it can't work with objects implementing TunnelProtocol.
• Internal factory method creates new Tunnel but then it erases the type by casting to any TunnelProtocol

Maybe what you meant to do was:

• Introduce associated type in TunnelStoreProtocol, i.e:

protocol TunnelStoreProtocol {
    associatedtype TunnelType: TunnelProtocol
    func createNewTunnel() -> TunnelType
    func getPersistentTunnels() -> [TunnelType]
}

• Declare TunnelStore with generic constraint, i.e:

class TunnelStore<TunnelType: TunnelProtocol>

Use TunnelType internally?

You're right that we want to use an erased type here. I didn't feel like doing it until I would have to write actual tests. I have fixed that now.

---

ios/PacketTunnelCore/Actor/State+Extensions.swift line 2 at r15 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Mhm probably should be State+Extensions.swift

Awkward... Done !

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 287 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

1. You probably mean to expect .disconnected state and not .initial in here.
2. Since you assign a new sink within expect(), the first value it emits should be current value (.disconnected) and this test will finish before actor.reconnect() executes. So again I can only think of using a short timeout and inverse expectation to ensure that actor remains in the same state.

Done.
Thanks for catching that !

---

ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift line 235 at r16 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Given that we use a channel behind:

• actor.start() and actor.stop() should schedule two commands right away.
• error state is scheduled after "stop" because actor.setErrorState() is called from the sink, so to verify that it doesn't do anything, you could introduce an inverse expectation:

let ex = expectation(description: "Should not enter blocked state")
ex.isInverted = true

then plug it in error case:

case .error:
  ex.fulfill()

Finally wait for all of them:

 await fulfillment(of: [connectingStateExpectation, disconnectedStateExpectation, ex], timeout: 1)

I believe that fulfillment(of:) would wait for 1 second before concluding that "ex" hasn't been fulfilled which would be correct given that we don't want it to be fulfilled, hence it's inverted expectation.

We can also wait for 100ms after we got the stopped expectation. Since the inverted expectation will blow up if fulfilled, once we have reached the stopped state, we know we don't need to wait any longer.

[pronebird]

Reviewed 6 of 6 files at r17.
Reviewable status: complete! all files reviewed, all discussions resolved

[buggmagnet]

Reviewed 1 of 76 files at r18, 30 of 58 files at r19, 13 of 20 files at r20, 33 of 33 files at r21, all commit messages.
Reviewable status: 14 of 91 files reviewed, 2 unresolved discussions

---

ios/MullvadVPNTests/AddressCacheTests.swift line 50 at r21 (raw file):

        let timeDifference = dateAfterUpdate.timeIntervalSince(dateBeforeUpdate)

        XCTAssertNotEqual(0.0, timeDifference)

I got fed up of this test failing frequently on my machine (the test was incorrect to begin with)
This should fix it for good. (famous last words)

---

ios/PacketTunnelCoreTests/ActorTests.swift line 19 at r20 (raw file):

import XCTest

final class ActorTests: XCTestCase {

This file got back here by mistake, I will delete it.

[pronebird]

Reviewed 1 of 76 files at r18, 3 of 20 files at r20.
Reviewable status: 18 of 91 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Reviewed 3 of 3 files at r22, all commit messages.
Reviewable status: 18 of 91 files reviewed, all discussions resolved