Migrate testing repo #5189

dlon · 2023-09-23T16:19:50Z

Mostly a move of the old repository. Of the changes, the important ones are:

In ci-runtests.sh, the version under test is always HEAD. The correct way to run ci-runtests.sh for an arbitrary commit, tag, or release is to check out that commit first and just run the script. This makes it easy to make changes without breaking tests for older commits/versions.
I removed the dependency on old management client interfaces. The test is using the CLI instead, since the dependency tree was far too messy.
Switch to using GitHub Actions.

This change is

dlon

Reviewable status: 0 of 69 files reviewed, 1 unresolved discussion

test/BUILD_OS_IMAGE.md line 27 at r1 (raw file):

## Dependencies to install in the image

`xvfb` must be installed on the guest system. You will also need

xvfb and wireguard-tools are installed over SSH, so this is only true for the legacy method. The other packages are assumed to exist, though.

test/BUILD_OS_IMAGE.md line 40 at r1 (raw file):

```bash
dnf install libnss3 libgbm1 libasound2 libatk1.0-0 libatk-bridge2.0-0 libcups2 libgtk-3-0 wireguard-tools xorg-x11-server-Xvfb

Is this list really correct? I'm surprised the packages are named exactly the same on Fedora.

Jontified

Reviewed 69 of 69 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dlon)

test/ci-runtests.sh line 26 at r1 (raw file):

if [[ "$#" -lt 1 ]]; then
    print_usage

No need for function, just print "usage: $0 TEST_OS"

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

    // Ensure that the tunnel state transitions to "error". Fail if it transitions to some other
    // state.
    let new_state = next_state.await?;

No need to put future into next_state, just await it here?

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

            // NOTE: Many packets will likely be observed for API traffic. Rather than filtering all
            // of those specifically, simply fail if our probes are observed.
            packet.source.ip() == guest_ip && packet.destination.ip() == inet_destination.ip()

It seems nicer to have a whitelist for API endpoints rather than to just check if our specific pings are getting out. Since this functionality already existed why remove it? Hard to maintain?

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

        log::info!("Test whether tunnel traffic works");
        let geoip_lookup = geoip_lookup_with_retries(&rpc).await.unwrap();
        assert!(geoip_lookup.mullvad_exit_ip, "Exit ip is not from Mullvad");

This seems to be reused often enough to warrant a helper function. Seems especially good since we may want to add other sanity checks to a working internet in the future.

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

    // Set relays to use
    log::info!("Select relay");
    let relay_filter = |relay: &types::Relay| {

Nit: I feel like separating the closure into it's own variable reduces readability. What is nice about having the closure right in the function call is that I know at first glance that 1) the closure and the function call definitely relate to each other and 2) the closure will never be used in the future

Obviously not a problem for this PR

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

    log::info!("Select relay");
    let relay_filter = |relay: &types::Relay| {
        relay.active && relay.endpoint_type == i32::from(types::relay::RelayType::Wireguard)

Is there a reason we only pick wireguard relays here?

test/test-manager/src/vm/network/macos.rs line 61 at r1 (raw file):

        if let Some(address) = addr.address.as_ref().and_then(|addr| addr.as_sockaddr_in()) {
            let interface_ip = *SocketAddrV4::from(*address).ip();
            if interface_ip == NON_TUN_GATEWAY {

If Tart assigns this, how come this works? Is it always the same atm?

test/test-manager/src/vm/network/macos.rs line 74 at r1 (raw file):

async fn enable_forwarding() -> Result<()> {
    // Check if forwarding is enabled

Why check if forwarding is enabled if we won't act on this?

dlon

Reviewable status: 61 of 69 files reviewed, 6 unresolved discussions (waiting on @Jontified)

test/ci-runtests.sh line 26 at r1 (raw file):