Skip to content

Disallow skipping tx-sync tests in CI #3240

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

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Aug 14, 2024

Minor follow-up to #3020.

Previously, we'd always skip tx-sync tests if the
BITCOIND_EXE/ELECTRS_EXE environment variables would be unset. While this is especially fine for local testing, we still want to enforce tests failing if somehow the bitcoind/electrs downloading or caching in CI stops working. Here, we therefore add a CI_ENV variable that indicates we're indeed running in CI, and only skip if it's unset.

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.01%. Comparing base (fc21640) to head (43bc78c).
Report is 75 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3240      +/-   ##
==========================================
+ Coverage   89.74%   91.01%   +1.26%     
==========================================
  Files         122      127       +5     
  Lines      101921   114325   +12404     
  Branches   101921   114325   +12404     
==========================================
+ Hits        91470   104052   +12582     
+ Misses       7763     7684      -79     
+ Partials     2688     2589      -99     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ci/ci-tests.sh Outdated
@@ -44,7 +44,7 @@ cargo check --verbose --color always --features rpc-client,rest-client,tokio
popd

if [[ "$HOST_PLATFORM" != *windows* ]]; then
if [ -z "$BITCOIND_EXE" ] || [ -z "$ELECTRS_EXE" ]; then
if [ -z "$CI_ENV" ] && [ -z "$BITCOIND_EXE" ] || [ -z "$ELECTRS_EXE" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add ()s (IIRC more []s)? I dunno how to read whether ELECTRS_EXE overrides CI_ENV or not.

Copy link
Contributor Author

@tnull tnull Aug 14, 2024

Choose a reason for hiding this comment

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

Can we add ()s (IIRC more []s)? I dunno how to read whether ELECTRS_EXE overrides CI_ENV or not.

Heh, neither exactly, but now switched to [[ .. ]] which allows grouping. Seems shellcheck is fine with it. 🤷‍♂️

@tnull tnull force-pushed the 2024-08-add-ci-flag-for-bitcoind branch from bf79d8c to 7883f50 Compare August 14, 2024 16:10
@tnull tnull force-pushed the 2024-08-add-ci-flag-for-bitcoind branch from 7883f50 to 3571095 Compare August 15, 2024 17:15
@tnull
Copy link
Contributor Author

tnull commented Aug 15, 2024

Force-pushed the following changes:

> git diff-tree -U2 7883f502f 35710958f
diff --git a/ci/ci-tests.sh b/ci/ci-tests.sh
index 1a912016e..a0dc27d5a 100755
--- a/ci/ci-tests.sh
+++ b/ci/ci-tests.sh
@@ -45,20 +45,21 @@ popd

 if [[ "$HOST_PLATFORM" != *windows* ]]; then
+       pushd lightning-transaction-sync
+       echo -e "\n\nChecking Transaction Sync Clients with features."
+       cargo check --verbose --color always --features esplora-blocking
+       cargo check --verbose --color always --features esplora-async
+       cargo check --verbose --color always --features esplora-async-https
+       cargo check --verbose --color always --features electrum
+
        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."
        else
-               echo -e "\n\nBuilding and testing Transaction Sync Clients with features"
-               pushd lightning-transaction-sync
-
+               echo -e "\n\nTesting Transaction Sync Clients with features."
                cargo test --verbose --color always --features esplora-blocking
-               cargo check --verbose --color always --features esplora-blocking
                cargo test --verbose --color always --features esplora-async
-               cargo check --verbose --color always --features esplora-async
                cargo test --verbose --color always --features esplora-async-https
-               cargo check --verbose --color always --features esplora-async-https
                cargo test --verbose --color always --features electrum
-               cargo check --verbose --color always --features electrum
-               popd
        fi
+       popd
 fi

Previously, we'd always skip tx-sync tests if the
`BITCOIND_EXE`/`ELECTRS_EXE` environment variables would be unset. While
this is especially fine for local testing, we still want to enforce
tests failing if somehow the `bitcoind`/`electrs` downloading or caching
in CI stops working. Here, we therefore add a `CI_ENV` variable that
indicates we're indeed running in CI, and only skip if it's unset.
@tnull tnull force-pushed the 2024-08-add-ci-flag-for-bitcoind branch from 3571095 to 43bc78c Compare August 16, 2024 15:41
@tnull
Copy link
Contributor Author

tnull commented Aug 16, 2024

Force-pushed once more:

> git diff-tree -U2 35710958f 43bc78ce3
diff --git a/ci/ci-tests.sh b/ci/ci-tests.sh
index a0dc27d5a..43d6aeaa4 100755
--- a/ci/ci-tests.sh
+++ b/ci/ci-tests.sh
@@ -54,4 +54,5 @@ if [[ "$HOST_PLATFORM" != *windows* ]]; then
        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 --tests
        else
                echo -e "\n\nTesting Transaction Sync Clients with features."

@tnull tnull requested a review from TheBlueMatt August 18, 2024 07:30
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks! Just gonna land this cause its CI-specific and trivial

@TheBlueMatt TheBlueMatt merged commit dd37077 into lightningdevkit:main Aug 19, 2024
18 of 21 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.

2 participants