Skip to content

Commit 1b04589

Browse files
DanTupCommit Queue
authored and
Commit Queue
committed
[analysis_server] Improve error reported by handleExpectedRequest when the expected request never arrives
The `handleExpectedRequest` test method sends a request to the server and expects a request back (for example executing a command that will trigger server-to-client applyEdit). There is a timeout (since this request may never come), but in the case where the timeout is caused by the outbound request failing, we should report that failure instead (since it is certainly the cause of the timeout). This changes the code to capture the error from the outbound request, and _if_ the expected request never comes, throws that in preference to the timeout. Before: ``` 00:05 +0 -1: ApplyAllFixesInWorkspace | test_partFile_issue59572 [E] TimeoutException after 0:00:05.000000: Future not completed test\lsp\server_abstract.dart 1143:15 LspAnalysisServerTestMixin.expectRequest.<fn> dart:async/zone.dart 1517:47 _rootRun dart:async/zone.dart 1422:19 _CustomZone.run dart:async/future_impl.dart 1036:34 Future.timeout.<fn> ===== asynchronous gap =========================== test\lsp\server_abstract.dart 1139:29 LspAnalysisServerTestMixin.expectRequest ===== asynchronous gap =========================== test\lsp\server_abstract.dart 1190:27 LspAnalysisServerTestMixin.handleExpectedRequest ===== asynchronous gap =========================== test\lsp\server_abstract.dart 155:27 AbstractLspAnalysisServerTest.executeForEdits ``` After: ``` 00:05 +0 -1: ApplyAllFixesInWorkspace | test_partFile_issue59572 [E] { "code": -32006, "message": "dart.edit.fixAllInWorkspace requires a single Map argument" } test\lsp\server_abstract.dart 1211:7 LspAnalysisServerTestMixin.handleExpectedRequest.<fn> dart:async/zone.dart 1538:47 _rootRunUnary dart:async/zone.dart 1429:19 _CustomZone.runUnary dart:async/future_impl.dart 229:22 _FutureListener.handleError dart:async/future_impl.dart 944:47 Future._propagateToListeners.handleError dart:async/future_impl.dart 965:13 Future._propagateToListeners dart:async/future_impl.dart 730:5 Future._completeError ``` Additionally, if the timeout is not because of a failed outbound request, the error message is now like "Did not receive the expected workspace/applyEdit request from the server in the timeout period" instead of "Future not completed". Fixes #59780 Change-Id: I8b8cc25194390ffffbe034eae313504792a42211 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/403460 Reviewed-by: Samuel Rawlins <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Brian Wilkerson <[email protected]>
1 parent d80fab4 commit 1b04589

File tree

1 file changed

+27
-10
lines changed

1 file changed

+27
-10
lines changed

pkg/analysis_server/test/lsp/server_abstract.dart

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1134,7 +1134,15 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin
11341134
var firstRequest = requestsFromServer.firstWhere((n) => n.method == method);
11351135
await f();
11361136

1137-
var requestFromServer = await firstRequest.timeout(timeout);
1137+
var requestFromServer = await firstRequest.timeout(
1138+
timeout,
1139+
onTimeout:
1140+
() =>
1141+
throw TimeoutException(
1142+
'Did not receive the expected $method request from the server in the timeout period',
1143+
timeout,
1144+
),
1145+
);
11381146

11391147
expect(requestFromServer, isNotNull);
11401148
return requestFromServer;
@@ -1174,6 +1182,7 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin
11741182
Duration timeout = const Duration(seconds: 5),
11751183
}) async {
11761184
late Future<T> outboundRequest;
1185+
Object? outboundRequestError;
11771186

11781187
// Run [f] and wait for the incoming request from the server.
11791188
var incomingRequest = await expectRequest(method, () {
@@ -1182,15 +1191,23 @@ mixin LspAnalysisServerTestMixin on LspRequestHelpersMixin, LspEditHelpersMixin
11821191
outboundRequest = f();
11831192

11841193
// Because we don't await this future until "later", if it throws the
1185-
// error is treated as unhandled and will fail the test. Attaching an
1186-
// error handler prevents that, though since the Future completed with
1187-
// an error it will still be handled as such when the future is later
1188-
// awaited.
1189-
1190-
// TODO(srawlins): Fix this static error.
1191-
// ignore: body_might_complete_normally_catch_error
1192-
outboundRequest.catchError((_) {});
1193-
});
1194+
// error is treated as unhandled and will fail the test even if expected.
1195+
// Instead, capture the error and suppress it. But if we time out (in
1196+
// which case we will never return outboundRequest), then we'll raise this
1197+
// error.
1198+
outboundRequest.then(
1199+
(_) {},
1200+
onError: (e) {
1201+
outboundRequestError = e;
1202+
return null;
1203+
},
1204+
);
1205+
}, timeout: timeout).catchError((Object timeoutException) {
1206+
// We timed out waiting for the request from the server. Probably this is
1207+
// because our outbound request for some reason, so if we have an error
1208+
// for that, then throw it. Otherwise, propogate the timeout.
1209+
throw outboundRequestError ?? timeoutException;
1210+
}, test: (e) => e is TimeoutException);
11941211

11951212
// Handle the request from the server and send the response back.
11961213
var clientsResponse = await handler(

0 commit comments

Comments
 (0)