Fix flakiness in test_tor_connect#4521
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
lightning-net-tokio/src/lib.rs
Outdated
| let mut google_addresses: Vec<_> = "google.com:80".to_socket_addrs().unwrap().collect(); | ||
| let ipv6_pos = google_addresses.iter().position(|a| a.is_ipv6()).expect("must resolve at least one ipv6 address"); | ||
| let mut google_ipv6 = google_addresses.remove(ipv6_pos); | ||
| let ipv4_pos = google_addresses.iter().position(|a| a.is_ipv4()).expect("must resolve at least one ipv4 address"); | ||
| let mut google_ipv4 = google_addresses.remove(ipv4_pos); |
There was a problem hiding this comment.
Nit: to_socket_addrs() performs blocking DNS resolution inside an async fn. For a test this is unlikely to cause issues with the single-threaded #[tokio::test] runtime, but tokio::net::lookup_host("google.com:80") would be the idiomatic async alternative.
More importantly: the two expect calls assume google.com always resolves to both IPv4 and IPv6 addresses. If a CI runner's DNS resolver strips AAAA records (some corporate/restricted environments do this), or if there's a transient DNS hiccup returning only one address family, this introduces a new source of flakiness — which is ironic given the PR's purpose. Consider using find with a fallback/skip instead of expect, e.g.:
let ipv6_pos = match google_addresses.iter().position(|a| a.is_ipv6()) {
Some(pos) => pos,
None => { eprintln!("Skipping: no IPv6 address resolved for google.com"); return; }
};
There was a problem hiding this comment.
Seem like valid points? (fwiw, we should also switch to async resolution in LDK Node, cf. lightningdevkit/ldk-node#854).
There was a problem hiding this comment.
If you don't mind I'll take the first point, I'd like to see the second point actually manifest before silently skipping over an IPv4 or IPv6 case :)
|
I've reviewed the full diff against the current code. The author has already addressed the No new issues found beyond what was flagged in my prior review. SummaryNo new issues found. The prior review's concern about The fix is mechanically sound: dynamically resolving Google's IPs eliminates the hardcoded-address flakiness, type conversions via |
tankyleo
force-pushed
the
2026-03-tor-connect-flakiness
branch
from
684fb30 to
cbe5c61
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4521 +/- ##
==========================================
+ Coverage 86.14% 86.19% +0.05%
==========================================
Files 160 160
Lines 108046 108410 +364
Branches 108046 108410 +364
==========================================
+ Hits 93080 93448 +368
+ Misses 12346 12326 -20
- Partials 2620 2636 +16
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:
|
tankyleo
force-pushed
the
2026-03-tor-connect-flakiness
branch
from
cbe5c61 to
20e943e
Compare
4 participants
Fixes #4519