fix(iroh): Try to fix flaky test_token_passthrough test #1419
Conversation
This seems to fail often on CI on the emulated platforms when the machines are busy. And the failures we see are timeouts. So very likely increasing this timeout should help. Does a few smaller fixes: - Only bind on localhost instead of exposing us to the world. Tests should not expose themselves to the world. - Make sure that the receiver dropping is caught in a panic.
| @@ -877,7 +877,7 @@ async fn test_token_passthrough() -> Result<()> { | |||
| let rt = test_runtime(); | |||
| let expected = b"hello".to_vec(); | |||
| let (db, hash) = create_test_db([("test", expected.clone())]); | |||
| let addr = "0.0.0.0:0".parse().unwrap(); | |||
| let addr = SocketAddr::from((Ipv4Addr::LOCALHOST, 0)); | |||
There was a problem hiding this comment.
this is just drive by boyscouting, right?
There was a problem hiding this comment.
not sure what you mean, but this is not crucial to the fix as the description says. it is an orthogonal fix because I think no test should ever open public ports
There was a problem hiding this comment.
But this is no change, no? It's just a nicer way to write the same thing without parse and unwrap.
There was a problem hiding this comment.
It's that as well. but I also is a change. before it listened on 0.0.0.0, now it listens on 127.0.0.1.
| @@ -894,7 +894,7 @@ async fn test_token_passthrough() -> Result<()> { | |||
| if let iroh_bytes::provider::Event::GetRequestReceived { token: tok, .. } = | |||
| bp_msg | |||
| { | |||
| events_sender.send(tok).ok(); | |||
| events_sender.send(tok).expect("receiver dropped"); | |||
There was a problem hiding this comment.
Why does this fix the test?
There was a problem hiding this comment.
it doesn't. as per description it ensures we get a panic if this ever is an issue in one of the flakes. this is more belt-and-braces as i expect the timeout to work, but with flaky tests you can't be explicit enough.
Description
This seems to fail often on CI on the emulated platforms when the
machines are busy. And the failures we see are timeouts. So very
likely increasing this timeout should help.
Does a few smaller fixes:
Only bind on localhost instead of exposing us to the world. Tests
should not expose themselves to the world.
Make sure that the receiver dropping is caught in a panic.
Notes & open questions
Change checklist
Documentation updates if relevant.