White-list relays in an end to end test framework config

[Serock3]

Picking up the work started by @MarkusPettersson98, with the new format decided in https://linear.app/mullvad/issue/DES-1052/be-able-to-whitelist-relays-in-an-end-to-end-test-run.

---

This change is 

[linear]

DES-1052 Be able to whitelist relays in an end-to-end test run

[MarkusPettersson98]

Reviewable status: 0 of 18 files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

test/test-manager/src/config/manifest.rs line 32 at r2 (raw file):

    /// The above example will set the locations for the test `daita` to  a custom list containing
    /// `se-got-wg-001` and `se-got-wg-002`. The `*` is a wildcard that will match any test
    /// name. The order of the list is important, as the first match will be used.

We should mention that the pattern is a glob pattern, which has a specification. Otherwise, it will be hard to figure out how to write the test patterns 😊

Code quote:

    /// The above example will set the locations for the test `daita` to  a custom list containing
    /// `se-got-wg-001` and `se-got-wg-002`. The `*` is a wildcard that will match any test
    /// name. The order of the list is important, as the first match will be used.

[Serock3]

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

---

test/test-manager/src/config/manifest.rs line 32 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

We should mention that the pattern is a glob pattern, which has a specification. Otherwise, it will be hard to figure out how to write the test patterns 😊

Done. I clarified the format in text in addition to the format.

[MarkusPettersson98]

Reviewed 12 of 18 files at r1, 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)

---

test/test-manager/src/config/manifest.rs line 2 at r3 (raw file):

//! Config definition.
//! TODO: Document struct and link to that documentation

TODO

Code quote:

//! TODO: Document struct and link to that documentation

---

test/test-manager/src/config/manifest.rs line 69 at r3 (raw file):

}

mod locations {

Consider converting manifest.rs to a proper module, and then turn locations into a free-standing module (file) within the manifest module 😊

Code quote:

mod locations

---

test/test-manager/src/tests/test_metadata.rs line 12 at r3 (raw file):

    pub priority: Option<i32>,
    // TODO: Document
    pub location: Option<Vec<String>>,

TODO

Code quote:

    // TODO: Document
    pub location: Option<Vec<String>>,

---

mullvad-cli/src/cmds/relay_constraints.rs line 20 at r3 (raw file):

#[derive(thiserror::Error, Debug)]
pub enum Error {
    #[error("Failed to parse location constraint from input: TODO")]

TODO

Code quote:

    #[error("Failed to parse location constraint from input: TODO")]

---

test/test-manager/src/tests/mod.rs line 203 at r3 (raw file):

    set_location_from_constraint(mullvad_client, LocationConstraint::CustomList { list_id })
        .await
        .context("Failed to set location to custom list")?;

⛏️ It might be a good idea to include the custom list id here for less cognitive load when debugging? 😊

Code quote:

.context("Failed to set location to custom list")?;

---

test/test-manager/src/main.rs line 205 at r3 (raw file):

            }
            ConfigArg::Set => todo!(),
            ConfigArg::GetPath => {

In unix speak, this command is usually referred to as which 😊

Code quote:

ConfigArg::GetPath

---

test/test-manager/src/main.rs line 307 at r3 (raw file):

                        config.mullvad_host.as_ref().unwrap()
                    );
                }

⛏️ Personally, I would find a match statement a bit easier to read in this case 😊

Code quote:

                if let Some(old_host) = config.mullvad_host.replace(mullvad_host) {
                    log::info!(
                        "Overriding Mullvad host from {old_host} to {}",
                        config.mullvad_host.as_ref().unwrap()
                    );
                } else {
                    log::info!(
                        "Setting Mullvad host to {}",
                        config.mullvad_host.as_ref().unwrap()
                    );
                }

---

test/test-manager/src/tests/helpers.rs line 1207 at r3 (raw file):

/// This is for simplify, as bridges default to using the server closest to the exit anyway, and
/// OpenVPN is slated for removal.
pub async fn set_location_from_constraint(

⛏️ Why not simply name this function set_location? 😊

Code quote:

pub async fn set_location_from_constraint

---

test/test-manager/src/tests/helpers.rs line 1207 at r3 (raw file):

/// This is for simplify, as bridges default to using the server closest to the exit anyway, and
/// OpenVPN is slated for removal.
pub async fn set_location_from_constraint(

Should use of this function be discouraged in tests? If the test-manager implicitly sets a location constraint in preparation of each test, using this function from within a test will override that behavior. We need to think carefully on how to document how to set location constraints in tests 😅

Code quote:

pub async fn set_location_from_constraint(

[MarkusPettersson98]

Reviewed 1 of 3 files at r4, 10 of 10 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Serock3)

[MarkusPettersson98]

Reviewed 7 of 8 files at r6, all commit messages.
Reviewable status: 26 of 27 files reviewed, 6 unresolved discussions (waiting on @Serock3)

---

test/test-manager/test_macro/src/lib.rs line 162 at r6 (raw file):

                Some(client) => client,
                None => unreachable!("invalid mullvad client")
            };

I don't think this is the intended behaviour. The function we are creating looks like this:

pub type TestWrapperFunction = fn(
    TestContext,
    ServiceClient,
    Option<MullvadProxyClient>,
) -> BoxFuture<'static, anyhow::Result<()>>;

Notice the third parameter - it has the same type as mullvad_client. We can simply pass mullvad_client to func_name and be done:)

Code quote:

            let mullvad_client = match mullvad_client {
                Some(client) => client,
                None => unreachable!("invalid mullvad client")
            };

[MarkusPettersson98]

Reviewed 2 of 4 files at r7, 7 of 7 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

test/test-manager/docs/config.md line 27 at r8 (raw file):

## Per-test relay selection

It is possible to configure which relay(s) should be selected on a test-per-test basis by providing the "test_locations"

⛏️ Would this look nicer (when rendered) if it was surrounded by backticks?:)

"test_locations" <-> "test_locations"

Code quote:

"test_locations"

[MarkusPettersson98]

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

[MarkusPettersson98]

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