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

Fix tests that fail on macOS 14 #5441

Merged
merged 5 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 13 additions & 22 deletions .github/workflows/desktop-e2e.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: Desktop - End-to-end tests

Check warning on line 1 in .github/workflows/desktop-e2e.yml

View workflow job for this annotation

GitHub Actions / check-formatting

1:1 [document-start] missing document start "---"
on:

Check warning on line 2 in .github/workflows/desktop-e2e.yml

View workflow job for this annotation

GitHub Actions / check-formatting

2:1 [truthy] truthy value should be one of [false, true]
schedule:
- cron: '0 0 * * *'
workflow_dispatch:
Expand Down Expand Up @@ -56,7 +56,7 @@
name: Linux end-to-end tests
needs: build-linux
if: '!cancelled()'
runs-on: [self-hosted, desktop-test, Linux] # app-test-linux

Check warning on line 59 in .github/workflows/desktop-e2e.yml

View workflow job for this annotation

GitHub Actions / check-formatting

59:49 [comments] too few spaces before comment
timeout-minutes: 240
strategy:
fail-fast: false
Expand All @@ -70,11 +70,10 @@
path: ~/.cache/mullvad-test/packages
- name: Checkout repository
uses: actions/checkout@v4
with:
fetch-tags: true
- name: Run end-to-end tests
shell: bash -ieo pipefail {0}
run: |
git fetch --tags --force
./test/ci-runtests.sh ${{ matrix.os }}
- uses: actions/upload-artifact@v3
if: '!cancelled()'
Expand Down Expand Up @@ -126,7 +125,7 @@
needs: build-windows
if: '!cancelled()'
name: Windows end-to-end tests
runs-on: [self-hosted, desktop-test, Linux] # app-test-linux

Check warning on line 128 in .github/workflows/desktop-e2e.yml

View workflow job for this annotation

GitHub Actions / check-formatting

128:49 [comments] too few spaces before comment
timeout-minutes: 240
strategy:
fail-fast: false
Expand All @@ -140,11 +139,10 @@
path: ~/.cache/mullvad-test/packages
- name: Checkout repository
uses: actions/checkout@v4
with:
fetch-tags: true
- name: Run end-to-end tests
shell: bash -ieo pipefail {0}
run: |
git fetch --tags --force
./test/ci-runtests.sh ${{ matrix.os }}
- uses: actions/upload-artifact@v3
if: '!cancelled()'
Expand All @@ -154,36 +152,30 @@

build-macos:
if: ${{ !startsWith(github.ref, 'refs/tags/') && github.ref != 'refs/heads/main' }}
runs-on: macos-latest
runs-on: [self-hosted, desktop-test, macOS] # app-test-macos-arm

Check warning on line 155 in .github/workflows/desktop-e2e.yml

View workflow job for this annotation

GitHub Actions / check-formatting

155:49 [comments] too few spaces before comment
steps:
- name: Checkout repository
uses: actions/checkout@v2
- name: Checkout submodules
run: git submodule update --init --depth=1
- name: Install Go
uses: actions/setup-go@v3
with:
go-version: 1.18.5
- name: Install Protoc
uses: arduino/setup-protoc@v1
uses: arduino/setup-protoc@v2
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}
- uses: actions/setup-node@v3
with:
token: ${{ secrets.GITHUB_TOKEN }}
node-version: 18
- name: Install Rust
uses: actions-rs/[email protected]
with:
toolchain: stable
target: aarch64-apple-darwin
default: true
- name: Install Go
uses: actions/setup-go@v3
with:
go-version: 1.18.5
cache: 'npm'
cache-dependency-path: gui/package-lock.json
- name: Build app
run: ./build.sh --universal
run: ./build.sh
- name: Build test executable
run: ./gui/scripts/build-test-executable.sh aarch64-apple-darwin
# FIXME: This fails for some reason, but the artifact is built
continue-on-error: true
run: ./gui/scripts/build-test-executable.sh
- uses: actions/upload-artifact@v3
if: '!cancelled()'
with:
Expand All @@ -196,7 +188,7 @@
needs: build-macos
if: '!cancelled()'
name: macOS end-to-end tests
runs-on: [self-hosted, desktop-test, macOS] # app-test-macos-arm

