Achieve 85%+ test coverage target with 57 new tests #489
Conversation
Added comprehensive test coverage for WebSocketRequest and utils.js to reach the 85%+ overall coverage target. Changes: - Added test/unit/core/request-cookies-origin.test.mjs (40 tests) - Cookie validation (name, value, path, domain, expires, maxage) - Cookie sanitization behavior - Secure and HttpOnly flags - Origin headers for WebSocket v8/v13 - Protocol validation - Added test/unit/core/utils-additional.test.mjs (17 tests) - noop, extend, eventEmitterListenerCount - bufferAllocUnsafe, bufferFromString with multiple encodings - BufferingLogger functionality - Updated TEST_SUITE_MODERNIZATION_PLAN.md - Overall progress: 80% → 85% complete - Test count: 559 → 616 passing tests - Coverage metrics updated to reflect achievement - Updated CLAUDE.md with script cheat sheet - Comprehensive testing command reference - Quick reference for common workflows Coverage improvements: - Overall: 79.99% → 85.05% (+5.06%) ✅ - WebSocketRequest: 69.78% → 90.24% (+20.46%) - WebSocketConnection: 78.40% → 80.57% (+2.17%) - utils.js: 33.84% → 73.84% (+40%) - Branch: 83.52% → 84.72% - Functions: 78.71% → 81.95% All 616 unit tests passing ✅ All 517 Autobahn protocol tests passing ✅ Zero lint errors ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary of ChangesHello @theturtle32, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's test suite by achieving the targeted 85%+ overall code coverage. It introduces 57 new, comprehensive tests focusing on the WebSocketRequest component's cookie and origin handling, as well as various utility functions in utils.js. These additions bolster the robustness and reliability of core WebSocket functionalities, ensuring better stability and adherence to protocol standards. The accompanying documentation has also been updated to reflect this progress and provide clearer guidance on testing workflows. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the project's test coverage, successfully achieving the 85% target. The 57 new tests for WebSocketRequest and utils.js are comprehensive and well-written, covering many edge cases and error conditions. The updates to the documentation in CLAUDE.md and TEST_SUITE_MODERNIZATION_PLAN.md are also clear and provide great visibility into the project's progress.
I have a few suggestions to further improve the quality and robustness of the new tests, primarily related to using vitest features for parameterized tests, ensuring test environment cleanup, and maintaining code consistency. Overall, this is a fantastic contribution to the project's health and maintainability.
| it('should create a BufferingLogger when debug is enabled', () => { | ||
| // Enable debug for this test | ||
| const originalDebug = process.env.DEBUG; | ||
| process.env.DEBUG = 'test:enabled:*'; | ||
|
|
||
| // Force require cache clear for debug module | ||
| const debugModule = require('debug'); | ||
| debugModule.enable('test:enabled:*'); | ||
|
|
||
| const logger = utils.BufferingLogger('test:enabled:logger', 'id456'); | ||
|
|
||
| expect(typeof logger).toBe('function'); | ||
| expect(typeof logger.printOutput).toBe('function'); | ||
|
|
||
| // Test logging functionality | ||
| logger('Test message', 'arg1', 'arg2'); | ||
|
|
||
| // The logger should buffer messages | ||
| expect(typeof logger.printOutput).toBe('function'); | ||
|
|
||
| // Restore original DEBUG setting | ||
| if (originalDebug) { | ||
| process.env.DEBUG = originalDebug; | ||
| debugModule.enable(originalDebug); | ||
| } else { | ||
| delete process.env.DEBUG; | ||
| debugModule.disable(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This test modifies process.env.DEBUG to test the BufferingLogger. If an assertion within the test fails, the cleanup code at the end will not be executed. This can cause side effects and make other tests in the suite flaky. To ensure the environment is always restored, please wrap the test logic in a try...finally block.
it('should create a BufferingLogger when debug is enabled', () => {
const originalDebug = process.env.DEBUG;
const debugModule = require('debug');
try {
process.env.DEBUG = 'test:enabled:*';
debugModule.enable('test:enabled:*');
const logger = utils.BufferingLogger('test:enabled:logger', 'id456');
expect(typeof logger).toBe('function');
expect(typeof logger.printOutput).toBe('function');
// Test logging functionality
logger('Test message', 'arg1', 'arg2');
// The logger should buffer messages
expect(typeof logger.printOutput).toBe('function');
} finally {
// Restore original DEBUG setting
if (originalDebug) {
process.env.DEBUG = originalDebug;
debugModule.enable(originalDebug);
} else {
delete process.env.DEBUG;
debugModule.disable();
}
}
});| it('should reject protocol with separator character', () => { | ||
| const separators = ['(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']', '?', '=', '{', '}']; | ||
|
|
||
| separators.forEach(sep => { | ||
| const newSocket = new MockSocket(); | ||
| newSocket.remoteAddress = '127.0.0.1'; | ||
|
|
||
| const newHttpRequest = { | ||
| url: '/', | ||
| headers: { | ||
| 'host': 'localhost', | ||
| 'upgrade': 'websocket', | ||
| 'connection': 'Upgrade', | ||
| 'sec-websocket-version': '13', | ||
| 'sec-websocket-key': 'dGhlIHNhbXBsZSBub25jZQ==', | ||
| 'sec-websocket-protocol': `test${sep}protocol` | ||
| } | ||
| }; | ||
|
|
||
| const newRequest = new WebSocketRequest(newSocket, newHttpRequest, { maxReceivedFrameSize: 0x10000 }); | ||
| newRequest.readHandshake(); | ||
|
|
||
| expect(() => { | ||
| newRequest.accept(`test${sep}protocol`, null, null); | ||
| }).toThrow(/Illegal character.* in subprotocol/); | ||
|
|
||
| newSocket.removeAllListeners(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test iterates through a list of separators within a single it block. If one of the separators causes a test failure, it can be difficult to identify which one failed. Consider refactoring this to use it.each to create a separate test case for each separator. This improves test reporting and makes debugging easier.
it.each(['(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']', '?', '=', '{', '}'])('should reject protocol with separator character: %s', (sep) => {
const newSocket = new MockSocket();
newSocket.remoteAddress = '127.0.0.1';
const newHttpRequest = {
url: '/',
headers: {
'host': 'localhost',
'upgrade': 'websocket',
'connection': 'Upgrade',
'sec-websocket-version': '13',
'sec-websocket-key': 'dGhlIHNhbXBsZSBub25jZQ==',
'sec-websocket-protocol': `test${sep}protocol`
}
};
const newRequest = new WebSocketRequest(newSocket, newHttpRequest, { maxReceivedFrameSize: 0x10000 });
newRequest.readHandshake();
expect(() => {
newRequest.accept(`test${sep}protocol`, null, null);
}).toThrow(/Illegal character.* in subprotocol/);
newSocket.removeAllListeners();
});| describe('eventEmitterListenerCount', () => { | ||
| it('should return listener count for an event', () => { | ||
| const EventEmitter = require('events').EventEmitter; | ||
| const emitter = new EventEmitter(); | ||
|
|
||
| const listener1 = () => {}; | ||
| const listener2 = () => {}; | ||
|
|
||
| emitter.on('test', listener1); | ||
| emitter.on('test', listener2); | ||
|
|
||
| const count = utils.eventEmitterListenerCount(emitter, 'test'); | ||
| expect(count).toBe(2); | ||
| }); | ||
|
|
||
| it('should return 0 for event with no listeners', () => { | ||
| const EventEmitter = require('events').EventEmitter; | ||
| const emitter = new EventEmitter(); | ||
|
|
||
| const count = utils.eventEmitterListenerCount(emitter, 'nonexistent'); | ||
| expect(count).toBe(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The EventEmitter is being required twice inside this describe block (lines 47 and 61). To improve code clarity and adhere to ES module conventions used in this project, it's better to import it once at the top of the file.
Here's how you could refactor it:
// At the top of the file (e.g. line 7)
import { EventEmitter } from 'events';
// ... later in the file
describe('eventEmitterListenerCount', () => {
it('should return listener count for an event', () => {
const emitter = new EventEmitter();
// ...
});
it('should return 0 for event with no listeners', () => {
const emitter = new EventEmitter();
// ...
});
});Fixed all high and medium priority issues from the code review: 1. HIGH PRIORITY: Added try-finally block to BufferingLogger test - Ensures process.env.DEBUG is always restored even if test fails - Prevents flaky tests and side effects on other tests 2. MEDIUM PRIORITY: Refactored separator character test to use it.each - Changed from forEach loop to it.each for better test reporting - Now creates 17 individual test cases instead of 1 combined test - Makes it easier to identify which separator character fails - Increased test count from 616 to 632 total tests 3. MEDIUM PRIORITY: Fixed EventEmitter imports - Moved EventEmitter import to top of file - Removed duplicate require() calls - Better code organization and ES module conventions All 632 tests passing ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
Successfully achieved the 85%+ overall coverage target by adding comprehensive tests for WebSocketRequest and utils.js components.
Changes
New Test Files
test/unit/core/request-cookies-origin.test.mjs (40 tests)
test/unit/core/utils-additional.test.mjs (17 tests)
Updated Files
TEST_SUITE_MODERNIZATION_PLAN.md
CLAUDE.md
Coverage Improvements
Test Results
Test Plan
All existing tests continue to pass:
🤖 Generated with Claude Code