test(iroh-blobs): comment out ignored test (that is not a flaky test) (…

…#2559)

## Description

There's this test that was introduced in [the `local_pool`
PR](#2517).
It was ignored via `#[ignore = "todo"]`. Notably, it's *not flaky*, it
always fails.
Our flaky tests are run with `cargo nextest run --run-ignored all
[...]`. We can't be more specific with the `ignore`d tests. The only
options are `default`, `ignored-only` and `all`.
This kind of test is really hard to write. IIUC, `#[should_panic]` can
only test for the panic happening in the thread that the test is
initiated in, it doesn't detect panics that are thrown in threads
spawned from the test. I assume this is the reason writing this test was
abandoned.

Keeping this test with the `#[ignore = "todo"]` on it means we're always
running it in our flaky test suite, which is confusing. We thought this
test was flaky, but it's not. IMO it's better to comment it out/remove
it than to pollute our flaky test results.

## Breaking Changes

None

## Notes & open questions

In this PR I'm commenting this test. Should we remove it instead?
Or do people have ideas on how to make this test work?
Do we have an idea what we're *expecting* of our `LocalPool`
implementation? Should a panic on one of the threads cause a panic in
the `finish()` function?

## Change checklist

- [X] Self-review.
- ~~[ ] 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.~~
- ~~[ ] Tests if relevant.~~
- [X] All breaking changes documented.

iroh-blobs/src/util/local_pool.rs

@@ -634,20 +634,20 @@ mod tests {
         assert_eq!(c2.load(std::sync::atomic::Ordering::SeqCst), 0);
     }
 
-    #[tokio::test]
-    #[should_panic]
-    #[ignore = "todo"]
-    async fn test_panic() {
-        let _ = tracing_subscriber::fmt::try_init();
-        let pool = LocalPool::new(Config {
-            threads: 2,
-            ..Config::default()
-        });
-        pool.spawn_detached(|| async {
-            panic!("test panic");
-        });
-        // we can't use shutdown here, because we need to allow time for the
-        // panic to happen.
-        pool.finish().await;
-    }
+    // #[tokio::test]
+    // #[should_panic]
+    // #[ignore = "todo"]
+    // async fn test_panic() {
+    //     let _ = tracing_subscriber::fmt::try_init();
+    //     let pool = LocalPool::new(Config {
+    //         threads: 2,
+    //         ..Config::default()
+    //     });
+    //     pool.spawn_detached(|| async {
+    //         panic!("test panic");
+    //     });
+    //     // we can't use shutdown here, because we need to allow time for the
+    //     // panic to happen.
+    //     pool.finish().await;
+    // }
 }

iroh/tests/client.rs

@@ -8,7 +8,6 @@ use iroh_gossip::{
 };
 use iroh_net::{key::SecretKey, NodeAddr};
 use testresult::TestResult;
-use tokio::task::JoinHandle;
 
 /// Spawn an iroh node in a separate thread and tokio runtime, and return
 /// the address and client.
@@ -35,11 +34,11 @@ fn spawn_node() -> (NodeAddr, Iroh) {
 }
 
 /// Await `n` messages from a stream of gossip events.
-fn await_messages(
+async fn await_messages(
     mut stream: impl Stream<Item = anyhow::Result<Event>> + Unpin + Send + Sync + 'static,
     n: usize,
-) -> JoinHandle<Vec<Bytes>> {
-    tokio::spawn(async move {
+) -> TestResult<Vec<Bytes>> {
+    let handle = tokio::spawn(async move {
         let mut res = Vec::new();
         #[allow(clippy::single_match)]
         while let Some(msg) = stream.next().await {
@@ -54,10 +53,13 @@ fn await_messages(
             }
         }
         res
-    })
+    });
+
+    Ok(tokio::time::timeout(std::time::Duration::from_secs(60), handle).await??)
 }
 
 #[tokio::test]
+#[ignore = "flaky"]
 async fn gossip_smoke() -> TestResult {
     let _ = tracing_subscriber::fmt::try_init();
     let (addr1, node1) = spawn_node();