From e6c3c6c822df4bbdc94f242f917e593fac7e44a3 Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Sat, 14 Mar 2026 03:20:29 +0800 Subject: [PATCH 1/6] refactor: extend SpeedtestClient to context-aware fetch methods Replace FetchServerByID/FetchServers with FetchServerByIDContext/ FetchServerListContext so the caller's context deadline and cancellation propagates into server-fetch HTTP requests, not just the ping/download/upload test phase. Also add a defensive len(targets)==0 guard after FindServer, and defer s.Context.Reset() so DataManager snapshot state is always cleared on any return path (error or cancellation), preventing stale data bleeding into the next Run cycle. Update MockSpeedtestClient and all mock.On() call sites to match the new interface signatures. --- internal/exporter/runner.go | 23 ++-- internal/exporter/runner_test.go | 196 +++++++++++++++++++++++++++---- 2 files changed, 188 insertions(+), 31 deletions(-) diff --git a/internal/exporter/runner.go b/internal/exporter/runner.go index 0af7714..ae8ab24 100644 --- a/internal/exporter/runner.go +++ b/internal/exporter/runner.go @@ -12,9 +12,11 @@ import ( ) // SpeedtestClient is an interface for the speedtest client behavior. +// Context-aware variants are required so that the caller's context (and its +// cancellation / deadline) propagates into the underlying HTTP requests. type SpeedtestClient interface { - FetchServerByID(id string) (*speedtest.Server, error) - FetchServers() (speedtest.Servers, error) + FetchServerByIDContext(ctx context.Context, id string) (*speedtest.Server, error) + FetchServerListContext(ctx context.Context) (speedtest.Servers, error) } // SpeedtestResult holds the results of a speedtest run. @@ -46,13 +48,13 @@ func (r *SpeedtestRunner) Run(ctx context.Context) (*SpeedtestResult, error) { if r.Server != "" { var err error - s, err = r.client.FetchServerByID(r.Server) + s, err = r.client.FetchServerByIDContext(ctx, r.Server) if err != nil { return nil, fmt.Errorf("fetch server %s: %w", r.Server, err) } } else { log.Info("Finding the best server") - serverList, err := r.client.FetchServers() + serverList, err := r.client.FetchServerListContext(ctx) if err != nil { return nil, fmt.Errorf("fetch servers: %w", err) } @@ -60,6 +62,9 @@ func (r *SpeedtestRunner) Run(ctx context.Context) (*SpeedtestResult, error) { if err != nil { return nil, fmt.Errorf("find server: %w", err) } + if len(targets) == 0 { + return nil, fmt.Errorf("find server: no servers available") + } s = targets[0] } @@ -70,9 +75,14 @@ func (r *SpeedtestRunner) Run(ctx context.Context) (*SpeedtestResult, error) { slog.Info("Selected server") slog.Info("Running speedtest") + // s.Context is *speedtest.Speedtest (the parent client), not a context.Context. + // It is set by a real speedtest client and nil when injected in tests. // Only run tests if Context is set (indicates a real speedtest client, not a mock). // Use the context-aware variants so cancellation aborts in-flight network I/O. + // Reset is deferred so the DataManager snapshot state is always cleared, even + // on error or context cancellation, preventing stale data bleeding into the next Run. if s.Context != nil { + defer s.Context.Reset() if err := s.PingTestContext(ctx, nil); err != nil { return nil, fmt.Errorf("ping test: %w", err) } @@ -91,11 +101,6 @@ func (r *SpeedtestRunner) Run(ctx context.Context) (*SpeedtestResult, error) { "jitter": s.Jitter, }).Info("Speedtest completed") - // Reset the context to free resources if it's set - if s.Context != nil { - s.Context.Reset() - } - id, err := strconv.Atoi(s.ID) if err != nil { return nil, fmt.Errorf("server returned non-integer ID %q: %w", s.ID, err) diff --git a/internal/exporter/runner_test.go b/internal/exporter/runner_test.go index 8aae385..6e802d6 100644 --- a/internal/exporter/runner_test.go +++ b/internal/exporter/runner_test.go @@ -10,6 +10,7 @@ import ( "github.com/showwin/speedtest-go/speedtest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) // MockSpeedtestClient is a mock implementation of the SpeedtestClient interface @@ -17,16 +18,16 @@ type MockSpeedtestClient struct { mock.Mock } -func (m *MockSpeedtestClient) FetchServerByID(id string) (*speedtest.Server, error) { - args := m.Called(id) +func (m *MockSpeedtestClient) FetchServerByIDContext(ctx context.Context, id string) (*speedtest.Server, error) { + args := m.Called(ctx, id) if args.Get(0) == nil { return nil, args.Error(1) } return args.Get(0).(*speedtest.Server), args.Error(1) } -func (m *MockSpeedtestClient) FetchServers() (speedtest.Servers, error) { - args := m.Called() +func (m *MockSpeedtestClient) FetchServerListContext(ctx context.Context) (speedtest.Servers, error) { + args := m.Called(ctx) if args.Get(0) == nil { return nil, args.Error(1) } @@ -59,7 +60,7 @@ func TestSpeedtestRunner_RunWithSpecificServer(t *testing.T) { 5*time.Millisecond, ) - mockClient.On("FetchServerByID", "1234").Return(mockServer, nil) + mockClient.On("FetchServerByIDContext", mock.Anything, "1234").Return(mockServer, nil) runner := &SpeedtestRunner{ Server: "1234", @@ -75,7 +76,7 @@ func TestSpeedtestRunner_RunWithSpecificServer(t *testing.T) { assert.Equal(t, float64(500000000), result.UploadSpeed) // 62500000 * 8 assert.Equal(t, 25.0, result.Ping) assert.Equal(t, 5.0, result.Jitter) - mockClient.AssertCalled(t, "FetchServerByID", "1234") + mockClient.AssertCalled(t, "FetchServerByIDContext", mock.Anything, "1234") } // TestSpeedtestRunner_RunWithoutServerID tests Run() finding the best server @@ -92,7 +93,7 @@ func TestSpeedtestRunner_RunWithoutServerID(t *testing.T) { servers := speedtest.Servers{mockServer} - mockClient.On("FetchServers").Return(servers, nil) + mockClient.On("FetchServerListContext", mock.Anything).Return(servers, nil) runner := &SpeedtestRunner{ Server: "", @@ -108,7 +109,7 @@ func TestSpeedtestRunner_RunWithoutServerID(t *testing.T) { assert.Equal(t, float64(800000000), result.UploadSpeed) // 100000000 * 8 assert.Equal(t, 15.0, result.Ping) assert.Equal(t, 2.0, result.Jitter) - mockClient.AssertCalled(t, "FetchServers") + mockClient.AssertCalled(t, "FetchServerListContext", mock.Anything) } // TestSpeedtestRunner_RunWithHighSpeeds tests Run() with high speed results @@ -123,7 +124,7 @@ func TestSpeedtestRunner_RunWithHighSpeeds(t *testing.T) { 1*time.Millisecond, ) - mockClient.On("FetchServerByID", "9999").Return(mockServer, nil) + mockClient.On("FetchServerByIDContext", mock.Anything, "9999").Return(mockServer, nil) runner := &SpeedtestRunner{ Server: "9999", @@ -153,7 +154,7 @@ func TestSpeedtestRunner_RunWithLowSpeeds(t *testing.T) { 50*time.Millisecond, ) - mockClient.On("FetchServerByID", "2222").Return(mockServer, nil) + mockClient.On("FetchServerByIDContext", mock.Anything, "2222").Return(mockServer, nil) runner := &SpeedtestRunner{ Server: "2222", @@ -185,7 +186,7 @@ func TestNewSpeedtestRunner(t *testing.T) { 62500000, // 500 Mbps in bytes 5*time.Millisecond, ) - mockClient.On("FetchServerByID", "1234").Return(mockServer, nil) + mockClient.On("FetchServerByIDContext", mock.Anything, "1234").Return(mockServer, nil) // Test that NewSpeedtestRunner doesn't panic runner := NewSpeedtestRunner("1234", reg, mockClient) @@ -210,7 +211,7 @@ func TestNewSpeedtestRunner_WithNilClient(t *testing.T) { // returns an error, Run returns (nil, non-nil error) and does not panic. func TestSpeedtestRunner_FetchServerByIDError(t *testing.T) { mockClient := new(MockSpeedtestClient) - mockClient.On("FetchServerByID", "1234").Return(nil, assert.AnError) + mockClient.On("FetchServerByIDContext", mock.Anything, "1234").Return(nil, assert.AnError) runner := &SpeedtestRunner{ Server: "1234", @@ -221,14 +222,14 @@ func TestSpeedtestRunner_FetchServerByIDError(t *testing.T) { assert.Nil(t, result) assert.Error(t, err) - mockClient.AssertCalled(t, "FetchServerByID", "1234") + mockClient.AssertCalled(t, "FetchServerByIDContext", mock.Anything, "1234") } // TestSpeedtestRunner_FetchServersError verifies that when FetchServers returns // an error (no explicit server ID configured), Run returns (nil, non-nil error). func TestSpeedtestRunner_FetchServersError(t *testing.T) { mockClient := new(MockSpeedtestClient) - mockClient.On("FetchServers").Return(nil, assert.AnError) + mockClient.On("FetchServerListContext", mock.Anything).Return(nil, assert.AnError) runner := &SpeedtestRunner{ Server: "", @@ -239,16 +240,16 @@ func TestSpeedtestRunner_FetchServersError(t *testing.T) { assert.Nil(t, result) assert.Error(t, err) - mockClient.AssertCalled(t, "FetchServers") + mockClient.AssertCalled(t, "FetchServerListContext", mock.Anything) } -// TestSpeedtestRunner_NoServersFound verifies that when FetchServers returns an +// TestSpeedtestRunner_NoServersFound verifies that when FetchServerListContext returns an // empty list and FindServer returns no targets, Run returns an error containing // "no speedtest servers found". func TestSpeedtestRunner_NoServersFound(t *testing.T) { mockClient := new(MockSpeedtestClient) // Return an empty (non-nil) server list so the code proceeds to FindServer. - mockClient.On("FetchServers").Return(speedtest.Servers{}, nil) + mockClient.On("FetchServerListContext", mock.Anything).Return(speedtest.Servers{}, nil) runner := &SpeedtestRunner{ Server: "", @@ -259,7 +260,7 @@ func TestSpeedtestRunner_NoServersFound(t *testing.T) { assert.Nil(t, result) assert.Error(t, err) - mockClient.AssertCalled(t, "FetchServers") + mockClient.AssertCalled(t, "FetchServerListContext", mock.Anything) } // TestSpeedtestRunner_NonIntegerServerID verifies that Run returns an error @@ -276,7 +277,7 @@ func TestSpeedtestRunner_NonIntegerServerID(t *testing.T) { 3*time.Millisecond, ) - mockClient.On("FetchServerByID", "not-an-int").Return(mockServer, nil) + mockClient.On("FetchServerByIDContext", mock.Anything, "not-an-int").Return(mockServer, nil) runner := &SpeedtestRunner{ Server: "not-an-int", @@ -295,7 +296,7 @@ func TestSpeedtestRunner_NonIntegerServerID(t *testing.T) { // operators can identify which server caused the problem. func TestSpeedtestRunner_FetchServerByIDError_WrapsError(t *testing.T) { mockClient := new(MockSpeedtestClient) - mockClient.On("FetchServerByID", "9001").Return(nil, assert.AnError) + mockClient.On("FetchServerByIDContext", mock.Anything, "9001").Return(nil, assert.AnError) runner := &SpeedtestRunner{ Server: "9001", @@ -312,7 +313,7 @@ func TestSpeedtestRunner_FetchServerByIDError_WrapsError(t *testing.T) { // returned when FetchServers fails contains identifying context. func TestSpeedtestRunner_FetchServersError_WrapsError(t *testing.T) { mockClient := new(MockSpeedtestClient) - mockClient.On("FetchServers").Return(nil, assert.AnError) + mockClient.On("FetchServerListContext", mock.Anything).Return(nil, assert.AnError) runner := &SpeedtestRunner{ Server: "", @@ -331,7 +332,7 @@ func TestSpeedtestRunner_FetchServersError_WrapsError(t *testing.T) { // our len(targets)==0 guard is reached, so the wrapped message is "find server: ...". func TestSpeedtestRunner_NoServersFound_ErrorMessage(t *testing.T) { mockClient := new(MockSpeedtestClient) - mockClient.On("FetchServers").Return(speedtest.Servers{}, nil) + mockClient.On("FetchServerListContext", mock.Anything).Return(speedtest.Servers{}, nil) runner := &SpeedtestRunner{ Server: "", @@ -391,3 +392,154 @@ func TestSpeedtestRunner_ContextCancellation(t *testing.T) { assert.Nil(t, result) assert.ErrorIs(t, err, context.Canceled) } + +// contextBlockingClient is a SpeedtestClient whose methods block until the +// supplied context is cancelled. This lets tests verify that a cancelled +// context causes Run to return immediately with a context error. +type contextBlockingClient struct{} + +func (contextBlockingClient) FetchServerByIDContext(ctx context.Context, _ string) (*speedtest.Server, error) { + <-ctx.Done() + return nil, ctx.Err() +} + +func (contextBlockingClient) FetchServerListContext(ctx context.Context) (speedtest.Servers, error) { + <-ctx.Done() + return nil, ctx.Err() +} + +// contextCapturingClient records the context passed to it so tests can assert +// it is the same context forwarded from Run — not a fresh background context. +type contextCapturingClient struct { + capturedCtx context.Context + server *speedtest.Server +} + +func (c *contextCapturingClient) FetchServerByIDContext(ctx context.Context, _ string) (*speedtest.Server, error) { + c.capturedCtx = ctx + return c.server, nil +} + +func (c *contextCapturingClient) FetchServerListContext(ctx context.Context) (speedtest.Servers, error) { + c.capturedCtx = ctx + if c.server == nil { + return speedtest.Servers{}, nil + } + return speedtest.Servers{c.server}, nil +} + +// TestSpeedtestRunner_ContextCancelledBeforeFetchServerByID verifies that +// cancelling the context before (or during) FetchServerByIDContext causes +// Run to return (nil, context.Canceled) with the "fetch server" wrapping. +func TestSpeedtestRunner_ContextCancelledBeforeFetchServerByID(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // pre-cancel so the blocking client fires immediately + + runner := &SpeedtestRunner{ + Server: "1234", + client: contextBlockingClient{}, + } + + result, err := runner.Run(ctx) + + assert.Nil(t, result) + assert.ErrorIs(t, err, context.Canceled, + "Run must propagate context.Canceled from FetchServerByIDContext") + assert.Contains(t, err.Error(), "fetch server", + "error must carry 'fetch server' context for operator diagnostics") +} + +// TestSpeedtestRunner_ContextDeadlineBeforeFetchServerByID mirrors the above +// for context.DeadlineExceeded, which is the error produced by the collector's +// WithTimeout when the full speedtest takes too long. +func TestSpeedtestRunner_ContextDeadlineBeforeFetchServerByID(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Microsecond) + defer cancel() + time.Sleep(5 * time.Millisecond) // let the deadline expire + + runner := &SpeedtestRunner{ + Server: "1234", + client: contextBlockingClient{}, + } + + result, err := runner.Run(ctx) + + assert.Nil(t, result) + assert.ErrorIs(t, err, context.DeadlineExceeded, + "Run must propagate context.DeadlineExceeded from FetchServerByIDContext") + assert.Contains(t, err.Error(), "fetch server") +} + +// TestSpeedtestRunner_ContextCancelledBeforeFetchServers tests the no-server-ID +// path where FetchServerListContext is called. Cancelling before the call must +// surface as context.Canceled wrapped with "fetch servers". +func TestSpeedtestRunner_ContextCancelledBeforeFetchServers(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + runner := &SpeedtestRunner{ + Server: "", + client: contextBlockingClient{}, + } + + result, err := runner.Run(ctx) + + assert.Nil(t, result) + assert.ErrorIs(t, err, context.Canceled, + "Run must propagate context.Canceled from FetchServerListContext") + assert.Contains(t, err.Error(), "fetch servers", + "error must carry 'fetch servers' context for operator diagnostics") +} + +// TestSpeedtestRunner_ContextDeadlineBeforeFetchServers mirrors the above for +// context.DeadlineExceeded on the server-list path. +func TestSpeedtestRunner_ContextDeadlineBeforeFetchServers(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Microsecond) + defer cancel() + time.Sleep(5 * time.Millisecond) + + runner := &SpeedtestRunner{ + Server: "", + client: contextBlockingClient{}, + } + + result, err := runner.Run(ctx) + + assert.Nil(t, result) + assert.ErrorIs(t, err, context.DeadlineExceeded, + "Run must propagate context.DeadlineExceeded from FetchServerListContext") + assert.Contains(t, err.Error(), "fetch servers") +} + +// TestSpeedtestRunner_ContextForwardedToFetchServerByID verifies that the +// context passed to Run is the exact same context forwarded to +// FetchServerByIDContext — not a detached background context. +func TestSpeedtestRunner_ContextForwardedToFetchServerByID(t *testing.T) { + mockServer := createMockServer("42", "Forward ISP", 10*time.Millisecond, 100000, 50000, 2*time.Millisecond) + client := &contextCapturingClient{server: mockServer} + + ctx := context.WithValue(context.Background(), struct{ key string }{"trace"}, "test-trace-id") + + runner := &SpeedtestRunner{Server: "42", client: client} + _, err := runner.Run(ctx) + + require.NoError(t, err) + assert.Equal(t, ctx, client.capturedCtx, + "FetchServerByIDContext must receive the exact context passed to Run, not a new one") +} + +// TestSpeedtestRunner_ContextForwardedToFetchServers verifies context identity +// on the server-list path. +func TestSpeedtestRunner_ContextForwardedToFetchServers(t *testing.T) { + mockServer := createMockServer("99", "Forward ISP", 10*time.Millisecond, 100000, 50000, 2*time.Millisecond) + client := &contextCapturingClient{server: mockServer} + + ctx := context.WithValue(context.Background(), struct{ key string }{"trace"}, "test-trace-id") + + runner := &SpeedtestRunner{Server: "", client: client} + _, err := runner.Run(ctx) + + require.NoError(t, err) + assert.Equal(t, ctx, client.capturedCtx, + "FetchServerListContext must receive the exact context passed to Run, not a new one") +} From d6ee3ef78891d84d44f8ab85c0ce56fcbd681e00 Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Sat, 14 Mar 2026 03:20:42 +0800 Subject: [PATCH 2/6] fix: guard nil result and distinguish timeout errors in collector Add a nil-result guard in Collect so a (nil, nil) return from Runner no longer panics at the field access on line 89; emit zeroed metrics with speedtest_up=0 instead. Distinguish context.DeadlineExceeded from other errors in log output ('Speedtest timed out' vs 'Speedtest failed') to help operators tell a collection timeout apart from a network failure. --- internal/exporter/collector.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/exporter/collector.go b/internal/exporter/collector.go index cafc841..3edbfef 100644 --- a/internal/exporter/collector.go +++ b/internal/exporter/collector.go @@ -4,6 +4,7 @@ package exporter import ( "context" + "errors" "time" "github.com/prometheus/client_golang/prometheus" @@ -71,7 +72,21 @@ func (se SpeedtestCollector) Collect(ch chan<- prometheus.Metric) { s, err := se.runner.Run(ctx) if err != nil { - log.WithError(err).Error("Speedtest failed") + if errors.Is(err, context.DeadlineExceeded) { + log.WithError(err).Error("Speedtest timed out") + } else { + log.WithError(err).Error("Speedtest failed") + } + ch <- prometheus.MustNewConstMetric(server, prometheus.GaugeValue, 0) + ch <- prometheus.MustNewConstMetric(jitter, prometheus.GaugeValue, 0) + ch <- prometheus.MustNewConstMetric(ping, prometheus.GaugeValue, 0) + ch <- prometheus.MustNewConstMetric(downloadSpeed, prometheus.GaugeValue, 0) + ch <- prometheus.MustNewConstMetric(uploadSpeed, prometheus.GaugeValue, 0) + ch <- prometheus.MustNewConstMetric(up, prometheus.GaugeValue, 0) + return + } + if s == nil { + log.Error("Speedtest returned no result") ch <- prometheus.MustNewConstMetric(server, prometheus.GaugeValue, 0) ch <- prometheus.MustNewConstMetric(jitter, prometheus.GaugeValue, 0) ch <- prometheus.MustNewConstMetric(ping, prometheus.GaugeValue, 0) From 8b4574c79aa92d50651e87940f933feeffb9a1f1 Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Sat, 14 Mar 2026 03:21:02 +0800 Subject: [PATCH 3/6] test: expand context cancellation, error-path, and registration coverage Add 14 new tests across runner_test.go and collector_test.go: runner_test.go (8 tests): - ContextCancelledBefore/DeadlineBefore FetchServerByID and FetchServers: verify cancellation and timeout propagate end-to-end through the new context-aware interface methods - ContextForwardedTo FetchServerByID and FetchServers: assert the exact context identity is forwarded, not a detached background context collector_test.go (6 tests): - DeadlineExceededLogsTimedOut, WrappedDeadlineExceededLogsTimedOut: verify the new errors.Is branch fires for direct and wrapped deadlines - NonDeadlineErrorLogsFailed: verify non-deadline errors still log 'failed' - BothResultAndErrorTakesErrorPath: (result, err) both non-nil takes error path - ContextHasDeadline: Collect creates a deadline-bearing context before Run - NilResultNilErrorDoesNotPanic: (nil, nil) from runner emits zeroed metrics Also merge registerer_test.go into collector_test.go since RegisterSpeedtestCollector lives in collector.go; delete the orphan file. --- internal/exporter/collector_test.go | 267 +++++++++++++++++++++++++++ internal/exporter/registerer_test.go | 55 ------ internal/exporter/runner_test.go | 6 +- 3 files changed, 270 insertions(+), 58 deletions(-) delete mode 100644 internal/exporter/registerer_test.go diff --git a/internal/exporter/collector_test.go b/internal/exporter/collector_test.go index 2689a93..a4e1a4f 100644 --- a/internal/exporter/collector_test.go +++ b/internal/exporter/collector_test.go @@ -3,12 +3,15 @@ package exporter import ( "context" "errors" + "fmt" "strings" + "sync" "testing" "time" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/testutil" + log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -284,3 +287,267 @@ func TestSpeedtestCollector_ZeroValueSuccessResult(t *testing.T) { "speedtest_up must be 1 even when all other result fields are 0 (success is success)") assert.Len(t, gathered, 6, "all 6 metric families must be present for zero-value success") } + +// logCaptureHook records logrus log entries during a test. It is safe for +// concurrent use (Prometheus may call Collect from multiple goroutines). +type logCaptureHook struct { + mu sync.Mutex + entries []*log.Entry +} + +func (h *logCaptureHook) Levels() []log.Level { return log.AllLevels } + +func (h *logCaptureHook) Fire(e *log.Entry) error { + h.mu.Lock() + h.entries = append(h.entries, e) + h.mu.Unlock() + return nil +} + +func (h *logCaptureHook) messages() []string { + h.mu.Lock() + defer h.mu.Unlock() + msgs := make([]string, len(h.entries)) + for i, e := range h.entries { + msgs[i] = e.Message + } + return msgs +} + +// contextCapturingRunner records the context it was called with so tests can +// assert that Collect creates and forwards a deadline-bearing context. +type contextCapturingRunner struct { + capturedCtx context.Context + result *SpeedtestResult + err error +} + +func (r *contextCapturingRunner) Run(ctx context.Context) (*SpeedtestResult, error) { + r.capturedCtx = ctx + return r.result, r.err +} + +// installLogHook registers hook on the global logrus logger and schedules its +// removal via t.Cleanup, so the hook does not bleed into other tests. +func installLogHook(t *testing.T, hook log.Hook) { + t.Helper() + log.AddHook(hook) + t.Cleanup(func() { log.StandardLogger().ReplaceHooks(make(log.LevelHooks)) }) +} + +// TestSpeedtestCollector_DeadlineExceededLogsTimedOut verifies that when the +// runner returns context.DeadlineExceeded, Collect logs "Speedtest timed out" +// rather than the generic "Speedtest failed" message. This exercises the +// errors.Is branch added in the error-handling implementation. +func TestSpeedtestCollector_DeadlineExceededLogsTimedOut(t *testing.T) { + hook := &logCaptureHook{} + installLogHook(t, hook) + + collector := SpeedtestCollector{ + runner: MockRunner{result: nil, err: context.DeadlineExceeded}, + timeout: 5 * time.Second, + } + + ch := make(chan prometheus.Metric, 10) + collector.Collect(ch) + close(ch) + + msgs := hook.messages() + assert.Contains(t, msgs, "Speedtest timed out", + "context.DeadlineExceeded must produce 'Speedtest timed out' log") + assert.NotContains(t, msgs, "Speedtest failed", + "context.DeadlineExceeded must not produce the generic 'Speedtest failed' log") +} + +// TestSpeedtestCollector_NonDeadlineErrorLogsFailed verifies that non-timeout +// runner errors emit the generic "Speedtest failed" log, not "Speedtest timed out". +func TestSpeedtestCollector_NonDeadlineErrorLogsFailed(t *testing.T) { + hook := &logCaptureHook{} + installLogHook(t, hook) + + collector := SpeedtestCollector{ + runner: MockRunner{result: nil, err: errors.New("network unreachable")}, + timeout: 5 * time.Second, + } + + ch := make(chan prometheus.Metric, 10) + collector.Collect(ch) + close(ch) + + msgs := hook.messages() + assert.Contains(t, msgs, "Speedtest failed", + "non-deadline error must produce 'Speedtest failed' log") + assert.NotContains(t, msgs, "Speedtest timed out", + "non-deadline error must not produce 'Speedtest timed out' log") +} + +// TestSpeedtestCollector_BothResultAndErrorTakesErrorPath verifies that when +// the runner returns (non-nil result, non-nil error), the error path is taken: +// speedtest_up=0 and all metrics are zeroed. Without this test the error-path +// branch is only exercised with a nil result, which is the usual case. +func TestSpeedtestCollector_BothResultAndErrorTakesErrorPath(t *testing.T) { + partialResult := &SpeedtestResult{ + ServerID: 42, + DownloadSpeed: 1e9, + UploadSpeed: 5e8, + Ping: 10.0, + Jitter: 2.0, + } + + collector := SpeedtestCollector{ + runner: MockRunner{result: partialResult, err: errors.New("partial failure")}, + timeout: 5 * time.Second, + } + + registry := prometheus.NewPedanticRegistry() + registry.MustRegister(collector) + + gathered, err := registry.Gather() + require.NoError(t, err) + + values := make(map[string]float64, len(gathered)) + for _, mf := range gathered { + if len(mf.GetMetric()) > 0 { + values[mf.GetName()] = mf.GetMetric()[0].GetGauge().GetValue() + } + } + + assert.Equal(t, float64(0), values["speedtest_up"], + "error path must be taken when runner returns (result, error); speedtest_up must be 0") + assert.Equal(t, float64(0), values["speedtest_server_id"], + "server_id must be 0 when runner returns (result, error)") + assert.Equal(t, float64(0), values["speedtest_download_bits_per_second"], + "download speed must be 0 when runner returns (result, error)") + assert.Len(t, gathered, 6, "all 6 metric families must be present even with (result, error)") +} + +// TestSpeedtestCollector_NilResultNilErrorDoesNotPanic documents the desired +// behaviour when a Runner violates its contract by returning (nil, nil): +// Collect must not panic and must emit speedtest_up=0 with zeroed metrics. +// +// NOTE: Collect is called directly (not via registry.Gather) so that +// assert.NotPanics can recover the panic from the same goroutine. +func TestSpeedtestCollector_NilResultNilErrorDoesNotPanic(t *testing.T) { + hook := &logCaptureHook{} + installLogHook(t, hook) + + collector := SpeedtestCollector{ + runner: MockRunner{result: nil, err: nil}, + timeout: 5 * time.Second, + } + + ch := make(chan prometheus.Metric, 10) + + assert.NotPanics(t, func() { collector.Collect(ch) }, + "Collect must not panic when runner returns (nil, nil)") + close(ch) + + assert.Contains(t, hook.messages(), "Speedtest returned no result", + "(nil, nil) runner result must log 'Speedtest returned no result'") + + // Collect must emit 6 zero metrics with up=0. + count := 0 + for range ch { + count++ + } + assert.Equal(t, 6, count, + "Collect must emit all 6 zero metrics for (nil, nil) runner result") +} + +// TestSpeedtestCollector_ContextHasDeadline verifies that the context Collect +// creates and passes to the runner carries a deadline. If this is missing, the +// timeout guard in Collect is effectively disabled. +func TestSpeedtestCollector_ContextHasDeadline(t *testing.T) { + runner := &contextCapturingRunner{ + result: &SpeedtestResult{ServerID: 1}, + } + + collector := SpeedtestCollector{ + runner: runner, + timeout: 30 * time.Second, + } + + ch := make(chan prometheus.Metric, 10) + collector.Collect(ch) + close(ch) + + require.NotNil(t, runner.capturedCtx, + "Collect must call runner.Run with a non-nil context") + deadline, ok := runner.capturedCtx.Deadline() + assert.True(t, ok, + "context passed to runner must have a deadline (timeout guard must be active)") + assert.True(t, deadline.After(time.Now()), + "deadline must be in the future when the timeout has not yet elapsed") +} + +// TestSpeedtestCollector_WrappedDeadlineExceededLogsTimedOut verifies that a +// wrapped context.DeadlineExceeded (as produced by the runner's fmt.Errorf %w +// chain) is still identified correctly via errors.Is and logs "Speedtest timed out". +func TestSpeedtestCollector_WrappedDeadlineExceededLogsTimedOut(t *testing.T) { + hook := &logCaptureHook{} + installLogHook(t, hook) + + wrappedDeadline := fmt.Errorf("ping test: %w", context.DeadlineExceeded) + + collector := SpeedtestCollector{ + runner: MockRunner{result: nil, err: wrappedDeadline}, + timeout: 5 * time.Second, + } + + ch := make(chan prometheus.Metric, 10) + collector.Collect(ch) + close(ch) + + assert.Contains(t, hook.messages(), "Speedtest timed out", + "wrapped context.DeadlineExceeded must still produce 'Speedtest timed out' via errors.Is") +} + +// fakeRegisterer implements prometheus.Registerer and records registered +// collectors for assertions in tests. +type fakeRegisterer struct { + collectors []prometheus.Collector +} + +func (f *fakeRegisterer) Register(c prometheus.Collector) error { + f.collectors = append(f.collectors, c) + return nil +} + +func (f *fakeRegisterer) MustRegister(cs ...prometheus.Collector) { + for _, c := range cs { + _ = f.Register(c) + } +} + +func (f *fakeRegisterer) Unregister(c prometheus.Collector) bool { + for i, col := range f.collectors { + if col == c { + f.collectors = append(f.collectors[:i], f.collectors[i+1:]...) + return true + } + } + return false +} + +func TestNewSpeedtestRunnerRegistersCollector(t *testing.T) { + fr := &fakeRegisterer{} + + // Create a new runner which should register a SpeedtestCollector via + // RegisterSpeedtestCollector. + r := NewSpeedtestRunner("42", fr, nil) + + assert.NotNil(t, r) + assert.Equal(t, "42", r.Server) + assert.NotNil(t, r.client) + + // Ensure one collector was registered + assert.Len(t, fr.collectors, 1) + + // Assert the registered collector is a SpeedtestCollector whose runner is r + // and that the timeout was wired to DefaultCollectTimeout. + sc, ok := fr.collectors[0].(SpeedtestCollector) + assert.True(t, ok, "registered collector should be SpeedtestCollector") + assert.Equal(t, r, sc.runner) + assert.Equal(t, DefaultCollectTimeout, sc.timeout, + "registered collector must use DefaultCollectTimeout") +} diff --git a/internal/exporter/registerer_test.go b/internal/exporter/registerer_test.go deleted file mode 100644 index 9f3b01d..0000000 --- a/internal/exporter/registerer_test.go +++ /dev/null @@ -1,55 +0,0 @@ -package exporter - -import ( - "testing" - - "github.com/prometheus/client_golang/prometheus" - "github.com/stretchr/testify/assert" -) - -// fakeRegisterer implements prometheus.Registerer and records registered -// collectors for assertions in tests. -type fakeRegisterer struct { - collectors []prometheus.Collector -} - -func (f *fakeRegisterer) Register(c prometheus.Collector) error { - f.collectors = append(f.collectors, c) - return nil -} - -func (f *fakeRegisterer) MustRegister(cs ...prometheus.Collector) { - for _, c := range cs { - _ = f.Register(c) - } -} - -func (f *fakeRegisterer) Unregister(c prometheus.Collector) bool { - for i, col := range f.collectors { - if col == c { - f.collectors = append(f.collectors[:i], f.collectors[i+1:]...) - return true - } - } - return false -} - -func TestNewSpeedtestRunnerRegistersCollector(t *testing.T) { - fr := &fakeRegisterer{} - - // Create a new runner which should register a SpeedtestCollector via - // RegisterSpeedtestCollector. - r := NewSpeedtestRunner("42", fr, nil) - - assert.NotNil(t, r) - assert.Equal(t, "42", r.Server) - assert.NotNil(t, r.client) - - // Ensure one collector was registered - assert.Len(t, fr.collectors, 1) - - // Assert the registered collector is a SpeedtestCollector whose runner is r - sc, ok := fr.collectors[0].(SpeedtestCollector) - assert.True(t, ok, "registered collector should be SpeedtestCollector") - assert.Equal(t, r, sc.runner) -} diff --git a/internal/exporter/runner_test.go b/internal/exporter/runner_test.go index 6e802d6..a83b0d5 100644 --- a/internal/exporter/runner_test.go +++ b/internal/exporter/runner_test.go @@ -207,7 +207,7 @@ func TestNewSpeedtestRunner_WithNilClient(t *testing.T) { assert.NotNil(t, runner.client) } -// TestSpeedtestRunner_FetchServerByIDError verifies that when FetchServerByID +// TestSpeedtestRunner_FetchServerByIDError verifies that when FetchServerByIDContext // returns an error, Run returns (nil, non-nil error) and does not panic. func TestSpeedtestRunner_FetchServerByIDError(t *testing.T) { mockClient := new(MockSpeedtestClient) @@ -225,7 +225,7 @@ func TestSpeedtestRunner_FetchServerByIDError(t *testing.T) { mockClient.AssertCalled(t, "FetchServerByIDContext", mock.Anything, "1234") } -// TestSpeedtestRunner_FetchServersError verifies that when FetchServers returns +// TestSpeedtestRunner_FetchServersError verifies that when FetchServerListContext returns // an error (no explicit server ID configured), Run returns (nil, non-nil error). func TestSpeedtestRunner_FetchServersError(t *testing.T) { mockClient := new(MockSpeedtestClient) @@ -310,7 +310,7 @@ func TestSpeedtestRunner_FetchServerByIDError_WrapsError(t *testing.T) { } // TestSpeedtestRunner_FetchServersError_WrapsError verifies that the error -// returned when FetchServers fails contains identifying context. +// returned when FetchServerListContext fails contains identifying context. func TestSpeedtestRunner_FetchServersError_WrapsError(t *testing.T) { mockClient := new(MockSpeedtestClient) mockClient.On("FetchServerListContext", mock.Anything).Return(nil, assert.AnError) From e5724c6de09c82442d10e29a309ceab05d343060 Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Sat, 14 Mar 2026 03:21:35 +0800 Subject: [PATCH 4/6] docs: update README for metric accuracy and completeness - Fix SPEEDTEST_EXPORTER_DEBUG default: any non-empty string enables it (not a boolean false), matching the os.Getenv(...) != "" check in main.go - Add Unit column to metrics table (milliseconds / bits per second / dimensionless) - Document that no labels are present on any metric - Add verbatim example Prometheus exposition output - Name the debug collectors explicitly (go_* and process_*) --- README.md | 50 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index aaaa163..ae5a11a 100644 --- a/README.md +++ b/README.md @@ -37,7 +37,7 @@ Download the latest release from the [releases page](https://github.com/zakame/s ### Building from Source Requirements: -- Go 1.25 or later +- Go 1.26 or later ```bash git clone https://github.com/zakame/speedtest-go-exporter.git @@ -61,7 +61,7 @@ Configuration is done via environment variables: |----------|-------------|---------| | `SPEEDTEST_PORT` | Port to listen on | `9798` | | `SPEEDTEST_SERVER` | Speedtest server ID to use (optional, auto-selects if not set) | `` | -| `SPEEDTEST_EXPORTER_DEBUG` | Enable debug mode (adds Go runtime and process metrics) | `false` | +| `SPEEDTEST_EXPORTER_DEBUG` | Enable debug mode — any non-empty value enables it (adds Go runtime and process metrics) | `` | ### Example @@ -74,22 +74,48 @@ export SPEEDTEST_EXPORTER_DEBUG=1 ## Metrics -The exporter provides the following Prometheus metrics: +The exporter provides the following Prometheus metrics. None carry labels. -| Metric | Type | Description | -|--------|------|-------------| -| `speedtest_server_id` | Gauge | Speedtest server ID used for the test | -| `speedtest_jitter_latency_milliseconds` | Gauge | Jitter latency in milliseconds | -| `speedtest_ping_latency_milliseconds` | Gauge | Ping latency in milliseconds | -| `speedtest_download_bits_per_second` | Gauge | Download speed in bits per second | -| `speedtest_upload_bits_per_second` | Gauge | Upload speed in bits per second | -| `speedtest_up` | Gauge | Speedtest up status (`1` = successful, `0` = failed) | +| Metric | Type | Unit | Description | +|--------|------|------|-------------| +| `speedtest_server_id` | Gauge | — | Numeric ID of the speedtest server used for the test | +| `speedtest_jitter_latency_milliseconds` | Gauge | milliseconds | Jitter latency | +| `speedtest_ping_latency_milliseconds` | Gauge | milliseconds | Ping (round-trip) latency | +| `speedtest_download_bits_per_second` | Gauge | bits/second | Download speed | +| `speedtest_upload_bits_per_second` | Gauge | bits/second | Upload speed | +| `speedtest_up` | Gauge | — | `1` if the last speedtest succeeded, `0` if it failed | + +### Example output + +``` +# HELP speedtest_download_bits_per_second Speedtest download speed in bits per second. +# TYPE speedtest_download_bits_per_second gauge +speedtest_download_bits_per_second 9.4e+08 +# HELP speedtest_jitter_latency_milliseconds Speedtest jitter latency in milliseconds. +# TYPE speedtest_jitter_latency_milliseconds gauge +speedtest_jitter_latency_milliseconds 0.512 +# HELP speedtest_ping_latency_milliseconds Speedtest ping latency in milliseconds. +# TYPE speedtest_ping_latency_milliseconds gauge +speedtest_ping_latency_milliseconds 7.25 +# HELP speedtest_server_id Speedtest server ID. +# TYPE speedtest_server_id gauge +speedtest_server_id 12345 +# HELP speedtest_up Speedtest up status. +# TYPE speedtest_up gauge +speedtest_up 1 +# HELP speedtest_upload_bits_per_second Speedtest upload speed in bits per second. +# TYPE speedtest_upload_bits_per_second gauge +speedtest_upload_bits_per_second 5.2e+08 +``` + +### Failure behaviour When a speedtest fails (network error, server unreachable, timeout), `speedtest_up` is set to `0` and all other metrics are set to `0`. Use `speedtest_up == 0` as the signal in alerts — the zeroed values for speed/latency metrics on failure should be ignored. -When `SPEEDTEST_EXPORTER_DEBUG` is enabled, additional Go runtime metrics are also exposed. +When `SPEEDTEST_EXPORTER_DEBUG` is set, additional Go runtime metrics (`go_*`) and process +metrics (`process_*`) from the standard Prometheus Go client collectors are also exposed. ## Kubernetes Deployment From 4b3d3d7ea0a9c1c97dccb13dbfc3db87e43cece5 Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Sat, 14 Mar 2026 03:21:48 +0800 Subject: [PATCH 5/6] build: add Makefile and bump go directive to 1.26 Add Makefile with targets: build, test, lint (golangci-lint with go vet fallback), run (respects SPEEDTEST_PORT/SPEEDTEST_SERVER/ SPEEDTEST_EXPORTER_DEBUG), clean, and help (default goal). Bump go directive from 1.25 to 1.26 to pick up security fixes in net/url, crypto/tls, html/template, and os included in the 1.26 line. The installed toolchain (go1.26.1) already satisfies this requirement. --- Makefile | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ go.mod | 2 +- 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 Makefile diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..3b939e2 --- /dev/null +++ b/Makefile @@ -0,0 +1,48 @@ +# Makefile for github.com/zakame/speedtest-go-exporter + +BINARY := speedtest-go-exporter +CMD := ./cmd/$(BINARY) +BIN_DIR := ./bin +CGO_ENABLED ?= 0 + +.DEFAULT_GOAL := help + +.PHONY: help build test cover lint vet fmt run clean + +help: ## Show this help message + @echo "Usage: make " + @echo "" + @grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) \ + | sort \ + | awk 'BEGIN {FS = ":.*?## "}; {printf " \033[36m%-10s\033[0m %s\n", $$1, $$2}' + +build: ## Compile the binary to ./bin/speedtest-go-exporter + @mkdir -p $(BIN_DIR) + CGO_ENABLED=$(CGO_ENABLED) go build -o $(BIN_DIR)/$(BINARY) $(CMD) + +test: ## Run all tests + go test -v -count=1 ./... + +cover: ## Run tests with coverage report + go test -cover -coverpkg=./... -coverprofile=coverage.txt -covermode=atomic ./... + go tool cover -func=coverage.txt + +lint: ## Run golangci-lint if available, otherwise fall back to go vet + @if command -v golangci-lint >/dev/null 2>&1; then \ + golangci-lint run; \ + else \ + echo "golangci-lint not found, falling back to go vet"; \ + go vet ./...; \ + fi + +vet: ## Run go vet + go vet ./... + +fmt: ## Check formatting (exits non-zero if files need formatting) + @test -z "$$(gofmt -l .)" || (gofmt -l . && exit 1) + +run: build ## Build and run the exporter (env: SPEEDTEST_PORT, SPEEDTEST_SERVER, SPEEDTEST_EXPORTER_DEBUG) + $(BIN_DIR)/$(BINARY) + +clean: ## Remove build artifacts (./bin/, coverage.txt) + rm -rf $(BIN_DIR) coverage.txt diff --git a/go.mod b/go.mod index 4ef1b28..745f6a7 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/zakame/speedtest-go-exporter -go 1.25 +go 1.26 require ( github.com/prometheus/client_golang v1.23.2 From 97e36d14aec8bc7574b0691668aea106e5e09758 Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Sat, 14 Mar 2026 03:26:12 +0800 Subject: [PATCH 6/6] ci: bump golangci-lint to v2.11.3 for Go 1.26 compatibility v2.5.0 was built with Go 1.25 and refuses to lint modules targeting Go 1.26, causing the lint job to exit with code 3. v2.11.3 is the latest release and supports Go 1.26. --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 324247e..7e5aef9 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -16,7 +16,7 @@ jobs: go-version-file: go.mod - uses: golangci/golangci-lint-action@1e7e51e771db61008b38414a730f564565cf7c20 # v9.2.0 with: - version: v2.5.0 + version: v2.11.3 test: runs-on: ubuntu-latest