Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @rablador)

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

		7A42DEC92A05164100B209BE /* SettingsInputCell.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A42DEC82A05164100B209BE /* SettingsInputCell.swift */; };
		7A42DECD2A09064C00B209BE /* SelectableSettingsCell.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A42DECC2A09064C00B209BE /* SelectableSettingsCell.swift */; };
		7A6B4F592AB8412E00123853 /* TunnelMonitorTimings.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A6B4F582AB8412E00123853 /* TunnelMonitorTimings.swift */; };

Please move the file after TunnelMonitorProtocol.swift

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 62 at r1 (raw file):
nit
I would say that the documentation bit is redundant here, we already established the timeouts when creating the timings variable.

The part that says

default first connection attempt starts at 4 second timeout, then doubles with each subsequent attempt, while being capped at 15s max.

Is probably more relevant in the TunnelMonitor file, or maybe the TunnelMonitorTimings file itself.

But it's not a big deal either.

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 73 at r1 (raw file):

        // Calculate the amount of time necessary to perform the test adding some leeway.
        let timeout = expectedTimings.reduce(1000, +)

On my machine, this runs roughly in 900ms, so that leeway could work. But it can easily run slower in a VM / on the CI environment.

Just today I had a test where it ran in 43.017 seconds instead of the original 42 seconds.

So it would also fail here for the wrong reasons.

I suggest being conserative and tripling that value for now, so 3 seconds expected at most.
This should be low enough that we never wait too long, and high enough that if there is a deadlock, it doesn't make the test runner stall for too long before moving on.

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 98 at r1 (raw file):
Currently it's printing

[1/3] connectionLost, time elapsed: 100s

You can use the following trick to avoid computing time manually
let measuredTimeElapsed = Measurement(value: Double(timeElapsed), unit: UnitDuration.milliseconds)

Then if you want to convert you can simply do measuredTimeElapsed.converted(to: UnitDuration.seconds)

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 148 at r1 (raw file):

    private func roundToHundreds(_ value: Int) -> Int {
        return (value / 100 * 100) + ((value % 100) / 50 * 100)

Couple of things come to mind here

• This could probably be an extension on Int instead
• The first expression (value / 100 * 100) is redundant... Unless we're dealing with Double

Which we are, because we're doing maths on TimeInterval
Also I think the whole computation is unnecessary.
Instead of rounding our values, we should just use the accuracy option of XCTestEqual

If I do the following instead
let elapsed = Int(Date().timeIntervalSince(startDate) * 1000) Then I can simply do XCTAssertEqual(elapsed, expectedDuration, accuracy: 10, "Some fancy error message here")`

Without doing any complicated maths.

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

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

Done.

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 62 at r1 (raw file):

Removed.

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 73 at r1 (raw file):

Not that much, though? Perhaps upping to 1500?
If each individual measure is about 100-300 millis, adding 3000 as leeway is a little overkill (not that it really matters). What I mean is that if we're going with your proposal below to use "accuracy" when asserting the time elapsed, I think 50ms would be good for that. Everything outside that will fail. So if all four asserts fail there's going to be 200ms added in total. Ie, if the tests takes more that 1100 millis it should fail anyway.

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 98 at r1 (raw file):

Oh, simply forgot to add an "m". I like your suggestion, but perhaps a little overkill?

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 148 at r1 (raw file):

The first expression (value / 100 * 100) is redundant...

Since we're dealing with integers there's some rounding going on here. Eg. 123 / 100 * 100 = 100.

The reason for normalising the values is that we - apart from the assertion - also print out what happens. So in Expected to report connection loss after \(expectedDuration)s, instead reported it after \(timeElapsed)s we probably want numbers that match.

I do agree that accuracy in XCTAssertEqual is generally a better choice (didn't know about it until now), but I'm not sure what to do about those logs. Maybe Expected to report connection loss after *roughly* \(expectedDuration)s [...] or something?

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

ios/PacketTunnelCoreTests/TunnelMonitorTests.swift line 98 at r1 (raw file):

Discussed offline and the decision was to leave it as-is

nice job !

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved