From 0d90ccc2ec221acab2620662a19e0da6765d4e05 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Fri, 24 Jan 2025 09:26:38 -0800 Subject: [PATCH] [analysis_server] Use unique request IDs in tests to avoid flaky tests Some of the LSP-over-Legacy tests were intermittently failing because the test did not correctly wait for the `setClientCapabilities` request to complete. The reason for this was that it used the ID `0` (from `_newRequestId`) but so did `setAnalysisRoots`. If the timing was right, the second request would appear to complete when the first did (because the response had id=0) and the test would start before the capabilities had actually been set (and since the test reads them directly off the server, they would be incorrect). This moves the `_nextRequestId` field down into a base class and updates the helpers for requests like `setAnalysisRoots` to use it too. Change-Id: Ice44191aad39c7ea81c40e501827557fe563e3d5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/405760 Reviewed-by: Brian Wilkerson Reviewed-by: Konstantin Shcheglov Commit-Queue: Brian Wilkerson --- .../test/analysis_server_base.dart | 32 +++++++++++++------ .../abstract_lsp_over_legacy.dart | 13 +++----- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/pkg/analysis_server/test/analysis_server_base.dart b/pkg/analysis_server/test/analysis_server_base.dart index e035286586ce..16d28670b342 100644 --- a/pkg/analysis_server/test/analysis_server_base.dart +++ b/pkg/analysis_server/test/analysis_server_base.dart @@ -88,6 +88,9 @@ abstract class ContextResolutionTest with ResourceProviderMixin { /// implementation are still fully verified. static final MemoryByteStore _sharedByteStore = MemoryByteStore(); + /// The next ID to use in a request to the server. + var nextRequestId = 0; + MemoryByteStore _byteStore = _sharedByteStore; final TestPluginManager pluginManager = TestPluginManager(); @@ -148,7 +151,10 @@ abstract class ContextResolutionTest with ResourceProviderMixin { handleSuccessfulRequest( AnalysisSetPriorityFilesParams( files.map((e) => e.path).toList(), - ).toRequest('0', clientUriConverter: server.uriConverter), + ).toRequest( + '${nextRequestId++}', + clientUriConverter: server.uriConverter, + ), ); } @@ -156,7 +162,10 @@ abstract class ContextResolutionTest with ResourceProviderMixin { await handleSuccessfulRequest( AnalysisSetPriorityFilesParams( files.map((e) => e.path).toList(), - ).toRequest('0', clientUriConverter: server.uriConverter), + ).toRequest( + '${nextRequestId++}', + clientUriConverter: server.uriConverter, + ), ); } @@ -171,7 +180,10 @@ abstract class ContextResolutionTest with ResourceProviderMixin { includedConverted, excludedConverted, packageRoots: {}, - ).toRequest('0', clientUriConverter: server.uriConverter), + ).toRequest( + '${nextRequestId++}', + clientUriConverter: server.uriConverter, + ), ); } @@ -214,9 +226,10 @@ abstract class ContextResolutionTest with ResourceProviderMixin { Future _setGeneralAnalysisSubscriptions() async { await handleSuccessfulRequest( - AnalysisSetGeneralSubscriptionsParams( - _analysisGeneralServices, - ).toRequest('0', clientUriConverter: server.uriConverter), + AnalysisSetGeneralSubscriptionsParams(_analysisGeneralServices).toRequest( + '${nextRequestId++}', + clientUriConverter: server.uriConverter, + ), ); } } @@ -264,9 +277,10 @@ class PubPackageAnalysisServerTest extends ContextResolutionTest ) async { (_analysisFileSubscriptions[service] ??= []).add(file.path); await handleSuccessfulRequest( - AnalysisSetSubscriptionsParams( - _analysisFileSubscriptions, - ).toRequest('0', clientUriConverter: server.uriConverter), + AnalysisSetSubscriptionsParams(_analysisFileSubscriptions).toRequest( + '${nextRequestId++}', + clientUriConverter: server.uriConverter, + ), ); } 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 7c770e337aac..c25f95a7d98c 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 @@ -150,9 +150,6 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest LspEditHelpersMixin, LspVerifyEditHelpersMixin, ClientCapabilitiesHelperMixin { - /// The next ID to use a request to the server. - var _nextRequestId = 0; - /// The last ID that was used for a legacy request. late String lastSentLegacyRequestId; @@ -207,7 +204,7 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest AnalysisUpdateContentParams({ convertPath(filePath): AddContentOverlay(content), }).toRequest( - '${_nextRequestId++}', + '${nextRequestId++}', clientUriConverter: server.uriConverter, ), ); @@ -241,7 +238,7 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest /// Creates a legacy request with an auto-assigned ID. Request createLegacyRequest(RequestParams params) { return params.toRequest( - '${_nextRequestId++}', + '${nextRequestId++}', clientUriConverter: server.uriConverter, ); } @@ -323,7 +320,7 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest AnalysisUpdateContentParams({ convertPath(filePath): RemoveContentOverlay(), }).toRequest( - '${_nextRequestId++}', + '${nextRequestId++}', clientUriConverter: server.uriConverter, ), ); @@ -341,7 +338,7 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest var request = ServerSetClientCapabilitiesParams( [], lspCapabilities: clientCapabilities, - ).toRequest('${_nextRequestId++}', clientUriConverter: server.uriConverter); + ).toRequest('${nextRequestId++}', clientUriConverter: server.uriConverter); await handleSuccessfulRequest(request); } @@ -377,7 +374,7 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest AnalysisUpdateContentParams({ convertPath(filePath): ChangeContentOverlay([edit]), }).toRequest( - '${_nextRequestId++}', + '${nextRequestId++}', clientUriConverter: server.uriConverter, ), );