Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LSP] Find usages of LSP returns null #921

Closed
sswaroopgupta opened this issue Jan 18, 2018 · 9 comments
Closed

[LSP] Find usages of LSP returns null #921

sswaroopgupta opened this issue Jan 18, 2018 · 9 comments
Labels

Comments

@sswaroopgupta
Copy link
Contributor

Expected behavior
Should give the usage statistics

Actual behavior

17:39:49.119 --> request #0: initialize: {"processId":14992,"rootPath":"\\gauge-lsp-tests\\data\\find-usages","rootUri":"gauge-lsp-tests/data/find-usages","capabilities":{"workspace":{"applyEdit":true,"didChangeConfiguration":{"dynamicRegistration":true},"didChangeWatchedFiles":{"dynamicRegistration":true},"symbol":{"dynamicRegistration":true},"executeCommand":{"dynamicRegistration":true}},"textDocument":{"synchronization":{"dynamicRegistration":true,"willSave":true,"willSaveWaitUntil":true,"didSave":true},"completion":{"dynamicRegistration":true,"completionItem":{"snippetSupport":true,"commitCharactersSupport":true}},"hover":{"dynamicRegistration":true},"signatureHelp":{"dynamicRegistration":true},"definition":{"dynamicRegistration":true},"references":{"dynamicRegistration":true},"documentHighlight":{"dynamicRegistration":true},"documentSymbol":{"dynamicRegistration":true},"codeAction":{"dynamicRegistration":true},"codeLens":{"dynamicRegistration":true},"formatting":{"dynamicRegistration":true},"rangeFormatting":{"dynamicRegistration":true},"onTypeFormatting":{"dynamicRegistration":true},"rename":{"dynamicRegistration":true},"documentLink":{"dynamicRegistration":true}}},"trace":"off","experimental":{}}
17:39:49.119 <-- result #0: initialize: {"capabilities":{"textDocumentSync":1,"completionProvider":{"resolveProvider":true,"triggerCharacters":["*","* ","\"","\u003c",":",","]},"definitionProvider":true,"documentSymbolProvider":true,"workspaceSymbolProvider":true,"codeActionProvider":true,"codeLensProvider":{},"documentFormattingProvider":true,"renameProvider":true}}
17:39:49.121 --> notif: initialized: {}
17:39:49.126 --> request #1: textDocument/codeLens: {"textDocument":{"uri":"gauge-lsp-tests/data/find-usages/tests/step_implementation.js"}}
17:39:49.127 <-- result #1: textDocument/codeLens: null

Steps to replicate

  • Run LSP Tests

Version

Gauge version: 0.9.8.nightly-2018-01-16
Commit Hash: bd76268

Plugins
-------
js (2.1.1.nightly-2018-01-16)
@sswaroopgupta
Copy link
Contributor Author

Note
This test passes with Released Gauge version - gauge-0.9.7-windows.x86_64.exe and fails on 0.9.8.nightly-2018-01-16

@sswaroopgupta
Copy link
Contributor Author

Currently it is passing for js. However it fails for the env=ruby-wd.

Note: The LSP test opens the implementation file directly.
Steps to replicate

  • gauge init ruby
  • In VSCode, open project with no files open by default. Open a .rb file
    No find usage statistics are displayed.

@sswaroopgupta
Copy link
Contributor Author

Gauge should honour LSP protocol. I should be able to fire any request after initialized. However find-usages is not working as expected after initialized.

initialize
initialized
textDocument/publishDiagnostics
textDocument/codeLens

is giving error -

        Failed Step: ensure code lens has details "specs/codelens/findUsages"
        Specification: ...\gauge-lsp-tests\specs\codelens\findUsages.spec:8
        Error Message: Error: unable to verify code lens details TypeError: Cannot read property '0' of null
        Stacktrace: 
        Error: unable to verify code lens details TypeError: Cannot read property '0' of null
            at Object.<anonymous> (...\gauge-lsp-tests\tests\execution.js:18:15)

Steps to replicate

  • LSP Tests - Should find usages

@riju91 riju91 self-assigned this Jan 29, 2018
@riju91 riju91 added in progress and removed ready labels Jan 29, 2018
@riju91
Copy link
Contributor

riju91 commented Jan 29, 2018

From LSP Point of view, initialize is a request. Client has to wait for the response according to LSP and then send the notification initialized after getting the response.

Currently
We are initializing runner when we are handling initialized notification.

The Test Failure
The LSP Test Client is waiting for the response from the initialize request, firing the initialized notification and then further requests and notifications are sent. Some of the subsequent requests fail because the runner initialization is taking some time during which subsequent requests are getting processed.

Probable Solution
we can initialize the runner in initialize request since the protocol ensures completion of initialization before handling any other requests.

If the client doesn't support dynamic registration, we can still support runner capabilities if we initialize the runner in LSP initialize request itself.

@riju91
Copy link
Contributor

riju91 commented Jan 30, 2018

Plan of Action

We plan to initialize gauge in the initialize request from the LSP client. These will be the list of features which gauge can support without initializing the runner.

After this when gauge receives initialized notification, we will initialize the runner and then add the list of runner capable features dynamically.

This will ensure that client is not sending any request which gauge is not prepared to handle because only after runner initialization is done, we will add the feature dynamically and only when runner initialization and feature addition is done, client will get a notification about the new features like codeLens (which would initially be false and after runner is up, it will be dynamically added).

This will also make sure that if runner fails to start, it won't affect the features that gauge can handle by itself without invoking the runner.

@sswaroopgupta
Copy link
Contributor Author

sswaroopgupta commented Feb 2, 2018

Consider the following case
TextDocument/didOpen is one call which can be sent without waiting for client register capability. Here Gauge triggers a publishDiagnostics notification from the server itself.
This requires the language runner to be initialized.

Since the client is not the one sending the request, how should this be handled?

@nehashri
Copy link
Contributor

nehashri commented Feb 2, 2018

@sguptatw Not all diagnostics need to be published at the time of opening a project. Since runner takes time to come up, we can publish the parse errors as diagnostics once a file opened. Once runner is up we can always republish the diagnostics; this time with all errors, parse errors and validation errors. This way even if runner takes time to come up, the user will still be able to see all parse errors.
Also, another point to note is that, validation errors take to time to be shown even if runner is up. This happens because there is an additional time take for gauge to talk to runner and come back with the information.

@sriv
Copy link
Member

sriv commented Feb 9, 2018

Ruby LSP tests are passing on osx. Do we need windows support for ruby?

@sswaroopgupta
Copy link
Contributor Author

With the latest working as expected

@ghost ghost removed the ready label Mar 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

6 participants