From 84444f7c7efc01a367eea680d37c1657bde56bd1 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Wed, 15 Apr 2026 21:50:17 +0200 Subject: [PATCH] =?utf8?q?refactor(cli):=20complete=20remaining=20audit=20?= =?utf8?q?items=20=E2=80=94=20validate=20status,=20extract=20helpers,=20ad?= =?utf8?q?d=208=20test=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit - Fix #28: Validate ResponsePayload status is string before cast in handleMessage() - Fix #43: Extract settleHandler() private method to reduce duplication - Fix #41: Normalize createMockWS → createMockWs for consistency - Fix #39: Extract captureStream() parameterized helper to replace captureStdout/captureStderr - Fix #47: Filter empty arrays in displayGenericPayload to avoid spurious output - Test #21-#22: Add ConnectionError edge cases (empty cause, non-Error cause) - Test #23: Add ServerFailureError without hashIdsFailed - Test #24: Add post-connect onerror rejection test - Test #25: Add displayGenericPayload failure path test - Test #26: Add NaN and Infinity timeout validation tests All tests pass, zero lint warnings. --- ui/cli/src/output/table.ts | 7 +- ui/cli/tests/lifecycle.test.ts | 11 +++ ui/cli/tests/output.test.ts | 65 +++++++------- ui/common/src/client/WebSocketClient.ts | 14 +-- ui/common/tests/WebSocketClient.test.ts | 110 ++++++++++++++++++++---- 5 files changed, 148 insertions(+), 59 deletions(-) diff --git a/ui/cli/src/output/table.ts b/ui/cli/src/output/table.ts index 4ba1f353..ec971d7e 100644 --- a/ui/cli/src/output/table.ts +++ b/ui/cli/src/output/table.ts @@ -42,8 +42,11 @@ export const outputTable = (payload: ResponsePayload): void => { const displayGenericPayload = (payload: ResponsePayload): void => { const { status, ...rest } = payload - if (Object.keys(rest).length > 0) { - process.stdout.write(JSON.stringify(rest, null, 2) + '\n') + const meaningful = Object.fromEntries( + Object.entries(rest).filter(([, v]) => v != null && !(Array.isArray(v) && v.length === 0)) + ) + if (Object.keys(meaningful).length > 0) { + process.stdout.write(JSON.stringify(meaningful, null, 2) + '\n') } else if (status === ResponseStatus.SUCCESS) { process.stdout.write(chalk.green('✓ Success\n')) } else { diff --git a/ui/cli/tests/lifecycle.test.ts b/ui/cli/tests/lifecycle.test.ts index b8a1e740..a8ca7aa4 100644 --- a/ui/cli/tests/lifecycle.test.ts +++ b/ui/cli/tests/lifecycle.test.ts @@ -20,6 +20,17 @@ await describe('lifecycle', async () => { assert.strictEqual(err.cause, cause) }) + await it('should create ConnectionError without cause suffix when cause message is empty', () => { + const err = new ConnectionError('ws://localhost:8080', new Error('')) + assert.strictEqual(err.message, 'Failed to connect to ws://localhost:8080') + }) + + await it('should create ConnectionError without cause suffix when cause is not an Error', () => { + const err = new ConnectionError('ws://localhost:8080', 'string cause') + assert.strictEqual(err.message, 'Failed to connect to ws://localhost:8080') + assert.strictEqual(err.cause, 'string cause') + }) + await it('should export executeCommand function', () => { assert.strictEqual(typeof executeCommand, 'function') }) diff --git a/ui/cli/tests/output.test.ts b/ui/cli/tests/output.test.ts index ef246092..0b7cf253 100644 --- a/ui/cli/tests/output.test.ts +++ b/ui/cli/tests/output.test.ts @@ -9,32 +9,18 @@ import { printError } from '../src/output/human.js' import { outputJson, outputJsonError } from '../src/output/json.js' import { outputTable } from '../src/output/table.js' -const captureStdout = (fn: () => void): string => { +const captureStream = (stream: 'stderr' | 'stdout', fn: () => void): string => { + const target = stream === 'stdout' ? process.stdout : process.stderr const chunks: string[] = [] - const original = process.stdout.write.bind(process.stdout) - process.stdout.write = ((chunk: string): boolean => { + const original = target.write.bind(target) + target.write = ((chunk: string): boolean => { chunks.push(chunk) return true - }) as typeof process.stdout.write + }) as typeof target.write try { fn() } finally { - process.stdout.write = original - } - return chunks.join('') -} - -const captureStderr = (fn: () => void): string => { - const chunks: string[] = [] - const original = process.stderr.write.bind(process.stderr) - process.stderr.write = ((chunk: string): boolean => { - chunks.push(chunk) - return true - }) as typeof process.stderr.write - try { - fn() - } finally { - process.stderr.write = original + target.write = original } return chunks.join('') } @@ -57,7 +43,7 @@ await describe('output formatters', async () => { hashIdsSucceeded: ['cs-001', 'cs-002'], status: ResponseStatus.SUCCESS, } - const output = captureStdout(() => { + const output = captureStream('stdout', () => { outputJson(payload) }) const parsed = JSON.parse(output) as typeof payload @@ -67,7 +53,7 @@ await describe('output formatters', async () => { await it('should write valid JSON to stdout for failure payload', () => { const payload = { status: ResponseStatus.FAILURE } - const output = captureStdout(() => { + const output = captureStream('stdout', () => { outputJson(payload) }) const parsed = JSON.parse(output) as typeof payload @@ -75,7 +61,7 @@ await describe('output formatters', async () => { }) await it('should write error JSON to stdout', () => { - const output = captureStdout(() => { + const output = captureStream('stdout', () => { outputJsonError(new Error('test error')) }) const parsed = JSON.parse(output) as { error: boolean; message: string; status: string } @@ -85,7 +71,7 @@ await describe('output formatters', async () => { }) await it('should handle non-Error objects in JSON error output', () => { - const output = captureStdout(() => { + const output = captureStream('stdout', () => { outputJsonError('string error') }) const parsed = JSON.parse(output) as { message: string } @@ -97,7 +83,7 @@ await describe('output formatters', async () => { hashIdsSucceeded: ['cs-001'], status: ResponseStatus.SUCCESS, } - const output = captureStdout(() => { + const output = captureStream('stdout', () => { outputTable(payload) }) assert.ok(output.includes('cs-001')) @@ -105,7 +91,7 @@ await describe('output formatters', async () => { await it('should display generic payload when no hash IDs present', () => { const payload = { state: { version: '1.0' }, status: ResponseStatus.SUCCESS } - const output = captureStdout(() => { + const output = captureStream('stdout', () => { outputTable(payload) }) assert.ok(output.includes('version')) @@ -113,14 +99,14 @@ await describe('output formatters', async () => { await it('should write generic success when no hash IDs in table mode', () => { const payload = { status: ResponseStatus.SUCCESS } - const output = captureStdout(() => { + const output = captureStream('stdout', () => { outputTable(payload) }) assert.ok(output.includes('Success')) }) await it('should write error message via printError', () => { - const output = captureStderr(() => { + const output = captureStream('stderr', () => { printError('oops') }) assert.ok(output.includes('oops')) @@ -132,7 +118,7 @@ await describe('output formatters', async () => { hashIdsSucceeded: ['cs-100'], status: ResponseStatus.SUCCESS, } - const output = captureStdout(() => { + const output = captureStream('stdout', () => { formatter.output(payload) }) const parsed = JSON.parse(output) as typeof payload @@ -145,7 +131,7 @@ await describe('output formatters', async () => { hashIdsSucceeded: ['cs-200'], status: ResponseStatus.SUCCESS, } - const output = captureStdout(() => { + const output = captureStream('stdout', () => { formatter.output(payload) }) assert.ok(output.includes('cs-200')) @@ -153,7 +139,7 @@ await describe('output formatters', async () => { await it('should handle error with JSON formatter', () => { const formatter = createFormatter(true) - const output = captureStdout(() => { + const output = captureStream('stdout', () => { formatter.error(new Error('json err')) }) const parsed = JSON.parse(output) as { message: string } @@ -162,7 +148,7 @@ await describe('output formatters', async () => { await it('should handle error with table formatter', () => { const formatter = createFormatter(false) - const output = captureStderr(() => { + const output = captureStream('stderr', () => { formatter.error(new Error('table err')) }) assert.ok(output.includes('table err')) @@ -181,7 +167,7 @@ await describe('output formatters', async () => { ], status: ResponseStatus.FAILURE, } - const output = captureStderr(() => { + const output = captureStream('stderr', () => { outputTable(payload) }) assert.ok(output.includes('Station not found')) @@ -201,7 +187,7 @@ await describe('output formatters', async () => { ], status: ResponseStatus.FAILURE, } - const output = captureStderr(() => { + const output = captureStream('stderr', () => { outputTable(payload) }) assert.ok(output.includes('Unknown error')) @@ -221,7 +207,7 @@ await describe('output formatters', async () => { ], status: ResponseStatus.FAILURE, } - const output = captureStderr(() => { + const output = captureStream('stderr', () => { outputTable(payload) }) assert.ok(output.includes('(unknown)')) @@ -233,10 +219,17 @@ await describe('output formatters', async () => { hashIdsFailed: ['cs-001'], status: ResponseStatus.FAILURE, } - const output = captureStderr(() => { + const output = captureStream('stderr', () => { outputTable(payload) }) assert.ok(output.includes('cs-001')) assert.ok(!output.includes('Error')) }) + + await it('should display generic failure status on stderr', () => { + const output = captureStream('stderr', () => { + outputTable({ status: ResponseStatus.FAILURE }) + }) + assert.ok(output.includes('failure')) + }) }) diff --git a/ui/common/src/client/WebSocketClient.ts b/ui/common/src/client/WebSocketClient.ts index 188ef798..44e18201 100644 --- a/ui/common/src/client/WebSocketClient.ts +++ b/ui/common/src/client/WebSocketClient.ts @@ -170,15 +170,14 @@ export class WebSocketClient { if ( responsePayload == null || typeof responsePayload !== 'object' || - !('status' in responsePayload) + !('status' in responsePayload) || + typeof (responsePayload as { status: unknown }).status !== 'string' ) { - clearTimeout(handler.timeoutId) - this.responseHandlers.delete(uuid) + this.settleHandler(uuid, handler) handler.reject(new Error('Server sent malformed response payload')) return } - clearTimeout(handler.timeoutId) - this.responseHandlers.delete(uuid) + this.settleHandler(uuid, handler) const payload = responsePayload as ResponsePayload if (payload.status === ResponseStatus.SUCCESS) { handler.resolve(payload) @@ -186,4 +185,9 @@ export class WebSocketClient { handler.reject(new ServerFailureError(payload)) } } + + private settleHandler (uuid: UUIDv4, handler: ResponseHandler): void { + clearTimeout(handler.timeoutId) + this.responseHandlers.delete(uuid) + } } diff --git a/ui/common/tests/WebSocketClient.test.ts b/ui/common/tests/WebSocketClient.test.ts index 24065011..3d41944a 100644 --- a/ui/common/tests/WebSocketClient.test.ts +++ b/ui/common/tests/WebSocketClient.test.ts @@ -12,7 +12,7 @@ import { AuthenticationType, ProcedureName, ResponseStatus } from '../src/types/ /** * @returns Mock WebSocket with trigger methods for testing. */ -function createMockWS (): WebSocketLike & { +function createMockWs (): WebSocketLike & { sentMessages: string[] triggerClose: () => void triggerError: (message: string) => void @@ -80,7 +80,7 @@ function createMockWS (): WebSocketLike & { await describe('WebSocketClient', async () => { await it('should connect successfully', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -94,7 +94,7 @@ await describe('WebSocketClient', async () => { }) await it('should build protocol-basic-auth credentials correctly', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() let capturedProtocols: string | string[] = '' const factory: WebSocketFactory = (_url, protocols) => { capturedProtocols = protocols @@ -121,7 +121,7 @@ await describe('WebSocketClient', async () => { }) await it('should send SRPC formatted request', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -149,7 +149,7 @@ await describe('WebSocketClient', async () => { }) await it('should correlate responses by UUID', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -179,7 +179,7 @@ await describe('WebSocketClient', async () => { }) await it('should reject with ServerFailureError containing the payload', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -215,8 +215,15 @@ await describe('WebSocketClient', async () => { ) }) + await it('should create ServerFailureError without station count when hashIdsFailed is absent', () => { + const payload = { status: ResponseStatus.FAILURE } as ResponsePayload + const err = new ServerFailureError(payload) + assert.strictEqual(err.message, 'Server returned failure status') + assert.strictEqual(err.payload, payload) + }) + await it('should handle connection errors', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -235,7 +242,7 @@ await describe('WebSocketClient', async () => { }) await it('should reject pending requests on disconnect', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -255,7 +262,7 @@ await describe('WebSocketClient', async () => { }) await it('should reject request when WebSocket is not open', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -277,7 +284,7 @@ await describe('WebSocketClient', async () => { }) await it('should build wss URL when secure is true', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() let capturedUrl = '' const factory: WebSocketFactory = url => { capturedUrl = url @@ -297,7 +304,7 @@ await describe('WebSocketClient', async () => { }) await it('should ignore malformed messages', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -316,7 +323,7 @@ await describe('WebSocketClient', async () => { }) await it('should reject on malformed response payload with matching UUID', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -338,7 +345,7 @@ await describe('WebSocketClient', async () => { }) await it('should reject connect if socket closes before open', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -358,7 +365,7 @@ await describe('WebSocketClient', async () => { }) await it('should respect explicit short timeout on sendRequest', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const factory: WebSocketFactory = () => mockWs const client = new WebSocketClient(factory, { host: 'localhost', @@ -391,7 +398,7 @@ await describe('WebSocketClient', async () => { }) await it('should reject sendRequest with timeoutMs = 0', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const client = new WebSocketClient( () => mockWs, { @@ -420,7 +427,7 @@ await describe('WebSocketClient', async () => { }) await it('should reject sendRequest with timeoutMs = -1', async () => { - const mockWs = createMockWS() + const mockWs = createMockWs() const client = new WebSocketClient( () => mockWs, { @@ -447,4 +454,75 @@ await describe('WebSocketClient', async () => { } ) }) + + await it('should reject sendRequest with NaN timeout', async () => { + const mockWs = createMockWs() + const factory: WebSocketFactory = () => mockWs + const client = new WebSocketClient(factory, { + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }) + setTimeout(() => { + mockWs.triggerOpen() + }, 0) + await client.connect() + await assert.rejects( + client.sendRequest(ProcedureName.LIST_CHARGING_STATIONS, {}, Number.NaN), + (error: Error) => { + assert.ok(error.message.includes('Invalid timeout')) + assert.ok(error.message.includes('NaN')) + return true + } + ) + client.disconnect() + }) + + await it('should reject sendRequest with Infinity timeout', async () => { + const mockWs = createMockWs() + const factory: WebSocketFactory = () => mockWs + const client = new WebSocketClient(factory, { + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }) + setTimeout(() => { + mockWs.triggerOpen() + }, 0) + await client.connect() + await assert.rejects( + client.sendRequest(ProcedureName.LIST_CHARGING_STATIONS, {}, Number.POSITIVE_INFINITY), + (error: Error) => { + assert.ok(error.message.includes('Invalid timeout')) + assert.ok(error.message.includes('Infinity')) + return true + } + ) + client.disconnect() + }) + + await it('should reject pending requests when post-connect error occurs', async () => { + const mockWs = createMockWs() + const factory: WebSocketFactory = () => mockWs + const client = new WebSocketClient(factory, { + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }) + setTimeout(() => { + mockWs.triggerOpen() + }, 0) + await client.connect() + + const requestPromise = client.sendRequest(ProcedureName.LIST_CHARGING_STATIONS, {}) + mockWs.triggerError('connection lost') + + await assert.rejects(requestPromise, (error: Error) => { + assert.ok(error.message.includes('connection lost')) + return true + }) + }) }) -- 2.43.0