Add WebSocketMockServer and tests for WebSocketEngine #5187
Conversation
…een implementations/targets
✅ Deploy Preview for apollo-android-docs canceled.
|
...ime/src/appleMain/kotlin/com/apollographql/apollo3/network/ws/NSURLSessionWebSocketEngine.kt
Show resolved
Hide resolved
...mockserver/src/commonMain/kotlin/com/apollographql/apollo3/mockserver/WebSocketMockServer.kt
Outdated
Show resolved
Hide resolved
...mockserver/src/commonMain/kotlin/com/apollographql/apollo3/mockserver/WebSocketMockServer.kt
Outdated
Show resolved
Hide resolved
...es/apollo-runtime/src/jsMain/kotlin/com/apollographql/apollo3/network/http/KtorHttpEngine.kt
Show resolved
Hide resolved
|
|
||
| @Test | ||
| fun gzipTestKtor() = gzipTest(KtorHttpEngine()) |
There was a problem hiding this comment.
Any objection to have 2 test tasks that substitute the engine constructor? Like we do for integration tests and Java vs Kotlin models? I can try in a follow up PR if you're up for it
There was a problem hiding this comment.
Yes that would be nice, duplicating all test methods is annoying. Not so sure about how to do it though 😅 so you can go ahead if you have ideas 🙏
| class Connect(val sessionId: String, val headers: Map<String, String>) : WebSocketEvent() | ||
|
|
||
| @ApolloExperimental | ||
| class TextFrame(val sessionId: String, val text: String) : WebSocketEvent() |
There was a problem hiding this comment.
Naming bikeshedding: I thought a Text message was split accross multiple frames and therefore this would need to be called TextMessage (or just Text)? Similarly, WebSocketEvent could be named WebSocketMessage?
There was a problem hiding this comment.
In events however you can receive "frames" (which I agree it could make sense to rename "messages"), but also other things that are not really messages (connect, close, error) IMO.
There was a problem hiding this comment.
Gotcha 👍
I was expecting an API similar to MockWebserver where we don't have anything like Connect or Error. All we can get is the HTTP request.
Similarly for WebSockets, I would expect to be able to record the initial HTTP handshake request as well as subsequent Close/Ping/Pong control frames (or messages because these messages are always a single frame) and data messages (not individual frames).
Just my 2 cents, feel free to discard.
...mockserver/src/commonMain/kotlin/com/apollographql/apollo3/mockserver/WebSocketMockServer.kt
Outdated
Show resolved
Hide resolved
| * @param readTimeoutMillis The request timeout in milliseconds. The request timeout is the time period required to process an HTTP call: from sending a request to receiving a response. | ||
| */ | ||
| actual class DefaultHttpEngine constructor(private val connectTimeoutMillis: Long, private val readTimeoutMillis: Long) : HttpEngine { |
There was a problem hiding this comment.
While we're at it, do you mind renaming readTimeoutMillis to requestTimeoutMillis? Ultimately, I'd like us to support readTimeoutMillis because:
- I think it's a better parameter because it does not depend on the response size
requestTimeoutcan be emulated quite easily usingwithTimeoutwhilereadTimeoutMillisactually requires low-level configuration
There was a problem hiding this comment.
So all in all it'd be:
connectTimeout: the initial TCP handshake timeoutreadTimeout: the socket timeout on subsequent readsrequestTimeout(not very useful): the overall timeout of the request
There was a problem hiding this comment.
I think renaming readTimeoutMillis to requestTimeoutMillis is breaking right?
Also, in my tests, readTimeoutMillis didn't seem to work actually 😅 But I'll try again to confirm.
In any case I think it makes sense to add a constructor with the 3 parameters? Give more power to the user.
There was a problem hiding this comment.
I think renaming readTimeoutMillis to requestTimeoutMillis is breaking right?
Indeed, source breaking, which unfortunately isn't tracked by kbcv.
In any case I think it makes sense to add a constructor with the 3 parameters? Give more power to the user.
I'd vote for only 2: connectTimeout and readTimeout. requestTimeout belongs in higher layers using withTimeout. That is if readTimeout is working as expected of course.
There was a problem hiding this comment.
I just checked and socketTimeout doesn't work on Ktor JS. I'll open an issue. In the meantime we could use socketTimeout for Jvm and Apple, but requestTimeout for JS?
There was a problem hiding this comment.
I think so? If you're talking about the expect/actual constructor that takes a simple timeoutMillis parameter then it's fine to map it to socketTimeout on Jvm/Apple and requestTimeout on JS as long as it's properly documented.
Then for overloaded (non-actual) constructors, my preference would go to connectTimeout and readTImeout and leave requestTimeout out of it.
No description provided.