[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 <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: DanTup

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,15 +151,21 @@ abstract class ContextResolutionTest with ResourceProviderMixin {
     handleSuccessfulRequest(
       AnalysisSetPriorityFilesParams(
         files.map((e) => e.path).toList(),
-      ).toRequest('0', clientUriConverter: server.uriConverter),
+      ).toRequest(
+        '${nextRequestId++}',
+        clientUriConverter: server.uriConverter,
+      ),
     );
   }
 
   Future<void> setPriorityFiles2(List<File> files) async {
     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<void> _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,
+      ),
     );
   }
 

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,
       ),
     );