-
Notifications
You must be signed in to change notification settings - Fork 341
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
Reduce flakiness of test_ui_tunnel_settings
#7166
Conversation
dc6735c
to
893e1bb
Compare
893e1bb
to
7c0ecba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)
test/test-manager/src/tests/ui.rs
line 108 at r1 (raw file):
.build(), ) .await?;
⛏️ The LowLatency
list won't apply for the OpenVPN connection as is, right?
Code quote:
// NOTE: This test connects multiple times using various settings, some of which may cauase a
// significant increase in connection time, e.g. multihop and OpenVPN. For this reason, it is
// preferable to only target low latency servers.
use helpers::custom_lists::LowLatency;
// tunnel-state.spec precondition: a single WireGuard relay should be selected
log::info!("Select WireGuard relay");
let entry = helpers::constrain_to_relay(
&mut mullvad_client,
RelayQueryBuilder::new()
.wireguard()
.location(LowLatency)
.build(),
)
.await?;
mullvad-relay-selector/src/relay_selector/matcher.rs
line 251 at r1 (raw file):
log::warn!("Resolved non-existent custom list with id {list_id:?}"); ResolvedLocationConstraint(vec![]) }),
Why shouldn't we print the custom list ID?
Code quote:
.unwrap_or_else(|| {
log::warn!("Resolved non-existent custom list with id {list_id:?}");
ResolvedLocationConstraint(vec![])
}),
gui/test/e2e/installed/state-dependent/tunnel-state.spec.ts
line 38 at r1 (raw file):
const relay = page.getByTestId('hostname-line'); const inIp = page.locator(':text("In") + span'); const outIp = page.locator(':text("Out") + div > span').first();
⛏️ Is it obvious why first
is called here? Is it because there could be multiple "Out IPs", and that the first one is the IPv4 address? If so, would it make sense to leave a small comment here? 😊
Code quote:
const outIp = page.locator(':text("Out") + div > span').first();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Serock3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)
Since IPv6 is default for macOS, this would fail everytime on that platform.
7c0ecba
to
90ee7b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 8 files reviewed, all discussions resolved (waiting on @MarkusPettersson98)
test/test-manager/src/tests/ui.rs
line 108 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
⛏️ The
LowLatency
list won't apply for the OpenVPN connection as is, right?
I think it will, even though it is not obvious!
gui/test/e2e/installed/state-dependent/tunnel-state.spec.ts
line 38 at r1 (raw file):
Previously, MarkusPettersson98 (Markus Pettersson) wrote…
⛏️ Is it obvious why
first
is called here? Is it because there could be multiple "Out IPs", and that the first one is the IPv4 address? If so, would it make sense to leave a small comment here? 😊
Note at all! Good catch, I added a comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
Fixes a bug where
test_ui_tunnel_settings
failed on macOS since it has IPv6 enabled by default, which caused the parsing of the outbound IP to fail.The PR also sets the "LowLatency" custom list for the test, since it connects multiple times with different settings, including multihop and OpenVPN, and thus is highly dependent on connection stability.
CI job which ran the test 10 times in a row: https://github.com/mullvad/mullvadvpn-app/actions/runs/11838399382.
This change is