[analysis_server] Add support/tests for reverse-requests for LSP-over-Legacy

This adds shared tests for sending workspace/applyEdit reverse-requests that run for both the LSP and Legacy servers.

It involved moving some code out of the base LSP test onto mixins to be used by LSP-over-Legacy tests and I extracted some mixins that can be used in tests to provide a common interface to both servers for writing shared tests.

Change-Id: I8c6d09f220b2593680547311a4fce66013f86e2e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/404420
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: DanTup

pkg/analysis_server/lib/src/legacy_analysis_server.dart

@@ -462,7 +462,8 @@ class LegacyAnalysisServer extends AnalysisServer {
   }
 
   @override
-  get editorClientCapabilities => _editorClientCapabilities;
+  lsp.LspClientCapabilities get editorClientCapabilities =>
+      _editorClientCapabilities;
 
   /// The [Future] that completes when analysis is complete.
   Future<void> get onAnalysisComplete {

pkg/analysis_server/lib/src/utilities/mocks.dart

@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:async';
+import 'dart:convert';
 
 import 'package:analysis_server/protocol/protocol.dart';
 import 'package:analysis_server/src/channel/channel.dart';
@@ -15,12 +16,21 @@ import 'package:watcher/watcher.dart';
 
 /// A mock [ServerCommunicationChannel] for testing [AnalysisServer].
 class MockServerChannel implements ServerCommunicationChannel {
+  /// A controller for the stream of requests and responses from the client to
+  /// the server.
   StreamController<RequestOrResponse> requestController =
       StreamController<RequestOrResponse>();
-  StreamController<Response> responseController =
-      StreamController<Response>.broadcast();
+
+  /// A controller for the stream of requests and responses from the server to
+  /// the client.
+  StreamController<RequestOrResponse> responseController =
+      StreamController<RequestOrResponse>.broadcast();
+
+  /// A controller for the stream of notifications from the server to the
+  /// client.
   StreamController<Notification> notificationController =
       StreamController<Notification>.broadcast(sync: true);
+
   Completer<Response>? errorCompleter;
 
   List<Response> responsesReceived = [];
@@ -41,6 +51,11 @@ class MockServerChannel implements ServerCommunicationChannel {
   @override
   Stream<RequestOrResponse> get requests => requestController.stream;
 
+  /// Return the broadcast stream of server-to-client requests.
+  Stream<Request> get serverToClientRequests {
+    return responseController.stream.where((r) => r is Request).cast<Request>();
+  }
+
   @override
   void close() {
     _closed = true;
@@ -71,6 +86,7 @@ class MockServerChannel implements ServerCommunicationChannel {
   @override
   void sendRequest(Request request) {
     serverRequestsSent.add(request);
+    responseController.add(request);
   }
 
   @override
@@ -89,13 +105,30 @@ class MockServerChannel implements ServerCommunicationChannel {
   /// with the [request] has been received.
   ///
   /// The value of the future will be the received response.
-  Future<Response> simulateRequestFromClient(Request request) {
+  Future<Response> simulateRequestFromClient(Request request) async {
     if (_closed) {
       throw Exception('simulateRequestFromClient after connection closed');
     }
+
+    // Round-trip via JSON to ensure all types are fully serialized as they
+    // would be in a real setup.
+    request =
+        Request.fromJson(
+          jsonDecode(jsonEncode(request)) as Map<String, Object?>,
+        )!;
+
     // Wrap send request in future to simulate WebSocket.
-    Future(() => requestController.add(request));
-    return waitForResponse(request);
+    unawaited(Future(() => requestController.add(request)));
+    var response = await waitForResponse(request);
+
+    // Round-trip via JSON to ensure all types are fully serialized as they
+    // would be in a real setup.
+    response =
+        Response.fromJson(
+          jsonDecode(jsonEncode(response)) as Map<String, Object?>,
+        )!;
+
+    return response;
   }
 
   /// Send the given [response] to the server as if it had been sent from the
@@ -117,9 +150,10 @@ class MockServerChannel implements ServerCommunicationChannel {
   /// has already been sent to the server.
   Future<Response> waitForResponse(Request request) {
     var id = request.id;
-    return responseController.stream.firstWhere(
-      (response) => response.id == id,
-    );
+    return responseController.stream
+        .where((r) => r is Response)
+        .cast<Response>()
+        .firstWhere((response) => response.id == id);
   }
 }
 

pkg/analysis_server/test/analysis_server_base.dart

@@ -224,7 +224,9 @@ abstract class ContextResolutionTest with ResourceProviderMixin {
 class PubPackageAnalysisServerTest extends ContextResolutionTest
     with MockPackagesMixin, ConfigurationFilesMixin, TestMacros {
   // TODO(scheglov): Consider turning it back into a getter.
-  late String testFilePath = '$testPackageLibPath/test.dart';
+  late String testFilePath = resourceProvider.convertPath(
+    '$testPackageLibPath/test.dart',
+  );
 
   /// Return a list of the experiments that are to be enabled for tests in this
   /// class, an empty list if there are no experiments that should be enabled.

pkg/analysis_server/test/domain_server_test.dart

@@ -124,7 +124,7 @@ class ServerDomainTest extends PubPackageAnalysisServerTest {
     ).toRequest('1', clientUriConverter: server.uriConverter);
 
     await handleSuccessfulRequest(request);
-    var effectiveCapabilities = server.editorClientCapabilities!;
+    var effectiveCapabilities = server.editorClientCapabilities;
     expect(
       effectiveCapabilities.hoverContentFormats,
       equals([lsp.MarkupKind.PlainText]),

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

@@ -25,6 +25,10 @@ abstract class AbstractLspOverLegacyTest
         ),
       );
 
+  @override
+  Stream<RequestMessage> get requestsFromServer =>
+      throw 'Reverse-requests not currently supported for LSP-over-Legacy integration tests';
+
   /// The URI for the macro-generated content for [testFileUri].
   Uri get testFileMacroUri =>
       Uri.file(testFile).replace(scheme: macroClientUriScheme);
@@ -65,4 +69,9 @@ abstract class AbstractLspOverLegacyTest
       return fromJson(lspResponse.result as R);
     }
   }
+
+  @override
+  void sendResponseToServer(ResponseMessage response) {
+    throw 'Reverse-requests not currently supported for LSP-over-Legacy integration tests';
+  }
 }

pkg/analysis_server/test/lsp/request_helpers_mixin.dart

@@ -5,6 +5,7 @@
 import 'dart:async';
 
 import 'package:analysis_server/lsp_protocol/protocol.dart';
+import 'package:analysis_server/src/lsp/client_capabilities.dart';
 import 'package:analysis_server/src/lsp/constants.dart';
 import 'package:analysis_server/src/lsp/mapping.dart';
 import 'package:analysis_server/src/services/completion/dart/feature_computer.dart';
@@ -14,7 +15,7 @@ import 'package:collection/collection.dart';
 import 'package:language_server_protocol/json_parsing.dart';
 import 'package:path/path.dart' as path;
 import 'package:test/test.dart' as test show expect;
-import 'package:test/test.dart' hide expect;
+import 'package:test/test.dart';
 
 import 'change_verifier.dart';
 
@@ -169,6 +170,8 @@ mixin LspRequestHelpersMixin {
 
   Stream<NotificationMessage> get notificationsFromServer;
 
+  Stream<RequestMessage> get requestsFromServer;
+
   Future<List<CallHierarchyIncomingCall>?> callHierarchyIncoming(
     CallHierarchyItem item,
   ) {
@@ -225,6 +228,29 @@ 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,
@@ -777,6 +803,71 @@ 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, ToJsonable? params) {
     var id = Either2<int, String>.t1(_id++);
     return RequestMessage(
@@ -851,6 +942,22 @@ 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,
+      ),
+    );
+  }
+
+  /// Sends a ResponseMessage to the server, completing a reverse
+  /// (server-to-client) request.
+  void sendResponseToServer(ResponseMessage response);
+
   Future<List<TypeHierarchyItem>?> typeHierarchySubtypes(
     TypeHierarchyItem item,
   ) {
@@ -1079,15 +1186,64 @@ mixin LspRequestHelpersMixin {
 /// Extends [LspEditHelpersMixin] with methods for accessing file state and
 /// information about the project to build paths.
 mixin LspVerifyEditHelpersMixin on LspEditHelpersMixin {
+  LspClientCapabilities get editorClientCapabilities;
+
   path.Context get pathContext;
 
   String get projectFolderPath;
 
   ClientUriConverter get uriConverter;
 
+  /// Executes a function which is expected to call back to the client to apply
+  /// a [WorkspaceEdit].
+  ///
+  /// Returns a [LspChangeVerifier] that can be used to verify changes.
+  Future<LspChangeVerifier> executeForEdits(
+    Future<Object?> Function() function, {
+    ApplyWorkspaceEditResult? applyEditResult,
+  }) async {
+    ApplyWorkspaceEditParams? editParams;
+
+    var commandResponse = await handleExpectedRequest<
+      Object?,
+      ApplyWorkspaceEditParams,
+      ApplyWorkspaceEditResult
+    >(
+      Method.workspace_applyEdit,
+      ApplyWorkspaceEditParams.fromJson,
+      function,
+      handler: (edit) {
+        // When the server sends the edit back, just keep a copy and say we
+        // applied successfully (it'll be verified by the caller).
+        editParams = edit;
+        return applyEditResult ?? ApplyWorkspaceEditResult(applied: true);
+      },
+    );
+    // Successful edits return an empty success() response.
+    expect(commandResponse, isNull);
+
+    // Ensure the edit came back, and using the expected change type.
+    expect(editParams, isNotNull);
+    var edit = editParams!.edit;
+
+    var expectDocumentChanges = editorClientCapabilities.documentChanges;
+    expect(edit.documentChanges, expectDocumentChanges ? isNotNull : isNull);
+    expect(edit.changes, expectDocumentChanges ? isNull : isNotNull);
+
+    return LspChangeVerifier(this, edit);
+  }
+
   /// A function to get the current contents of a file to apply edits.
   String? getCurrentFileContent(Uri uri);
 
+  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),
+  });
+
   /// Formats a path relative to the project root always using forward slashes.
   ///
   /// This is used in the text format for comparing edits.