test(systray): cover IPC handlers, menu formatter, shutdown#26
test(systray): cover IPC handlers, menu formatter, shutdown#26kevinelliott merged 2 commits intomainfrom
Conversation
…racking Raises internal/systray coverage from 0.2% to 3.2% by testing the parts of App that don't require a running systray event loop: - handleListAgents: empty, populated, and slice-copy isolation (the response mustn't share backing storage with App.agents) - handleGetAgent: found, not-found, invalid payload error path - handleGetStatus: counts, update detection, uptime, timestamp fields - handleIPCMessage: dispatch table for list, status, unknown types - formatAgentMenuTitle: ● vs ⬆ prefix, method parenthetical, empty method fallback - requestShutdown: idempotent single-close, concurrent safety under 32 parallel callers - track/untrackDialog: add, remove, and miss-is-noop The remaining ~97% is GUI-glue code (onReady, handleMenuClicks, updateMenu, dialog windows) that requires a running systray library and platform event loop to exercise meaningfully. Covering those is out of scope for unit tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds unit tests in internal/systray to raise coverage by exercising IPC handlers and other “pure” App logic that doesn’t require a running systray event loop.
Changes:
- Adds a lightweight test
Appharness and fixtures for agent installations/versions. - Adds coverage for IPC dispatch + handlers (
list,get_agent,get_status) and menu title formatting. - Adds coverage for shutdown signaling (
requestShutdown) and dialog process tracking (trackDialog/untrackDialog).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmd1 := exec.Command("true") | ||
| cmd2 := exec.Command("true") |
There was a problem hiding this comment.
Using exec.Command("true") is not portable (Windows CI runs these tests) and also triggers a PATH lookup at construction time. Since trackDialog/untrackDialog only rely on pointer identity, use inert *exec.Cmd values (e.g., &exec.Cmd{}) or construct commands in an OS-conditional way to keep the test platform-agnostic.
| cmd1 := exec.Command("true") | |
| cmd2 := exec.Command("true") | |
| cmd1 := &exec.Cmd{} | |
| cmd2 := &exec.Cmd{} |
| } | ||
|
|
||
| func mustParseVersion(v string) agent.Version { | ||
| out, _ := agent.ParseVersion(v) |
There was a problem hiding this comment.
mustParseVersion ignores the ParseVersion error (out, _ := ...), which can cause tests to silently proceed with a zero-value version if parsing ever fails (e.g., if version syntax rules change). Consider failing fast here (t.Helper + require/no error, or panic) so test setup can’t mask real regressions.
| out, _ := agent.ParseVersion(v) | |
| out, err := agent.ParseVersion(v) | |
| if err != nil { | |
| panic(err) | |
| } |
| resp, _ := a.handleListAgents(context.Background(), req) | ||
|
|
||
| var out ipc.ListAgentsResponse | ||
| _ = resp.DecodePayload(&out) |
There was a problem hiding this comment.
This test ignores DecodePayload’s error (and the handler error). If the handler ever returns an error response or the payload schema changes, the test can pass incorrectly or panic later. Suggest asserting err == nil, resp.Type == Success, and checking DecodePayload’s error.
| resp, _ := a.handleListAgents(context.Background(), req) | |
| var out ipc.ListAgentsResponse | |
| _ = resp.DecodePayload(&out) | |
| resp, err := a.handleListAgents(context.Background(), req) | |
| if err != nil { | |
| t.Fatalf("unexpected err: %v", err) | |
| } | |
| if resp.Type != ipc.MessageTypeSuccess { | |
| t.Fatalf("Type = %q, want Success", resp.Type) | |
| } | |
| var out ipc.ListAgentsResponse | |
| if err := resp.DecodePayload(&out); err != nil { | |
| t.Fatalf("decode: %v", err) | |
| } |
| var out ipc.GetAgentResponse | ||
| _ = resp.DecodePayload(&out) |
There was a problem hiding this comment.
This test doesn’t assert resp.Type (Success) or check DecodePayload’s error. If handleGetAgent returns an error response (or decoding fails), out.Agent will remain nil and the test will still pass, masking the bug. Please assert response type and handle decode errors.
| var out ipc.GetAgentResponse | |
| _ = resp.DecodePayload(&out) | |
| if resp.Type != ipc.MessageTypeSuccess { | |
| t.Fatalf("Type = %q, want Success", resp.Type) | |
| } | |
| var out ipc.GetAgentResponse | |
| if err := resp.DecodePayload(&out); err != nil { | |
| t.Fatalf("DecodePayload() error = %v", err) | |
| } |
| t.Fatalf("Type = %q, want Success", resp.Type) | ||
| } | ||
| var out ipc.StatusResponse | ||
| _ = resp.DecodePayload(&out) |
There was a problem hiding this comment.
DecodePayload’s error is ignored here. If handleGetStatus ever returns an error response or the payload schema changes, this test could read zero-values and produce misleading failures. Recommend checking the decode error before asserting on out fields.
| _ = resp.DecodePayload(&out) | |
| if err := resp.DecodePayload(&out); err != nil { | |
| t.Fatalf("DecodePayload() error = %v", err) | |
| } |
Align trailing comments in mkInstallation arg list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promote the Unreleased section to 1.2.0 and capture the UX / operability work landed since v1.1.0: - #20 install/update progress streaming with -v - #21 gRPC server wire-up in helper - #22, #26 test coverage (cli/output 0→82%, systray 0.2→3.2%) - #24 RefreshOnStart field removal - #27 bubbles v0.21 -> v1.0 - #28 go:embed baseline catalog (fresh `go install` works offline) - #29 pkg/logging slog wrapper wired into main and grpc recovery Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Raises `internal/systray` statement coverage from 0.2% → 3.2% by testing the parts of `App` that don't require a running systray event loop.
Covered
Not covered (out of scope)
~97% of `systray.go` is GUI-glue (`onReady`, `handleMenuClicks`, `updateMenu`, dialog windows) that requires a running systray library and platform event loop. Unit tests for those would effectively re-test the `getlantern/systray` library; better served by a manual smoke test or a small e2e harness in a future PR.
Test plan
🤖 Generated with Claude Code