Check warning on line 191 in .github/workflows/desktop-e2e.yml

View workflow job for this annotation

GitHub Actions / check-formatting

191:49 [comments] too few spaces before comment
timeout-minutes: 240
strategy:
fail-fast: false
Expand All @@ -210,11 +202,10 @@
path: ~/Library/Caches/mullvad-test/packages
- name: Checkout repository
uses: actions/checkout@v4
with:
fetch-tags: true
- name: Run end-to-end tests
shell: bash -ieo pipefail {0}
run: |
git fetch --tags --force
./test/ci-runtests.sh ${{ matrix.os }}
- uses: actions/upload-artifact@v3
if: '!cancelled()'
Expand Down
48 changes: 41 additions & 7 deletions mullvad-daemon/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1899,9 +1899,15 @@ where
.await
{
Ok(settings_changed) => {
Self::oneshot_send(tx, Ok(()), "set_allow_lan response");
if settings_changed {
self.send_tunnel_command(TunnelCommand::AllowLan(allow_lan));
self.send_tunnel_command(TunnelCommand::AllowLan(
allow_lan,
oneshot_map(tx, |tx, ()| {
Self::oneshot_send(tx, Ok(()), "set_allow_lan response");
}),
));
} else {
Self::oneshot_send(tx, Ok(()), "set_allow_lan response");
}
}
Err(e) => {
Expand Down Expand Up @@ -1946,11 +1952,15 @@ where
.await
{
Ok(settings_changed) => {
Self::oneshot_send(tx, Ok(()), "set_block_when_disconnected response");
if settings_changed {
self.send_tunnel_command(TunnelCommand::BlockWhenDisconnected(
block_when_disconnected,
oneshot_map(tx, |tx, ()| {
Self::oneshot_send(tx, Ok(()), "set_block_when_disconnected response");
}),
));
} else {
Self::oneshot_send(tx, Ok(()), "set_block_when_disconnected response");
}
}
Err(e) => {
Expand Down Expand Up @@ -2148,12 +2158,18 @@ where
.await
{
Ok(settings_changed) => {
Self::oneshot_send(tx, Ok(()), "set_dns_options response");
if settings_changed {
let settings = self.settings.to_settings();
let resolvers =
dns::addresses_from_options(&settings.tunnel_options.dns_options);
self.send_tunnel_command(TunnelCommand::Dns(resolvers));
self.send_tunnel_command(TunnelCommand::Dns(
resolvers,
oneshot_map(tx, |tx, ()| {
Self::oneshot_send(tx, Ok(()), "set_dns_options response");
}),
));
} else {
Self::oneshot_send(tx, Ok(()), "set_dns_options response");
}
}
Err(e) => {
Expand Down Expand Up @@ -2396,7 +2412,8 @@ where
&& (*self.target_state == TargetState::Secured || self.settings.auto_connect)
{
log::debug!("Blocking firewall during shutdown since system is going down");
self.send_tunnel_command(TunnelCommand::BlockWhenDisconnected(true));
let (tx, _rx) = oneshot::channel();
self.send_tunnel_command(TunnelCommand::BlockWhenDisconnected(true, tx));
}

self.state.shutdown(&self.tunnel_state);
Expand All @@ -2408,7 +2425,8 @@ where
// without causing the service to be restarted.

if *self.target_state == TargetState::Secured {
self.send_tunnel_command(TunnelCommand::BlockWhenDisconnected(true));
let (tx, _rx) = oneshot::channel();
self.send_tunnel_command(TunnelCommand::BlockWhenDisconnected(true, tx));
}
self.target_state.lock();
}
Expand Down Expand Up @@ -2569,3 +2587,19 @@ fn new_selector_config(settings: &Settings) -> SelectorConfig {
relay_overrides: settings.relay_overrides.clone(),
}
}

/// Consume a oneshot sender of `T1` and return a sender that takes a different type `T2`. `forwarder` should map `T1` back to `T2` and
/// send the result back to the original receiver.
fn oneshot_map<T1: Send + 'static, T2: Send + 'static>(
tx: oneshot::Sender<T1>,
forwarder: impl Fn(oneshot::Sender<T1>, T2) + Send + 'static,
) -> oneshot::Sender<T2> {
let (new_tx, new_rx) = oneshot::channel();
tokio::spawn(async move {
match new_rx.await {
Ok(result) => forwarder(tx, result),
Err(oneshot::Canceled) => (),
}
});
new_tx
}
4 changes: 4 additions & 0 deletions mullvad-management-interface/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ impl MullvadProxyClient {
super::new_rpc_client().await.map(Self)
}

pub fn from_rpc_client(client: crate::ManagementServiceClient) -> Self {
Self(client)
}

pub async fn connect_tunnel(&mut self) -> Result<bool> {
Ok(self
.0
Expand Down
70 changes: 41 additions & 29 deletions talpid-core/src/tunnel_state_machine/connected_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ impl ConnectedState {
use self::EventConsequence::*;

match command {
Some(TunnelCommand::AllowLan(allow_lan)) => {
if let Err(error_cause) = shared_values.set_allow_lan(allow_lan) {
Some(TunnelCommand::AllowLan(allow_lan, complete_tx)) => {
let consequence = if let Err(error_cause) = shared_values.set_allow_lan(allow_lan) {
self.disconnect(shared_values, AfterDisconnect::Block(error_cause))
} else {
match self.set_firewall_policy(shared_values) {
Expand All @@ -230,43 +230,55 @@ impl ConnectedState {
AfterDisconnect::Block(ErrorStateCause::SetFirewallPolicyError(error)),
),
}
}
};
let _ = complete_tx.send(());
consequence
}
Some(TunnelCommand::AllowEndpoint(endpoint, tx)) => {
shared_values.allowed_endpoint = endpoint;
let _ = tx.send(());
SameState(self)
}
Some(TunnelCommand::Dns(servers)) => match shared_values.set_dns_servers(servers) {
Ok(true) => {
if let Err(error) = self.set_firewall_policy(shared_values) {
return self.disconnect(
shared_values,
AfterDisconnect::Block(ErrorStateCause::SetFirewallPolicyError(error)),
);
}

match self.set_dns(shared_values) {
#[cfg(target_os = "android")]
Ok(()) => self.disconnect(shared_values, AfterDisconnect::Reconnect(0)),
#[cfg(not(target_os = "android"))]
Ok(()) => SameState(self),
Err(error) => {
log::error!("{}", error.display_chain_with_msg("Failed to set DNS"));
self.disconnect(
Some(TunnelCommand::Dns(servers, complete_tx)) => {
let consequence = match shared_values.set_dns_servers(servers) {
Ok(true) => {
if let Err(error) = self.set_firewall_policy(shared_values) {
return self.disconnect(
shared_values,
AfterDisconnect::Block(ErrorStateCause::SetDnsError),
)
AfterDisconnect::Block(ErrorStateCause::SetFirewallPolicyError(
error,
)),
);
}

match self.set_dns(shared_values) {
#[cfg(target_os = "android")]
Ok(()) => self.disconnect(shared_values, AfterDisconnect::Reconnect(0)),
#[cfg(not(target_os = "android"))]
Ok(()) => SameState(self),
Err(error) => {
log::error!(
"{}",
error.display_chain_with_msg("Failed to set DNS")
);
self.disconnect(
shared_values,
AfterDisconnect::Block(ErrorStateCause::SetDnsError),
)
}
}
}
}
Ok(false) => SameState(self),
Err(error_cause) => {
self.disconnect(shared_values, AfterDisconnect::Block(error_cause))
}
},
Some(TunnelCommand::BlockWhenDisconnected(block_when_disconnected)) => {
Ok(false) => SameState(self),
Err(error_cause) => {
self.disconnect(shared_values, AfterDisconnect::Block(error_cause))
}
};
let _ = complete_tx.send(());
consequence
}
Some(TunnelCommand::BlockWhenDisconnected(block_when_disconnected, complete_tx)) => {
shared_values.block_when_disconnected = block_when_disconnected;
let _ = complete_tx.send(());
SameState(self)
}
Some(TunnelCommand::IsOffline(is_offline)) => {
Expand Down
27 changes: 17 additions & 10 deletions talpid-core/src/tunnel_state_machine/connecting_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,12 +392,14 @@ impl ConnectingState {
use self::EventConsequence::*;

match command {
Some(TunnelCommand::AllowLan(allow_lan)) => {
if let Err(error_cause) = shared_values.set_allow_lan(allow_lan) {
Some(TunnelCommand::AllowLan(allow_lan, complete_tx)) => {
let consequence = if let Err(error_cause) = shared_values.set_allow_lan(allow_lan) {
self.disconnect(shared_values, AfterDisconnect::Block(error_cause))
} else {
self.reset_firewall(shared_values)
}
};
let _ = complete_tx.send(());
consequence
}
Some(TunnelCommand::AllowEndpoint(endpoint, tx)) => {
if shared_values.allowed_endpoint != endpoint {
Expand All @@ -418,14 +420,19 @@ impl ConnectingState {
let _ = tx.send(());
SameState(self)
}
Some(TunnelCommand::Dns(servers)) => match shared_values.set_dns_servers(servers) {
#[cfg(target_os = "android")]
Ok(true) => self.disconnect(shared_values, AfterDisconnect::Reconnect(0)),
Ok(_) => SameState(self),
Err(cause) => self.disconnect(shared_values, AfterDisconnect::Block(cause)),
},
Some(TunnelCommand::BlockWhenDisconnected(block_when_disconnected)) => {
Some(TunnelCommand::Dns(servers, complete_tx)) => {
let consequence = match shared_values.set_dns_servers(servers) {
#[cfg(target_os = "android")]
Ok(true) => self.disconnect(shared_values, AfterDisconnect::Reconnect(0)),
Ok(_) => SameState(self),
Err(cause) => self.disconnect(shared_values, AfterDisconnect::Block(cause)),
};
let _ = complete_tx.send(());
consequence
}
Some(TunnelCommand::BlockWhenDisconnected(block_when_disconnected, complete_tx)) => {
shared_values.block_when_disconnected = block_when_disconnected;
let _ = complete_tx.send(());
SameState(self)
}
Some(TunnelCommand::IsOffline(is_offline)) => {
Expand Down
10 changes: 6 additions & 4 deletions talpid-core/src/tunnel_state_machine/disconnected_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ impl TunnelState for DisconnectedState {
use self::EventConsequence::*;

match runtime.block_on(commands.next()) {
Some(TunnelCommand::AllowLan(allow_lan)) => {
Some(TunnelCommand::AllowLan(allow_lan, complete_tx)) => {
if shared_values.allow_lan != allow_lan {
// The only platform that can fail is Android, but Android doesn't support the
// "block when disconnected" option, so the following call never fails.
Expand All @@ -138,6 +138,7 @@ impl TunnelState for DisconnectedState {

Self::set_firewall_policy(shared_values, false);
}
let _ = complete_tx.send(());
SameState(self)
}
Some(TunnelCommand::AllowEndpoint(endpoint, tx)) => {
Expand All @@ -148,15 +149,15 @@ impl TunnelState for DisconnectedState {
let _ = tx.send(());
SameState(self)
}
Some(TunnelCommand::Dns(servers)) => {
Some(TunnelCommand::Dns(servers, complete_tx)) => {
// Same situation as allow LAN above.
shared_values
.set_dns_servers(servers)
.expect("Failed to reconnect after changing custom DNS servers");

let _ = complete_tx.send(());
SameState(self)
}
Some(TunnelCommand::BlockWhenDisconnected(block_when_disconnected)) => {
Some(TunnelCommand::BlockWhenDisconnected(block_when_disconnected, complete_tx)) => {
if shared_values.block_when_disconnected != block_when_disconnected {
shared_values.block_when_disconnected = block_when_disconnected;
Self::set_firewall_policy(shared_values, true);
Expand All @@ -178,6 +179,7 @@ impl TunnelState for DisconnectedState {
Self::reset_dns(shared_values);
}
}
let _ = complete_tx.send(());
SameState(self)
}
Some(TunnelCommand::IsOffline(is_offline)) => {
Expand Down
Loading
Loading