Fix code smells (#36)

Fix code smells detected by SonarCloud

.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

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",

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

src/adapters/discovery/api.ts

@@ -3,7 +3,7 @@ import type { Logger } from 'homebridge'
 
 export interface HapDiscoveryConfig {
     config: Pick<Config, 'debug' | 'pin' | 'refresh_interval' | 'discovery_timeout' | 'request_timeout'>
-    log: Logger
+    log?: Logger
 }
 
 export type HapDiscover = (config: HapDiscoveryConfig) => Promise<Device[]>

src/adapters/discovery/hap_node_js_client.ts

@@ -12,34 +12,43 @@ type RejectFunc = (error: unknown) => void
 const clientMap: Record<string, HAPNodeJSClient> = {}
 const promiseMap: Record<string, [ResolveFunc, RejectFunc]> = {}
 
-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 }) => {

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) {
+                metrics.push(extractMetrics(service, timestamp, labels))
-                    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.flat()
+}
+
+function extractMetrics(service: Service, timestamp: Date, labels: Record<string, string>) {
+    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
 }
 

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,
+        )
     }
 }
 

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)
+    })
+})