matrix-org / matrix-rust-sdk

Matrix Client-Server SDK for Rust
Apache License 2.0
1.26k stars 251 forks source link

Integration tests for redaction failing #1069

Open gnunicorn opened 2 years ago

gnunicorn commented 2 years ago

since 1a57170970d3f4fa354c40f2fd243abbf9315be6 the redaction integration tests fail - name_static almost every time on CI, the other has been observed outside of CI to fail, too:

test tests::redaction::test_redacting_name_static ... FAILED
test tests::redaction::test_redacting_name ... ok

failures:

---- tests::redaction::test_redacting_name stdout ----
thread 'tests::redaction::test_redacting_name' panicked at 'dispatch dropped without returning error', /home/ben/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.14.20/src/client/conn.rs:329:35
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    tests::redaction::test_redacting_name

test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.61s
jplatte commented 2 years ago

This seems like an issue other people are facing too with hyper. Are we sharing the hyper client between multiple async runtimes perhaps? That's one way to trigger the error according to issues. These two seem to contain the most info:

gnunicorn commented 2 years ago

multiple async runtimes perhaps?

does #[tokio::test] create a fresh runtime for every test? It would make sense, that it does. And then, yes. Our integration tests reuse existing clients stored in a dashmap static USERS: Lazy<Mutex<HashMap<String, (Client, TempDir)>>>.

jplatte commented 2 years ago

Yes, that is what #[tokio::test] does. Do we really need to reuse clients across tests, are they that expensive to create?

gnunicorn commented 2 years ago

I think the main problem here is e2ee, so they are created for the entire test suite with the proper sled store in tmp (which he hold the tmpdir for so that it doesn't get scraped on drop), the other that we register with the server if they don't exist before (locally) - which saves a few roundtrips, too. I suppose we could change that. Maybe it does make sense to provide some TestClient that we can just hammer in some info (like the homeserver) to prevent the unnecessary roundtrips?

jplatte commented 2 years ago

The other option would be to not use #[tokio::test], and share the runtime across tests as well.