Synchronously send error response on message decoding failure #334
Conversation
flowtoolz
commented
Oct 14, 2020
flowtoolz
commented
- Replaces two logs since those don't result in response messages anymore
- Adds a func for sending messages synchronously, to get the two messages out before fatalError
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
| @@ -229,17 +233,19 @@ public final class JSONRPCConnection { | |||
| continue MESSAGE_LOOP | |||
| } | |||
| case .unknown: | |||
| log("error decoding message: \(error.message)", level: .error) | |||
| send(.errorResponse(ResponseError(error), id: nil), | |||
There was a problem hiding this comment.
We can't test this behaviour directly due to the fatalError, but it might be good to add a test for the JSONRPCConnection receiving an error with nil id. For example, if you manually send an errorResponse(..., id: nil) make sure it doesn't crash the receiving connection. This would fit in Tests/LanguageServerProtocolJSONRPCTests/ConnectionTests.swift
There was a problem hiding this comment.
With this one I better ask you before going down the wrong rabbit hole ... I don't see an easy way to test that a JSONRPCMessage or a concrete error response actually reaches the receiver. I'd want to write somethin like this:
func testSendErrorResponseWithNilID() {
let client = connection.client
let expectation = self.expectation(description: "received message")
client.handleNextMessage { (message: JSONRPCMessage) in
expectation.fulfill()
}
connection.serverConnection._send(.errorResponse(.cancelled, id: nil))
waitForExpectations(timeout: 10)
}
There was a problem hiding this comment.
Ah I see we don't have a good way to test this right now. Let's skip this.
|
@swift-ci please test |
|
Done. Thanks for the learning opportunity 😊 |
|
@swift-ci please test |
|
|
Oh, I guess this is now a decodable error response message: So decoding that produces no error in CodingTests.swift:161. If the test was only intended to detect the missing ID, I guess I should remove the test? |
|
Sounds good to me |
|
@swift-ci please test |
|
done. this time, regenerating the Linux tests had no effect, just so you know 🤷🏼♂️ |
…ang#334) * Replaces two logs since those don't result in response messages anymore * Adds a func for sending messages synchronously, to get the two messages out before fatalError
