Closed Strum355 closed 1 year ago
Thanks for the PR.
I can definitely see how this can make things a little more immediate as far as testing is concerned. I'm not sure whether it's a good idea to provide a method like this in general although I don't really have a strong opinion for it or against it.
I suppose one possibility could be to only enable/compile ::inner
in test mode using #[cfg(test)]
. But I don't know of any other crates that use that pattern for test-specific methods like this.
Any thoughts @ebkalderon?
Unfortunately I dont think thats currently supported by rust :pensive: https://github.com/rust-lang/cargo/issues/8379
Thanks for looping me in! I don't have a particularly strong stance for or against this addition either. Personally, when I write servers I try to make the underlying state machine easily testable on its own without needing access to tower-lsp
functionality at all. Still, I understand that this may be impossible or otherwise satisfactory to everyone's needs.
I wonder why wrapping Mock
in the LspService
and then retrieving it again is necessary compared to calling the <Mock as LanguageServer>
methods directly in tests? I presume this may be because most servers need a Client
handle in order to initialize themselves, and there's currently no way to create one (for good reason).
Perhaps we could approach this shortcoming another way by instead shipping a test harness of some kind with tower-lsp
or as a separate crate? Something like:
use tower_lsp::test;
#[tokio::test(flavor = "current_thread")]
async fn test_server() {
// This sets up a `Client` and `ClientSocket` pair that allow for
// manual testing of the server.
//
// The returned tuple is of type `(Arc<Mock>, ClientSocket)`.
let (server, mut socket) = test::server(|client| Mock { client });
// Call `LanguageServer` methods directly, examine internal state, etc.
assert!(server.initialize(...).await.is_ok());
// Let's assume one server method must make a client request.
let s = server.clone();
let handle = tokio::spawn(async move { s.initialized(...).await });
// Reply to the request as if we were the client.
let request = socket.next().await.unwrap();
socket.send(...).await.unwrap();
// We can still inspect `server`'s state and call other methods
///at any time during this.
let response = handle.await.unwrap();
assert!(response.is_ok());
}
Any thoughts on this idea? It could be used either as an alternative to or in conjunction with this PR's approach.
I wonder why wrapping
Mock
in theLspService
and then retrieving it again is necessary compared to calling the<Mock as LanguageServer>
methods directly in tests? I presume this may be because most servers need aClient
handle in order to initialize themselves, and there's currently no way to create one (for good reason).
That raises a good point I hadn't made the connection with before, seeing as I'd have direct access to my LanguageServer
impl, this PR mightn't be necessary for me (as of currently). I was mostly going off https://github.com/wasm-lsp/wasm-lsp-server/blob/a0e29c9aa32a240b96f48ee072fa087e2212bfcb/crates/server/tests/all/lsp.rs#L41-L52 so far, but theres a lot of extra here that I likely don't need
For now, this is probably okay to expose. I don't see any harm in adding it, though I'd like to improve the testing situation in a more holistic way down the road (see #335 and #229). Thanks for the contribution, @Strum355!
The utility for this is mostly in testing, wherein I want to be able to test inner state of the server contained within the
LspService
. This avoids the need to use thecustom_method
feature as a workaround, as the constraints on types returned and ceremony around sending/receiving jsonrpc messages is too much hassle imo.Have tested this locally with my own
LanguageServer
impl and all seemed :ok: so far :slightly_smiling_face: