Skip to content

Commit

Permalink
tests(iroh): Add some context to test errors (#3066)
Browse files Browse the repository at this point in the history
## Description

The flaky tests are being super weird and making no sense.  This is
clutching at straws for any hint.

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

/cc @ramfox 

Two slightly odd things I spotted while looking at this:

- The use of 240.0.0.1 as "bad" IP address returned in discovery. Maybe
one from the test nets in RFC 5737 would be better?
- The endpoints accept a connection and immediately drop it. This could
potentially send the CONNECTION_CLOSE before the ACK from the connection
resulting in the connect call returning an error (BUT THAT WOULD BE A
NORMAL READABLE TEST FAILURE). This probably doesn't happen because it's
all local and very fast and Quinn has already ACKed the connection
before the code manages to drop it. Still, essentially racy code.

Fine, I added a fix for the 2nd.  But it's not the issue.

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.
  • Loading branch information
flub authored Jan 9, 2025
1 parent 423a986 commit 1ae820d
Showing 1 changed file with 10 additions and 3 deletions.
13 changes: 10 additions & 3 deletions iroh/src/discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ mod tests {
time::SystemTime,
};

use anyhow::Context;
use iroh_base::SecretKey;
use rand::Rng;
use tokio_util::task::AbortOnDropHandle;
Expand Down Expand Up @@ -604,8 +605,11 @@ mod tests {
};
let ep1_addr = NodeAddr::new(ep1.node_id());
// wait for out address to be updated and thus published at least once
ep1.node_addr().await?;
let _conn = ep2.connect(ep1_addr, TEST_ALPN).await?;
ep1.node_addr().await.context("waiting for NodeAddr")?;
let _conn = ep2
.connect(ep1_addr, TEST_ALPN)
.await
.context("connecting")?;
Ok(())
}

Expand Down Expand Up @@ -706,10 +710,13 @@ mod tests {
let handle = tokio::spawn({
let ep = ep.clone();
async move {
// Keep connections alive until the task is dropped.
let mut connections = Vec::new();
// we skip accept() errors, they can be caused by retransmits
while let Some(connecting) = ep.accept().await.and_then(|inc| inc.accept().ok()) {
let _conn = connecting.await?;
// Just accept incoming connections, but don't do anything with them.
let conn = connecting.await?;
connections.push(conn);
}

anyhow::Ok(())
Expand Down

0 comments on commit 1ae820d

Please sign in to comment.