Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E2E tests: remove cargo dependency #6632

Conversation

magnus-lindstrom
Copy link
Contributor

@magnus-lindstrom magnus-lindstrom commented Aug 16, 2024

What is introduced with this PR

This commit enables the usage of the dist/ directory, and also adds mullvad-version to it so that test-by-version.sh can operate without rust installed at all.

Also added a /dev/null redirect of a cd output so that one's able to use CDPATH while running the tests.

Why this is wanted

I would like to be able to run the e2e tests on a VM without having to compile the binaries at all, since that increases the disk size necessary for running the tests. The disk space needed drops by 20-30G this way.

How to test

  1. Build all executables with scripts/container-run.sh scripts/build.sh.
  2. Clone mullvadvpn-app on a system without rust installed.
  3. Copy the dist folder that was created in step 1 to mullvadvpn-app/test/dist
  4. Run the end to end tests using test-by-version.sh, which should not fail due to cargo missing.

This change is Reviewable

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@dlon @Serock3 What do you think?

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

@magnus-lindstrom magnus-lindstrom force-pushed the feature/use-standalone-binaries-in-tests branch 3 times, most recently from 08f6006 to 15e0623 Compare August 16, 2024 11:05
@magnus-lindstrom
Copy link
Contributor Author

Updated scripts to require an env var TEST_DIST_DIR to be set for dist binaries to be used, as suggested in slack by @MarkusPettersson98 . Furthermore, I thought that it would be best that in the case where the variable is defined, then all binaries in dist/ need to be there, so that you do not end up in a situation where TEST_DIST_DIR is specified, but one or more of those binaries do not exist and are silently built and used from the currently checked out commit.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @magnus-lindstrom)


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

    echo "  - TEST_REPORT : path to save the test results in a structured format"
    echo "Optional environment variables:"
    echo "  - TEST_DIST_DIR: Relative path to a directory with prebuilt binaries as produced by scripts/build.sh."

"Optional environment variables:" is echoed twice 😊

Code quote:

    echo "Optional environment variables:"
    echo "  - APP_VERSION: The version of the app to test (defaults to the latest stable release)"
    echo "  - APP_PACKAGE_TO_UPGRADE_FROM: The package version to upgrade from (defaults to none)"
    echo "  - TEST_FILTERS: specifies which tests to run (defaults to all)"
    echo "  - TEST_REPORT : path to save the test results in a structured format"
    echo "Optional environment variables:"
    echo "  - TEST_DIST_DIR: Relative path to a directory with prebuilt binaries as produced by scripts/build.sh."

test/scripts/test-utils.sh line 245 at r2 (raw file):

        echo "**********************************"
        echo "* Using test-runner in dist/"

Replace dist/ with $TEST_DIST_DIR 😊

Code quote:

echo "* Using test-runner in dist/"

@magnus-lindstrom magnus-lindstrom force-pushed the feature/use-standalone-binaries-in-tests branch from 15e0623 to 0ef2d93 Compare August 16, 2024 11:44
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @magnus-lindstrom)

@magnus-lindstrom magnus-lindstrom force-pushed the feature/use-standalone-binaries-in-tests branch from 0ef2d93 to 23c87f4 Compare August 16, 2024 11:54
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This commit enables the usage of the dist/ directory, and also adds
mullvad-version to it so that test-by-version.sh can operate without
rust installed at all.

To make use of predefined binaries in a separate directory, refer to
that directory by using the env var TEST_DIST_DIR=<dir path> and the
binaries will be used if they can be found there. If TEST_DIST_DIR is
specified, all of the following binaries need to be there:
- connection-checker
- mullvad-version
- test-manager
- test-runner

Also added a /dev/null redirect of a cd output so that one's able to use
CDPATH while running the tests.
@magnus-lindstrom magnus-lindstrom force-pushed the feature/use-standalone-binaries-in-tests branch from 23c87f4 to b0ccea7 Compare August 16, 2024 12:17
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

@MarkusPettersson98 MarkusPettersson98 merged commit 2172b85 into mullvad:main Aug 16, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants