-
Notifications
You must be signed in to change notification settings - Fork 339
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
Fix MSRV tests and drop internet-required test #2577
Conversation
`blockstream.info` is currently down, causing our CI to fail. This shouldn't really be a thing, so we drop the blockstream.info-based test here. More generally, I'm not really a fan of having tests which run (outside of CI) and call out to external servers - a developer working on LDK shouldn't have to have internet access to run our test suite and shouldn't be registering their presence with a third party to run our tests.
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2577 +/- ##
========================================
Coverage 90.61% 90.62%
========================================
Files 112 113 +1
Lines 58859 59217 +358
Branches 58859 59217 +358
========================================
+ Hits 53335 53665 +330
- Misses 5524 5552 +28
☔ View full report in Codecov by Sentry. |
Still a few CI fails |
2437f51
to
1f41457
Compare
|
||
#[tokio::test] | ||
#[cfg(any(feature = "esplora-async-https", feature = "esplora-blocking"))] | ||
async fn test_esplora_connects_to_public_server() { |
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.
Def. ACK on dropping this as it was a crutch anyways. I had originally added this as a regression test as we previously missed the https
feature, which only yielded an error when connecting to HTTPS endpoints during runtime, and this was the easiest way to assert we can connect without issue.
ci/ci-tests.sh
Outdated
# The memchr crate switched to an MSRV of 1.60 starting with v2.6.0 | ||
# This is currently only a release dependency via core2, which we intend to work with | ||
# rust-bitcoin to remove soon. | ||
[ "$RUSTC_MINOR_VERSION" -lt 60 ] && cargo update -p memchr --precise "2.5.0" --verbose |
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.
Weirdly this fails (sometimes) in CI due to
+ cargo update -p memchr --precise 2.5.0 --verbose
error: package ID specification `memchr` did not match any packages
Error: Process completed with exit code 101.
We're working with rust-bitcoin to remove the `core2` dependency at rust-bitcoin/rust-bitcoin#2066 but until that lands and we can upgrade rust-bitcoin we're stuck with it. In the mean time, we should still pass our MSRV tests.
1f41457
to
ba12a86
Compare
... and the merge commit fails CI again... 🫠 |
No description provided.