test: add unit tests for node-agent heartbeat loop (WOP-2133)#782
test: add unit tests for node-agent heartbeat loop (WOP-2133)#782
Conversation
Reviewer's GuideAdds a comprehensive vitest suite for the NodeAgent WebSocket heartbeat loop and reconnect/backoff behavior, along with a vitest config tweak to ensure the ws mock works when NodeAgent is imported from the published @wopr-network/platform-core package. Sequence diagram for NodeAgent heartbeat loop behavior under testsequenceDiagram
actor Tester
participant NodeAgent
participant DockerManager
participant WebSocket
participant Timer
Tester->>NodeAgent: new NodeAgent(config, dockerManager)
Tester->>NodeAgent: start()
NodeAgent->>DockerManager: initialize()
NodeAgent->>WebSocket: new WebSocket(platformUrl)
Note over WebSocket: open event triggers heartbeat
Tester->>WebSocket: emit("open")
WebSocket-->>NodeAgent: open
NodeAgent->>Timer: startHeartbeatInterval(heartbeatIntervalMs)
NodeAgent->>WebSocket: send(heartbeatPayload)
loop every heartbeatIntervalMs
Timer-->>NodeAgent: heartbeatTick
NodeAgent->>WebSocket: check readyState
alt readyState == OPEN
NodeAgent->>DockerManager: collectHeartbeat()
DockerManager-->>NodeAgent: heartbeatMetrics or error
NodeAgent->>WebSocket: send(heartbeatPayload)
else readyState != OPEN
NodeAgent-->>NodeAgent: skip send
end
end
Note over NodeAgent,WebSocket: collectHeartbeat error does not stop loop
Tester->>NodeAgent: stop()
NodeAgent->>Timer: clearHeartbeatInterval()
NodeAgent->>WebSocket: close()
Timer-->>NodeAgent: no further ticks
NodeAgent-->>Tester: stop() is idempotent
Sequence diagram for NodeAgent WebSocket reconnect and backoff behavior under testsequenceDiagram
actor Tester
participant NodeAgent
participant WebSocket as WebSocket_n
participant ReconnectTimer
Tester->>NodeAgent: start()
NodeAgent->>WebSocket: connect()
Tester->>WebSocket: emit("open")
WebSocket-->>NodeAgent: open
NodeAgent-->>NodeAgent: resetReconnectDelayTo1s()
loop repeated closes
Note over WebSocket,NodeAgent: WebSocket closes unexpectedly
Tester->>WebSocket: emit("close")
WebSocket-->>NodeAgent: close
NodeAgent->>ReconnectTimer: scheduleReconnect(currentDelayMs)
alt stop() was called
Tester->>NodeAgent: stop()
NodeAgent->>ReconnectTimer: cancelReconnect()
ReconnectTimer-->>NodeAgent: no reconnect
else stop() not called
ReconnectTimer-->>NodeAgent: reconnectDue
NodeAgent->>WebSocket: new WebSocket(platformUrl)
NodeAgent-->>NodeAgent: increaseDelayWithExponentialBackoff()
alt delayExceedsMax
NodeAgent-->>NodeAgent: capDelayAt30s()
end
Tester->>WebSocket: emit("open")
WebSocket-->>NodeAgent: open
NodeAgent-->>NodeAgent: resetReconnectDelayTo1s()
end
end
Note over NodeAgent,WebSocket: error events are logged/handled without crash
Class diagram for test doubles and collaborators in NodeAgent heartbeat testsclassDiagram
class SimpleEmitter {
-_listeners: Record
+on(event: string, fn: function) SimpleEmitter
+emit(event: string, args: unknown) boolean
}
class FakeWebSocket {
<<mock>>
+static OPEN: number
+static CLOSING: number
+static CLOSED: number
+static CONNECTING: number
+readyState: number
+send(): void
+close(): void
+constructor(url: string, opts: unknown)
}
class DockerManager {
<<external>>
+constructor(docker: object)
}
class NodeAgent {
<<external>>
+constructor(config: object, dockerManager: DockerManager)
+start(): Promise~void~
+stop(): void
}
class TestHelpers {
<<utility>>
+mockDockerode(): object
+validConfig(overrides: object): object
+getWsInstances(): Array
+resetWsInstances(): void
}
SimpleEmitter <|-- FakeWebSocket
TestHelpers ..> FakeWebSocket
TestHelpers ..> DockerManager
TestHelpers ..> NodeAgent
NodeAgent --> DockerManager
NodeAgent --> FakeWebSocket
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
beforeEach/afterEachblocks for the twodescribesuites are nearly identical (fake timers, system time,resetWsInstances,fetchstub); consider extracting a shared setup helper or higher-leveldescribeto reduce duplication and keep test configuration consistent. - In
FakeWebSocket,readyStateis initialized asOPEN(1); if the real client code depends on theCONNECTING → OPENtransition, you might want to start atCONNECTING(0) and rely only on the explicitopenevent to more closely mirror real WebSocket behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `beforeEach`/`afterEach` blocks for the two `describe` suites are nearly identical (fake timers, system time, `resetWsInstances`, `fetch` stub); consider extracting a shared setup helper or higher-level `describe` to reduce duplication and keep test configuration consistent.
- In `FakeWebSocket`, `readyState` is initialized as `OPEN` (1); if the real client code depends on the `CONNECTING → OPEN` transition, you might want to start at `CONNECTING` (0) and rely only on the explicit `open` event to more closely mirror real WebSocket behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds a focused test suite around the NodeAgent WebSocket heartbeat/reconnect behavior (from @wopr-network/platform-core) and adjusts Vitest configuration so vi.mock("ws") can reliably intercept that dependency during tests.
Changes:
- Configure Vitest to inline
@wopr-network/platform-coreso deep dependency mocks (notablyws) apply insideNodeAgent. - Add
src/node-agent/heartbeat.test.tscovering heartbeat scheduling, stop behavior, reconnect exponential backoff, and delay reset/capping.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| vitest.config.ts | Inlines @wopr-network/platform-core to allow mocking ws within platform-core’s NodeAgent during tests. |
| src/node-agent/heartbeat.test.ts | New unit tests validating NodeAgent heartbeat loop behavior and WebSocket reconnect backoff logic with extensive module mocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
vitest.config.ts
Outdated
| // Inline platform-core so that vi.mock("ws") intercepts it inside NodeAgent. | ||
| inline: ["@wopr-network/platform-core"], |
src/node-agent/heartbeat.test.ts
Outdated
| await agent.start(); | ||
| const ws = getWsInstances()[0]; | ||
| ws.emit("open"); | ||
| await vi.advanceTimersByTimeAsync(0); | ||
|
|
||
| expect(ws.send).toHaveBeenCalledTimes(1); | ||
|
|
||
| agent.stop(); | ||
| expect(ws.close).toHaveBeenCalled(); | ||
|
|
||
| await vi.advanceTimersByTimeAsync(15000); | ||
| expect(ws.send).toHaveBeenCalledTimes(1); |
| it("stop() is safe to call multiple times without throwing", async () => { | ||
| const config = validConfig(); | ||
| const { docker } = mockDockerode(); | ||
| const dockerManager = new DockerManager(docker as never); | ||
| const agent = new NodeAgent(config, dockerManager); | ||
|
|
||
| await agent.start(); | ||
| const ws = getWsInstances()[0]; | ||
| ws.emit("open"); | ||
| await vi.advanceTimersByTimeAsync(0); | ||
|
|
||
| expect(() => { | ||
| agent.stop(); | ||
| agent.stop(); | ||
| }).not.toThrow(); | ||
| expect(ws.close).toHaveBeenCalledTimes(1); | ||
| }); |
src/node-agent/heartbeat.test.ts
Outdated
| await agent.start(); | ||
| expect(getWsInstances()).toHaveLength(1); | ||
|
|
||
| agent.stop(); | ||
| getWsInstances()[0].emit("close"); | ||
|
|
||
| await vi.advanceTimersByTimeAsync(60_000); | ||
| expect(getWsInstances()).toHaveLength(1); |
Coverage Report
File CoverageNo changed files found. |
Greptile SummaryThis PR adds 11 unit tests for the Key issues found:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/node-agent/heartbeat.test.ts | New test file adding 11 unit tests for NodeAgent heartbeat loop and WebSocket reconnect backoff. Good overall coverage, but FakeWebSocket's SimpleEmitter is missing off/removeListener/once methods that the real ws.WebSocket inherits from EventEmitter, making tests brittle if NodeAgent uses those methods. One test also has a misleading comment on reconnect timing. |
| vitest.config.ts | Adds server.deps.inline: ["@wopr-network/platform-core"] globally, affecting all 170+ test files that import from platform-core. This introduces bundle overhead across the full test suite and creates a risk of unexpected mock interference and module identity changes in unrelated tests. |
Sequence Diagram
sequenceDiagram
participant T as Test
participant A as NodeAgent
participant FWS as FakeWebSocket
T->>A: new NodeAgent(config, dockerManager)
T->>A: agent.start()
A->>FWS: new WebSocket(url) [intercepted by vi.mock]
FWS-->>A: ws instance stored
T->>FWS: ws.emit("open")
A->>A: startHeartbeat() → collectHeartbeat()
A->>FWS: ws.send(JSON heartbeat) [immediate]
loop every heartbeatIntervalMs
A->>A: collectHeartbeat()
A->>FWS: ws.send(JSON heartbeat)
end
T->>FWS: ws.emit("close")
A->>A: stopHeartbeat() → clearInterval
A->>A: scheduleReconnect(delay)
Note over A: delay doubles on each close (1s→2s→4s…cap 30s)<br/>delay resets to 1s on successful open
A->>FWS: new WebSocket(url) [after delay]
T->>A: agent.stop()
A->>FWS: ws.close()
A->>A: stopped=true → no reconnect
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/node-agent/heartbeat.test.ts
Line: 13-27
Comment:
**`SimpleEmitter` missing `off`/`removeListener`/`once`**
`SimpleEmitter` only implements `on` and `emit`. The real `ws.WebSocket` extends Node's `EventEmitter`, which also exposes `removeListener`, `off`, `once`, and `removeAllListeners`. If `NodeAgent` calls any of these (e.g. to tear down listeners on close before reconnecting, or to register a one-shot `once("open", ...)` handler), the tests will throw a `TypeError` at runtime and the assertion results become unreliable.
For example, if `NodeAgent` does:
```ts
this.ws.once("open", () => { /* reset backoff */ });
```
that would immediately throw `TypeError: this.ws.once is not a function`, causing the tests to fail with an error rather than an assertion failure, making the root cause harder to diagnose.
Consider adding the missing `EventEmitter` methods to `SimpleEmitter`, or extending Node's built-in `EventEmitter` inside the `vi.hoisted` block:
```ts
class FakeWS {
static OPEN = 1;
static CLOSING = 2;
static CLOSED = 3;
static CONNECTING = 0;
readyState = 1;
send = vi.fn();
close = vi.fn();
private _emitter = new (require("node:events").EventEmitter)();
on(event: string, fn: (...args: unknown[]) => void) { this._emitter.on(event, fn); return this; }
once(event: string, fn: (...args: unknown[]) => void) { this._emitter.once(event, fn); return this; }
off(event: string, fn: (...args: unknown[]) => void) { this._emitter.off(event, fn); return this; }
removeListener(event: string, fn: (...args: unknown[]) => void) { this._emitter.removeListener(event, fn); return this; }
emit(event: string, ...args: unknown[]) { return this._emitter.emit(event, ...args); }
constructor(_url: string, _opts?: unknown) {
instances.push(this);
}
}
```
Note: `require()` is fine inside `vi.hoisted` because hoisted code runs in a CJS-compatible context before ESM module resolution.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: vitest.config.ts
Line: 6-11
Comment:
**Global `server.deps.inline` affects all 170+ test files**
`server.deps.inline` is a global Vitest setting — every test in the suite now bundles `@wopr-network/platform-core` through Vite. There are 170+ other test files in `src/` and `tests/` that already import from `@wopr-network/platform-core` (including `src/api/routes/fleet.test.ts`, `src/trpc/routers/billing.test.ts`, `src/fleet/services.test.ts`, and many others). Consequences:
1. **Build-time overhead**: All tests now incur Vite bundling for platform-core on every run, adding startup latency to the full suite.
2. **Mock bleed risk**: Any test file in the suite that also calls `vi.mock("ws")` — even for unrelated reasons — will now intercept `ws` inside platform-core, which may or may not be the intent. Currently no other test file mocks `ws`, but this creates a hidden dependency that is easy to break silently.
3. **Module identity changes**: Inlining causes Vite to re-process the package, which can change the module instance identity (class instances, singletons). Tests that check `instanceof` against a platform-core class may start failing unexpectedly.
Consider scoping this to only the heartbeat tests by extracting a `vitest.config.heartbeat.ts` for this file, or by using Vitest's `project` workspace isolation, so the side-effects are contained to the tests that require it.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/node-agent/heartbeat.test.ts
Line: 300-306
Comment:
**Misleading comment on reconnect timing**
The comment says "Reconnect fires 1s after close (total 6s from close)", but the reconnect timer starts immediately when `ws1.emit("close")` is called. With a 1 s initial backoff delay, the reconnect fires at **t+1000 ms from close** — which is already within the first `advanceTimersByTimeAsync(5000)` call. By the time the second advance runs, `getWsInstances()` already has length 2.
The assertion is correct, but the comment incorrectly implies the reconnect happens 6 s after close. This makes the test read as though it's verifying a 6 s delay, when it is actually verifying that the reconnect happened sometime during the preceding 5 s advance. Consider splitting the advance to precisely verify the 1 s boundary:
```ts
// At 999ms — reconnect not yet fired
await vi.advanceTimersByTimeAsync(999);
expect(ws1.send).toHaveBeenCalledTimes(1); // still cleared
expect(getWsInstances()).toHaveLength(1);
// At 1000ms — reconnect fires
await vi.advanceTimersByTimeAsync(1);
expect(getWsInstances()).toHaveLength(2);
```
This also makes the test self-documenting without needing the comment.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 6f23b4a
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| class SimpleEmitter { | ||
| private _listeners: Record<string, Array<(...args: unknown[]) => void>> = {}; | ||
|
|
||
| on(event: string, fn: (...args: unknown[]) => void) { | ||
| if (!this._listeners[event]) this._listeners[event] = []; | ||
| this._listeners[event].push(fn); | ||
| return this; | ||
| } | ||
|
|
||
| emit(event: string, ...args: unknown[]): boolean { | ||
| const fns = this._listeners[event] ?? []; | ||
| for (const fn of fns) fn(...args); | ||
| return fns.length > 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
SimpleEmitter missing off/removeListener/once
SimpleEmitter only implements on and emit. The real ws.WebSocket extends Node's EventEmitter, which also exposes removeListener, off, once, and removeAllListeners. If NodeAgent calls any of these (e.g. to tear down listeners on close before reconnecting, or to register a one-shot once("open", ...) handler), the tests will throw a TypeError at runtime and the assertion results become unreliable.
For example, if NodeAgent does:
this.ws.once("open", () => { /* reset backoff */ });that would immediately throw TypeError: this.ws.once is not a function, causing the tests to fail with an error rather than an assertion failure, making the root cause harder to diagnose.
Consider adding the missing EventEmitter methods to SimpleEmitter, or extending Node's built-in EventEmitter inside the vi.hoisted block:
class FakeWS {
static OPEN = 1;
static CLOSING = 2;
static CLOSED = 3;
static CONNECTING = 0;
readyState = 1;
send = vi.fn();
close = vi.fn();
private _emitter = new (require("node:events").EventEmitter)();
on(event: string, fn: (...args: unknown[]) => void) { this._emitter.on(event, fn); return this; }
once(event: string, fn: (...args: unknown[]) => void) { this._emitter.once(event, fn); return this; }
off(event: string, fn: (...args: unknown[]) => void) { this._emitter.off(event, fn); return this; }
removeListener(event: string, fn: (...args: unknown[]) => void) { this._emitter.removeListener(event, fn); return this; }
emit(event: string, ...args: unknown[]) { return this._emitter.emit(event, ...args); }
constructor(_url: string, _opts?: unknown) {
instances.push(this);
}
}Note: require() is fine inside vi.hoisted because hoisted code runs in a CJS-compatible context before ESM module resolution.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/node-agent/heartbeat.test.ts
Line: 13-27
Comment:
**`SimpleEmitter` missing `off`/`removeListener`/`once`**
`SimpleEmitter` only implements `on` and `emit`. The real `ws.WebSocket` extends Node's `EventEmitter`, which also exposes `removeListener`, `off`, `once`, and `removeAllListeners`. If `NodeAgent` calls any of these (e.g. to tear down listeners on close before reconnecting, or to register a one-shot `once("open", ...)` handler), the tests will throw a `TypeError` at runtime and the assertion results become unreliable.
For example, if `NodeAgent` does:
```ts
this.ws.once("open", () => { /* reset backoff */ });
```
that would immediately throw `TypeError: this.ws.once is not a function`, causing the tests to fail with an error rather than an assertion failure, making the root cause harder to diagnose.
Consider adding the missing `EventEmitter` methods to `SimpleEmitter`, or extending Node's built-in `EventEmitter` inside the `vi.hoisted` block:
```ts
class FakeWS {
static OPEN = 1;
static CLOSING = 2;
static CLOSED = 3;
static CONNECTING = 0;
readyState = 1;
send = vi.fn();
close = vi.fn();
private _emitter = new (require("node:events").EventEmitter)();
on(event: string, fn: (...args: unknown[]) => void) { this._emitter.on(event, fn); return this; }
once(event: string, fn: (...args: unknown[]) => void) { this._emitter.once(event, fn); return this; }
off(event: string, fn: (...args: unknown[]) => void) { this._emitter.off(event, fn); return this; }
removeListener(event: string, fn: (...args: unknown[]) => void) { this._emitter.removeListener(event, fn); return this; }
emit(event: string, ...args: unknown[]) { return this._emitter.emit(event, ...args); }
constructor(_url: string, _opts?: unknown) {
instances.push(this);
}
}
```
Note: `require()` is fine inside `vi.hoisted` because hoisted code runs in a CJS-compatible context before ESM module resolution.
How can I resolve this? If you propose a fix, please make it concise.
vitest.config.ts
Outdated
| server: { | ||
| deps: { | ||
| // Inline platform-core so that vi.mock("ws") intercepts it inside NodeAgent. | ||
| inline: ["@wopr-network/platform-core"], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Global server.deps.inline affects all 170+ test files
server.deps.inline is a global Vitest setting — every test in the suite now bundles @wopr-network/platform-core through Vite. There are 170+ other test files in src/ and tests/ that already import from @wopr-network/platform-core (including src/api/routes/fleet.test.ts, src/trpc/routers/billing.test.ts, src/fleet/services.test.ts, and many others). Consequences:
- Build-time overhead: All tests now incur Vite bundling for platform-core on every run, adding startup latency to the full suite.
- Mock bleed risk: Any test file in the suite that also calls
vi.mock("ws")— even for unrelated reasons — will now interceptwsinside platform-core, which may or may not be the intent. Currently no other test file mocksws, but this creates a hidden dependency that is easy to break silently. - Module identity changes: Inlining causes Vite to re-process the package, which can change the module instance identity (class instances, singletons). Tests that check
instanceofagainst a platform-core class may start failing unexpectedly.
Consider scoping this to only the heartbeat tests by extracting a vitest.config.heartbeat.ts for this file, or by using Vitest's project workspace isolation, so the side-effects are contained to the tests that require it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: vitest.config.ts
Line: 6-11
Comment:
**Global `server.deps.inline` affects all 170+ test files**
`server.deps.inline` is a global Vitest setting — every test in the suite now bundles `@wopr-network/platform-core` through Vite. There are 170+ other test files in `src/` and `tests/` that already import from `@wopr-network/platform-core` (including `src/api/routes/fleet.test.ts`, `src/trpc/routers/billing.test.ts`, `src/fleet/services.test.ts`, and many others). Consequences:
1. **Build-time overhead**: All tests now incur Vite bundling for platform-core on every run, adding startup latency to the full suite.
2. **Mock bleed risk**: Any test file in the suite that also calls `vi.mock("ws")` — even for unrelated reasons — will now intercept `ws` inside platform-core, which may or may not be the intent. Currently no other test file mocks `ws`, but this creates a hidden dependency that is easy to break silently.
3. **Module identity changes**: Inlining causes Vite to re-process the package, which can change the module instance identity (class instances, singletons). Tests that check `instanceof` against a platform-core class may start failing unexpectedly.
Consider scoping this to only the heartbeat tests by extracting a `vitest.config.heartbeat.ts` for this file, or by using Vitest's `project` workspace isolation, so the side-effects are contained to the tests that require it.
How can I resolve this? If you propose a fix, please make it concise.| // Advance past heartbeat interval — old timer cleared, no sends on ws1 | ||
| await vi.advanceTimersByTimeAsync(5000); | ||
| expect(ws1.send).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Reconnect fires 1s after close (total 6s from close) | ||
| await vi.advanceTimersByTimeAsync(1000); | ||
| expect(getWsInstances()).toHaveLength(2); |
There was a problem hiding this comment.
Misleading comment on reconnect timing
The comment says "Reconnect fires 1s after close (total 6s from close)", but the reconnect timer starts immediately when ws1.emit("close") is called. With a 1 s initial backoff delay, the reconnect fires at t+1000 ms from close — which is already within the first advanceTimersByTimeAsync(5000) call. By the time the second advance runs, getWsInstances() already has length 2.
The assertion is correct, but the comment incorrectly implies the reconnect happens 6 s after close. This makes the test read as though it's verifying a 6 s delay, when it is actually verifying that the reconnect happened sometime during the preceding 5 s advance. Consider splitting the advance to precisely verify the 1 s boundary:
// At 999ms — reconnect not yet fired
await vi.advanceTimersByTimeAsync(999);
expect(ws1.send).toHaveBeenCalledTimes(1); // still cleared
expect(getWsInstances()).toHaveLength(1);
// At 1000ms — reconnect fires
await vi.advanceTimersByTimeAsync(1);
expect(getWsInstances()).toHaveLength(2);This also makes the test self-documenting without needing the comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/node-agent/heartbeat.test.ts
Line: 300-306
Comment:
**Misleading comment on reconnect timing**
The comment says "Reconnect fires 1s after close (total 6s from close)", but the reconnect timer starts immediately when `ws1.emit("close")` is called. With a 1 s initial backoff delay, the reconnect fires at **t+1000 ms from close** — which is already within the first `advanceTimersByTimeAsync(5000)` call. By the time the second advance runs, `getWsInstances()` already has length 2.
The assertion is correct, but the comment incorrectly implies the reconnect happens 6 s after close. This makes the test read as though it's verifying a 6 s delay, when it is actually verifying that the reconnect happened sometime during the preceding 5 s advance. Consider splitting the advance to precisely verify the 1 s boundary:
```ts
// At 999ms — reconnect not yet fired
await vi.advanceTimersByTimeAsync(999);
expect(ws1.send).toHaveBeenCalledTimes(1); // still cleared
expect(getWsInstances()).toHaveLength(1);
// At 1000ms — reconnect fires
await vi.advanceTimersByTimeAsync(1);
expect(getWsInstances()).toHaveLength(2);
```
This also makes the test self-documenting without needing the comment.
How can I resolve this? If you propose a fix, please make it concise.
|
/improve |
|
wopr-auto review: ISSUES FOUND. Three issues to address before merge: 1. Missing try/finally in three tests (timer leak risk) Tests at lines 245-263, 265-281, and 439-453 call try {
await agent.start();
// assertions...
} finally {
agent.stop();
}2. SimpleEmitter missing
3.
Awaiting human approval after fixes. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
wopr-auto re-review: ISSUES FOUND. The three original issues (try/finally, SimpleEmitter once/off, global deps.inline) are all resolved in the current diff. One new issue remains: Heartbeat tests excluded from CI coverage run The CI Fix: add the heartbeat config to "test:coverage": "vitest run --coverage && vitest run --config vitest.config.heartbeat.ts"All other prior findings are resolved. Awaiting human approval after this fix. |
Cover heartbeat interval firing, error resilience, WebSocket reconnect
with exponential backoff (capped at 30s), delay reset on successful
connection, and clean shutdown. Add server.deps.inline for platform-core
so vi.mock("ws") intercepts within the published package.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add try/finally to stop() tests so cleanup runs even if assertions throw - Add once/off/removeListener to SimpleEmitter to prevent runtime errors - Scope server.deps.inline to vitest.config.heartbeat.ts (not global) - Exclude heartbeat test from vitest.config.ts; run via separate config - Update test script to run both configs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TSavo
force-pushed
the
agent/coder-2133/wop-2133-heartbeat-tests
branch
from
a90d7d2 to
fc1556d
Compare
|
✅ wopr-auto review: LGTM. No issues found. Awaiting human approval to enter merge queue. |
Summary
Closes WOP-2133
src/node-agent/heartbeat.test.tswith 11 unit tests covering theNodeAgentheartbeat loop and WebSocket reconnect behaviorvi.hoisted()+vi.mock("ws")with aFakeWebSocketclass (custom minimal event emitter + staticOPENconstant) to control WebSocket lifecycleserver.deps.inline: ["@wopr-network/platform-core"]tovitest.config.tssovi.mock("ws")intercepts inside the published packageTests cover:
openevent, then at configured intervalcollectHeartbeat(Docker daemon down) doesn't crash the loop — heartbeat continuesreadyState !== OPENstop()clears interval and closes WebSocketstop()is idempotent (safe to call multiple times)openstop()Test plan
npm run checkpasses (biome + tsc + raw-sql)npx vitest run src/node-agent/heartbeat.test.ts— 11/11 passGenerated with Claude Code
Summary by Sourcery
Add comprehensive unit tests for the NodeAgent heartbeat loop and WebSocket reconnect backoff behavior, and adjust Vitest config to support mocking WebSocket usage inside the platform-core package.
Build:
Tests:
Note
Add unit tests for node-agent heartbeat loop and WebSocket reconnect backoff
vi.hoistedto define aFakeWebSocketand intercept thewsmodule, alongside mocks fornode:os,node:fs/promises, and Docker viamockDockerode.@wopr-network/platform-coreso thatvi.mock('ws')correctly intercepts the WebSocket import within NodeAgent.npm testandnpm run test:coverageusing the heartbeat-specific config.Macroscope summarized fc1556d.