Add API access method tests #5798

MarkusPettersson98 · 2024-02-09T15:12:26Z

This PR add tests for custom API access methods to the integration test suite.

This change is

linear · 2024-02-09T15:12:28Z

DES-607 Add API access method daemon tests

dlon

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

test/test-manager/src/tests/access_methods.rs line 59 at r1 (raw file):

    let bridge = relay_list
        .relays()
        .filter(|relay| matches!(relay.endpoint_data, RelayEndpointData::Bridge))

This should probably filter on active too. Maybe it should also select a random relay to avoid the problem we had earlier.

dlon

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

dlon

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

dlon

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

test/.cargo/config.toml line 5 at r4 (raw file):

[env]
LIBMNL_LIB_DIR   = { value = "../dist-assets/binaries", relative = true }

Awesome. I wonder if we can just use this in the main workspace as well (instead of using env.sh).

Is setting SDKROOT required for macOS?

dlon

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

dlon

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

talpid-future/src/lib.rs line 4 at r6 (raw file):

#![deny(missing_docs)]

pub mod future_retry;

Nit: future_ could probably be removed?

dlon

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

dlon

Reviewed 15 of 15 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

dlon

Reviewed 6 of 6 files at r9, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)

mullvad-api/src/lib.rs line 177 at r11 (raw file):

            disable_tls: false,
            force_direct: force_direct
                .map(|force_direct_env| force_direct_env.to_lowercase() != "false")

We seem to check if var != "0" for other boolean env vars. false might be objectively better, but it's inconsistent, so maybe we should stick to 0?

mullvad-daemon/src/api.rs line 247 at r9 (raw file):

        let (index, next) = Self::get_next_inner(
            access_method_settings.cardinality(),

Off by one?

test/test-manager/src/tests/ui.rs line 167 at r9 (raw file):

    // `MULLVAD_API_FORCE_DIRECT=false` and restarting the daemon.

    let (host, endpoint) = (mullvad_api::API.host(), mullvad_api::API.address());

We should probably take these from TEST_CONFIG. See get_app_env().

test/test-manager/src/tests/ui.rs line 223 at r11 (raw file):

    tokio::time::timeout(
        std::time::Duration::from_secs(60),
        rpc.set_daemon_environment(env),

In my opinion, we should either make sure that the test always cleans up the environment, somehow, or set must_succeed = true on it. Otherwise, we get sort of an inconsistent state for any remaining tests.

dlon

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)

mullvad-api/src/lib.rs line 256 at r11 (raw file):

        ];

        if env_vars.map(Self::read_var).iter().any(Option::is_some) {

Nice trick!

MarkusPettersson98

Reviewable status: 23 of 32 files reviewed, 3 unresolved discussions (waiting on @dlon)

mullvad-api/src/lib.rs line 177 at r11 (raw file):