Use a global test client for all unit tests #130
Conversation
crates/lsp/src/handlers.rs
Outdated
| client | ||
| .register_capability(regs) | ||
| .instrument(span.exit()) | ||
| .await?; | ||
| if !regs.is_empty() { | ||
| client | ||
| .register_capability(regs) | ||
| .instrument(span.exit()) | ||
| .await?; | ||
| } | ||
|
|
There was a problem hiding this comment.
Now that we send initialized(), this handle_initialized() is fired and the server was sending this request to the client that the client test code was not expecting. Since the test client doesn't actually have the capability required here, I guarded it like this to avoid sending the request entirely.
(Note this was quite hard to debug, as it seems like the codec code can't figure out how to deserialize this message from the server at all)
| if is_test_client(client_info) { | ||
| // During parallel testing, `set_global_default()` gets called multiple times | ||
| // per process. That causes it to error, but we ignore this. | ||
| tracing::subscriber::set_global_default(subscriber).ok(); | ||
| } else { | ||
| tracing::subscriber::set_global_default(subscriber) | ||
| .expect("Should be able to set the global subscriber exactly once."); | ||
| } | ||
| tracing::subscriber::set_global_default(subscriber) | ||
| .expect("Should be able to set the global subscriber exactly once."); |
| self.recv_response().await; | ||
|
|
||
| let response = self.recv_response().await; | ||
| assert_eq!(&id, response.id()); |
There was a problem hiding this comment.
I switched id() to return jsonrpc::Id because I found it was quite useful to add in some extra asserts to ensure that a call to recv_response() actually returns the response you were hoping for
| @@ -54,5 +54,4 @@ jobs: | |||
| env: | |||
| AIR_LOG_LEVEL: trace | |||
| # `--nocapture` to see our own `tracing` logs | |||
| # `--test-threads 1` to ensure `tracing` logs aren't interleaved | |||
| run: cargo test -- --nocapture --test-threads 1 | |||
| run: cargo test -- --nocapture | |||
There was a problem hiding this comment.
it's nice to make CI and local testing more consistent.
| let mut client = init_test_client().await; | ||
| #[test] | ||
| fn test_format_minimal_diff() { | ||
| with_client(|client| async { |
There was a problem hiding this comment.
A bit unfortunate to increase nesting but the explicitness is nice.
| /// a `&mut TestClient` to the unit test. | ||
| static TEST_CLIENT: OnceCell<Mutex<TestClient>> = OnceCell::const_new(); | ||
|
|
||
| /// `#[tokio::test]` creates a new runtime for each individual test. That doesn't work |
There was a problem hiding this comment.
It will be nice to remove all these tokio complications.
And rename a few files to reflect new location
Otherwise we end up sending our test client a request here that it isn't expecting at this time, since it didn't say it supported these capabilities
This is definitely required for lsp-server, otherwise it can't proceed, but is also the right thing to do as a client in general
With new support for sending `initialized()` too
DavisVaughan
force-pushed
the
feature/test-global-client
branch
from
30d6178 to
64feef0
Compare
A feature plucked from #126, and a step towards allowing dynamic setting of
logLevelanddependencyLogLevelsin the logging code, which are going to require some global state.Here are notes from that other PR about this:
The async code makes this quite tricky.
[#tokio::test]creates 1 runtime per test, which completely screws you up if you have a global object that needs to be safely synchronized across tests, because you can't really synchronize across runtimes. The temporary solution used here is to use a global test runtime as well, and just callblock_on()for each test. See tokio-rs/tokio#2374 for more.The better solution in the future will be to remove the async code in favor of lsp-server, which will simplify everything down to this: https://github.com/posit-dev/air/blob/feature/ruff/crates/server/src/test/client.rs
I've also added in the ability for our test client to send the
initialized()notification. This will end up being required when we switch to lsp-server, as lsp-server waits forinitialized()before letting us proceed on the server side.