fix(site): add tests for createMockWebSocket
#19172
Conversation
| publishOpen: (event: Event) => void; | ||
|
|
||
| readonly isConnectionOpen: boolean; | ||
| readonly clientSentData: readonly SocketSendData[]; |
There was a problem hiding this comment.
This was one part I was a little torn on, and I'd be open to alternative ways to track data sent from the mock socket to the mock server
It feels like the "boy scout" way of doing this would be with subscription callbacks, but since we'd likely just be checking the contents for a single isolated test case, that'd add more boilerplate to all test cases when we really just care about what data got sent
| "http://www.dog.ceo/roll-over", | ||
| "https://www.dog.ceo/roll-over", |
| expect(onClose).toHaveBeenCalledTimes(1); | ||
| expect(onClose).toHaveBeenCalledWith(closeEvent); |
There was a problem hiding this comment.
this test makes me feel like I'm looking at a freshly ironed shirt. super nice!
| for (const jd of jsonData) { | ||
| socket.addEventListener("message", onMessage); | ||
| server.publishMessage( | ||
| new MessageEvent<string>("message", { |
There was a problem hiding this comment.
| new MessageEvent<string>("message", { | |
| new MessageEvent("message", { |
locally it looks like <string> is being inferred correctly, but if you want it to be more explicit I'm fine with that
|
|
||
| expect(onMessage).toHaveBeenCalledTimes(1); | ||
| expect(onMessage).toHaveBeenCalledWith( | ||
| new MessageEvent<string>("message", { |
| }); | ||
|
|
||
| it("Only registers each socket event handler once", () => { | ||
| const [socket, server] = createMockWebSocket("wss://www.dog.ceo/borf"); |
| type CallbackStore = { | ||
| [K in keyof EventMap]: ((event: EventMap[K]) => void)[]; | ||
| }; |
There was a problem hiding this comment.
also I'd rather define this type outside of the function. it doesn't capture any local types.
| let activeProtocol: string; | ||
| if (Array.isArray(protocols)) { | ||
| activeProtocol = protocols[0] ?? ""; | ||
| } else if (typeof protocols === "string") { | ||
| activeProtocol = protocols; | ||
| } else { | ||
| activeProtocol = ""; | ||
| } |
There was a problem hiding this comment.
why bother accepting multiple protocols if we only ever use the one? protocols isn't referenced after here
| if (!subscribers.includes(callback)) { | ||
| subscribers.push(callback); | ||
| } |
There was a problem hiding this comment.
what if we used Sets instead to enforce uniqueness? they're more friendly for this than Arrays
| const subscribers = store[eventType]; | ||
| if (subscribers.includes(callback)) { | ||
| const updated = store[eventType].filter((c) => c !== callback); | ||
| store[eventType] = updated as CallbackStore[E]; | ||
| } |
There was a problem hiding this comment.
| const subscribers = store[eventType]; | |
| if (subscribers.includes(callback)) { | |
| const updated = store[eventType].filter((c) => c !== callback); | |
| store[eventType] = updated as CallbackStore[E]; | |
| } | |
| store[eventType].delete(callback); |
...and then this especially gets simplified
| get clientSentData() { | ||
| return [...sentData]; | ||
| }, |
There was a problem hiding this comment.
cheeky little copy for immutability, nice.
| onopen: null, | ||
| dispatchEvent: jest.fn(), | ||
|
|
||
| send: (data) => { |
There was a problem hiding this comment.
Can we make send a jest.fn() so that its possible to do something like this?
expect(mockWebSocket.send).toHaveBeenCalledWith(
expect.stringContaining('"instance_type":"t3.medium"'),
);
send: jest.fn((data) => {
if (!isOpen) {
return;
}
sentData.push(data);
})
Needed for #19126 and #18679
Changes made
createWebSocketto dedicated file and addressed edge cases for making it a reliable mock