chore(pglt_lsp): add lifecycle test #214
Conversation
crates/pglt_lsp/tests/server.rs
Outdated
| use tower_lsp::lsp_types::DidCloseTextDocumentParams; | ||
| use tower_lsp::lsp_types::DidOpenTextDocumentParams; | ||
| use tower_lsp::lsp_types::InitializeResult; | ||
| use tower_lsp::lsp_types::InitializedParams; | ||
| use tower_lsp::lsp_types::PublishDiagnosticsParams; | ||
| use tower_lsp::lsp_types::TextDocumentContentChangeEvent; | ||
| use tower_lsp::lsp_types::TextDocumentIdentifier; | ||
| use tower_lsp::lsp_types::TextDocumentItem; | ||
| use tower_lsp::lsp_types::VersionedTextDocumentIdentifier; |
There was a problem hiding this comment.
the use statements look a bit odd? something wrong with your nvim config?
There was a problem hiding this comment.
copy-pasta 😂 fixed it
| macro_rules! url { | ||
| ($path:literal) => { | ||
| if cfg!(windows) { | ||
| lsp::Url::parse(concat!("file:///z%3A/workspace/", $path)).unwrap() | ||
| } else { | ||
| lsp::Url::parse(concat!("file:///workspace/", $path)).unwrap() | ||
| } | ||
| }; | ||
| } |
| .context("call() returned an error") | ||
| .and_then(|res| { | ||
| if let Some(res) = res { | ||
| bail!("shutdown returned {:?}", res) |
There was a problem hiding this comment.
Are we shutting down here?
| .await | ||
| .map_err(Error::msg) | ||
| .context("call() returned an error") | ||
| .and_then(|res| { |
There was a problem hiding this comment.
Hmm, would it be more flexible to let test callers handle the response themselves?
There was a problem hiding this comment.
yeah but every call will want to deserialise it I guess. but lets reiterate when we wrote a few tests 👍
crates/pglt_lsp/tests/server.rs
Outdated
| self.notify( | ||
| "textDocument/didOpen", | ||
| DidOpenTextDocumentParams { | ||
| text_document: TextDocumentItem { | ||
| uri: url!("document.js"), | ||
| language_id: String::from("javascript"), | ||
| version: 0, | ||
| text: text.to_string(), | ||
| }, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Just a feeling: I think we need to be careful about what we abstract away and what we force the tests to do themselves.
For methods that are always called like shutdown, initialize etc. I think it makes sense to provide convenience methods.
But I'm not super about e.g. open_document – a bug might slip in because our testing logic contains too much "business logic".
Also, this is javascript 🤣
But yeah, WDYT? I think it's a fine line
There was a problem hiding this comment.
yeah agreed. I removed some of the methods and we reiterate when we have added a few tests
adds a basic lifecycle test to the lsp server. will expand on it later. maybe we can even have a "recording" of the users actions via logs, parse these recordings and replay them.