Make test-manager easier to run on any app version

[Serock3]

• The first three commits fixes a couple if issues preventing the test-manager from finding release build packages.
• The last commit restructures the convenience scripts. ci-runtests.sh should now only be used by the GitHub actions, the new test-by-version.sh script can be used to manually test any pre-built app version
• The remaining commits cleans up the CLI, polishes the logs and error messages and fixes a flaky test.

Fixes DES-1092

---

This change is 

[linear]

DES-1092 Simplify running test-manager with just latest build and no upgrade

[MarkusPettersson98]

Reviewed 11 of 13 files at r1, 12 of 12 files at r2, 2 of 4 files at r3.
Reviewable status: 21 of 23 files reviewed, 4 unresolved discussions (waiting on @Serock3)

---

test/test-by-version.sh line 3 at r2 (raw file):

#!/usr/bin/env bash

set -eu

Would be nice with a small introductory comment which explains the intended use of this script, preferably explaining the script's arguments 😊

Code quote:

#!/usr/bin/env bash

set -eu

---

test/test-manager/src/main.rs line 31 at r1 (raw file):

    /// Create or edit a VM config
    Set {
        /// Name of the config

Nit: Maybe change to something like "Name of the test runner config (vm)" ? 😊

Otherwise, just "Name of the VM config" to be consistent with the other commands Remove/List/RunVM

Code quote:

/// Name of the config

---

test/test-manager/src/network_monitor.rs line 101 at r2 (raw file):

            }
            IpHeaderProtocols::Icmp => {}
            proto => log::warn!("ignoring v4 packet, transport/protocol type {proto}"),

Now when I think about it, maybe this log should be debug instead. Might be spammy with warn?

Code quote:

            proto => log::warn!("ignoring v4 packet, transport/protocol type {proto}"),

---

test/test-manager/src/network_monitor.rs line 143 at r2 (raw file):

            }
            IpHeaderProtocols::Icmpv6 => {}
            proto => log::warn!("ignoring v6 packet, transport/protocol type {proto}"),

Now when I think about it, maybe this log should be debug instead. Might be spammy with warn?

Code quote:

            proto => log::warn!("ignoring v6 packet, transport/protocol type {proto}"),

---

test/test-manager/src/package.rs line 183 at r2 (raw file):

        assert_eq!(capture, "2024.3");
    }
}

❤️

Code quote:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_version_regex() {
        let path = Path::new("../some/path/MullvadVPN-2024.4-beta1-dev-f7df8e_amd64.deb");
        let capture = get_version_from_path(path).unwrap();
        assert_eq!(capture, "2024.4-beta1-dev-f7df8e");

        let path = Path::new("../some/path/MullvadVPN-2024.4-beta1-f7df8e_amd64.deb");
        let capture = get_version_from_path(path).unwrap();
        assert_eq!(capture, "2024.4-beta1-f7df8e");

        let path = Path::new("../some/path/MullvadVPN-2024.4-dev-f7df8e_amd64.deb");
        let capture = get_version_from_path(path).unwrap();
        assert_eq!(capture, "2024.4-dev-f7df8e");

        let path = Path::new("../some/path/MullvadVPN-2024.3_amd64.deb");
        let capture = get_version_from_path(path).unwrap();
        assert_eq!(capture, "2024.3");
    }
}

[Serock3]

Reviewable status: 21 of 24 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)

---

test/test-by-version.sh line 3 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Would be nice with a small introductory comment which explains the intended use of this script, preferably explaining the script's arguments 😊

Yup, Magnus also commented on that. Added a usage function. What do you think?

---

test/test-manager/src/main.rs line 31 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Nit: Maybe change to something like "Name of the test runner config (vm)" ? 😊

Otherwise, just "Name of the VM config" to be consistent with the other commands Remove/List/RunVM

Good catch. I wrote VM config in all other places but missed it here. I also added run 'test-manager list' to see available configs to the explanation for vm

---

test/test-manager/src/network_monitor.rs line 101 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Now when I think about it, maybe this log should be debug instead. Might be spammy with warn?

Looking at then logs from the CI, this string doesn't appear at all. Have you seen it a lot?

---

test/test-manager/src/network_monitor.rs line 143 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Now when I think about it, maybe this log should be debug instead. Might be spammy with warn?

see above

[MarkusPettersson98]

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

---

test/test-by-version.sh line 3 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Yup, Magnus also commented on that. Added a usage function. What do you think?

Much better!

---

test/test-manager/src/main.rs line 31 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Good catch. I wrote VM config in all other places but missed it here. I also added run 'test-manager list' to see available configs to the explanation for vm

Ah, nice! Did you update the comment which I quoted yet? 😊

---

test/test-manager/src/network_monitor.rs line 101 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Looking at then logs from the CI, this string doesn't appear at all. Have you seen it a lot?

I've seen it, not much though. I don't think the event is that interesting though, which is probably why it was debug to begin with 😅

[Serock3]

Reviewable status: 22 of 24 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

test/test-manager/src/main.rs line 31 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Ah, nice! Did you update the comment which I quoted yet? 😊

Whoops 🤦, fixed now

---

test/test-manager/src/network_monitor.rs line 101 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I've seen it, not much though. I don't think the event is that interesting though, which is probably why it was debug to begin with 😅

Alright, lowered it to debug

---

test/test-manager/src/network_monitor.rs line 143 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

see above

Lowered to debug

[MarkusPettersson98]

Reviewed 1 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

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

[MarkusPettersson98]

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

---

test/scripts/test-utils.sh line 247 at r7 (raw file):

    if [[ -z "${TEST_REPORT+x}" ]]; then
        echo "'TEST_REPORT' env not set, not saving test report"

Is it explained anywhere what TEST_REPORT is expected to be? A directory? A path to a file? etc 😊

Code quote:

        echo "'TEST_REPORT' env not set, not saving test report"

[Serock3]

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

---

test/scripts/test-utils.sh line 247 at r7 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Is it explained anywhere what TEST_REPORT is expected to be? A directory? A path to a file? etc 😊

I added it to the usage function intest-by-version.sh and improved the CLI help text

[MarkusPettersson98]

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

[MarkusPettersson98]

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