From 549a78500a72855412863422b6fae4dc3b6ae3d6 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 15 Jan 2025 12:28:45 -0800 Subject: [PATCH] [analysis_server] Add a shared test interface to simplify sharing tests between LSP, LSP-over-Legacy This is refactor extracted from an upcoming change (to make property editor tests run against both servers) to make that change smaller and easier to review. There are some existing shared tests that run for both LSP and Legacy servers, but they currently do not touch much server API (one is for DTD and one tests reverse-requests). Migrating other tests (such as EditableArguments) requires some additional API be the same between the different test/server base classes. This change adds an `abstract interface class SharedTestInterface` to serve as a common interface for methods that shared tests need to use that have different implementations between LSP and Legacy. For example, updating the overlays in an LSP-over-Legacy test needs to use the Legacy APIs for updating the overlay and not the LSP ones (so we can't just use the LSP methods like we would for calling something like getHover for LSP-over-Legacy). It also: - adds some new futures to the LSP test base to match the behaviour of the legacy one (wait for in-progress analysis) - replaces the shared mixins with real base classes that implement the shared interface (for ex. `abstract class SharedLspOverLegacyTest extends LspOverLegacyTest implements SharedTestInterface`) to make it easier to create shared tests - renames `sendLspRequest` to `sendLspRequestToClient` to make it clearer what direction this method is for Change-Id: I070c2c005b11b9afd8a87aa22b04972a9dde2320 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/404680 Reviewed-by: Phil Quitslund Commit-Queue: Brian Wilkerson Reviewed-by: Brian Wilkerson Commit-Queue: Phil Quitslund --- .../test/lsp/server_abstract.dart | 68 +++++++++++++++++-- .../test/lsp/workspace_apply_edit_test.dart | 7 +- .../abstract_lsp_over_legacy.dart | 53 ++++++++++++--- .../workspace_apply_edit_test.dart | 5 +- .../test/shared/shared_test_interface.dart | 52 ++++++++++++++ .../shared_workspace_apply_edit_tests.dart | 4 +- 6 files changed, 165 insertions(+), 24 deletions(-) create mode 100644 pkg/analysis_server/test/shared/shared_test_interface.dart diff --git a/pkg/analysis_server/test/lsp/server_abstract.dart b/pkg/analysis_server/test/lsp/server_abstract.dart index ef7f05d04b84..1b67adaa77e6 100644 --- a/pkg/analysis_server/test/lsp/server_abstract.dart +++ b/pkg/analysis_server/test/lsp/server_abstract.dart @@ -33,6 +33,7 @@ import 'package:unified_analytics/unified_analytics.dart'; import '../mocks.dart'; import '../mocks_lsp.dart'; +import '../shared/shared_test_interface.dart'; import '../support/configuration_files.dart'; import '../test_macros.dart'; import 'change_verifier.dart'; @@ -248,6 +249,10 @@ abstract class AbstractLspAnalysisServerTest _previousContextBuilds = server.contextBuilds; } + Future sendLspRequestToClient(Method method, Object params) { + return server.sendRequest(method, params); + } + @override Future sendNotificationToServer( NotificationMessage notification, @@ -882,9 +887,19 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin /// server. bool failTestOnErrorDiagnostic = true; + /// A completer for [initialAnalysis]. + final Completer _initialAnalysisCompleter = Completer(); + + /// A completer for [currentAnalysis]. + Completer _currentAnalysisCompleter = Completer()..complete(); + /// [analysisOptionsPath] as a 'file:///' [Uri]. Uri get analysisOptionsUri => pathContext.toUri(analysisOptionsPath); + /// A [Future] that completes when the current analysis completes (or is + /// already completed if no analysis is in progress). + Future get currentAnalysis => _currentAnalysisCompleter.future; + /// A stream of [NotificationMessage]s from the server that may be errors. Stream get errorNotificationsFromServer { return notificationsFromServer.where(_isErrorNotification); @@ -895,8 +910,7 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin serverCapabilities.experimental as Map? ?? {}; /// A [Future] that completes with the first analysis after initialization. - Future get initialAnalysis => - initialized ? Future.value() : waitForAnalysisComplete(); + Future get initialAnalysis => _initialAnalysisCompleter.future; bool get initialized => _clientCapabilities != null; @@ -1152,6 +1166,16 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin notificationsFromServer.listen((notification) async { if (notification.method == Method.progress) { await _handleProgress(notification); + } else if (notification.method == CustomMethods.analyzerStatus) { + var params = AnalyzerStatusParams.fromJson( + notification.params as Map, + ); + + if (params.isAnalyzing) { + _handleAnalysisBegin(); + } else { + _handleAnalysisEnd(); + } } }); @@ -1592,6 +1616,19 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin return outlineParams.outline; } + void _handleAnalysisBegin() async { + assert(_currentAnalysisCompleter.isCompleted); + _currentAnalysisCompleter = Completer(); + } + + void _handleAnalysisEnd() async { + if (!_initialAnalysisCompleter.isCompleted) { + _initialAnalysisCompleter.complete(); + } + assert(!_currentAnalysisCompleter.isCompleted); + _currentAnalysisCompleter.complete(); + } + Future _handleProgress(NotificationMessage request) async { var params = ProgressParams.fromJson( request.params as Map, @@ -1607,6 +1644,15 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin if (WorkDoneProgressEnd.canParse(params.value, nullLspJsonReporter)) { validProgressTokens.remove(params.token); } + + if (params.token == analyzingProgressToken) { + if (WorkDoneProgressBegin.canParse(params.value, nullLspJsonReporter)) { + _handleAnalysisBegin(); + } + if (WorkDoneProgressEnd.canParse(params.value, nullLspJsonReporter)) { + _handleAnalysisEnd(); + } + } } Future _handleWorkDoneProgressCreate(RequestMessage request) async { @@ -1642,16 +1688,24 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin } } -/// A mixin that provides a common interface for [AbstractLspAnalysisServerTest] classes -/// to work with shared tests. -mixin SharedLspAnalysisServerTestMixin on AbstractLspAnalysisServerTest { +/// An [AbstractLspAnalysisServerTest] that provides an implementation of +/// [SharedTestInterface] to allow tests to be shared between server/test kinds. +abstract class SharedAbstractLspAnalysisServerTest + extends AbstractLspAnalysisServerTest + implements SharedTestInterface { + @override String get testFilePath => join(projectFolderPath, 'lib', 'test.dart'); + @override + Uri get testFileUri => toUri(testFilePath); + + @override void createFile(String path, String content) { newFile(path, content); } - Future sendLspRequest(Method method, Object params) { - return server.sendRequest(method, params); + @override + Future initializeServer() async { + await initialize(); } } diff --git a/pkg/analysis_server/test/lsp/workspace_apply_edit_test.dart b/pkg/analysis_server/test/lsp/workspace_apply_edit_test.dart index 59f4a39e329e..2d6165247a92 100644 --- a/pkg/analysis_server/test/lsp/workspace_apply_edit_test.dart +++ b/pkg/analysis_server/test/lsp/workspace_apply_edit_test.dart @@ -14,16 +14,15 @@ void main() { } @reflectiveTest -class WorkspaceApplyEditTest extends AbstractLspAnalysisServerTest +class WorkspaceApplyEditTest extends SharedAbstractLspAnalysisServerTest with - // Tests are defined in SharedLspAnalysisServerTestMixin because they + // Tests are defined in SharedWorkspaceApplyEditTests because they // are shared and run for both LSP and Legacy servers. - SharedLspAnalysisServerTestMixin, SharedWorkspaceApplyEditTests { @override Future initializeServer() async { await initialize(); - await initialAnalysis; + await currentAnalysis; } @override diff --git a/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart b/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart index 40c08bd004ba..95a225a067fd 100644 --- a/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart +++ b/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart @@ -24,6 +24,7 @@ import '../lsp/change_verifier.dart'; import '../lsp/request_helpers_mixin.dart'; import '../lsp/server_abstract.dart'; import '../services/completion/dart/text_expectations.dart'; +import '../shared/shared_test_interface.dart'; class EventsCollector { final ContextResolutionTest test; @@ -258,12 +259,10 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest var error = lspResponse.error; if (error != null) { throw error; - } else if (T == Null) { + } else { return lspResponse.result == null ? null as T - : throw 'Expected Null response but got ${lspResponse.result}'; - } else { - return fromJson(lspResponse.result as R); + : fromJson(lspResponse.result as R); } } @@ -311,6 +310,17 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest } } + Future removeOverlay(String filePath) { + return handleSuccessfulRequest( + AnalysisUpdateContentParams({ + convertPath(filePath): RemoveContentOverlay(), + }).toRequest( + '${_nextRequestId++}', + clientUriConverter: server.uriConverter, + ), + ); + } + /// Send the configured LSP client capabilities to the server in a /// `server.setClientCapabilities` request. Future sendClientCapabilities() async { @@ -369,15 +379,42 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest } } -/// A mixin that provides a common interface for [LspOverLegacyTest] classes -/// to work with shared tests. -mixin SharedLspOverLegacyMixin on LspOverLegacyTest { +/// A [LspOverLegacyTest] that provides an implementation of +/// [SharedTestInterface] to allow tests to be shared between server/test kinds. +abstract class SharedLspOverLegacyTest extends LspOverLegacyTest + implements SharedTestInterface { + // TODO(dantup): Support this for LSP-over-Legacy shared tests. + var failTestOnErrorDiagnostic = false; + + @override + Future get currentAnalysis => waitForTasksFinished(); + + @override + Future closeFile(Uri uri) async { + await removeOverlay(fromUri(uri)); + } + + @override void createFile(String path, String content) { newFile(path, content); } + @override + Future openFile(Uri uri, String content, {int version = 1}) async { + await addOverlay(fromUri(uri), content); + } + + @override + Future replaceFile(int newVersion, Uri uri, String content) async { + // For legacy, we can use addOverlay to replace the whole file. + await addOverlay(fromUri(uri), content); + } + /// Wraps an LSP request up and sends it from the server to the client. - Future sendLspRequest(Method method, Object params) async { + Future sendLspRequestToClient( + Method method, + Object params, + ) async { var id = server.nextServerRequestId++; // Round-trip through JSON to ensure everything becomes basic types and we // don't have instances of classes like `Either2<>` in the JSON. diff --git a/pkg/analysis_server/test/lsp_over_legacy/workspace_apply_edit_test.dart b/pkg/analysis_server/test/lsp_over_legacy/workspace_apply_edit_test.dart index aab6924e7ada..45495f5f5f7c 100644 --- a/pkg/analysis_server/test/lsp_over_legacy/workspace_apply_edit_test.dart +++ b/pkg/analysis_server/test/lsp_over_legacy/workspace_apply_edit_test.dart @@ -14,11 +14,10 @@ void main() { } @reflectiveTest -class WorkspaceApplyEditTest extends LspOverLegacyTest +class WorkspaceApplyEditTest extends SharedLspOverLegacyTest with - // Tests are defined in SharedLspAnalysisServerTestMixin because they + // Tests are defined in SharedWorkspaceApplyEditTests because they // are shared and run for both LSP and Legacy servers. - SharedLspOverLegacyMixin, SharedWorkspaceApplyEditTests { @override Future initializeServer() async { diff --git a/pkg/analysis_server/test/shared/shared_test_interface.dart b/pkg/analysis_server/test/shared/shared_test_interface.dart new file mode 100644 index 000000000000..47f3fb884aa7 --- /dev/null +++ b/pkg/analysis_server/test/shared/shared_test_interface.dart @@ -0,0 +1,52 @@ +// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +/// A common interface that can be implemented by a set of base classes to +/// allow tests to be written to run in different configurations +/// (LSP and LSP-over-Legacy, and in-process and out-of-process). +/// +/// Implementations should use the appropriate APIs for the given server, for +/// example a test running against LSP will send a `textDocument/didOpen` +/// notification from [openFile] whereas when using the legacy server (even for +/// LSP-over-Legacy tests) will send an `analysis.updateContent` request. +abstract interface class SharedTestInterface { + /// A future that completes when the current analysis completes. + /// + /// If there is no analysis in progress, completes immediately. + Future get currentAnalysis; + + /// Sets whether the test should fail if error diagnostics are generated. + /// + /// This is used to avoid accidentally including invalid code in tests but can + /// be overridden for tests that are deliberately testing invalid code. + set failTestOnErrorDiagnostic(bool value); + + /// Gets the full normalized file path of a file named "test.dart" in the test + /// project. + String get testFilePath; + + /// Gets a file:/// URI for [testFilePath]; + Uri get testFileUri => Uri.file(testFilePath); + + /// Tells the server that file with [uri] has been closed and any overlay + /// should be removed. + Future closeFile(Uri uri); + + /// Creates a file at [filePath] with the given [content]. + void createFile(String filePath, String content); + + /// Performs standard initialization of the server, including starting + /// the server (an external process for integration tests) and sending any + /// initialization/analysis roots, and waiting for initial analysis to + /// complete. + Future initializeServer(); + + /// Tells the server that the file with [uri] has been opened and has the + /// given [content]. + Future openFile(Uri uri, String content, {int version = 1}); + + /// Tells the server that the file with [uri] has had it's content replaced + /// with [content]. + Future replaceFile(int newVersion, Uri uri, String content); +} diff --git a/pkg/analysis_server/test/shared/shared_workspace_apply_edit_tests.dart b/pkg/analysis_server/test/shared/shared_workspace_apply_edit_tests.dart index ced3997b395d..3cb3a19afb04 100644 --- a/pkg/analysis_server/test/shared/shared_workspace_apply_edit_tests.dart +++ b/pkg/analysis_server/test/shared/shared_workspace_apply_edit_tests.dart @@ -31,7 +31,7 @@ mixin SharedWorkspaceApplyEditTests /// Overridden by test subclasses to send LSP requests from the server to /// the client. - Future sendLspRequest(Method method, Object params); + Future sendLspRequestToClient(Method method, Object params); test_applyEdit_existingFile() async { var code = TestCode.parse(''' @@ -147,7 +147,7 @@ inserted<<<<<<<<<< ApplyWorkspaceEditResult? receivedApplyEditResult; var verifier = await executeForEdits(() async { - var result = await sendLspRequest( + var result = await sendLspRequestToClient( Method.workspace_applyEdit, ApplyWorkspaceEditParams(edit: workspaceEdit), );