Remove redundant cargo check/test calls from CI scripts#4459
Remove redundant cargo check/test calls from CI scripts#4459joostjager wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
ci/ci-tests-features.sh
Outdated
| @@ -6,12 +6,10 @@ source "$(dirname "$0")/ci-tests-common.sh" | |||
|
|
|||
| echo -e "\n\nChecking and testing lightning with features" | |||
There was a problem hiding this comment.
If you remove the check part, you might want to update the printed message here and elsewhere to remove the Checking part.
There was a problem hiding this comment.
Removed. I left it in place in ci-tests-sync.sh, because there is still some only-checking in non CI.
|
|
||
| if [ -z "$CI_ENV" ] && [[ -z "$BITCOIND_EXE" || -z "$ELECTRS_EXE" ]]; then | ||
| echo -e "\n\nSkipping testing Transaction Sync Clients due to BITCOIND_EXE or ELECTRS_EXE being unset." | ||
| cargo check -p lightning-transaction-sync --tests |
joostjager
force-pushed
the
ci-cleanup-redundant-checks
branch
from
03629f0 to
6b0eeea
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4459 +/- ##
==========================================
+ Coverage 85.97% 86.09% +0.12%
==========================================
Files 159 159
Lines 104722 105185 +463
Branches 104722 105185 +463
==========================================
+ Hits 90030 90559 +529
+ Misses 12191 12115 -76
- Partials 2501 2511 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
Review courtesy of Claude
ci/ci-tests-sync.sh
Outdated
| @@ -7,23 +7,17 @@ source "$(dirname "$0")/ci-tests-common.sh" | |||
| echo -e "\n\nChecking and testing Block Sync Clients with features" | |||
There was a problem hiding this comment.
I left it in place (#4459 (comment)), but also fine with removing. Removed.
| cargo check --tests --quiet --color always | ||
|
|
||
| WORKSPACE_MEMBERS=( $(cat Cargo.toml | tr '\n' '\r' | sed 's/\r //g' | tr '\r' '\n' | grep '^members =' | sed 's/members.*=.*\[//' | tr -d '"' | tr ',' ' ') ) | ||
|
|
||
| echo -e "\n\nTesting the workspace, except lightning-transaction-sync." | ||
| echo -e "\n\nTesting the workspace." | ||
| cargo test --quiet --color always |
There was a problem hiding this comment.
I think the goal is to fail as fast as possible - perhaps 🤷♂️
| cargo check -p lightning-transaction-sync --tests --quiet --color always --features esplora-async-https | ||
| cargo check -p lightning-transaction-sync --tests --quiet --color always --features electrum | ||
| else | ||
| echo -e "\n\nTesting Transaction Sync Clients with features." |
There was a problem hiding this comment.
Should this also test without any features?
There was a problem hiding this comment.
We should indeed be consistent. The crate doesn't contains anything without a feature it seems (lib.rs), but maybe we do want to check that it doesn't error. Added the test without features. diff
joostjager
force-pushed
the
ci-cleanup-redundant-checks
branch
from
6b0eeea to
ab5a336
Compare
- Remove `cargo check` calls that immediately followed `cargo test` with the same crate and features, as `cargo test` already compiles everything that `cargo check` would. - Add `--tests` to `cargo check` calls that precede `cargo test`, so test code is also checked for compilation errors. - Replace per-member `cargo doc` loop with `cargo doc --workspace`. - Move feature-specific `cargo check` for lightning-transaction-sync into the skip branch only, avoiding redundant work when tests run. - Remove redundant `cargo test -p lightning-custom-message` already covered by the workspace-level `cargo test`. - Fix stale "except lightning-transaction-sync" comments, as that crate was moved back into the workspace in b19848e. AI tools were used in preparing this commit.
joostjager
force-pushed
the
ci-cleanup-redundant-checks
branch
from
ab5a336 to
c0cdaaa
Compare
4 participants
cargo checkcalls that immediately followedcargo testwith the same crate and features, ascargo testalready compiles everything thatcargo checkwould.--teststocargo checkcalls that precedecargo test, so test code is also checked for compilation errors.cargo docloop withcargo doc --workspace.cargo checkfor lightning-transaction-sync into the skip branch only, avoiding redundant work when tests run.cargo test -p lightning-custom-messagealready covered by the workspace-levelcargo test.AI tools were used in preparing this commit.