Refactor test manager cleanup logic

[Serock3]

Currently, the cleanup code only resets a small subset of the daemon state, and many tests implicitly rely on being left in a certain state by the previous. This is inherently flaky, as one test failing may cause the following ones to also fail. It is also very inflexible, as it doesn't allow testing arbitrary single tests, or changing the order of tests.

• Refactor the cleanup to completely and deterministically make sure the correct daemon version is installed, logged in and disconnected with all setting cleared before each test
• Allow reordering tests

Fixes DES-1137

---

This change is 

[linear]

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

[dlon]

Reviewed 10 of 20 files at r1, 1 of 1 files at r2.
Reviewable status: 9 of 21 files reviewed, 4 unresolved discussions (waiting on @Serock3)

---

test/README.md line 148 at r1 (raw file):

Note on ci-runtests.sh

Should this be changed to scripts/ci-runtests.sh?

---

test/scripts/build-runner-image.sh line 37 at r1 (raw file):

            "${SCRIPT_DIR}/../target/$TARGET/release/test-runner.exe" \
            "${SCRIPT_DIR}/../target/$TARGET/release/connection-checker.exe" \
            "${PACKAGE_FOLDER}/"*.exe \

What's the reason for renaming this from _DIR? Isn't that less consistent with how we name variables in general?

---

test/scripts/ssh-setup.sh line 116 at r1 (raw file):

# Install required packages
if which apt &>/dev/null; then
    echo "not doing install_packages_apt"

Should this be reverted?

---

test/scripts/test-utils.sh line 6 at r1 (raw file):

CALLER_DIR=$(pwd)
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"

source test-utils.sh will overwrite variables in the calling script, for example $SCRIPT_DIR will refer to the directory of test-utils.sh after you've sourced it.

I kind of think we should either hide these "private" variables somehow, or turn this script into a "program". That is, execute the script rather than source it, and parse the first argument to the script as a command.

[dlon]

Reviewable status: 9 of 21 files reviewed, 5 unresolved discussions (waiting on @Serock3)

---

test/scripts/build-runner-image.sh line 37 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

What's the reason for renaming this from _DIR? Isn't that less consistent with how we name variables in general?

Ah, because of its name in test-manager. Should we do the opposite and rename the argument to packages-dir? :)

---

test/scripts/test-utils.sh line 29 at r1 (raw file):

if [[ ("$(uname -s)" == "Darwin") ]]; then
    export CACHE_FOLDER=$HOME/Library/Caches/mullvad-test/packages

Could we instead export PACKAGES_DIR/PACKAGE_FOLDER and not define it anywhere except in this script?

[dlon]

Reviewed 1 of 20 files at r1.
Reviewable status: 10 of 21 files reviewed, 8 unresolved discussions (waiting on @Serock3)

---

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

    Set {
        /// Name of the config
        name: String,

I think we should rename name to vm for consistency.

---

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

    Remove {
        /// Name of the config
        name: String,

I think we should rename name to vm for consistency.

---

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

    Update {
        /// Name of the VM config
        name: String,

I think we should rename name to vm for consistency.

[Serock3]

Reviewable status: 6 of 22 files reviewed, 5 unresolved discussions (waiting on @dlon)

---

test/README.md line 148 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should this be changed to scripts/ci-runtests.sh?

Done

---

test/scripts/build-runner-image.sh line 37 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Ah, because of its name in test-manager. Should we do the opposite and rename the argument to packages-dir? :)

We could to that, if you really prefer dir over package, yes

---

test/scripts/test-utils.sh line 29 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could we instead export PACKAGES_DIR/PACKAGE_FOLDER and not define it anywhere except in this script?

In test-by-version.sh, you are expected to provide PACKAGE_FOLDER yourself, ci-runtests.sh just sets it to the cache folder

---

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

Previously, dlon (David Lönnhager) wrote…

I think we should rename name to vm for consistency.

Alright, I though just vm seemed a bit weird in the context. But I agree that it is more consistent

---

test/scripts/ssh-setup.sh line 116 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should this be reverted?

Oh, woops. That wasn't intended to be committed. Good catch.

[Serock3]

Reviewable status: 6 of 22 files reviewed, 5 unresolved discussions (waiting on @dlon)

---

test/scripts/test-utils.sh line 6 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

source test-utils.sh will overwrite variables in the calling script, for example $SCRIPT_DIR will refer to the directory of test-utils.sh after you've sourced it.

I kind of think we should either hide these "private" variables somehow, or turn this script into a "program". That is, execute the script rather than source it, and parse the first argument to the script as a command.

There seems to be no way to prevent global variables from being sourced (in functions you can write local, but not outside functions). I refactored the globals to be functions instead, and called them where necessary

---

test/scripts/test-utils.sh line 29 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

In test-by-version.sh, you are expected to provide PACKAGE_FOLDER yourself, ci-runtests.sh just sets it to the cache folder

That wasn't the case when I wrote the comment, men but it is now

[Serock3]

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

---

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

Previously, Serock3 (Sebastian Holmin) wrote…

Alright, I though just vm seemed a bit weird in the context. But I agree that it is more consistent

Done

[dlon]

Reviewed 6 of 20 files at r1, 5 of 7 files at r4, 2 of 3 files at r6, 7 of 7 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

test/scripts/test-utils.sh line 6 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

There seems to be no way to prevent global variables from being sourced (in functions you can write local, but not outside functions). I refactored the globals to be functions instead, and called them where necessary

Nit: Maybe get_script_dir should be renamed to get_utils_diror similar because it's public.

---

test/scripts/test-utils.sh line 29 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

That wasn't the case when I wrote the comment, men but it is now

Ah. It might be fine for all scripts to have PACKAGE_DIR be overrideable. Then this could all be handled in the utils script.

---

test/test-manager/src/tests/install.rs line 290 at r6 (raw file):

    // Re-install the app to ensure that the next test can run
    test_install_new_app(_ctx, rpc).await?;

If something fails earlier in this test, then daemon will not be reinstalled, so all remaining tests will also fail.

[Serock3]

Reviewable status: 18 of 27 files reviewed, all discussions resolved (waiting on @dlon)

---

test/scripts/test-utils.sh line 29 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Ah. It might be fine for all scripts to have PACKAGE_DIR be overrideable. Then this could all be handled in the utils script.

I though that it was a nice design to export CACHE_FOLDER regardless, since it could be useful for someone writing their own script. If you prefer, I could define a function, like

Code snippet:

function get_package_dir {
    local package_dir
    if [[ -z "${PACKAGE_DIR+x}" ]]; then
        package_dir="$CACHE_FOLDER"
    else
        package_dir="$PACKAGE_DIR"
    fi
    create_package_dir "$package_dir"
    echo "$package_dir"
}

And call it inside `test-utils.sh` instead of using `PACKAGE_DIR` directly, so that the defaulting behaviour is moved to `test-utils.sh`

---

test/test-manager/src/tests/install.rs line 290 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

If something fails earlier in this test, then daemon will not be reinstalled, so all remaining tests will also fail.

Should be fixed with the new cleanup logic

[dlon]

Reviewed 9 of 9 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)

---

test/scripts/test-utils.sh line 29 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I though that it was a nice design to export CACHE_FOLDER regardless, since it could be useful for someone writing their own script. If you prefer, I could define a function, like

I like it. :) How do you feel about getting rid of CACHE_DIR completely with your function?

