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

Reduce flakiness of test_ui_tunnel_settings #7166

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

Serock3
Copy link
Contributor

@Serock3 Serock3 commented Nov 12, 2024

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 Reviewable

@Serock3 Serock3 self-assigned this Nov 12, 2024
Copy link

linear bot commented Nov 12, 2024

@Serock3 Serock3 force-pushed the test_ui_tunnel_settings-fails-a-lot-des-1351 branch 2 times, most recently from dc6735c to 893e1bb Compare November 14, 2024 10:49
@Serock3 Serock3 force-pushed the test_ui_tunnel_settings-fails-a-lot-des-1351 branch from 893e1bb to 7c0ecba Compare November 14, 2024 15:57
@Serock3 Serock3 marked this pull request as ready for review November 14, 2024 15:58
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a 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();

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 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: all files reviewed, 2 unresolved discussions (waiting on @Serock3)

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 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: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

@Serock3 Serock3 force-pushed the test_ui_tunnel_settings-fails-a-lot-des-1351 branch from 7c0ecba to 90ee7b5 Compare November 14, 2024 16:34
Copy link
Contributor Author

@Serock3 Serock3 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: 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 :)

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a 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: :shipit: complete! all files reviewed, all discussions resolved

@Serock3 Serock3 merged commit 7db530d into main Nov 14, 2024
57 checks passed
@Serock3 Serock3 deleted the test_ui_tunnel_settings-fails-a-lot-des-1351 branch November 14, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants