Skip to content

Commit 43a0d3a

Browse files
ParkreinerParkreiner
committed
fix: make testing boundaries more formal
1 parent 79c7ffd commit 43a0d3a

File tree

3 files changed

+91
-41
lines changed

3 files changed

+91
-41
lines changed

‎site/src/modules/resources/useAgentLogs.test.ts

Lines changed: 71 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import {
77
createMockWebSocket,
88
} from "testHelpers/websockets";
99
import { OneWayWebSocket } from "utils/OneWayWebSocket";
10-
import { createUseAgentLogs } from "./useAgentLogs";
10+
import { type OnError, createUseAgentLogs } from "./useAgentLogs";
1111

1212
const millisecondsInOneMinute = 60_000;
1313

@@ -30,9 +30,16 @@ function generateMockLogs(
3030
});
3131
}
3232

33-
// A mutable object holding the most recent mock WebSocket publisher. The inner
34-
// value will change as the hook opens/closes new connections
35-
type PublisherResult = { current: MockWebSocketPublisher };
33+
// A mutable object holding the most recent mock WebSocket publisher. Inner
34+
// value will be undefined if the hook is disabled on mount, but will otherwise
35+
// have some kind of value
36+
type PublisherResult = { current: MockWebSocketPublisher | undefined };
37+
38+
type MountHookOptions = Readonly<{
39+
initialAgentId: string;
40+
enabled?: boolean;
41+
onError?: OnError;
42+
}>;
3643

3744
type MountHookResult = Readonly<{
3845
rerender: (props: { agentId: string; enabled: boolean }) => void;
@@ -43,12 +50,10 @@ type MountHookResult = Readonly<{
4350
hookResult: { current: readonly WorkspaceAgentLog[] };
4451
}>;
4552

46-
function mountHook(initialAgentId: string): MountHookResult {
47-
// Have to cheat the types a little bit to avoid a chicken-and-the-egg
48-
// scenario. publisherResult will be initialized with an undefined current
49-
// value, but it'll be guaranteed not to be undefined by the time mountHook
50-
// returns its value.
51-
const publisherResult: Partial<PublisherResult> = { current: undefined };
53+
function mountHook(options: MountHookOptions): MountHookResult {
54+
const { initialAgentId, enabled = true, onError = jest.fn() } = options;
55+
56+
const publisherResult: PublisherResult = { current: undefined };
5257
const useAgentLogs = createUseAgentLogs((agentId, params) => {
5358
return new OneWayWebSocket({
5459
apiRoute: `/api/v2/workspaceagents/${agentId}/logs`,
@@ -62,29 +67,32 @@ function mountHook(initialAgentId: string): MountHookResult {
6267
return mockSocket;
6368
},
6469
});
65-
});
70+
}, onError);
6671

6772
const { result, rerender } = renderHook(
6873
({ agentId, enabled }) => useAgentLogs(agentId, enabled),
69-
{ initialProps: { agentId: initialAgentId, enabled: true } },
74+
{ initialProps: { agentId: initialAgentId, enabled: enabled } },
7075
);
7176

7277
return {
7378
rerender,
7479
hookResult: result,
75-
publisherResult: publisherResult as PublisherResult,
80+
publisherResult,
7681
};
7782
}
7883

