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 setup for writing tests for TunnelManager #5232

Merged

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Oct 4, 2023

As the name implies, this PR makes TunnelManager testable. No tests have been ran just yet.


This change is Reviewable

@buggmagnet buggmagnet added the iOS Issues related to iOS label Oct 4, 2023
@buggmagnet buggmagnet self-assigned this Oct 4, 2023
@buggmagnet buggmagnet force-pushed the tunnel-async-actor-tests branch 4 times, most recently from 485e858 to 9be4c65 Compare October 5, 2023 14:09
@buggmagnet buggmagnet force-pushed the tunnel-async-actor-tests-tunnel-manager branch 2 times, most recently from 48be1a8 to 8651d05 Compare October 6, 2023 08:32
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 58 of 58 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/TunnelManager/StopTunnelOperation.swift line 39 at r1 (raw file):

        case .connected, .connecting, .reconnecting, .waitingForConnectivity(.noConnection), .error:
            guard var tunnel = interactor.tunnel else {

let should be fine here.


ios/MullvadVPN/TunnelManager/Tunnel.swift line 23 at r1 (raw file):

}

protocol TunnelProtocol: AnyObject, Equatable {

There's quite a lot going on in this protocol. I understand that it's here mainly to facilitate testing (hence this PR), but maybe we should divide it into appropriate chunks now that we're abstracting things anyway?


ios/MullvadVPNTests/RESTRequestExecutor+Stubs.swift line 36 at r1 (raw file):

        guard let success = success else { throw POSIXError(.EINVAL) }

        return try await withCheckedThrowingContinuation { continuation in

Simplify with return success()?

@buggmagnet buggmagnet force-pushed the tunnel-async-actor-tests-tunnel-manager branch from 8651d05 to 63ae31a Compare October 9, 2023 09:24
Copy link
Contributor Author

@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: 56 of 58 files reviewed, 3 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/TunnelManager/Tunnel.swift line 23 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

There's quite a lot going on in this protocol. I understand that it's here mainly to facilitate testing (hence this PR), but maybe we should divide it into appropriate chunks now that we're abstracting things anyway?

I agree, however it would be outside of the scope of this (already) large PR.
We should definitely do it in a follow up PR instead.


ios/MullvadVPNTests/RESTRequestExecutor+Stubs.swift line 36 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Simplify with return success()?

Done.
I think in the future we will want to make this more configurable for advanced testing.


ios/MullvadVPN/TunnelManager/StopTunnelOperation.swift line 39 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

let should be fine here.

Done.

Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

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

@buggmagnet buggmagnet merged commit a9bc6b9 into tunnel-async-actor-tests Oct 9, 2023
4 checks passed
@buggmagnet buggmagnet deleted the tunnel-async-actor-tests-tunnel-manager branch October 9, 2023 09:36
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.

2 participants