Improve test coverage and fix bugs#17
Conversation
Raises overall coverage (-coverpkg=./...) from ~63% to ~70% by adding tests for previously uncovered code paths: - homeassistant.State.Update (0% -> 100%) - Button, LightSensor entity logic - Light/LightGroup/InputBoolean service calls via the mock HA server (registered happy paths, not just the unregistered error case) - halautomations.Timer builder and conditional firing - store async writer flush/close lifecycle - CLI helpers parseDuration, parseTime, and compileGlobs Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCZq8eURnwy99CRfcb71CA
Lifts hassws package coverage from 3.1% to ~84% by adding a white-box test suite that drives a real hassws.Client against the existing mock WebSocket server: connect/auth (happy, dial-fail, auth-invalid), CallService, GetStates, SubscribeEvents(Raw), heartbeat keepalive, stale connection, forced disconnect, and graceful server close, plus pure-unit tests for the message-id, state, channel-read, and not-connected paths. Bug fixes surfaced while writing these tests: - client: sendMessageStreamResponses leaked the response listener when the underlying send failed; it now removes the listener on error. - client: Close/shutdown/read/send panicked on a nil conn when called before Connect; they now no-op or return ErrNotConnected. - client: renamed the misspelled readMesssageFromChannel to readMessageFromChannel. - message_types: removed the unused MessageTypeAuthChallenge/AuthRequest/ AuthResponse constants (the auth flow uses string literals). - server (mock): serialized all client-connection writes (auth handshake, Close, shutdown, socket assignment) under the same lock as SendMessage. Gorilla forbids concurrent writers, and the unsynchronized paths raced under -race once the client was driven directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCZq8eURnwy99CRfcb71CA
dansimau
changed the title
Raises the cross-package total (make test metric, -coverpkg=./...) from ~72% to ~84% by covering previously untested code: - logger (34.6% -> 85%): colorDiff/prettifyJSON, InfoDiff/DebugJSON, the *Context wrappers and context extraction helpers, Start/Stop lifecycle, pruneLogs, and the global convenience wrappers. - cmd/hal/commands (24% -> 70%): command constructors, printLogs, printEntitiesTable, printEntityStateJSON, makeEmitter, prune-flag validation, and the runEntities/runLogs/runShowEntity/runPruneLogs paths (driven against a throwaway DB via a temp working dir). - perf (0% -> 100%): Timer. - root hal: findEntities/FindEntities reflection walker and the service-call error branches of Light/LightGroup/InputBoolean when the connection is not connected. - automations: PrintDebug and the sensor-lights builder methods (WithCondition, WithConditionScene, TurnsOnLights/OffLights, SetScene). Tests only; no production code changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCZq8eURnwy99CRfcb71CA
dansimau
changed the title
Fix a latent bug and layer additional coverage on top of the parallel "Extend test coverage" commit already on this branch. Bug: the context-aware loggers (logger.InfoContext/ErrorContext/etc.) extracted the entity ID and automation name using a function-local contextKey type, while hal.NewAutomationContext stored them under hal.contextKey. context.Value matches keys by dynamic type as well as value, so the types never compared equal and every DB log written from an automation context silently dropped its entity ID and automation name. Moved the canonical keys to exported logger.EntityIDKey/ AutomationNameKey and pointed hal/context.go at them. Complementary tests (deduped against the parallel coverage commit; overlapping files were reconciled to keep the more thorough version): - logger: populated-context regression guards for the fix (metadata reaches the DB; explicit "automation" arg not overridden by context), writeLogText level filtering, and the pruneLogs delete-error branch. - hal: nil-connection ErrEntityNotRegistered for the *Context light methods; initial state sync (syncStates) via a new SetStates hook on the mock hassws.Server; Timer New/Cancel/IsRunning/reset (mock clock). - store: AsyncWriter edge paths (enqueue before start / after shutdown, double start, error logging). - cmd/hal/commands: fuller runLogs/runEntities/runShowEntity command paths (entity/from-to filters, invalid inputs, found + seeded rows). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dansimau
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c0f5c0a5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| aw.Enqueue(func(*gorm.DB) error { | ||
| ran = true | ||
| return nil | ||
| }) | ||
| assert.Assert(t, ran) |
There was a problem hiding this comment.
When this test calls Enqueue after Shutdown, both branches in AsyncWriter.Enqueue can be ready: the context is already cancelled, but the buffered queue can still accept a send even though the worker has exited. If the send branch wins, the operation is queued forever and ran remains false; I reproduced this with go test ./... and repeated go test ./store -run TestAsyncWriterEnqueueAfterShutdownRunsSynchronously -count=1. This makes the newly added coverage test flaky/failing unless Enqueue is changed to prefer the shutdown path or the assertion is adjusted.
Useful? React with 👍 / 👎.
| ctx = context.WithValue(ctx, logger.EntityIDKey, triggerEntityID) | ||
| ctx = context.WithValue(ctx, logger.AutomationNameKey, automationName) |
There was a problem hiding this comment.
For downstream code that already builds contexts with hal.EntityIDKey or hal.AutomationNameKey, moving the only usable keys to the logger package removes those exported symbols from the public hal API, so those callers no longer compile after upgrading. These lines correctly use the shared logger keys internally, but the root package should keep compatibility aliases (or another migration path) so existing automations can still attach metadata without importing the logger package directly.
Useful? React with 👍 / 👎.
| MessageTypeAuthChallenge MessageType = "auth_challenge" | ||
| MessageTypeAuthRequest MessageType = "auth_request" | ||
| MessageTypeAuthResponse MessageType = "auth_response" | ||
| MessageTypeCallService MessageType = "call_service" |
There was a problem hiding this comment.
For downstream users of the public hassws package, deleting MessageTypeAuthChallenge, MessageTypeAuthRequest, and MessageTypeAuthResponse is a source-breaking API change: code that switches on or constructs auth frames with those exported constants will no longer compile even though the auth structs and handshake behavior remain. Please keep compatibility aliases or deprecate them before removal.
Useful? React with 👍 / 👎.
Enqueue's two-case select raced between the buffered queue send and ctx.Done(): after Shutdown the context is cancelled but the queue can still accept a send, so Go picked between them at random. When the send won, the op was queued to a channel the (already-exited) worker never drains — silently dropping the write, and wedging WaitForWrites forever since its sentinel op never runs. Check ctx.Done() in a non-blocking select before attempting the queue send so a shutting-down writer deterministically runs the op inline, as the code already intended. Fixes the flaky TestAsyncWriterEnqueueAfterShutdownRunsSynchronously. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MCZq8eURnwy99CRfcb71CA
2 participants
No description provided.