Replace lightning-block-sync test that depended on foo.com
#2655
Conversation
Codecov ReportAll modified lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2655 +/- ##
==========================================
+ Coverage 89.00% 89.27% +0.27%
==========================================
Files 112 112
Lines 87157 88472 +1315
Branches 87157 88472 +1315
==========================================
+ Hits 77574 78986 +1412
+ Misses 7339 7301 -38
+ Partials 2244 2185 -59
☔ View full report in Codecov by Sentry. |
|
Looks like https://bitcoincore.org is down? |
lightning-block-sync/src/http.rs
Outdated
| assert!(http_addr.is_some(), "Expected socket address"); | ||
| while let Some(addr) = http_addr { |
There was a problem hiding this comment.
nit: Could use peekable and iterate with a for loop.
There was a problem hiding this comment.
Sure, dont even need peekable.
TheBlueMatt
force-pushed
the
2023-10-no-test-net
branch
2 times, most recently
from
0753f20 to
039ffa0
Compare
Our tests should generally not rely on internet access, and should not rely on the behavior of any given remote server. However, one of the `endpoint_tests` in `lightning-block-sync::http` relied on `foo.com` resolving to a single socket address, which both might change in the future and makes our tests fail without internet.
TheBlueMatt
force-pushed
the
2023-10-no-test-net
branch
from
039ffa0 to
631f989
Compare
| } | ||
| assert!(std_addrs.next().is_none()); |
There was a problem hiding this comment.
I guess we are assuming std_addrs contains at least one item? Probably fine but just want to call that out.
There was a problem hiding this comment.
Otherwise, you need peekable.
There was a problem hiding this comment.
No? We only check that the two iterators are exactly equal, not that either contain anything.
There was a problem hiding this comment.
I mean, the old code made sure there was at least one with panic!("Expected socket address"). Without that, you may skip the check entirely if the size is zero, meaning the test isn't checking anything.
There was a problem hiding this comment.
Ah, yea, I don't think we need to check that. Like, in theory someone could have a horrendously misconfigured system where localhost doesn't resolve to anything. We shouldn't fail tests if that happens, though honestly I'm surprised that system is running and not randomly crashing in other places :)
Our tests should generally not rely on internet access, and should not rely on the behavior of any given remote server. However, one of the
endpoint_testsinlightning-block-sync::httprelied onfoo.comresolving to a single socket address, which both might change in the future and makes our tests fail without internet.