From 64165c3687c7a7fd1988697633fff4938a02ce42 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 16:01:13 +0000 Subject: [PATCH 1/2] Share a single vcgencmd runner between the temperature and throttled collectors TemperatureCollector and ThrottledCollector each implemented their own lazy vcgencmd detection/re-detection logic (exec.LookPath, throttled retry) verbatim. Extract that into a shared vcgencmdRunner type used by both, so the duplicated code collapses to one place and vcgencmd is looked up once (shared across both collectors) instead of once per collector. True single-exec batching of measure_temp and get_throttled isn't possible: vcgencmd only accepts one subcommand per invocation, so both collectors still shell out separately on the hot path. This change targets the actionable part of issue #66 - the duplicated detection/re-detection logic - rather than a batching scheme vcgencmd doesn't support. --- internal/collector/collector.go | 8 ++- internal/collector/temperature.go | 87 +++++++++--------------- internal/collector/temperature_test.go | 12 ++-- internal/collector/throttled.go | 79 +++++++--------------- internal/collector/throttled_test.go | 23 ++++++- internal/collector/vcgencmd.go | 94 ++++++++++++++++++++++++++ internal/collector/vcgencmd_test.go | 58 ++++++++++++++++ 7 files changed, 238 insertions(+), 123 deletions(-) create mode 100644 internal/collector/vcgencmd.go create mode 100644 internal/collector/vcgencmd_test.go diff --git a/internal/collector/collector.go b/internal/collector/collector.go index d5e4017..bb9cd7e 100644 --- a/internal/collector/collector.go +++ b/internal/collector/collector.go @@ -131,6 +131,10 @@ func New(cfg Config, log *slog.Logger) *Collector { alerts = alert.New(cfg.Thresholds, cfg.AlertFor) notifier = cfg.Notifier } + // TemperatureCollector and ThrottledCollector both shell out to + // vcgencmd; sharing one runner means vcgencmd is detected once instead + // of once per collector. + vcg := newVcgencmdRunner(time.Now) c := &Collector{ cfg: cfg, // Disks starts as [] rather than nil so it marshals as [] (not @@ -144,8 +148,8 @@ func New(cfg Config, log *slog.Logger) *Collector { memory: NewMemoryCollector(), disk: NewDiskCollector(), network: NewNetworkCollector(), - temp: NewTemperatureCollector(), - throttled: NewThrottledCollector(), + temp: NewTemperatureCollector(vcg), + throttled: NewThrottledCollector(vcg), sysInfo: NewSysInfoCollector(), updates: NewUpdatesCollector(cfg.UpdatesStaleThreshold), uptime: NewUptimeCollector(), diff --git a/internal/collector/temperature.go b/internal/collector/temperature.go index c75e3af..e2fe191 100644 --- a/internal/collector/temperature.go +++ b/internal/collector/temperature.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "os" - "os/exec" "path/filepath" "strconv" "strings" @@ -77,35 +76,33 @@ func readThermalZoneMilliC(zonePath string) (float64, error) { // TemperatureCollector reads CPU temperature from sysfs, with an optional // vcgencmd-sourced GPU/SoC reading on Raspberry Pi OS. // -// The thermal zone and vcgencmd path are resolved lazily and re-resolved -// (throttled) when they are still missing, so a sensor or driver that -// appears after the process started — e.g. a thermal module loaded late in -// boot, or a zone path that changes across a kernel/driver update — is -// picked up without restarting the collector. +// The thermal zone is resolved lazily and re-resolved (throttled) when it +// is still missing, so a sensor or driver that appears after the process +// started — e.g. a thermal module loaded late in boot, or a zone path that +// changes across a kernel/driver update — is picked up without restarting +// the collector. vcgencmd detection is handled by the shared vcg runner, +// which TemperatureCollector and ThrottledCollector both use. type TemperatureCollector struct { zoneGlob string // sysfs glob for thermal zones (overridable in tests) - mu sync.Mutex - now func() time.Time - zonePath string - zoneType string - lastZoneDetect time.Time - vcgencmdPath string // empty if vcgencmd is not available - vcgencmdDetected bool // whether a vcgencmd lookup has ever succeeded - lastVcgencmdDetect time.Time + mu sync.Mutex + now func() time.Time + zonePath string + zoneType string + lastZoneDetect time.Time + vcg *vcgencmdRunner // nil disables the GPU/SoC reading } -// NewTemperatureCollector auto-detects the CPU thermal zone and checks -// whether vcgencmd is available. Detection failures are not fatal: the -// collector still works, it just reports errors from Collect() until a -// thermal zone appears (e.g. useful for local development off-Pi). If the -// zone (or vcgencmd) is missing at construction, Collect re-attempts -// detection at most once every detectRetryInterval, so a sensor that shows -// up later is used automatically. -func NewTemperatureCollector() *TemperatureCollector { - c := &TemperatureCollector{zoneGlob: thermalZoneGlob, now: time.Now} +// NewTemperatureCollector auto-detects the CPU thermal zone. Detection +// failure is not fatal: the collector still works, it just reports errors +// from Collect() until a thermal zone appears (e.g. useful for local +// development off-Pi). If the zone is missing at construction, Collect +// re-attempts detection at most once every detectRetryInterval, so a sensor +// that shows up later is used automatically. vcg is the vcgencmd runner +// shared with ThrottledCollector; pass nil to disable the GPU/SoC reading. +func NewTemperatureCollector(vcg *vcgencmdRunner) *TemperatureCollector { + c := &TemperatureCollector{zoneGlob: thermalZoneGlob, now: time.Now, vcg: vcg} c.redetectZoneLocked() - c.redetectVcgencmdLocked() return c } @@ -127,23 +124,6 @@ func (c *TemperatureCollector) redetectZoneLocked() { } } -// redetectVcgencmdLocked retries exec.LookPath("vcgencmd") if it has never -// been found, throttled the same way as zone re-detection. -func (c *TemperatureCollector) redetectVcgencmdLocked() { - if c.vcgencmdDetected { - return - } - now := c.now() - if !c.lastVcgencmdDetect.IsZero() && now.Sub(c.lastVcgencmdDetect) < detectRetryInterval { - return - } - c.lastVcgencmdDetect = now - if path, err := exec.LookPath("vcgencmd"); err == nil { - c.vcgencmdPath = path - c.vcgencmdDetected = true - } -} - // Collect returns the current CPU temperature and, if vcgencmd is // available, the GPU/SoC temperature as a secondary reading. func (c *TemperatureCollector) Collect(ctx context.Context) (Temperature, *GPUTemperature, error) { @@ -176,31 +156,26 @@ func (c *TemperatureCollector) Collect(ctx context.Context) (Temperature, *GPUTe } temp := Temperature{Zone: c.zoneType, Celsius: celsius} - c.redetectVcgencmdLocked() - if c.vcgencmdPath == "" { - return temp, nil, nil - } - gpuTemp, err := c.readVcgencmdTemp(ctx) if err != nil { - // vcgencmd is an optional extra data point; its failure should not - // fail the whole collection. + // vcgencmd is an optional extra data point; its unavailability or + // failure should not fail the whole collection. return temp, nil, nil } return temp, &gpuTemp, nil } -// readVcgencmdTemp runs `vcgencmd measure_temp` and parses output of the -// form "temp=42.8'C". +// readVcgencmdTemp runs `vcgencmd measure_temp` (via the shared vcg runner) +// and parses output of the form "temp=42.8'C". func (c *TemperatureCollector) readVcgencmdTemp(ctx context.Context) (GPUTemperature, error) { - ctx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - - out, err := exec.CommandContext(ctx, c.vcgencmdPath, "measure_temp").Output() + if c.vcg == nil { + return GPUTemperature{}, errVcgencmdUnavailable + } + out, err := c.vcg.run(ctx, "measure_temp") if err != nil { - return GPUTemperature{}, fmt.Errorf("run vcgencmd: %w", err) + return GPUTemperature{}, err } - return parseVcgencmdTemp(string(out)) + return parseVcgencmdTemp(out) } func parseVcgencmdTemp(output string) (GPUTemperature, error) { diff --git a/internal/collector/temperature_test.go b/internal/collector/temperature_test.go index 8b1e3e0..f7ed0ab 100644 --- a/internal/collector/temperature_test.go +++ b/internal/collector/temperature_test.go @@ -115,9 +115,9 @@ func TestTemperatureCollector_Collect_RedetectsZone(t *testing.T) { now := time.Unix(1_700_000_000, 0) c := &TemperatureCollector{ - zoneGlob: glob, - now: func() time.Time { return now }, - vcgencmdDetected: true, // skip vcgencmd lookup in this test + zoneGlob: glob, + now: func() time.Time { return now }, + // vcg left nil: skips vcgencmd entirely for this test. } // No zone exists yet: Collect must fail. @@ -151,9 +151,9 @@ func TestTemperatureCollector_Collect_RedetectsAfterZoneVanishes(t *testing.T) { now := time.Unix(1_700_000_000, 0) c := &TemperatureCollector{ - zoneGlob: glob, - now: func() time.Time { return now }, - vcgencmdDetected: true, // skip vcgencmd lookup in this test + zoneGlob: glob, + now: func() time.Time { return now }, + // vcg left nil: skips vcgencmd entirely for this test. } // A zone exists at first and is cached by Collect. diff --git a/internal/collector/throttled.go b/internal/collector/throttled.go index b7e440d..c963b8c 100644 --- a/internal/collector/throttled.go +++ b/internal/collector/throttled.go @@ -2,12 +2,10 @@ package collector import ( "context" + "errors" "fmt" - "os/exec" "strconv" "strings" - "sync" - "time" ) // Bit positions in the bitmask reported by `vcgencmd get_throttled`. The @@ -31,70 +29,39 @@ const ( // systems without vcgencmd (e.g. development machines) it degrades to no // reading rather than failing. // -// The vcgencmd path is resolved lazily and re-resolved (throttled to at most -// once per detectRetryInterval) while it is still missing, mirroring -// TemperatureCollector, so a firmware tool that appears after startup is -// picked up without restarting the collector. +// vcgencmd detection/execution is delegated to a vcgencmdRunner shared with +// TemperatureCollector, so the lazy detection/re-detection logic and the +// exec.LookPath("vcgencmd") call both live and run in one place rather than +// being duplicated per collector. type ThrottledCollector struct { - mu sync.Mutex - now func() time.Time - vcgencmdPath string // empty if vcgencmd is not available - vcgencmdDetected bool // whether a vcgencmd lookup has ever succeeded - lastVcgencmdDetect time.Time + vcg *vcgencmdRunner // nil disables collection entirely } -// NewThrottledCollector checks whether vcgencmd is available. A missing -// vcgencmd is not fatal: Collect simply returns no reading until vcgencmd -// appears (re-detection is retried at most once per detectRetryInterval). -func NewThrottledCollector() *ThrottledCollector { - c := &ThrottledCollector{now: time.Now} - c.redetectVcgencmdLocked() - return c +// NewThrottledCollector wraps vcg, the vcgencmd runner shared with +// TemperatureCollector. A missing vcgencmd is not fatal: Collect simply +// returns no reading until vcgencmd appears (re-detection is retried by vcg +// at most once per detectRetryInterval). Pass nil to disable collection. +func NewThrottledCollector(vcg *vcgencmdRunner) *ThrottledCollector { + return &ThrottledCollector{vcg: vcg} } -// redetectVcgencmdLocked retries exec.LookPath("vcgencmd") if it has never -// been found, throttled to at most once per detectRetryInterval. Caller must -// hold c.mu (the constructor is single-threaded, so it also qualifies). -func (c *ThrottledCollector) redetectVcgencmdLocked() { - if c.vcgencmdDetected { - return - } - now := c.now() - if !c.lastVcgencmdDetect.IsZero() && now.Sub(c.lastVcgencmdDetect) < detectRetryInterval { - return - } - c.lastVcgencmdDetect = now - if path, err := exec.LookPath("vcgencmd"); err == nil { - c.vcgencmdPath = path - c.vcgencmdDetected = true - } -} - -// Collect runs `vcgencmd get_throttled` and decodes the bitmask. It returns -// (nil, nil) when vcgencmd is not available, so the throttled object is -// simply omitted from the snapshot off-Pi. +// Collect runs `vcgencmd get_throttled` (via the shared vcg runner) and +// decodes the bitmask. It returns (nil, nil) when vcgencmd is not +// available, so the throttled object is simply omitted from the snapshot +// off-Pi. func (c *ThrottledCollector) Collect(ctx context.Context) (*Throttled, error) { - c.mu.Lock() - defer c.mu.Unlock() - - // Collectors built as struct literals in tests may not set the clock. - if c.now == nil { - c.now = time.Now - } - - c.redetectVcgencmdLocked() - if c.vcgencmdPath == "" { + if c.vcg == nil { return nil, nil } - ctx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - - out, err := exec.CommandContext(ctx, c.vcgencmdPath, "get_throttled").Output() + out, err := c.vcg.run(ctx, "get_throttled") if err != nil { - return nil, fmt.Errorf("run vcgencmd get_throttled: %w", err) + if errors.Is(err, errVcgencmdUnavailable) { + return nil, nil + } + return nil, err } - t, err := parseThrottled(string(out)) + t, err := parseThrottled(out) if err != nil { return nil, err } diff --git a/internal/collector/throttled_test.go b/internal/collector/throttled_test.go index 0f65f60..d2dc427 100644 --- a/internal/collector/throttled_test.go +++ b/internal/collector/throttled_test.go @@ -90,13 +90,15 @@ func TestParseThrottled_Malformed(t *testing.T) { } func TestThrottledCollector_Collect_NoVcgencmd(t *testing.T) { - // A collector that has never found vcgencmd (and whose throttled + // A collector whose vcg runner has never found vcgencmd (and whose // re-detection window has not elapsed) must report no reading rather // than failing, so the throttled object is simply omitted off-Pi. now := time.Unix(1_700_000_000, 0) c := &ThrottledCollector{ - now: func() time.Time { return now }, - lastVcgencmdDetect: now, // suppress the LookPath retry in this test + vcg: &vcgencmdRunner{ + now: func() time.Time { return now }, + lastDetect: now, // suppress the LookPath retry in this test + }, } got, err := c.Collect(context.Background()) if err != nil { @@ -106,3 +108,18 @@ func TestThrottledCollector_Collect_NoVcgencmd(t *testing.T) { t.Fatalf("expected no throttled reading without vcgencmd, got %+v", got) } } + +func TestThrottledCollector_Collect_NilRunner(t *testing.T) { + // A collector with no vcg runner at all (vcgencmd disabled for this + // collector) must also degrade to no reading rather than panicking. + c := &ThrottledCollector{} + + got, err := c.Collect(context.Background()) + + if err != nil { + t.Fatalf("Collect: %v", err) + } + if got != nil { + t.Fatalf("expected no throttled reading with nil vcg runner, got %+v", got) + } +} diff --git a/internal/collector/vcgencmd.go b/internal/collector/vcgencmd.go new file mode 100644 index 0000000..4370696 --- /dev/null +++ b/internal/collector/vcgencmd.go @@ -0,0 +1,94 @@ +package collector + +import ( + "context" + "errors" + "fmt" + "os/exec" + "strings" + "sync" + "time" +) + +// vcgencmdTimeout bounds a single vcgencmd invocation so a hung firmware +// call cannot stall a fast tick indefinitely. +const vcgencmdTimeout = 5 * time.Second + +// errVcgencmdUnavailable is returned by run when vcgencmd has not (yet) been +// found on the system. It is not a failure: TemperatureCollector and +// ThrottledCollector both treat it as "no reading available this tick" +// rather than a collection error. +var errVcgencmdUnavailable = errors.New("vcgencmd not available") + +// vcgencmdRunner resolves the vcgencmd binary once and runs subcommands +// against it. TemperatureCollector and ThrottledCollector share a single +// instance so the lazy detection/re-detection logic - previously +// duplicated verbatim in both collectors - lives in one place, and the +// exec.LookPath("vcgencmd") call itself only ever runs once instead of +// once per collector. +type vcgencmdRunner struct { + mu sync.Mutex + now func() time.Time + path string // empty if vcgencmd is not available + // detected reports whether a lookup has ever succeeded. Re-detection is + // throttled (detectRetryInterval) while it has not, so a missing + // vcgencmd doesn't cost an exec.LookPath call on every tick. + detected bool + lastDetect time.Time +} + +// newVcgencmdRunner resolves the vcgencmd binary path, if available. A +// missing vcgencmd is not fatal: run re-attempts detection at most once per +// detectRetryInterval, mirroring TemperatureCollector's thermal-zone +// re-detection, so vcgencmd becoming available after startup is picked up +// without restarting the collectors. +func newVcgencmdRunner(now func() time.Time) *vcgencmdRunner { + if now == nil { + now = time.Now + } + r := &vcgencmdRunner{now: now} + r.mu.Lock() + r.redetectLocked() + r.mu.Unlock() + return r +} + +// redetectLocked (re)resolves the vcgencmd path if it is currently unknown, +// throttled to at most once per detectRetryInterval. Caller must hold r.mu. +func (r *vcgencmdRunner) redetectLocked() { + if r.detected { + return + } + now := r.now() + if !r.lastDetect.IsZero() && now.Sub(r.lastDetect) < detectRetryInterval { + return + } + r.lastDetect = now + if path, err := exec.LookPath("vcgencmd"); err == nil { + r.path = path + r.detected = true + } +} + +// run executes `vcgencmd ` with a bounded timeout and returns +// its trimmed stdout. It returns errVcgencmdUnavailable, without attempting +// an exec, if vcgencmd has not been detected. +func (r *vcgencmdRunner) run(ctx context.Context, subcommand string) (string, error) { + r.mu.Lock() + r.redetectLocked() + path := r.path + r.mu.Unlock() + + if path == "" { + return "", errVcgencmdUnavailable + } + + ctx, cancel := context.WithTimeout(ctx, vcgencmdTimeout) + defer cancel() + + out, err := exec.CommandContext(ctx, path, subcommand).Output() + if err != nil { + return "", fmt.Errorf("run vcgencmd %s: %w", subcommand, err) + } + return strings.TrimSpace(string(out)), nil +} diff --git a/internal/collector/vcgencmd_test.go b/internal/collector/vcgencmd_test.go new file mode 100644 index 0000000..a770710 --- /dev/null +++ b/internal/collector/vcgencmd_test.go @@ -0,0 +1,58 @@ +package collector + +import ( + "context" + "errors" + "testing" + "time" +) + +func TestVcgencmdRunner_Run_Unavailable(t *testing.T) { + // detected: true with no path means a lookup already ran and found + // nothing; run must report unavailable without attempting an exec. + r := &vcgencmdRunner{detected: true} + + _, err := r.run(context.Background(), "measure_temp") + + if !errors.Is(err, errVcgencmdUnavailable) { + t.Fatalf("run() error = %v, want errVcgencmdUnavailable", err) + } +} + +func TestVcgencmdRunner_Run_ThrottlesRedetection(t *testing.T) { + // A lookup was already attempted (and failed) just now; a second run() + // within detectRetryInterval must not attempt another lookup. + now := time.Unix(1_700_000_000, 0) + r := &vcgencmdRunner{ + now: func() time.Time { return now }, + lastDetect: now, + } + + _, err := r.run(context.Background(), "measure_temp") + + if !errors.Is(err, errVcgencmdUnavailable) { + t.Fatalf("run() error = %v, want errVcgencmdUnavailable", err) + } + if r.lastDetect != now { + t.Fatalf("lastDetect = %v, want unchanged %v (re-detection should be throttled)", r.lastDetect, now) + } +} + +func TestVcgencmdRunner_Run_RetriesAfterThrottleWindow(t *testing.T) { + now := time.Unix(1_700_000_000, 0) + r := &vcgencmdRunner{ + now: func() time.Time { return now }, + lastDetect: now, + } + + // Advance past the throttle window: a fresh lookup attempt is made + // (and updates lastDetect), even though vcgencmd still isn't found in + // this test environment. + now = now.Add(detectRetryInterval + time.Second) + if _, err := r.run(context.Background(), "measure_temp"); !errors.Is(err, errVcgencmdUnavailable) { + t.Fatalf("run() error = %v, want errVcgencmdUnavailable", err) + } + if r.lastDetect != now { + t.Fatalf("lastDetect = %v, want updated to %v after the throttle window elapsed", r.lastDetect, now) + } +} From c663f97cc97d14af9cfb218f669140bff082ad28 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 16:18:54 +0000 Subject: [PATCH 2/2] Exercise the vcgencmd exec paths in tests to satisfy the coverage gate The previous commit's tests never actually ran a subprocess through vcgencmdRunner.run(): every test kept vcg nil or pre-set "unavailable", so the real exec.CommandContext success/failure paths, and exec.LookPath's success path, were untested new code and tripped SonarCloud's coverage gate. Add a writeFakeVcgencmd test helper (mirroring the existing aptPath: "true" stub-command pattern in updates_test.go) that writes a small executable shell script standing in for vcgencmd, and use it to exercise: vcgencmdRunner.run() executing successfully and failing, redetectLocked() finding a binary via PATH, and the resulting success/failure/malformed-output behavior through TemperatureCollector.Collect and ThrottledCollector.Collect end to end. --- internal/collector/temperature_test.go | 46 ++++++++++++++++++++++ internal/collector/testhelpers_test.go | 14 +++++++ internal/collector/throttled_test.go | 39 +++++++++++++++++++ internal/collector/vcgencmd_test.go | 54 ++++++++++++++++++++++++++ 4 files changed, 153 insertions(+) diff --git a/internal/collector/temperature_test.go b/internal/collector/temperature_test.go index f7ed0ab..9392ff1 100644 --- a/internal/collector/temperature_test.go +++ b/internal/collector/temperature_test.go @@ -102,6 +102,52 @@ func TestTemperatureCollector_Collect(t *testing.T) { } } +func TestTemperatureCollector_Collect_WithGPUTemp(t *testing.T) { + root := t.TempDir() + writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000") + scriptDir := t.TempDir() + path := writeFakeVcgencmd(t, scriptDir, "fake-vcgencmd", `echo "temp=42.8'C"`) + + c := &TemperatureCollector{ + zonePath: filepath.Join(root, "thermal_zone0"), + zoneType: "cpu-thermal", + vcg: &vcgencmdRunner{detected: true, path: path}, + } + temp, gpuTemp, err := c.Collect(context.Background()) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if diffFloat(temp.Celsius, 50.0) > 0.001 { + t.Fatalf("Celsius = %v, want 50.0", temp.Celsius) + } + if gpuTemp == nil || diffFloat(gpuTemp.Celsius, 42.8) > 0.001 { + t.Fatalf("gpuTemp = %+v, want Celsius=42.8", gpuTemp) + } +} + +func TestTemperatureCollector_Collect_VcgencmdExecFails(t *testing.T) { + root := t.TempDir() + writeThermalZone(t, root, "thermal_zone0", "cpu-thermal", "50000") + scriptDir := t.TempDir() + path := writeFakeVcgencmd(t, scriptDir, "fake-vcgencmd", "exit 1") + + c := &TemperatureCollector{ + zonePath: filepath.Join(root, "thermal_zone0"), + zoneType: "cpu-thermal", + vcg: &vcgencmdRunner{detected: true, path: path}, + } + temp, gpuTemp, err := c.Collect(context.Background()) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if diffFloat(temp.Celsius, 50.0) > 0.001 { + t.Fatalf("Celsius = %v, want 50.0", temp.Celsius) + } + if gpuTemp != nil { + t.Fatalf("expected no GPU temp when vcgencmd exec fails, got %+v", gpuTemp) + } +} + func TestTemperatureCollector_Collect_NoZoneDetected(t *testing.T) { c := &TemperatureCollector{} if _, _, err := c.Collect(context.Background()); err == nil { diff --git a/internal/collector/testhelpers_test.go b/internal/collector/testhelpers_test.go index 39abfa8..a13e0de 100644 --- a/internal/collector/testhelpers_test.go +++ b/internal/collector/testhelpers_test.go @@ -24,3 +24,17 @@ func overwriteTempFile(t *testing.T, path, content string) { t.Fatalf("overwrite temp file %s: %v", path, err) } } + +// writeFakeVcgencmd writes an executable shell script standing in for the +// real vcgencmd binary (mirroring the "true"/"false" stub-command pattern +// used in updates_test.go) and returns its path. script is the script body, +// e.g. `echo "temp=42.8'C"` or `exit 1`, and runs regardless of the +// subcommand argument it's invoked with. +func writeFakeVcgencmd(t *testing.T, dir, name, script string) string { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte("#!/bin/sh\n"+script+"\n"), 0o755); err != nil { + t.Fatalf("write fake vcgencmd %s: %v", path, err) + } + return path +} diff --git a/internal/collector/throttled_test.go b/internal/collector/throttled_test.go index d2dc427..6d28122 100644 --- a/internal/collector/throttled_test.go +++ b/internal/collector/throttled_test.go @@ -109,6 +109,45 @@ func TestThrottledCollector_Collect_NoVcgencmd(t *testing.T) { } } +func TestThrottledCollector_Collect_Success(t *testing.T) { + dir := t.TempDir() + path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", `echo "throttled=0x50005"`) + c := &ThrottledCollector{vcg: &vcgencmdRunner{detected: true, path: path}} + + got, err := c.Collect(context.Background()) + + if err != nil { + t.Fatalf("Collect: %v", err) + } + if got == nil || got.Raw != "0x50005" || !got.UnderVoltageNow || !got.ThrottledNow { + t.Fatalf("Collect() = %+v, want decoded reading for throttled=0x50005", got) + } +} + +func TestThrottledCollector_Collect_MalformedOutput(t *testing.T) { + dir := t.TempDir() + path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", `echo "garbage output"`) + c := &ThrottledCollector{vcg: &vcgencmdRunner{detected: true, path: path}} + + _, err := c.Collect(context.Background()) + + if err == nil { + t.Fatal("expected error for malformed vcgencmd get_throttled output") + } +} + +func TestThrottledCollector_Collect_ExecFails(t *testing.T) { + dir := t.TempDir() + path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", "exit 1") + c := &ThrottledCollector{vcg: &vcgencmdRunner{detected: true, path: path}} + + _, err := c.Collect(context.Background()) + + if err == nil { + t.Fatal("expected error when vcgencmd get_throttled exits non-zero") + } +} + func TestThrottledCollector_Collect_NilRunner(t *testing.T) { // A collector with no vcg runner at all (vcgencmd disabled for this // collector) must also degrade to no reading rather than panicking. diff --git a/internal/collector/vcgencmd_test.go b/internal/collector/vcgencmd_test.go index a770710..d9b3fc5 100644 --- a/internal/collector/vcgencmd_test.go +++ b/internal/collector/vcgencmd_test.go @@ -3,6 +3,7 @@ package collector import ( "context" "errors" + "runtime" "testing" "time" ) @@ -56,3 +57,56 @@ func TestVcgencmdRunner_Run_RetriesAfterThrottleWindow(t *testing.T) { t.Fatalf("lastDetect = %v, want updated to %v after the throttle window elapsed", r.lastDetect, now) } } + +func TestVcgencmdRunner_Run_ExecutesSubcommand(t *testing.T) { + dir := t.TempDir() + path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", `echo "temp=42.8'C"`) + r := &vcgencmdRunner{detected: true, path: path} + + out, err := r.run(context.Background(), "measure_temp") + + if err != nil { + t.Fatalf("run: %v", err) + } + if out != "temp=42.8'C" { + t.Fatalf("run() output = %q, want %q", out, "temp=42.8'C") + } +} + +func TestVcgencmdRunner_Run_CommandFails(t *testing.T) { + dir := t.TempDir() + path := writeFakeVcgencmd(t, dir, "fake-vcgencmd", "exit 1") + r := &vcgencmdRunner{detected: true, path: path} + + _, err := r.run(context.Background(), "measure_temp") + + if err == nil { + t.Fatal("expected error when vcgencmd exits non-zero") + } + if errors.Is(err, errVcgencmdUnavailable) { + t.Fatalf("run() error = %v, want a real execution error, not errVcgencmdUnavailable", err) + } +} + +func TestNewVcgencmdRunner_NilClockDefaultsToTimeNow(t *testing.T) { + r := newVcgencmdRunner(nil) + + if r.now == nil { + t.Fatal("newVcgencmdRunner(nil) left now nil, want it defaulted to time.Now") + } +} + +func TestNewVcgencmdRunner_DetectsBinaryOnPath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake vcgencmd fixture is a Unix shell script") + } + dir := t.TempDir() + path := writeFakeVcgencmd(t, dir, "vcgencmd", "true") + t.Setenv("PATH", dir) + + r := newVcgencmdRunner(time.Now) + + if !r.detected || r.path != path { + t.Fatalf("newVcgencmdRunner() detected=%v path=%q, want detected=true path=%q", r.detected, r.path, path) + } +}