Make testing more reliable and realistic #87
Conversation
- Use `OnceLock` for `AUXILIARY_EVENT_TX` because it can't be set more than once in real air usage. This also has the benefit of removing the `static mut` rustc yells at us about. - Move LSP client tests to integration tests. This ensures they run sequentially and in their own process. This more accurately represents real usage and avoids writing to `AUXILIARY_EVENT_TX` multiple times (or overwriting it mid test). - Add `TESTING` global to turn off logs during testing. Emitting log notifications to the Client during testing is problematic because we use `recv_response()` to track request/response lifecycle and log requests end up getting in the way. Ideally I think we'd find a way to filter out the logs and keep "recv()"-ing instead of using a global like this to avoid them entirely.
| lsp::start_lsp(tokio::io::stdin(), tokio::io::stdout()).await; | ||
| lsp::start_server(tokio::io::stdin(), tokio::io::stdout()).await; |
There was a problem hiding this comment.
Happy to rename this back but i got a little confused seeing TestClient accept a lambda function that called start_lsp(). I thought start_lsp() did something client related when I saw that.
| unsafe { | ||
| #[allow(static_mut_refs)] | ||
| if let Some(val) = AUXILIARY_EVENT_TX.get_mut() { | ||
| // Reset channel if already set. Happens e.g. on reconnection after a refresh. | ||
| *val = auxiliary_event_tx; | ||
| } else { | ||
| // Set channel for the first time | ||
| AUXILIARY_EVENT_TX.set(auxiliary_event_tx).unwrap(); | ||
| } | ||
| } | ||
| AUXILIARY_EVENT_TX | ||
| .set(auxiliary_event_tx) | ||
| .expect("Auxiliary event channel can't be set more than once."); |
There was a problem hiding this comment.
I had the very important realization that unlike in the current ark setup, we won't ever go through here twice in air. There is no "reconnect" lifecycle to manage, our whole process is just going to get dropped and reestablished, so we don't have to worry about this anymore.
This was being triggered in our tests though - even if we ran the tests sequentially we'd come through here multiple times as we "start up" a new LSP within the same process. I've fixed that by running the LSP tests as integration tests with 1 client per process, truly ensuring we never run through here again.
| unsafe { | ||
| #[allow(static_mut_refs)] | ||
| if let Some(val) = AUXILIARY_EVENT_TX.get_mut() { | ||
| // Reset channel if already set. Happens e.g. on reconnection after a refresh. | ||
| *val = auxiliary_event_tx; | ||
| } else { | ||
| // Set channel for the first time | ||
| AUXILIARY_EVENT_TX.set(auxiliary_event_tx).unwrap(); | ||
| } | ||
| } | ||
| AUXILIARY_EVENT_TX | ||
| .set(auxiliary_event_tx) | ||
| .expect("Auxiliary event channel can't be set more than once."); |
There was a problem hiding this comment.
Notably you only see the expect() error when running with -- --nocapture.
This is because if the auxiliary or main loop threads panic, then we don't propagate the panic up to the main process at all. Tokio will return a JoinError if we actually await the JoinSet of the aux and main loops, we just need to do so in start_server() while we also do server.serve(service).await;. I couldn't easily figure that out here so I'm saving that for another PR.
But note that with this PR cargo test -- --nocapture no longer shows any panics, but did reliably before.
crates/lsp/tests/fixtures/mod.rs
Outdated
There was a problem hiding this comment.
Using the "old" style mod.rs here in fixtures/ is the recommended way to have integration test helpers
https://doc.rust-lang.org/book/ch11-03-test-organization.html#submodules-in-integration-tests
Nicely isolates the global
There was a problem hiding this comment.
We've decided to keep tests as unit tests because that is much more ergonomic for developing them.
To solve the global state issues we're going to:
- Move the thread spawner to local state
- Move the transmission channel for log messages to local state in the
tracingcrate configuration. This will be for non test builds. For test builds we'll just log to stderr.
This way we keep the possibility of writing tests as part of development files while still allowing tests to run in parallel. And we keep logging as free functions/macros that don't require passing state around.
Closes #79, alternative to #80 until we can fully move to
log::as our LSP log handler.Use
OnceLockforAUXILIARY_EVENT_TXbecause it can't be set more than once in real air usage. This also has the benefit of removing thestatic mutrustc yells at us about.Move LSP client unit tests to integration tests. This ensures they run sequentially and in their own process. This more accurately represents real usage and avoids writing to
AUXILIARY_EVENT_TXmultiple times (or overwriting it mid test) at the cost of more expensive test setup, but I'm ok with that tradeoff because of how closely this mimics real usage.Add
TESTINGglobal to turn off logs during integration testing. Emitting log notifications to the Client during testing is problematic because we userecv_response()to track request/response lifecycle and log requests end up getting in the way. Ideally I think we'd find a way to filter out the logs and keep "recv()"-ing instead of using a global like this to avoid them entirely. Remember we can't usecfg(test)during integration tests (only unit tests), so this was my next best alternative.