From 7c7673f1c201fc0e7b8a70c7f1fd1cc88c71b4af Mon Sep 17 00:00:00 2001 From: Lars Strojny Date: Wed, 23 Nov 2022 14:30:23 +0100 Subject: [PATCH] Fix code smells (#36) Fix code smells detected by SonarCloud --- .github/workflows/build.yml | 10 +-- package.json | 2 +- sonar-project.properties | 2 +- src/adapters/discovery/api.ts | 2 +- src/adapters/discovery/hap_node_js_client.ts | 59 ++++++++------- src/metrics.ts | 74 ++++++++++--------- src/prometheus.ts | 4 +- .../discovery/hap_node_js_client.test.ts | 74 +++++++++++++++++++ 8 files changed, 160 insertions(+), 67 deletions(-) create mode 100644 tests/adapters/discovery/hap_node_js_client.test.ts diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b90f5ff..693b896 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -21,7 +21,7 @@ jobs: - { node-version: 18.x, lint: true, static-analysis: true, tests: true } - { node-version: 19.x, lint: true, static-analysis: false, tests: true } - name: Node.js ${{ matrix.node-version }}${{ matrix.lint && ', lint' || '' }}${{ matrix.static-analysis && ', static analysis' || ''}}${{ matrix.tests && ', test' || '' }}, build + name: Node.js ${{ matrix.node-version }}${{ matrix.lint && ', lint' || '' }}${{ matrix.tests && ', test' || '' }}${{ matrix.static-analysis && ', static analysis' || ''}}, build steps: - uses: actions/checkout@v3 @@ -77,6 +77,10 @@ jobs: run: npm run lint if: ${{ matrix.lint }} + - name: Run tests + run: npm test + if: ${{ matrix.tests }} + - name: SonarCloud scan uses: SonarSource/sonarcloud-github-action@master env: @@ -84,9 +88,5 @@ jobs: SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} if: ${{ matrix.static-analysis }} - - name: Run tests - run: npm test - if: ${{ matrix.tests }} - - name: Build the project run: npm run build diff --git a/package.json b/package.json index 5873426..f8e1655 100644 --- a/package.json +++ b/package.json @@ -18,7 +18,7 @@ "scripts": { "lint": "ifNotCi() { test \"$CI\" && echo \"$2\" || echo \"$1\"; }; `npm bin`/tsc --noEmit && `npm bin`/prettier --ignore-path=.gitignore `ifNotCi --write \"--check --cache --cache-strategy content\"` '**/**.{ts,js,json}' && `npm bin`/eslint `ifNotCi --fix \"--cache --cache-strategy content\"` --ignore-path=.gitignore '**/**.{ts,js,json}'", "start": "npm run build && npm run link && nodemon", - "test": "ifNotCi() { test \"$CI\" && echo \"$2\" || echo \"$1\"; }; npm run code-generation && `npm bin`/jest `ifNotCi --watchAll`", + "test": "ifNotCi() { test \"$CI\" && echo \"$2\" || echo \"$1\"; }; npm run code-generation && `npm bin`/jest `ifNotCi --watchAll --collect-coverage`", "link": "npm install --no-save file:///$PWD/", "build": "rimraf ./dist .tsbuildinfo && npm run code-generation && tsc", "code-generation": "./code-generation/hap-gen.js && ./code-generation/config-scheme-gen.js", diff --git a/sonar-project.properties b/sonar-project.properties index 11e742c..756d6e6 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -3,4 +3,4 @@ sonar.organization=lstrojny sonar.sources=. sonar.exclusions=tests/** sonar.tests=tests -sonar.log.level=DEBUG +sonar.javascript.lcov.reportPaths=./coverage/lcov.info diff --git a/src/adapters/discovery/api.ts b/src/adapters/discovery/api.ts index 183f472..ce61ee2 100644 --- a/src/adapters/discovery/api.ts +++ b/src/adapters/discovery/api.ts @@ -3,7 +3,7 @@ import type { Logger } from 'homebridge' export interface HapDiscoveryConfig { config: Pick - log: Logger + log?: Logger } export type HapDiscover = (config: HapDiscoveryConfig) => Promise diff --git a/src/adapters/discovery/hap_node_js_client.ts b/src/adapters/discovery/hap_node_js_client.ts index 060d321..a033691 100644 --- a/src/adapters/discovery/hap_node_js_client.ts +++ b/src/adapters/discovery/hap_node_js_client.ts @@ -12,34 +12,43 @@ type RejectFunc = (error: unknown) => void const clientMap: Record = {} const promiseMap: Record = {} -function startDiscovery(logger: Logger, config: HAPNodeJSClientConfig, resolve: ResolveFunc, reject: RejectFunc) { +function startDiscovery( + logger: Logger | undefined, + config: HAPNodeJSClientConfig, + resolve: ResolveFunc, + reject: RejectFunc, +) { const key = JSON.stringify(config) - if (!clientMap[key]) { - logger.debug('Creating new HAP client') - const client = new HAPNodeJSClient(config) - client.on('Ready', (deviceData: unknown) => { - try { - const devices: Device[] = [] - - for (const device of checkBoundary(MaybeDevices, deviceData)) { - try { - devices.push(checkBoundary(DeviceBoundary, device)) - } catch (e) { - logger.error('Boundary check for device data failed %o %s', e, JSON.stringify(device, null, 4)) - } - } - - if (promiseMap[key]) promiseMap[key][0](devices) - } catch (e) { - if (promiseMap[key]) promiseMap[key][1](e) - } - }) - clientMap[key] = client - } else { - logger.debug('Reusing existing HAP client') - } promiseMap[key] = [resolve, reject] + + if (!clientMap[key]) { + logger?.debug('Creating new HAP client') + clientMap[key] = new HAPNodeJSClient(config) + clientMap[key].on('Ready', createDiscoveryHandler(logger, key)) + } else { + logger?.debug('Reusing existing HAP client') + } +} + +function createDiscoveryHandler(logger: Logger | undefined, key: string): (deviceData: unknown) => void { + return (deviceData: unknown) => { + try { + const devices: Device[] = [] + + for (const device of checkBoundary(MaybeDevices, deviceData)) { + try { + devices.push(checkBoundary(DeviceBoundary, device)) + } catch (e) { + logger?.error('Boundary check for device data failed %o %s', e, JSON.stringify(device, null, 4)) + } + } + + if (promiseMap[key]) promiseMap[key][0](devices) + } catch (e) { + if (promiseMap[key]) promiseMap[key][1](e) + } + } } export const hapNodeJsClientDiscover: HapDiscover = ({ config, log }) => { diff --git a/src/metrics.ts b/src/metrics.ts index 9981335..e11bc79 100644 --- a/src/metrics.ts +++ b/src/metrics.ts @@ -17,7 +17,7 @@ export class Metric { const METRICS_FILTER = ['Identifier'] export function aggregate(devices: Device[], timestamp: Date): Metric[] { - const metrics: Metric[] = [] + const metrics: Metric[][] = [] for (const device of devices) { for (const accessory of device.accessories.accessories) { @@ -27,42 +27,50 @@ export function aggregate(devices: Device[], timestamp: Date): Metric[] { ...getAccessoryLabels(accessory), ...getServiceLabels(service), } - for (const characteristic of service.characteristics) { - const format = characteristic.format - switch (format) { - case 'string': - case 'tlv8': - case 'data': - break - - case 'bool': - case 'float': - case 'int': - case 'uint8': - case 'uint16': - case 'uint32': - case 'uint64': - if (characteristic.value != null) { - if (METRICS_FILTER.includes(characteristic.description)) { - break - } - const name = formatName( - Uuids[service.type] || 'custom', - characteristic.description, - characteristic.unit, - ) - metrics.push(new Metric(name, characteristic.value, timestamp, labels)) - } - break - - default: - assertTypeExhausted(format) - } - } + metrics.push(extractMetrics(service, timestamp, labels)) } } } + return metrics.flat() +} + +function extractMetrics(service: Service, timestamp: Date, labels: Record) { + const metrics: Metric[] = [] + + for (const characteristic of service.characteristics) { + const format = characteristic.format + switch (format) { + case 'string': + case 'tlv8': + case 'data': + break + + case 'bool': + case 'float': + case 'int': + case 'uint8': + case 'uint16': + case 'uint32': + case 'uint64': + if (characteristic.value != null) { + if (METRICS_FILTER.includes(characteristic.description)) { + break + } + const name = formatName( + Uuids[service.type] || 'custom', + characteristic.description, + characteristic.unit, + ) + metrics.push(new Metric(name, characteristic.value, timestamp, labels)) + } + break + + default: + assertTypeExhausted(format) + } + } + return metrics } diff --git a/src/prometheus.ts b/src/prometheus.ts index a21908c..94790c7 100644 --- a/src/prometheus.ts +++ b/src/prometheus.ts @@ -26,7 +26,9 @@ export class MetricsRenderer { private metricName(name: string): string { name = name.replace(/^(.*_)?(total)_(.*)$/, '$1$3_$2') - return sanitizePrometheusMetricName(this.prefix.replace(/_+$/, '') + '_' + name) + return sanitizePrometheusMetricName( + this.prefix.split('').reverse().join('').replace(/^_+/, '').split('').reverse().join('') + '_' + name, + ) } } diff --git a/tests/adapters/discovery/hap_node_js_client.test.ts b/tests/adapters/discovery/hap_node_js_client.test.ts new file mode 100644 index 0000000..9a2e483 --- /dev/null +++ b/tests/adapters/discovery/hap_node_js_client.test.ts @@ -0,0 +1,74 @@ +import { afterAll, describe, expect, jest, test } from '@jest/globals' +import { hapNodeJsClientDiscover as discover } from '../../../src/adapters/discovery/hap_node_js_client' + +const intervals: NodeJS.Timer[] = [] + +let deviceData: unknown = null + +jest.mock('hap-node-client', () => ({ + HAPNodeJSClient: class { + on(event: string, fn: (data: unknown) => void) { + intervals.push(setInterval(() => fn(deviceData), 100)) + } + }, +})) + +const properDeviceData = { + instance: { + deviceID: 'bff926c2-ddbe-4141-b17f-f011e03e669c', + name: 'name', + url: 'http://bridge.local', + }, + accessories: { + accessories: [ + { + services: [ + { + type: 'SERVICE TYPE', + characteristics: [ + { + format: 'bool', + value: 1, + description: 'description', + type: 'CHARACTERISTIC TYPE', + }, + ], + }, + ], + }, + ], + }, +} + +const invalidDeviceData = {} + +const config = { + debug: false, + pin: '123-12-123', + refresh_interval: 10, + discovery_timeout: 10, + request_timeout: 10, +} + +describe('HAP NodeJS Client', () => { + afterAll(() => { + intervals.map((timer) => clearInterval(timer)) + }) + + test('Simple discovery', async () => { + deviceData = [properDeviceData] + expect(await discover({ config })).toHaveLength(1) + }) + + test('Connection pooling works', async () => { + deviceData = [properDeviceData] + expect(await discover({ config })).toHaveLength(1) + + expect(await discover({ config })).toHaveLength(1) + }) + + test('Invalid device data is ignored', async () => { + deviceData = [invalidDeviceData, properDeviceData] + expect(await discover({ config })).toHaveLength(1) + }) +})