Raise in executor in tests

[andyw8]

Motivation

We currently rescue StandardError in executor, which covers a wide range of things, including TypeError. That can result in test failure messages which are not very informative.

And while working on a YARP migration with @bitwise-aiden we discovered a flawed test which was being masked due to the rescue.

(I'll fix the linting if we agree to ahead with this).

Implementation

Be loud if an error occurs while running the tests.

Automated Tests

Updated

Manual Tests

n/a

[andyw8]

Wasn't sure if there was a more 'standard' way to name such a thing.

[paracycle]

I think it is better to make this just:

Suggested change

	raiseeif $RUBY_LSP_TEST
	raiseif $RUBY_LSP_TEST

so that the original source of the exception is kept, and it doesn't look like the exception was raised here. Ref: https://www.exceptionalcreatures.com/guides/advanced-rescue-and-raise#reraising-exceptions

[andyw8]

There is no " in the code above so this was returning nil.

[vinistock]

I think a better approach to surface this in executor tests would be to add a test helper that will execute a request and raise in tests if there's an error. Changing the actual runtime with a global variable is not really something we'd like to do.

How about something like this in executor_test

defrun_request(method:,params: {})result=@executor.execute({method: method,params: params})error=result.errorraiseerroriferrorresult.responseend

[github-actions]

Benchmark results in seconds (slowest at top)
textDocument/completion average: 0.263836 std_dev: 0.004946
textDocument/diagnostic average: 0.04009 std_dev: 0.01003
textDocument/definition average: 0.005214 std_dev: 0.003059
textDocument/selectionRange average: 0.004075 std_dev: 0.000493
textDocument/documentHighlight average: 0.002454 std_dev: 0.000125
textDocument/codeLens average: 0.002424 std_dev: 0.000124
textDocument/semanticTokens/full average: 0.002411 std_dev: 0.000347
textDocument/documentLink average: 0.002411 std_dev: 0.000238
textDocument/documentSymbol average: 0.002383 std_dev: 0.000163
textDocument/foldingRange average: 0.002226 std_dev: 0.000169
textDocument/semanticTokens/range average: 0.00158 std_dev: 0.000107
codeAction/resolve average: 0.001353 std_dev: 0.000121
textDocument/inlayHint average: 0.001349 std_dev: 6.8e-05
textDocument/hover average: 0.001321 std_dev: 7.0e-05
textDocument/onTypeFormatting average: 0.000813 std_dev: 6.9e-05
textDocument/formatting average: 0.000791 std_dev: 0.000168
textDocument/codeAction average: 0.000782 std_dev: 9.3e-05
================================================================================
Comparison with main branch:
textDocument/semanticTokens/full unchanged
textDocument/semanticTokens/range unchanged
textDocument/documentSymbol unchanged
textDocument/foldingRange unchanged
textDocument/formatting unchanged
textDocument/diagnostic unchanged
textDocument/documentLink unchanged
textDocument/inlayHint unchanged
textDocument/selectionRange unchanged
textDocument/documentHighlight unchanged
textDocument/hover unchanged
textDocument/codeAction unchanged
textDocument/onTypeFormatting unchanged
codeAction/resolve unchanged
textDocument/completion unchanged
textDocument/codeLens unchanged
textDocument/definition unchanged
================================================================================
Missing benchmarks:
RubyLsp::Requests::ShowSyntaxTree

[andyw8]

@vinistock yeah I like that approached, updated.

[vinistock]

Given that result only has response and error, and we already check for the error here, should we just return result.response and avoid having to do it in each call site?

[st0012]

Yeah I agree with this 👍

[andyw8]

Updated as per suggestion.

[andyw8]

Updated with @st0012's suggestion.