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 fix all in workspace test time out on 5 seconds #59780

Closed
FMorschel opened this issue Dec 20, 2024 · 2 comments
Closed

LSP fix all in workspace test time out on 5 seconds #59780

FMorschel opened this issue Dec 20, 2024 · 2 comments
Assignees
Labels
analyzer-dartfix Issues with the dartfix package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@FMorschel
Copy link
Contributor

FMorschel commented Dec 20, 2024

While working on https://dart-review.googlesource.com/c/sdk/+/401865 I added a test to pkg\analysis_server\test\lsp\commands\fix_all_in_workspace_test.dart as suggested by @DanTup at https://dart-review.googlesource.com/c/sdk/+/401840 for better debugging than the dart fix tests since they should work the same way.

While testing I added @soloTest to the newly added test AbstractLspAnalysisServerTest.test_partFile_issue59572. And gave a try to add argument: ['./lib/part.dart'] to the Command constructor on it. That can reliably reproduce the failure for the 5-second timeout on my machine. The stack trace:

TimeoutException after 0:00:05.000000: Future not completed

  dart:async/future_impl.dart 1025:22                               Future.timeout.<fn>
future_impl.dart:1025
  ===== asynchronous gap ===========================
  test\lsp\server_abstract.dart 1139:29                             LspAnalysisServerTestMixin.expectRequest
server_abstract.dart:1139
  ===== asynchronous gap ===========================
  test\lsp\server_abstract.dart 1181:27                             LspAnalysisServerTestMixin.handleExpectedRequest
server_abstract.dart:1181
  ===== asynchronous gap ===========================
  test\lsp\server_abstract.dart 155:27                              AbstractLspAnalysisServerTest.executeForEdits
server_abstract.dart:155
  ===== asynchronous gap ===========================
  test\lsp\server_abstract.dart 369:20                              AbstractLspAnalysisServerTest.verifyCommandEdits
server_abstract.dart:369
  ===== asynchronous gap ===========================
  test\lsp\commands\fix_all_in_workspace_test.dart 169:5            AbstractFixAllInWorkspaceTest.test_partFile_issue59572
fix_all_in_workspace_test.dart:169
  ===== asynchronous gap ===========================
  package:test_reflective_loader/test_reflective_loader.dart 261:5  _runTest
test_reflective_loader.dart:261
  ===== asynchronous gap ===========================
  package:test_api/src/backend/declarer.dart 229:9                  Declarer.test.<fn>.<fn>
declarer.dart:229
  ===== asynchronous gap ===========================
  package:test_api/src/backend/declarer.dart 227:7                  Declarer.test.<fn>
declarer.dart:227
  ===== asynchronous gap ===========================
  package:test_api/src/backend/invoker.dart 258:9                   Invoker._waitForOutstandingCallbacks.<fn>
invoker.dart:258

The place where it is failing pkg\analysis_server\test\lsp\server_abstract.dart, line 1139, inside LspAnalysisServerTestMixin.expectRequest. The expectRequest contains a timeout that defaults to that value but the method that calls it (LspAnalysisServerTestMixin.handleExpectedRequest) also has a timeout parameter that is not passing through (in this case not even in use).

After a talk with @DanTup on Discord about this, we think it may be safe to increase that value. We'll also take a look at the timings here and why adding these (@soloTest and parameters) slows down the execution. Creating this issue to track this work.


As a note, if we increase it a lot (tested with 5 min) the tests themselves fail at 30 seconds. The take from @DanTup was that this was intended to give some indication as to where would it be taking a long time to answer.

@FMorschel FMorschel added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Dec 20, 2024
@DanTup DanTup self-assigned this Dec 20, 2024
@srawlins srawlins added analyzer-dartfix Issues with the dartfix package type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Dec 27, 2024
@keertip keertip added the P2 A bug or feature request we're likely to work on label Dec 30, 2024
@DanTup
Copy link
Collaborator

DanTup commented Jan 7, 2025

The problem here seems to be that the request is failing, but we're not handling that correctly:

==> {"id":1,"jsonrpc":"2.0","method":"workspace/executeCommand","params":{"arguments":["./lib/part.dart"],"command":"dart.edit.fixAllInWorkspace.preview"}}
<== {"id":1,"jsonrpc":"2.0","error":{"code":-32006,"message":"dart.edit.fixAllInWorkspace.preview requires a single Map argument"}}

Probably what's written in this comment is not accurate, or makes assumptions that aren't true here. I'll take a look.

// Because we don't await this future until "later", if it throws the
// error is treated as unhandled and will fail the test. Attaching an
// error handler prevents that, though since the Future completed with
// an error it will still be handled as such when the future is later
// awaited.
// TODO(srawlins): Fix this static error.
// ignore: body_might_complete_normally_catch_error
outboundRequest.catchError((_) {});

@DanTup
Copy link
Collaborator

DanTup commented Jan 7, 2025

With https://dart-review.googlesource.com/c/sdk/+/403460, the "Future not completed" error will be better, like:

Did not receive the expected workspace/applyEdit request from the server in the timeout period

However in the case where the failure is caused by a failing outbound request (as it is with the steps above), the error from that request will be thrown instead:

00:05 +0 -1: ApplyAllFixesInWorkspace | test_partFile_issue59572 [E]

  {
      "code": -32006,
      "message": "dart.edit.fixAllInWorkspace requires a single Map argument"
  }

  test\lsp\server_abstract.dart 1211:7  LspAnalysisServerTestMixin.handleExpectedRequest.<fn>
  dart:async/zone.dart 1538:47          _rootRunUnary
  dart:async/zone.dart 1429:19          _CustomZone.runUnary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-dartfix Issues with the dartfix package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants