[analysis_server] Create a wrapper to allow LSP helpers to be used over DTD

A minor refactor towards supporting CodeActions over DTD.

The (existing) `LspRequestHelpersMixin` is a collection of helper methods like `getHover()` that allow sharing strongly-typed access to LSP methods across different kinds of tests (LSP, LSP-over-Legacy, and in-process vs out-of-process).

This adds a class (`DtdHelper`) that allows those same helpers to be used to call LSP over DTD (it has to be a separate instance because these helpers already exist on the base test classes to communicate with the server directly without DTD).

As part of this, the mixin was split up to separate request, reverse-requests and notifications, since only outbound requests apply over DTD and it cannot provide an implementation for the others.

This change also adds `textDocument/hover` to the allow-list so that some of these tests can be unskipped (since in an upcoming change the allow-list will be removed entirely anyway).

Change-Id: Id7bf40020cb5397e4cb8bbdefeed71745f58c4cc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428520
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: DanTup

pkg/analysis_server/lib/src/services/dart_tooling_daemon/dtd_services.dart

@@ -43,13 +43,12 @@ class DtdServices {
   /// particular, those with well defined results that are not affected by
   /// differences in client capabilities).
   static final allowedLspMethods = <Method>{
-    // When removing this allowlist or adding simple methods like
-    // textDocument/hover, skipped tests in `SharedDtdTests` can be unskipped.
-    // TODO(dantup): Enable this but add a flag so we can opt-in to experimental
-    //  handlers being exposed over DTD while in dev.
+    // TODO(dantup): Remove this allow list so that all shared methods are
+    //  available over DTD.
     CustomMethods.experimentalEcho,
     CustomMethods.dartTextDocumentEditableArguments,
     CustomMethods.dartTextDocumentEditArgument,
+    Method.textDocument_hover,
   };
 
   /// The name of the DTD service that methods will be registered under.

pkg/analysis_server/test/integration/lsp/abstract_lsp_over_legacy.dart

@@ -13,7 +13,11 @@ import '../support/integration_tests.dart';
 
 abstract class AbstractLspOverLegacyTest
     extends AbstractAnalysisServerIntegrationTest
-    with LspRequestHelpersMixin, LspEditHelpersMixin {
+    with
+        LspRequestHelpersMixin,
+        LspReverseRequestHelpersMixin,
+        LspNotificationHelpersMixin,
+        LspEditHelpersMixin {
   late final testFile = sourcePath('lib/test.dart');
 
   /// A stream of LSP [NotificationMessage]s from the server.

pkg/analysis_server/test/integration/lsp_server/integration_tests.dart

@@ -22,6 +22,8 @@ abstract class AbstractLspAnalysisServerIntegrationTest
     with
         ClientCapabilitiesHelperMixin,
         LspRequestHelpersMixin,
+        LspReverseRequestHelpersMixin,
+        LspNotificationHelpersMixin,
         LspEditHelpersMixin,
         LspAnalysisServerTestMixin {
   final List<String> vmArgs = [];

pkg/analysis_server/test/lsp/request_helpers_mixin.dart

@@ -74,6 +74,26 @@ mixin LspEditHelpersMixin {
   }
 }
 
+/// Helpers to simplify handling LSP notifications.
+mixin LspNotificationHelpersMixin {
+  /// A stream of [DartTextDocumentContentDidChangeParams] for any
+  /// `dart/textDocumentContentDidChange` notifications.
+  Stream<DartTextDocumentContentDidChangeParams>
+  get dartTextDocumentContentDidChangeNotifications => notificationsFromServer
+      .where(
+        (notification) =>
+            notification.method ==
+            CustomMethods.dartTextDocumentContentDidChange,
+      )
+      .map(
+        (message) => DartTextDocumentContentDidChangeParams.fromJson(
+          message.params as Map<String, Object?>,
+        ),
+      );
+
+  Stream<NotificationMessage> get notificationsFromServer;
+}
+
 mixin LspProgressNotificationsMixin {
   Stream<NotificationMessage> get notificationsFromServer;
 
@@ -143,25 +163,6 @@ mixin LspRequestHelpersMixin {
   /// should not be validated as being created by the server first.
   final clientProvidedTestWorkDoneToken = ProgressToken.t2('client-test');
 
-  /// A stream of [DartTextDocumentContentDidChangeParams] for any
-  /// `dart/textDocumentContentDidChange` notifications.
-  Stream<DartTextDocumentContentDidChangeParams>
-  get dartTextDocumentContentDidChangeNotifications => notificationsFromServer
-      .where(
-        (notification) =>
-            notification.method ==
-            CustomMethods.dartTextDocumentContentDidChange,
-      )
-      .map(
-        (message) => DartTextDocumentContentDidChangeParams.fromJson(
-          message.params as Map<String, Object?>,
-        ),
-      );
-
-  Stream<NotificationMessage> get notificationsFromServer;
-
-  Stream<RequestMessage> get requestsFromServer;
-
   Future<List<CallHierarchyIncomingCall>?> callHierarchyIncoming(
     CallHierarchyItem item,
   ) {
@@ -237,29 +238,6 @@ mixin LspRequestHelpersMixin {
   void expect(Object? actual, Matcher matcher, {String? reason}) =>
       test.expect(actual, matcher, reason: reason);
 
-  /// Expects a [method] request from the server after executing [f].
-  Future<RequestMessage> expectRequest(
-    Method method,
-    FutureOr<void> Function() f, {
-    Duration timeout = const Duration(seconds: 5),
-  }) async {
-    var firstRequest = requestsFromServer.firstWhere((n) => n.method == method);
-    await f();
-
-    var requestFromServer = await firstRequest.timeout(
-      timeout,
-      onTimeout:
-          () =>
-              throw TimeoutException(
-                'Did not receive the expected $method request from the server in the timeout period',
-                timeout,
-              ),
-    );
-
-    expect(requestFromServer, isNotNull);
-    return requestFromServer;
-  }
-
   Future<T> expectSuccessfulResponseTo<T, R>(
     RequestMessage request,
     T Function(R) fromJson,
@@ -875,71 +853,6 @@ mixin LspRequestHelpersMixin {
     );
   }
 
-  /// Executes [f] then waits for a request of type [method] from the server which
-  /// is passed to [handler] to process, then waits for (and returns) the
-  /// response to the original request.
-  ///
-  /// This is used for testing things like code actions, where the client initiates
-  /// a request but the server does not respond to it until it's sent its own
-  /// request to the client and it received a response.
-  ///
-  ///     Client                                 Server
-  ///     1. |- Req: textDocument/codeAction      ->
-  ///     1. <- Resp: textDocument/codeAction     -|
-  ///
-  ///     2. |- Req: workspace/executeCommand  ->
-  ///           3. <- Req: textDocument/applyEdits  -|
-  ///           3. |- Resp: textDocument/applyEdits ->
-  ///     2. <- Resp: workspace/executeCommand -|
-  ///
-  /// Request 2 from the client is not responded to until the server has its own
-  /// response to the request it sends (3).
-  Future<T> handleExpectedRequest<T, R, RR>(
-    Method method,
-    R Function(Map<String, dynamic>) fromJson,
-    Future<T> Function() f, {
-    required FutureOr<RR> Function(R) handler,
-    Duration timeout = const Duration(seconds: 5),
-  }) async {
-    late Future<T> outboundRequest;
-    Object? outboundRequestError;
-
-    // Run [f] and wait for the incoming request from the server.
-    var incomingRequest = await expectRequest(method, () {
-      // Don't return/await the response yet, as this may not complete until
-      // after we have handled the request that comes from the server.
-      outboundRequest = f();
-
-      // Because we don't await this future until "later", if it throws the
-      // error is treated as unhandled and will fail the test even if expected.
-      // Instead, capture the error and suppress it. But if we time out (in
-      // which case we will never return outboundRequest), then we'll raise this
-      // error.
-      outboundRequest.then(
-        (_) {},
-        onError: (e) {
-          outboundRequestError = e;
-          return null;
-        },
-      );
-    }, timeout: timeout).catchError((Object timeoutException) {
-      // We timed out waiting for the request from the server. Probably this is
-      // because our outbound request for some reason, so if we have an error
-      // for that, then throw it. Otherwise, propogate the timeout.
-      throw outboundRequestError ?? timeoutException;
-    }, test: (e) => e is TimeoutException);
-
-    // Handle the request from the server and send the response back.
-    var clientsResponse = await handler(
-      fromJson(incomingRequest.params as Map<String, Object?>),
-    );
-    respondTo(incomingRequest, clientsResponse);
-
-    // Return a future that completes when the response to the original request
-    // (from [f]) returns.
-    return outboundRequest;
-  }
-
   RequestMessage makeRequest(Method method, Object? params) {
     var id = Either2<int, String>.t1(_id++);
     return RequestMessage(
@@ -1014,24 +927,8 @@ mixin LspRequestHelpersMixin {
     return expectSuccessfulResponseTo(request, CompletionItem.fromJson);
   }
 
-  /// Sends [responseParams] to the server as a successful response to
-  /// a server-initiated [request].
-  void respondTo<T>(RequestMessage request, T responseParams) {
-    sendResponseToServer(
-      ResponseMessage(
-        id: request.id,
-        result: responseParams,
-        jsonrpc: jsonRpcVersion,
-      ),
-    );
-  }
-
   Future<ResponseMessage> sendRequestToServer(RequestMessage request);
 
-  /// Sends a ResponseMessage to the server, completing a reverse
-  /// (server-to-client) request.
-  void sendResponseToServer(ResponseMessage response);
-
   Future<List<TypeHierarchyItem>?> typeHierarchySubtypes(
     TypeHierarchyItem item,
   ) {
@@ -1263,6 +1160,120 @@ mixin LspRequestHelpersMixin {
   }
 }
 
+/// Helpers to simplify handling LSP reverse-requests.
+///
+/// The sending of responses must be supplied by the implementing class
+/// via [sendResponseToServer].
+mixin LspReverseRequestHelpersMixin {
+  /// A stream of reverse-requests from the server that can be responded to via
+  /// [sendResponseToServer].
+  Stream<RequestMessage> get requestsFromServer;
+
+  /// Expects a [method] request from the server after executing [f].
+  Future<RequestMessage> expectRequest(
+    Method method,
+    FutureOr<void> Function() f, {
+    Duration timeout = const Duration(seconds: 5),
+  }) async {
+    var firstRequest = requestsFromServer.firstWhere((n) => n.method == method);
+    await f();
+
+    var requestFromServer = await firstRequest.timeout(
+      timeout,
+      onTimeout:
+          () =>
+              throw TimeoutException(
+                'Did not receive the expected $method request from the server in the timeout period',
+                timeout,
+              ),
+    );
+
+    expect(requestFromServer, isNotNull);
+    return requestFromServer;
+  }
+
+  /// Executes [f] then waits for a request of type [method] from the server which
+  /// is passed to [handler] to process, then waits for (and returns) the
+  /// response to the original request.
+  ///
+  /// This is used for testing things like code actions, where the client initiates
+  /// a request but the server does not respond to it until it's sent its own
+  /// request to the client and it received a response.
+  ///
+  ///     Client                                 Server
+  ///     1. |- Req: textDocument/codeAction      ->
+  ///     1. <- Resp: textDocument/codeAction     -|
+  ///
+  ///     2. |- Req: workspace/executeCommand  ->
+  ///           3. <- Req: textDocument/applyEdits  -|
+  ///           3. |- Resp: textDocument/applyEdits ->
+  ///     2. <- Resp: workspace/executeCommand -|
+  ///
+  /// Request 2 from the client is not responded to until the server has its own
+  /// response to the request it sends (3).
+  Future<T> handleExpectedRequest<T, R, RR>(
+    Method method,
+    R Function(Map<String, dynamic>) fromJson,
+    Future<T> Function() f, {
+    required FutureOr<RR> Function(R) handler,
+    Duration timeout = const Duration(seconds: 5),
+  }) async {
+    late Future<T> outboundRequest;
+    Object? outboundRequestError;
+
+    // Run [f] and wait for the incoming request from the server.
+    var incomingRequest = await expectRequest(method, () {
+      // Don't return/await the response yet, as this may not complete until
+      // after we have handled the request that comes from the server.
+      outboundRequest = f();
+
+      // Because we don't await this future until "later", if it throws the
+      // error is treated as unhandled and will fail the test even if expected.
+      // Instead, capture the error and suppress it. But if we time out (in
+      // which case we will never return outboundRequest), then we'll raise this
+      // error.
+      outboundRequest.then(
+        (_) {},
+        onError: (e) {
+          outboundRequestError = e;
+          return null;
+        },
+      );
+    }, timeout: timeout).catchError((Object timeoutException) {
+      // We timed out waiting for the request from the server. Probably this is
+      // because our outbound request for some reason, so if we have an error
+      // for that, then throw it. Otherwise, propogate the timeout.
+      throw outboundRequestError ?? timeoutException;
+    }, test: (e) => e is TimeoutException);
+
+    // Handle the request from the server and send the response back.
+    var clientsResponse = await handler(
+      fromJson(incomingRequest.params as Map<String, Object?>),
+    );
+    respondTo(incomingRequest, clientsResponse);
+
+    // Return a future that completes when the response to the original request
+    // (from [f]) returns.
+    return outboundRequest;
+  }
+
+  /// Sends [responseParams] to the server as a successful response to
+  /// a server-initiated [request].
+  void respondTo<T>(RequestMessage request, T responseParams) {
+    sendResponseToServer(
+      ResponseMessage(
+        id: request.id,
+        result: responseParams,
+        jsonrpc: jsonRpcVersion,
+      ),
+    );
+  }
+
+  /// Sends a ResponseMessage to the server, completing a reverse
+  /// (server-to-client) request.
+  void sendResponseToServer(ResponseMessage response);
+}
+
 /// A mixin with helpers for verifying LSP edits in a given project.
 ///
 /// Extends [LspEditHelpersMixin] with methods for accessing file state and

pkg/analysis_server/test/lsp/server_abstract.dart

@@ -49,6 +49,8 @@ abstract class AbstractLspAnalysisServerTest
         ResourceProviderMixin,
         ClientCapabilitiesHelperMixin,
         LspRequestHelpersMixin,
+        LspReverseRequestHelpersMixin,
+        LspNotificationHelpersMixin,
         LspEditHelpersMixin,
         LspVerifyEditHelpersMixin,
         LspAnalysisServerTestMixin,
@@ -784,7 +786,12 @@ mixin ClientCapabilitiesHelperMixin {
   }
 }
 
-mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin
+mixin LspAnalysisServerTestMixin
+    on
+        LspRequestHelpersMixin,
+        LspReverseRequestHelpersMixin,
+        LspNotificationHelpersMixin,
+        LspEditHelpersMixin
     implements ClientCapabilitiesHelperMixin {
   late String projectFolderPath,
       mainFilePath,

pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart

@@ -148,6 +148,8 @@ class EventsPrinterConfiguration {}
 abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest
     with
         LspRequestHelpersMixin,
+        LspReverseRequestHelpersMixin,
+        LspNotificationHelpersMixin,
         LspEditHelpersMixin,
         ClientCapabilitiesHelperMixin,
         LspVerifyEditHelpersMixin,

pkg/analysis_server/test/shared/shared_code_actions_refactor_tests.dart

@@ -69,6 +69,7 @@ mixin SharedConvertMethodToGetterRefactorCodeActionsTests
         SharedTestInterface,
         CodeActionsTestMixin,
         LspRequestHelpersMixin,
+        LspReverseRequestHelpersMixin,
         LspEditHelpersMixin,
         LspVerifyEditHelpersMixin,
         ClientCapabilitiesHelperMixin {
@@ -118,6 +119,7 @@ mixin SharedExtractMethodRefactorCodeActionsTests
         SharedTestInterface,
         CodeActionsTestMixin,
         LspRequestHelpersMixin,
+        LspReverseRequestHelpersMixin,
         LspEditHelpersMixin,
         LspVerifyEditHelpersMixin,
         ClientCapabilitiesHelperMixin,