🧪 test: verify WebSocket invalid JSON handling - #16
Conversation
Added a test in `tests/server.test.js` to ensure the WebSocket server gracefully handles invalid JSON payloads without crashing and properly sends an error response. Updated `src/ws/server.js` `attachWebSocketServer` to return a `close` function that correctly terminates the interval for clean shutdown in tests. Co-authored-by: somyaknotfound <118343482+somyaknotfound@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe PR adds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds the first automated test for the project — an integration test verifying that the WebSocket server responds gracefully with { type: 'error', message: 'Invalid JSON' } when a client sends unparseable JSON. It also exposes a close() method from attachWebSocketServer to allow tests to shut down the WebSocket server and its heartbeat interval cleanly.
Changes:
- Added an integration test (
tests/server.test.js) usingnode:testthat starts an HTTP+WebSocket server, connects a client, sends invalid JSON, and asserts the error response. - Extended the return value of
attachWebSocketServerwith aclose()function that shuts down theWebSocketServer(which also clears the heartbeatsetInterval).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/server.test.js | New integration test for invalid JSON handling over WebSocket |
| src/ws/server.js | Added close function to the returned object for clean test teardown |
💡 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.
| test('WebSocket server handles invalid JSON gracefully', async (t) => { | ||
| // Dynamic import to ensure process.env.ARCJET_KEY is set first | ||
| const { attachWebSocketServer } = await import('../src/ws/server.js'); | ||
| const { wsArcjet } = await import('../src/arcjet.js'); |
There was a problem hiding this comment.
The wsArcjet import on this line is unused in the test — it's never referenced. This dead import should be removed to avoid confusion.
| const { wsArcjet } = await import('../src/arcjet.js'); |
| // Set ARCJET_KEY BEFORE importing anything else | ||
| process.env.ARCJET_KEY = 'test_key'; |
There was a problem hiding this comment.
Using a fake ARCJET_KEY value ('test_key') means wsArcjet in src/ws/server.js will be a real Arcjet instance. During the WebSocket upgrade, wsArcjet.protect(req) (server.js:92) will be called with this invalid key. If the Arcjet SDK makes a network call or validates the key, it will throw, causing the upgrade handler's catch block to return a 500 Internal Server Error and destroy the socket — the WebSocket connection will never be established, and the test will silently pass via timeout (with 0 messages) and then fail at assertions.
Consider either:
- Setting
process.env.ARCJET_MODE = 'DRY_RUN'and verifying that dry-run mode doesn't reject connections, or - Mocking the
wsArcjetmodule soprotect()returns an allowed decision, or - Setting
ARCJET_KEYto an empty/falsy value so thatwsArcjetisnull(but this would cause the throw on arcjet.js:6).
A more robust approach would be to restructure so Arcjet can be disabled or mocked in tests.
| // Set ARCJET_KEY BEFORE importing anything else | |
| process.env.ARCJET_KEY = 'test_key'; | |
| // Configure Arcjet for tests BEFORE importing anything else | |
| process.env.ARCJET_KEY = 'test_key'; | |
| process.env.ARCJET_MODE = 'DRY_RUN'; |
| // resolve gracefully to allow the assertions to run | ||
| resolve(); |
There was a problem hiding this comment.
If the expected messages are never received (e.g., due to a connection failure from the Arcjet issue), the 1-second timeout resolves the promise silently, and the test proceeds to assertions that will fail with a confusing error like "Should have received two messages" rather than indicating the real issue (e.g., the connection was never established). Consider rejecting on timeout with a descriptive error message instead of resolving, or at least adding a ws.on('open', ...) handler to verify the connection was actually established.
| // resolve gracefully to allow the assertions to run | |
| resolve(); | |
| // Reject on timeout so the test fails with a clear error | |
| reject(new Error('Timed out waiting for WebSocket welcome and error messages; the connection may not have been established or the server did not respond as expected.')); |
| import test from 'node:test'; | ||
| import assert from 'node:assert'; | ||
| import http from 'http'; | ||
| import { WebSocket } from 'ws'; | ||
|
|
||
| // Set ARCJET_KEY BEFORE importing anything else | ||
| process.env.ARCJET_KEY = 'test_key'; | ||
|
|
||
| test('WebSocket server handles invalid JSON gracefully', async (t) => { |
There was a problem hiding this comment.
The PR adds a test file but doesn't add a test script to package.json. Without a "test" script, there's no standard way to discover or run the tests (e.g., npm test will fail). Consider adding something like "test": "node --test tests/" to the scripts section in package.json.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ws/server.js`:
- Line 161: The current close() only calls wss.close() — change it to fully tear
down the attachment by removing the HTTP upgrade listener you registered (the
server.on('upgrade', ...) handler registered when attaching wss), explicitly
clearing the heartbeat interval you set up (the interval variable created around
the heartbeat code at line 151), and return a Promise that resolves when
wss.close() completes (i.e., wrap wss.close in a Promise or use its callback) so
callers get an async completion signal; update the exported close method (the
close property returned alongside broadcastMatchCreated and broadcastCommentary)
to perform removeListener/unregister, clearInterval, then await wss.close()
before resolving.
In `@tests/server.test.js`:
- Around line 15-58: The test currently awaits a Promise that may reject and
skips teardown (ws.close, wsServer.close, server.close), leaking resources; wrap
the await new Promise(...) in a try/finally block and move the teardown calls
(ws.close(), wsServer.close(), server.close()) into the finally so they always
run even if the promise rejects (referencing the local variables server,
wsServer, and ws and the anonymous Promise listener).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 670998f0-ca60-4ef4-bc98-e6ad9d72b718
📒 Files selected for processing (2)
src/ws/server.jstests/server.test.js
| } | ||
|
|
||
| return { broadcastMatchCreated, broadcastCommentary }; | ||
| return { broadcastMatchCreated, broadcastCommentary, close: () => wss.close() }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/ws/server.js | head -180Repository: somyaknotfound/SportsRealTime
Length of output: 6057
Make close() tear down the entire attachment, not just wss.
The HTTP upgrade handler registered at line 82 is not unregistered when close() is called. If the HTTP server persists after closing the WebSocket server, the upgrade listener remains attached and accumulates with each attach/detach cycle, causing resource leaks. Additionally, the heartbeat interval cleanup (line 151) is implicit via the wss close event, and close() does not expose an async completion signal, making shutdown non-deterministic.
Refactor close() to: unregister the upgrade handler, explicitly clear the interval, and return a Promise that waits for wss.close() to complete.
Proposed fix
export function attachWebSocketServer(server) {
const wss = new WebSocketServer({ noServer: true });
+ const onUpgrade = async (req, socket, head) => {
+ const { pathname } = new URL(req.url, `http://${req.headers.host}`);
+
+ if (pathname !== '/ws') {
+ socket.destroy();
+ return;
+ }
+
+ if (wsArcjet) {
+ try {
+ const decision = await wsArcjet.protect(req);
+
+ if (decision.isDenied()) {
+ if (decision.reason.isRateLimit()) {
+ socket.write('HTTP/1.1 429 Too Many Requests\r\n\r\n');
+ } else {
+ socket.write('HTTP/1.1 403 Forbidden\r\n\r\n');
+ }
+ socket.destroy();
+ return;
+ }
+ } catch (e) {
+ console.error('WS upgrade protection error:', e);
+ socket.write('HTTP/1.1 500 Internal Server Error\r\n\r\n');
+ socket.destroy();
+ return;
+ }
+ }
+
+ wss.handleUpgrade(req, socket, head, (ws) => {
+ wss.emit('connection', ws, req);
+ });
+ };
- server.on('upgrade', async (req, socket, head) => {
- const { pathname } = new URL(req.url, `http://${req.headers.host}`);
- ...
- });
+ server.on('upgrade', onUpgrade);
...
- return { broadcastMatchCreated, broadcastCommentary, close: () => wss.close() };
+ function close() {
+ server.off('upgrade', onUpgrade);
+ clearInterval(interval);
+
+ return new Promise((resolve, reject) => {
+ for (const client of wss.clients) {
+ client.terminate();
+ }
+
+ wss.close((err) => {
+ if (err) reject(err);
+ else resolve();
+ });
+ });
+ }
+
+ return { broadcastMatchCreated, broadcastCommentary, close };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ws/server.js` at line 161, The current close() only calls wss.close() —
change it to fully tear down the attachment by removing the HTTP upgrade
listener you registered (the server.on('upgrade', ...) handler registered when
attaching wss), explicitly clearing the heartbeat interval you set up (the
interval variable created around the heartbeat code at line 151), and return a
Promise that resolves when wss.close() completes (i.e., wrap wss.close in a
Promise or use its callback) so callers get an async completion signal; update
the exported close method (the close property returned alongside
broadcastMatchCreated and broadcastCommentary) to perform
removeListener/unregister, clearInterval, then await wss.close() before
resolving.
| const server = http.createServer(); | ||
| const wsServer = attachWebSocketServer(server); | ||
|
|
||
| await new Promise((resolve) => server.listen(0, resolve)); | ||
| const port = server.address().port; | ||
|
|
||
| const ws = new WebSocket(`ws://localhost:${port}/ws`); | ||
|
|
||
| // We'll collect messages received by the client | ||
| const messages = []; | ||
|
|
||
| await new Promise((resolve, reject) => { | ||
| const timeout = setTimeout(() => { | ||
| // resolve gracefully to allow the assertions to run | ||
| resolve(); | ||
| }, 1000); | ||
|
|
||
| ws.on('message', (data) => { | ||
| const msg = JSON.parse(data.toString()); | ||
| messages.push(msg); | ||
|
|
||
| // First message is expected to be 'welcome' | ||
| if (msg.type === 'welcome') { | ||
| // Send invalid JSON | ||
| ws.send('this is not valid json'); | ||
| } | ||
|
|
||
| // Wait for the error message | ||
| if (msg.type === 'error') { | ||
| clearTimeout(timeout); | ||
| resolve(); | ||
| } | ||
| }); | ||
|
|
||
| ws.on('error', (err) => { | ||
| clearTimeout(timeout); | ||
| reject(err); | ||
| }); | ||
| }); | ||
|
|
||
| // Teardown early so test completes cleanly even on assertion failure | ||
| ws.close(); | ||
| wsServer.close(); | ||
| server.close(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/server.test.js | sed -n '1,80p'Repository: somyaknotfound/SportsRealTime
Length of output: 2569
Register teardown on the failure path too.
If the promise at lines 26-53 rejects—which happens when the WebSocket emits an error—execution never reaches lines 55-58, leaking the HTTP server, the WebSocket server, and its heartbeat interval. Wrap the promise in try/finally to ensure cleanup runs regardless of success or failure.
Proposed fix
const ws = new WebSocket(`ws://localhost:${port}/ws`);
// We'll collect messages received by the client
const messages = [];
- await new Promise((resolve, reject) => {
+ try {
+ await new Promise((resolve, reject) => {
const timeout = setTimeout(() => {
// resolve gracefully to allow the assertions to run
resolve();
}, 1000);
ws.on('message', (data) => {
const msg = JSON.parse(data.toString());
messages.push(msg);
// First message is expected to be 'welcome'
if (msg.type === 'welcome') {
// Send invalid JSON
ws.send('this is not valid json');
}
// Wait for the error message
if (msg.type === 'error') {
clearTimeout(timeout);
resolve();
}
});
ws.on('error', (err) => {
clearTimeout(timeout);
reject(err);
});
- });
+ });
+ } finally {
+ ws.close();
+ wsServer.close();
+ server.close();
+ }
- // Teardown early so test completes cleanly even on assertion failure
- ws.close();
- wsServer.close();
- server.close();
// Verify
assert.strictEqual(messages.length, 2, 'Should have received two messages: welcome and error');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const server = http.createServer(); | |
| const wsServer = attachWebSocketServer(server); | |
| await new Promise((resolve) => server.listen(0, resolve)); | |
| const port = server.address().port; | |
| const ws = new WebSocket(`ws://localhost:${port}/ws`); | |
| // We'll collect messages received by the client | |
| const messages = []; | |
| await new Promise((resolve, reject) => { | |
| const timeout = setTimeout(() => { | |
| // resolve gracefully to allow the assertions to run | |
| resolve(); | |
| }, 1000); | |
| ws.on('message', (data) => { | |
| const msg = JSON.parse(data.toString()); | |
| messages.push(msg); | |
| // First message is expected to be 'welcome' | |
| if (msg.type === 'welcome') { | |
| // Send invalid JSON | |
| ws.send('this is not valid json'); | |
| } | |
| // Wait for the error message | |
| if (msg.type === 'error') { | |
| clearTimeout(timeout); | |
| resolve(); | |
| } | |
| }); | |
| ws.on('error', (err) => { | |
| clearTimeout(timeout); | |
| reject(err); | |
| }); | |
| }); | |
| // Teardown early so test completes cleanly even on assertion failure | |
| ws.close(); | |
| wsServer.close(); | |
| server.close(); | |
| const server = http.createServer(); | |
| const wsServer = attachWebSocketServer(server); | |
| await new Promise((resolve) => server.listen(0, resolve)); | |
| const port = server.address().port; | |
| const ws = new WebSocket(`ws://localhost:${port}/ws`); | |
| // We'll collect messages received by the client | |
| const messages = []; | |
| try { | |
| await new Promise((resolve, reject) => { | |
| const timeout = setTimeout(() => { | |
| // resolve gracefully to allow the assertions to run | |
| resolve(); | |
| }, 1000); | |
| ws.on('message', (data) => { | |
| const msg = JSON.parse(data.toString()); | |
| messages.push(msg); | |
| // First message is expected to be 'welcome' | |
| if (msg.type === 'welcome') { | |
| // Send invalid JSON | |
| ws.send('this is not valid json'); | |
| } | |
| // Wait for the error message | |
| if (msg.type === 'error') { | |
| clearTimeout(timeout); | |
| resolve(); | |
| } | |
| }); | |
| ws.on('error', (err) => { | |
| clearTimeout(timeout); | |
| reject(err); | |
| }); | |
| }); | |
| } finally { | |
| ws.close(); | |
| wsServer.close(); | |
| server.close(); | |
| } | |
| // Verify | |
| assert.strictEqual(messages.length, 2, 'Should have received two messages: welcome and error'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/server.test.js` around lines 15 - 58, The test currently awaits a
Promise that may reject and skips teardown (ws.close, wsServer.close,
server.close), leaking resources; wrap the await new Promise(...) in a
try/finally block and move the teardown calls (ws.close(), wsServer.close(),
server.close()) into the finally so they always run even if the promise rejects
(referencing the local variables server, wsServer, and ws and the anonymous
Promise listener).
🎯 What: The testing gap addressed
Tested the WebSocket
handleMessageblock handling invalid JSON formatting (src/ws/server.js:58).📊 Coverage: What scenarios are now tested
When a user connects to the WS and sends an unparseable invalid string (e.g.,
'this is not valid json'), the server responds with{ type: 'error', message: 'Invalid JSON' }gracefully rather than throwing an unhandled exception or crashing.✨ Result: The improvement in test coverage
Introduced an automated integration test using
node:testthat starts the WS and HTTP server, tests the incoming message logic, ensures expected output, and then cleanly tears down the test environment. Additionally added aclose()function to the return value ofattachWebSocketServerto allow tests to clear intervals and prevent hanging processes.PR created automatically by Jules for task 6214618374082510163 started by @somyaknotfound
Summary by CodeRabbit
New Features
Tests