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

Add CI workflows for test workspace #5575

Merged
merged 7 commits into from
Dec 18, 2023
Merged

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Dec 8, 2023

The testing framework living in test/ historically had no CI checks. It didn't use to have it while it was developed in a separate repository, nor did it get any during its adoption into the app repository.

This PR introduces some basic CI checks when application logic is touched in the test workspace:

  • rustfmt --check is run on all Rust source files
  • clippy is run with the same configuration as the main app
  • test-runner is compiled for all supported platforms (Linux, Windows & macOS)
  • test-manager is compiled for all supported host platforms (Linux & macOS)

This should hopefully stabilize the development done to the testing framework. There is still more to be done on the CI front in the testing framework, but this is a good start.

Fixes DES-502


This change is Reviewable

Copy link

linear bot commented Dec 8, 2023

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.

Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @MarkusPettersson98)


.github/workflows/clippy-test.yml line 7 at r2 (raw file):

  pull_request:
    paths:
      - 'test/**/*.rs'

This should probably include the workflow itself.


.github/workflows/rustfmt-test.yml line 7 at r2 (raw file):

  pull_request:
    paths:
      - 'test/**/*.rs'

Same as above.


.github/workflows/rustfmt-test.yml line 19 at r2 (raw file):

        uses: actions-rs/[email protected]
        with:
          toolchain: stable

I know it's taken from rustfmt.yml, but surely "Install nightly Rust" isn't correct here? :)


.github/workflows/test-runner.yml line 1 at r2 (raw file):

# Compile the test runner

Nit: We build test-manager as well here. Maybe the jobs should also be renamed from build-test-runner... to build-test-framework`.


.github/workflows/test-runner.yml line 7 at r2 (raw file):

  pull_request:
    paths:
      - "test/**"

Same as above about including the workflow itself.


.github/workflows/test-runner.yml line 8 at r2 (raw file):

    paths:
      - "test/**"
      # Maybe we should check if changes to the app code (mullvad-management-interface) will break the test runner

That seems sensible! We use far more crates than just mullvad-management-interface, though. Would it be crazy to run this on the same conditions as daemon.yml? That does seem like the right thing to do.


.github/workflows/test-runner.yml line 52 at r2 (raw file):

      - name: Build test runner
        working-directory: test
        run: cargo build --release -p test-runner --target ${{ matrix.target }}

Maybe we could squeeze in the test-manager build here by adding a condition:

if: matrix.target != 'x86_64-pc-windows-gnu'

And building the test-runner in a step afterwards, unconditionally.

Copy link
Contributor Author

@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.

Reviewable status: 1 of 10 files reviewed, 7 unresolved discussions (waiting on @dlon)


.github/workflows/clippy-test.yml line 7 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

This should probably include the workflow itself.

Good point 😊


.github/workflows/rustfmt-test.yml line 7 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Same as above.

Ack


.github/workflows/rustfmt-test.yml line 19 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

I know it's taken from rustfmt.yml, but surely "Install nightly Rust" isn't correct here? :)

He.. :)


.github/workflows/test-runner.yml line 1 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: We build test-manager as well here. Maybe the jobs should also be renamed from build-test-runner... to build-test-framework`.

Yeah, this should be de-duped


.github/workflows/test-runner.yml line 7 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Same as above about including the workflow itself.

Ack


.github/workflows/test-runner.yml line 8 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

That seems sensible! We use far more crates than just mullvad-management-interface, though. Would it be crazy to run this on the same conditions as daemon.yml? That does seem like the right thing to do.

Hmm, I guess it is not too costly to do so. Better safe than sorry 😊


.github/workflows/test-runner.yml line 52 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Maybe we could squeeze in the test-manager build here by adding a condition:

if: matrix.target != 'x86_64-pc-windows-gnu'

And building the test-runner in a step afterwards, unconditionally.

Makes sense 😊

@MarkusPettersson98 MarkusPettersson98 force-pushed the ci/add-test-runner-ci branch 2 times, most recently from b5dd597 to 8ffc2a1 Compare December 12, 2023 08:50
Copy link
Contributor Author

@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.

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


.github/workflows/test-runner.yml line 52 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Makes sense 😊

I solved this by instead splitting up the build process into three distinct jobs: build-test-framework-linux, build-test-framework-macos & build-test-runner-windows 😊

@MarkusPettersson98 MarkusPettersson98 force-pushed the ci/add-test-runner-ci branch 2 times, most recently from 7b1b951 to 4f08d09 Compare December 12, 2023 09:18
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.

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


.github/workflows/test-framework.yml line 8 at r3 (raw file):

    paths:
      - "test/**"
      - .github/workflows/test-framework.yml

These two should already be included by the paths below now.


.github/workflows/android-app.yml line 16 at r2 (raw file):

      - '!gui/**'
      - '!ios/**'
      - '!test/**'

This one probably could be kept.

Copy link
Contributor Author

@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.

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


.github/workflows/android-app.yml line 16 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

This one probably could be kept.

Good catch!


.github/workflows/test-framework.yml line 8 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

These two should already be included by the paths below now.

done

Copy link
Collaborator

@albin-mullvad albin-mullvad 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 2 files at r4.
Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @dlon)

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.

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

@MarkusPettersson98 MarkusPettersson98 force-pushed the ci/add-test-runner-ci branch 2 times, most recently from a27c4d8 to 1caa0b5 Compare December 13, 2023 10:02
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 4 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


.github/workflows/testframework.yml line 3 at r5 (raw file):

# Compile the test framework
---
name: DES Testframework - Build

Nit: Should probably be two words: test framework. Also, why not "Desktop"?

@MarkusPettersson98 MarkusPettersson98 force-pushed the ci/add-test-runner-ci branch 2 times, most recently from 2a5018e to 4d2fb8e Compare December 13, 2023 14:58
Copy link
Contributor Author

@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.

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


.github/workflows/testframework.yml line 3 at r5 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Should probably be two words: test framework. Also, why not "Desktop"?

Makes sense. I used the Linear convention (DES), but "Desktop" is aligned better with the current naming convention of other workflows.

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.

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

Check formatting of rust source code in the `test` workspace.
Check for code quality improvements on all rust source code with
`clippy` in the `test` workspace.
Build the `test-manager` for the host platforms (Linux & macOS) as well
as the `test-runner` for all supported platforms (Linux, Windows &
macOS).
Add some missing system libraries for compiling the test framework.
These are dependencies which the base `mullvad/mullvadvpn-app-build`
container does not include.
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.

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

@MarkusPettersson98 MarkusPettersson98 merged commit 9eb6d7f into main Dec 18, 2023
39 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the ci/add-test-runner-ci branch December 18, 2023 08:18
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