-
Notifications
You must be signed in to change notification settings - Fork 21
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.
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
Conversation
WalkthroughCI workflow updated: clippy was removed from the initial gating position and decoupled from tests; unit-tests and e2e-tests no longer depend on clippy. A new clippy job was added at the bottom that runs after Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant UT as unit-tests
participant E2E as e2e-tests
participant WU as wasm_ubuntu
participant WM as wasm_macos
participant CL as clippy
rect rgba(200,200,255,0.20)
note over GH: New CI flow (clippy decoupled)
GH->>UT: Run
GH->>E2E: Run
GH->>WU: Run
GH->>WM: Run
par Builds finish independently
UT-->>GH: Complete
E2E-->>GH: Complete
WU-->>GH: Complete
WM-->>GH: Complete
end
GH->>CL: Run after WU & WM complete
CL-->>GH: Lint results (-D warnings)
end
sequenceDiagram
autonumber
participant GH as GitHub Actions
participant CL as clippy
participant UT as unit-tests
participant E2E as e2e-tests
rect rgba(255,230,200,0.20)
note over GH: Prior CI flow (clippy gated)
GH->>CL: Run first
CL-->>GH: Complete
GH->>UT: Run (needed clippy)
GH->>E2E: Run (needed clippy)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
run: RUST_BACKTRACE=1 cargo test --workspace --exclude ark-secp256k1 | ||
|
||
e2e-tests: | ||
needs: [ clippy ] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
67e05d4
to
d2beeaa
Compare
Rebased on master and implemented the version described in #103 (comment) The reason to implement #103 (comment) is that I would like to run and build the e2e before caring about the clippy warnings that are important for the general sanity of the code base, but not essential to make a PoC like I was doing in #98 Please let me know what do you think |
d2beeaa
to
5f8121e
Compare
Sometimes it is nice to have a fixup commit and be able to run the tests without having to pass clippy or other repo rules passing before running the e2e tests. In other works clippy is a final touch check from the code prospective, so probably make sense move to the end. Signed-off-by: Vincenzo Palazzo <[email protected]>
5f8121e
to
438bc44
Compare
Make sense the point of @bonomat in #103 (comment), so closing this and I will keep this for reference, no need to spend more brain cycle on this :D |
Sometimes it is nice to have a fixup commit and be able to run the tests without having to pass clippy or other repo rules passing before running the e2e tests.
Built in #98, but independent from it
Summary by CodeRabbit
Chores
Notes