Why are the errors in the test being swallowed?

help
1

I've written a test helper to help create a new database from a template and
clean it up. Using a closure so that we know when should the database be
'live'. Using drop is the first thing that came to mind, but because the
database library, tokio-postgres, async drop can't be used. Looking for
workarounds I found the following post:

I did replaced the generic parameter with anyhow::Result so that I can use ?
inside the closure. The solution works, in that the databases are
cleaned. However when the early return of ? is taken the test passes. How do I
make it so erroring out early fails?

Below are the relevant excerpts, the full source is at https://git.sr.ht/~puercopop/blog/tree/default/item/pgtempdb/

// lib.rs
use pgpool::{Manager, Pool};
use std::panic::AssertUnwindSafe;
use std::{str::FromStr as _, sync::LazyLock};
use tokio_postgres::config::Config;
use uuid::Uuid;

// TODO:
// Based on
// https://users.rust-lang.org/t/following-discussions-related-to-async-drop/134532/2
//
// Rewrite this around with_testdb. We can drop the permanent connection for
// now. It will simplify ownership even if we need to create a new connection to
// the cluster every time we want to create or drop a new template database.

pub async fn with_testdb(
    f: impl AsyncFnOnce(&mut TestDatabase) -> anyhow::Result<()>,
) -> anyhow::Result<()> {
    let dealer = DatabaseDealer::new();
    let mut testdb = DatabaseDealer::deal(dealer).await.expect("unable to create a TestDatabase");
    let ret = futures::FutureExt::catch_unwind(AssertUnwindSafe(f(&mut testdb))).await;
    drop_db(testdb.dbname).await.expect("unable to destroy the TestDatabase");
    if let Err(panic) = ret {
        std::panic::resume_unwind(panic)
    }
    Ok(())
}

// lib_test.rs
// cargo test -p pgtempdb
use pgtempdb::with_testdb;

// FIXME: This test should raise an error due to the invalid SQL query. However
// the error is silently swallowed. If there is an error, the test should
// failed.
#[tokio::test]
async fn test_doesnt_swallow_errors() {
    let _ = with_testdb(async |testdb| {
        let conf = testdb.config.clone();
        let (client, conn) = conf.connect(tokio_postgres::NoTls).await?;
        tokio::spawn(async move {
            if let Err(e) = conn.await {
                eprint!("connection error: {}", e);
            }
        });
        let _row = client.query("SEL 1", &[]).await?;
        assert!(false);
        Ok(())
    })
    .await;
}
2

An Err isn't a panic.

If let _row = client.query("SEL 1", &[]).await?; fails, then it returns Err from the async closure. This Result is then inside the Ok variant of ret in with_testdb — ret has the type Result<Result<(), anyhow::Error>, Box<dyn Any + Send>>. You have two nested Results (one for panics and one for explicitly returned errors) and you need to check both of them.

Furthermore, if with_testdb returned an Err (which it never does, currently), test_doesnt_swallow_errors would be discarding it using let _ = ....

I would suggest that, in this kind of code, you should strictly avoid using if let in favor of match, and avoid _ patterns except when they are actually necessary. Writing out the additional match arms and patterns will help you ensure you are not accidentally discarding a meaningful Result.

    match ret {
        Err(panic) => std::panic::resume_unwind(panic),
        Err(Err(error)) => panic!("test function returned an error: {error}"),
        Ok(Ok(())) => {},
    }

And in test_doesnt_swallow_errors, remove the let _ = and return the error (the test harness will fail the test if it returns Err):

#[tokio::test]
async fn test_doesnt_swallow_errors() -> anyhow::Result<()> {
    with_testdb(async |testdb| {
        ...
    })
}
1 Like
3

Thanks for explanation. I had to change theErr(Err(_)) arm to to Ok(Err(_)). Which makes sense because if the first error is encountered then the code for the inner error doesn't run.

    match ret {
        Err(panic) => std::panic::resume_unwind(panic),
        Ok(Err(err)) => panic!("test failure: {err}"),
        Ok(Ok(())) => {}
    };
1 Like