implement getting a reference to the inner server from LspService and Router

[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 the custom_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 🆗 so far 🙂

[silvanshade]

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?

[Strum355]

Unfortunately I dont think thats currently supported by rust 😔 rust-lang/cargo#8379

[ebkalderon]

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.

[Strum355]

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).

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

[ebkalderon]

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!