function get_package_dir {
    if [[ -n "${PACKAGE_DIR+x}" ]]; then
        echo "$PACKAGE_DIR"
        return 0
    fi
    if [[ ("$(uname -s)" == "Darwin") ]]; then
        echo "$HOME/Library/Caches/mullvad-test/packages"
    elif [[ ("$(uname -s)" == "Linux") ]]; then
        echo "$HOME/.cache/mullvad-test/packages"
    else
        echo "Unsupported OS" 1>&2
        exit 1
    fi
}

---

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

    // verify that daemon is not already running
    if rpc.mullvad_daemon_get_status().await? != ServiceStatus::NotRunning {
        rpc.stop_mullvad_daemon().await.unwrap();

It would be more correct to uninstall the app here rather than stop it, since we're intending to test an install from a "clean" (uninstalled) state.

Arguably, though, we should just scrap the test. If it is now the responsibility of test-manager to ensure that the latest app is installed before a test runs, then this test is mostly confusing, IMO.

---

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

            log::info!("Failed to reach daemon after test, re-installing app");
            // Re-install the app to ensure that the next test can run
            install_app(rpc, &TEST_CONFIG.app_package_filename)

If it's test-manager's responsibility (not the responsibility of the tests themselves) to install the latest app, it should probably ensure that the app is installed before each test, and also that the correct version is installed.

Otherwise, it will unexpectedly not be installed before the first test, and not properly clean up the mess potentially made by other tests (such as installing an older version).

Maybe we could check this only in the case that a test has a Mullvad daemon client parameter. That would prevent pointless installs for the upgrade tests, etc. What do you think?

---

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

            log::info!("Failed to reach daemon after test, re-installing app");
            // Re-install the app to ensure that the next test can run
            install_app(rpc, &TEST_CONFIG.app_package_filename)

Can we split this into two functions? One for handling (re)installs, one for restarting/updating the environment. These seem like different things.

[linear]

DES-1137 Refactor cleanup routine between tests

[dlon]

Reviewed 3 of 4 files at r14, 3 of 3 files at r15, 17 of 20 files at r17, 7 of 10 files at r18, 10 of 10 files at r19, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Serock3)

---

test/test-manager/src/tests/account.rs line 86 at r19 (raw file):

    );

    mullvad_client.logout_account().await?;

Nit: This and the following line are not required (app isn't logged in).

---

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

/// Reset the daemons environment.
///
/// Will and restart or reinstall it if necessary.

Nit: Outdated comment.

---

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

pub fn get_filtered_tests(specified_tests: &[String]) -> Result<Vec<&TestMetadata>, anyhow::Error> {
    let mut tests = get_tests();
    tests.retain(|test| test.should_run_on_os(TEST_CONFIG.os));

Nit, but this might belong in get_tests?

---

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

    mut mullvad_client: MullvadProxyClient,
) -> Result<(), Error> {
    mullvad_client.connect_tunnel().await?;

Are you sure settings-import requires being connected? If so, use helpers::connect_and_wait to ensure it will actually wait for the connected state.

---

test/scripts/test-utils.sh line 53 at r17 (raw file):

    if [[ -n "${PACKAGE_DIR+x}" ]]; then
        # Resolve the package dir to an absolute path since cargo must be invoked from the test directory
        package_dir=$(cd "$PACKAGE_DIR" > /dev/null && pwd)

Will this not fail if the dir doesn't exist? Though maybe that's fine as long as stderr receives anything useful.

[dlon]

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