Skip to content

ci: remove the need to pass clippy for the e2e tests #103

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

ci: remove the need to pass clippy for the e2e tests #103

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,8 @@ jobs:
- name: Disallow squash! commits
run: git log --pretty=format:%s origin/master..HEAD | grep -zv squash!

clippy:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: extractions/setup-just@v2
- uses: dtolnay/rust-toolchain@master
with:
toolchain: stable
- uses: Swatinem/rust-cache@v2
- run: cargo clippy --all-targets --all-features -- -D warnings

unit-tests:
runs-on: ubuntu-latest
needs: [ clippy ]
steps:
- uses: actions/checkout@v4
- uses: extractions/setup-just@v2
Expand All @@ -66,7 +54,6 @@ jobs:
run: RUST_BACKTRACE=1 cargo test --workspace --exclude ark-secp256k1

e2e-tests:
needs: [ clippy ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The downside of this change though is that the e2e-tests will run twice if there was a clippy error.

Copy link
Collaborator Author

@vincenzopalazzo vincenzopalazzo Aug 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The downside of this change though is that the e2e-tests will run twice if there was a clippy error.

true, but I think it is important to have prototype code able to run e2e without focusing of crap clippy warning, will a git pre-commit hook addressing your concern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what we could do is have a step in ci.yml which ensures that the tests build with or without warnings (before running the clippy step). Then we can replace clippy here, to know that the e2e tests can actually run, which I think is the main purpose of this needs declaration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mh, do you think that the clippy could be the last step of the CI? I mean looks like just the right command for the final touch.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm okay with this tbh.

What do you think, @bonomat?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I always optimize for CI-minute usage, i.e. run the fastest checks first and not run long tasks first which will need to run again if a short one fails.
Assuming clippy fails, you will need to push again and we will run the whole e2e tests again (even if they succeeded in the first run).

Long story short: I'm not convinced 😅 we shouldn't use CI as a unit or e2e test testbed (also given they are sometimes flaky).

strategy:
fail-fast: false
matrix:
Expand Down Expand Up @@ -123,3 +110,15 @@ jobs:

- name: Build crates for WASM
run: PATH="/opt/homebrew/opt/llvm/bin:$PATH" cargo build -p ark-core -p ark-rest --target wasm32-unknown-unknown

clippy:
runs-on: ubuntu-latest
needs: [ wasm_ubuntu, wasm_macos ]
steps:
- uses: actions/checkout@v4
- uses: extractions/setup-just@v2
- uses: dtolnay/rust-toolchain@master
with:
toolchain: stable
- uses: Swatinem/rust-cache@v2
- run: cargo clippy --all-targets --all-features -- -D warnings
Loading