7984
describe("useAgentLogs", () => {
8085
it("Automatically sorts logs that are received out of order", async () => {
81-
const { hookResult, publisherResult } = mountHook(MockWorkspaceAgent.id);
86+
const { hookResult, publisherResult } = mountHook({
87+
initialAgentId: MockWorkspaceAgent.id,
88+
});
89+
8290
const logs = generateMockLogs(10, new Date("september 9, 1999"));
8391
const reversed = logs.toReversed();
8492

8593
for (const log of reversed) {
8694
act(() => {
87-
publisherResult.current.publishMessage(
95+
publisherResult.current?.publishMessage(
8896
new MessageEvent<string>("message", {
8997
data: JSON.stringify([log]),
9098
}),
@@ -94,54 +102,86 @@ describe("useAgentLogs", () => {
94102
await waitFor(() => expect(hookResult.current).toEqual(logs));
95103
});
96104

105+
it("Never creates a connection if hook is disabled on mount", () => {
106+
const { publisherResult } = mountHook({
107+
initialAgentId: MockWorkspaceAgent.id,
108+
enabled: false,
109+
});
110+
111+
expect(publisherResult.current).toBe(undefined);
112+
});
113+
97114
it("Automatically closes the socket connection when the hook is disabled", async () => {
98-
const { publisherResult, rerender } = mountHook(MockWorkspaceAgent.id);
99-
expect(publisherResult.current.isConnectionOpen()).toBe(true);
115+
const { publisherResult, rerender } = mountHook({
116+
initialAgentId: MockWorkspaceAgent.id,
117+
});
118+
119+
expect(publisherResult.current?.isConnectionOpen()).toBe(true);
100120
rerender({ agentId: MockWorkspaceAgent.id, enabled: false });
101121
await waitFor(() => {
102-
expect(publisherResult.current.isConnectionOpen()).toBe(false);
122+
expect(publisherResult.current?.isConnectionOpen()).toBe(false);
103123
});
104124
});
105125

106126
it("Automatically closes the old connection when the agent ID changes", () => {
107-
const { publisherResult, rerender } = mountHook(MockWorkspaceAgent.id);
127+
const { publisherResult, rerender } = mountHook({
128+
initialAgentId: MockWorkspaceAgent.id,
129+
});
130+
108131
const publisher1 = publisherResult.current;
109-
expect(publisher1.isConnectionOpen()).toBe(true);
132+
expect(publisher1?.isConnectionOpen()).toBe(true);
110133

111134
const newAgentId = `${MockWorkspaceAgent.id}-2`;
112135
rerender({ agentId: newAgentId, enabled: true });
113136

114137
const publisher2 = publisherResult.current;
115-
expect(publisher1.isConnectionOpen()).toBe(false);
116-
expect(publisher2.isConnectionOpen()).toBe(true);
138+
expect(publisher1?.isConnectionOpen()).toBe(false);
139+
expect(publisher2?.isConnectionOpen()).toBe(true);
140+
});
141+
142+
it("Calls error callback when error is received (but only while hook is enabled)", async () => {
143+
const onError = jest.fn();
144+
const { publisherResult, rerender } = mountHook({
145+
initialAgentId: MockWorkspaceAgent.id,
146+
// Start off disabled so that we can check that the callback is
147+
// never called when there is no connection
148+
enabled: false,
149+
onError,
150+
});
151+
152+
const errorEvent = new Event("error");
153+
act(() => publisherResult.current?.publishError(errorEvent));
154+
expect(onError).not.toHaveBeenCalled();
155+
156+
rerender({ agentId: MockWorkspaceAgent.id, enabled: true });
157+
act(() => publisherResult.current?.publishError(errorEvent));
158+
expect(onError).toHaveBeenCalledTimes(1);
117159
});
118160

119161
it("Clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => {
120-
const { hookResult, publisherResult, rerender } = mountHook(
121-
MockWorkspaceAgent.id,
122-
);
162+
const { hookResult, publisherResult, rerender } = mountHook({
163+
initialAgentId: MockWorkspaceAgent.id,
164+
});
123165

124166
// Send initial logs so that we have something to clear out later
125167
const initialLogs = generateMockLogs(3, new Date("april 5, 1997"));
126168
const initialEvent = new MessageEvent<string>("message", {
127169
data: JSON.stringify(initialLogs),
128170
});
129-
act(() => publisherResult.current.publishMessage(initialEvent));
171+
act(() => publisherResult.current?.publishMessage(initialEvent));
130172
await waitFor(() => expect(hookResult.current).toEqual(initialLogs));
131173

132-
// Disable the hook (and have the hook close the connection behind the
133-
// scenes)
174+
// Disable the hook
134175
rerender({ agentId: MockWorkspaceAgent.id, enabled: false });
135176
await waitFor(() => expect(hookResult.current).toHaveLength(0));
136177

137-
// Re-enable the hook (creating an entirely new connection), and send
138-
// new logs
178+
// Re-enable the hook and send new logs
139179
rerender({ agentId: MockWorkspaceAgent.id, enabled: true });
140180
const newLogs = generateMockLogs(3, new Date("october 3, 2005"));
141181
const newEvent = new MessageEvent<string>("message", {
142182
data: JSON.stringify(newLogs),
143183
});
144-
act(() => publisherResult.current.publishMessage(newEvent));
184+
act(() => publisherResult.current?.publishMessage(newEvent));
145185
await waitFor(() => expect(hookResult.current).toEqual(newLogs));
146186
});
147187
});

‎site/src/modules/resources/useAgentLogs.ts

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ type CreateSocket = (
1212
params?: WatchWorkspaceAgentLogsParams,
1313
) => OneWayWebSocket<WorkspaceAgentLog[]>;
1414

15-
export function createUseAgentLogs(createSocket: CreateSocket) {
15+
export type OnError = (errorEvent: Event) => void;
16+
17+
export function createUseAgentLogs(
18+
createSocket: CreateSocket,
19+
onError: OnError,
20+
) {
1621
return function useAgentLogs(
1722
agentId: string,
1823
enabled: boolean,
@@ -70,20 +75,25 @@ export function createUseAgentLogs(createSocket: CreateSocket) {
7075
});
7176

7277
socket.addEventListener("error", (e) => {
73-
console.error("Error in agent log socket: ", e);
74-
displayError(
75-
"Unable to watch agent logs",
76-
"Please try refreshing the browser",
77-
);
78+
onError(e);
7879
socket.close();
7980
});
8081

8182
return () => socket.close();
82-
}, [createSocket, agentId, enabled]);
83+
}, [createSocket, onError, agentId, enabled]);
8384

8485
return logs;
8586
};
8687
}
8788

8889
// The baseline implementation to use for production
89-
export const useAgentLogs = createUseAgentLogs(watchWorkspaceAgentLogs);
90+
export const useAgentLogs = createUseAgentLogs(
91+
watchWorkspaceAgentLogs,
92+
(errorEvent) => {
93+
console.error("Error in agent log socket: ", errorEvent);
94+
displayError(
95+
"Unable to watch agent logs",
96+
"Please try refreshing the browser",
97+
);
98+
},
99+
);

‎site/src/testHelpers/websockets.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type { WebSocketEventType } from "utils/OneWayWebSocket";
22

33
export type MockWebSocketPublisher = Readonly<{
44
publishMessage: (event: MessageEvent<string>) => void;
5-
publishError: (event: ErrorEvent) => void;
5+
publishError: (event: Event) => void;
66
publishClose: (event: CloseEvent) => void;
77
publishOpen: (event: Event) => void;
88
isConnectionOpen: () => boolean;
@@ -14,7 +14,7 @@ export function createMockWebSocket(
1414
): readonly [WebSocket, MockWebSocketPublisher] {
1515
type EventMap = {
1616
message: MessageEvent<string>;
17-
error: ErrorEvent;
17+
error: Event;
1818
close: CloseEvent;
1919
open: Event;
2020
};

0 commit comments

Comments
 (0)