From cca65ef85502e395e8877a3dbba84c71f319afb6 Mon Sep 17 00:00:00 2001 From: soup Date: Tue, 31 Mar 2026 22:27:16 +0200 Subject: [PATCH 1/5] feat(speedtest): add result URL support for LibreSpeed (#159) Capture the shareable link returned by librespeed-cli --share, persist it in a new result_url column, and render it as a clickable icon in the results table. The --share flag is opt-in via share_results config. --- config/config.toml | 2 + internal/config/config.go | 16 ++++++- .../postgres/021_add_result_url_postgres.sql | 2 + .../migrations/sqlite/021_add_result_url.sql | 2 + internal/database/speedtest.go | 3 ++ internal/speedtest/librespeed.go | 5 +++ internal/speedtest/librespeed_test.go | 28 ++++++++++++ internal/speedtest/result_handler.go | 6 +++ internal/speedtest/result_handler_test.go | 45 +++++++++++++++++++ internal/speedtest/types.go | 7 +-- internal/types/types.go | 1 + web/src/components/speedtest/columns.tsx | 27 ++++++++++- web/src/types/types.ts | 1 + 13 files changed, 138 insertions(+), 7 deletions(-) create mode 100644 internal/database/migrations/postgres/021_add_result_url_postgres.sql create mode 100644 internal/database/migrations/sqlite/021_add_result_url.sql create mode 100644 internal/speedtest/result_handler_test.go diff --git a/config/config.toml b/config/config.toml index 831f7f0..a5d8439 100644 --- a/config/config.toml +++ b/config/config.toml @@ -36,6 +36,8 @@ timeout = 10 [speedtest.librespeed] timeout = 60 +# Submit results to librespeed.org for a shareable link (sends telemetry) +share_results = false [geoip] country_database_path = "./GeoLite2-Country.mmdb" diff --git a/internal/config/config.go b/internal/config/config.go index 4a7e46c..ed3cb96 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -99,8 +99,9 @@ type IperfConfig struct { } type LibrespeedConfig struct { - ServersPath string `toml:"-"` - Timeout int `toml:"timeout" env:"LIBRESPEED_TIMEOUT"` + ServersPath string `toml:"-"` + Timeout int `toml:"timeout" env:"LIBRESPEED_TIMEOUT"` + ShareResults bool `toml:"share_results" env:"LIBRESPEED_SHARE_RESULTS"` } type PingConfig struct { @@ -540,6 +541,11 @@ func (c *Config) loadSpeedTestFromEnv() { c.SpeedTest.Librespeed.Timeout = val } } + if v := getEnv("LIBRESPEED_SHARE_RESULTS"); v != "" { + if val, err := strconv.ParseBool(v); err == nil { + c.SpeedTest.Librespeed.ShareResults = val + } + } } func (c *Config) loadPaginationFromEnv() { @@ -827,6 +833,12 @@ func (c *Config) WriteToml(w io.Writer) error { if _, err := fmt.Fprintf(w, "timeout = %d\n", cfg.SpeedTest.Librespeed.Timeout); err != nil { return err } + if _, err := fmt.Fprintf(w, "# Submit results to librespeed.org for a shareable link (sends telemetry)\n"); err != nil { + return err + } + if _, err := fmt.Fprintf(w, "share_results = %v\n", cfg.SpeedTest.Librespeed.ShareResults); err != nil { + return err + } if _, err := fmt.Fprintln(w, ""); err != nil { return err } diff --git a/internal/database/migrations/postgres/021_add_result_url_postgres.sql b/internal/database/migrations/postgres/021_add_result_url_postgres.sql new file mode 100644 index 0000000..b8f53b5 --- /dev/null +++ b/internal/database/migrations/postgres/021_add_result_url_postgres.sql @@ -0,0 +1,2 @@ +-- Add result URL column for shareable speed test links (e.g. LibreSpeed share URLs) +ALTER TABLE speed_tests ADD COLUMN result_url TEXT; diff --git a/internal/database/migrations/sqlite/021_add_result_url.sql b/internal/database/migrations/sqlite/021_add_result_url.sql new file mode 100644 index 0000000..b8f53b5 --- /dev/null +++ b/internal/database/migrations/sqlite/021_add_result_url.sql @@ -0,0 +1,2 @@ +-- Add result URL column for shareable speed test links (e.g. LibreSpeed share URLs) +ALTER TABLE speed_tests ADD COLUMN result_url TEXT; diff --git a/internal/database/speedtest.go b/internal/database/speedtest.go index d019905..7cc458a 100644 --- a/internal/database/speedtest.go +++ b/internal/database/speedtest.go @@ -23,6 +23,7 @@ func (s *service) SaveSpeedTest(ctx context.Context, result types.SpeedTestResul "latency": result.Latency, "jitter": result.Jitter, "is_scheduled": result.IsScheduled, + "result_url": result.ResultURL, } // Use provided created_at if available, otherwise default to current UTC time @@ -120,6 +121,7 @@ func (s *service) GetSpeedTests(ctx context.Context, timeRange string, page, lim "latency", "jitter", "is_scheduled", + "result_url", "created_at", ). OrderBy("created_at DESC"). @@ -146,6 +148,7 @@ func (s *service) GetSpeedTests(ctx context.Context, timeRange string, page, lim &result.Latency, &result.Jitter, &result.IsScheduled, + &result.ResultURL, &result.CreatedAt, ) if err != nil { diff --git a/internal/speedtest/librespeed.go b/internal/speedtest/librespeed.go index 4d78efb..6ec7fbe 100644 --- a/internal/speedtest/librespeed.go +++ b/internal/speedtest/librespeed.go @@ -151,6 +151,7 @@ func (r *LibrespeedRunner) RunTest(ctx context.Context, opts *types.TestOptions) UploadSpeed: librespeedResult.Upload, Latency: fmt.Sprintf("%.2f", librespeedResult.Ping), Jitter: librespeedResult.Jitter, + ResultURL: librespeedResult.Share, } // Final completion update @@ -172,6 +173,10 @@ func (r *LibrespeedRunner) RunTest(ctx context.Context, opts *types.TestOptions) func (r *LibrespeedRunner) buildArgs(opts *types.TestOptions) []string { args := []string{"--json"} + if r.config.ShareResults { + args = append(args, "--share") + } + if opts.IsPublicServer { // Keep CLI server IDs in sync with our fetched public list. args = append(args, "--server-json", librespeedPublicServersURL) diff --git a/internal/speedtest/librespeed_test.go b/internal/speedtest/librespeed_test.go index 7ff097f..caa2f77 100644 --- a/internal/speedtest/librespeed_test.go +++ b/internal/speedtest/librespeed_test.go @@ -29,6 +29,34 @@ func TestBuildArgsUsesServerJSONForPublicServers(t *testing.T) { }, args) } +func TestBuildArgsIncludesShareWhenEnabled(t *testing.T) { + runner := NewLibrespeedRunner(config.LibrespeedConfig{ + ServersPath: "/tmp/local-servers.json", + ShareResults: true, + }) + + args := runner.buildArgs(&types.TestOptions{ + IsPublicServer: true, + ServerIDs: []string{"123"}, + }) + + assert.Contains(t, args, "--share") +} + +func TestBuildArgsExcludesShareWhenDisabled(t *testing.T) { + runner := NewLibrespeedRunner(config.LibrespeedConfig{ + ServersPath: "/tmp/local-servers.json", + ShareResults: false, + }) + + args := runner.buildArgs(&types.TestOptions{ + IsPublicServer: true, + ServerIDs: []string{"123"}, + }) + + assert.NotContains(t, args, "--share") +} + func TestBuildArgsUsesLocalJSONForCustomServers(t *testing.T) { runner := NewLibrespeedRunner(config.LibrespeedConfig{ ServersPath: "/tmp/local-servers.json", diff --git a/internal/speedtest/result_handler.go b/internal/speedtest/result_handler.go index 45ec4aa..eaae170 100644 --- a/internal/speedtest/result_handler.go +++ b/internal/speedtest/result_handler.go @@ -57,6 +57,11 @@ func (h *DefaultResultHandler) SaveResult(ctx context.Context, result *Result, t jitterPtr = &result.Jitter } + var resultURL *string + if result.ResultURL != "" { + resultURL = &result.ResultURL + } + // Give the DB write up to 10s while still respecting upstream cancellations and values. saveCtx, saveCancel := context.WithTimeout(ctx, 10*time.Second) defer saveCancel() @@ -73,6 +78,7 @@ func (h *DefaultResultHandler) SaveResult(ctx context.Context, result *Result, t Latency: result.Latency, Jitter: jitterPtr, IsScheduled: opts.IsScheduled, + ResultURL: resultURL, CreatedAt: createdAt, }) if err != nil { diff --git a/internal/speedtest/result_handler_test.go b/internal/speedtest/result_handler_test.go new file mode 100644 index 0000000..f688a90 --- /dev/null +++ b/internal/speedtest/result_handler_test.go @@ -0,0 +1,45 @@ +// Copyright (c) 2024-2026, s0up and the autobrr contributors. +// SPDX-License-Identifier: GPL-2.0-or-later + +package speedtest + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestResultURLNormalization(t *testing.T) { + tests := []struct { + name string + resultURL string + wantNil bool + }{ + { + name: "empty string produces nil", + resultURL: "", + wantNil: true, + }, + { + name: "non-empty string produces pointer", + resultURL: "https://librespeed.org/results/?id=abc123", + wantNil: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var resultURL *string + if tt.resultURL != "" { + resultURL = &tt.resultURL + } + + if tt.wantNil { + assert.Nil(t, resultURL) + } else { + assert.NotNil(t, resultURL) + assert.Equal(t, tt.resultURL, *resultURL) + } + }) + } +} diff --git a/internal/speedtest/types.go b/internal/speedtest/types.go index 7533fa1..703f47e 100644 --- a/internal/speedtest/types.go +++ b/internal/speedtest/types.go @@ -18,6 +18,7 @@ type Result struct { UploadSpeed float64 `json:"uploadSpeed"` Latency string `json:"latency"` Jitter float64 `json:"jitter"` + ResultURL string `json:"resultUrl,omitempty"` Error string `json:"error,omitempty"` Download float64 `json:"-"` Upload float64 `json:"-"` @@ -58,13 +59,13 @@ type SpeedUpdate struct { type TestRunner interface { // RunTest executes a speed test and returns the result RunTest(ctx context.Context, opts *types.TestOptions) (*Result, error) - + // GetServers returns available servers for this test type GetServers() ([]ServerResponse, error) - + // GetTestType returns the test type identifier GetTestType() string - + // SetProgressCallback sets the callback for progress updates SetProgressCallback(callback func(types.SpeedUpdate)) } diff --git a/internal/types/types.go b/internal/types/types.go index 6cedd10..f2453c2 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -56,6 +56,7 @@ type SpeedTestResult struct { PacketLoss float64 `json:"packetLoss,omitempty"` Jitter *float64 `json:"jitter,omitempty"` IsScheduled bool `json:"isScheduled"` + ResultURL *string `json:"resultUrl,omitempty"` CreatedAt time.Time `json:"createdAt"` } diff --git a/web/src/components/speedtest/columns.tsx b/web/src/components/speedtest/columns.tsx index 35c2a11..656ab5c 100644 --- a/web/src/components/speedtest/columns.tsx +++ b/web/src/components/speedtest/columns.tsx @@ -11,6 +11,7 @@ import { createSortableHeader, createRightAlignedSortableHeader, } from "@/components/ui/data-table"; +import { ExternalLink } from "lucide-react"; import { cn } from "@/lib/utils"; import { TimeFormatSettings, formatDateTimeWithSettings } from "@/utils/timeSettings"; @@ -67,10 +68,21 @@ export const getSpeedTestColumns = ( header: "Server", cell: ({ row }) => ( {row.getValue("serverName")} + {row.original.resultUrl && ( + e.stopPropagation()} + title="View result on LibreSpeed" + > + + + )} ), enableHiding: false, // Always show server @@ -158,8 +170,19 @@ export const getSpeedTestMobileColumns = ( return (
-
+
{test.serverName} + {test.resultUrl && ( + e.stopPropagation()} + title="View result on LibreSpeed" + > + + + )}
Date: Tue, 31 Mar 2026 22:52:15 +0200 Subject: [PATCH 2/5] fix(speedtest): sanitize result URL, add aria-labels, use table-driven test Validate librespeed share URLs with net/url before persisting (reject non-http(s) or missing host). Add aria-label to icon-only result links for screen readers. Consolidate share flag tests into table-driven form. --- internal/speedtest/librespeed.go | 19 +++++- internal/speedtest/librespeed_test.go | 73 ++++++++++++++++-------- web/src/components/speedtest/columns.tsx | 2 + 3 files changed, 69 insertions(+), 25 deletions(-) diff --git a/internal/speedtest/librespeed.go b/internal/speedtest/librespeed.go index 6ec7fbe..b4e8c67 100644 --- a/internal/speedtest/librespeed.go +++ b/internal/speedtest/librespeed.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "os/exec" "strconv" @@ -151,7 +152,7 @@ func (r *LibrespeedRunner) RunTest(ctx context.Context, opts *types.TestOptions) UploadSpeed: librespeedResult.Upload, Latency: fmt.Sprintf("%.2f", librespeedResult.Ping), Jitter: librespeedResult.Jitter, - ResultURL: librespeedResult.Share, + ResultURL: sanitizeResultURL(librespeedResult.Share), } // Final completion update @@ -331,3 +332,19 @@ func parseCountryFromName(name string) string { } return "Unknown" } + +// sanitizeResultURL validates a share URL from librespeed-cli output. +// Returns the normalized URL if valid (http/https with a host), empty string otherwise. +func sanitizeResultURL(raw string) string { + if raw == "" { + return "" + } + u, err := url.Parse(raw) + if err != nil { + return "" + } + if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { + return "" + } + return u.String() +} diff --git a/internal/speedtest/librespeed_test.go b/internal/speedtest/librespeed_test.go index caa2f77..256ce92 100644 --- a/internal/speedtest/librespeed_test.go +++ b/internal/speedtest/librespeed_test.go @@ -29,32 +29,57 @@ func TestBuildArgsUsesServerJSONForPublicServers(t *testing.T) { }, args) } -func TestBuildArgsIncludesShareWhenEnabled(t *testing.T) { - runner := NewLibrespeedRunner(config.LibrespeedConfig{ - ServersPath: "/tmp/local-servers.json", - ShareResults: true, - }) - - args := runner.buildArgs(&types.TestOptions{ - IsPublicServer: true, - ServerIDs: []string{"123"}, - }) - - assert.Contains(t, args, "--share") +func TestBuildArgsShareFlag(t *testing.T) { + tests := []struct { + name string + shareResults bool + wantShare bool + }{ + {"included when enabled", true, true}, + {"excluded when disabled", false, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + runner := NewLibrespeedRunner(config.LibrespeedConfig{ + ServersPath: "/tmp/local-servers.json", + ShareResults: tt.shareResults, + }) + + args := runner.buildArgs(&types.TestOptions{ + IsPublicServer: true, + ServerIDs: []string{"123"}, + }) + + if tt.wantShare { + assert.Contains(t, args, "--share") + } else { + assert.NotContains(t, args, "--share") + } + }) + } } -func TestBuildArgsExcludesShareWhenDisabled(t *testing.T) { - runner := NewLibrespeedRunner(config.LibrespeedConfig{ - ServersPath: "/tmp/local-servers.json", - ShareResults: false, - }) - - args := runner.buildArgs(&types.TestOptions{ - IsPublicServer: true, - ServerIDs: []string{"123"}, - }) - - assert.NotContains(t, args, "--share") +func TestSanitizeResultURL(t *testing.T) { + tests := []struct { + name string + raw string + want string + }{ + {"valid https URL", "https://librespeed.org/results/?id=abc123", "https://librespeed.org/results/?id=abc123"}, + {"valid http URL", "http://librespeed.org/results/?id=abc123", "http://librespeed.org/results/?id=abc123"}, + {"empty string", "", ""}, + {"no scheme", "librespeed.org/results", ""}, + {"javascript scheme", "javascript:alert(1)", ""}, + {"ftp scheme", "ftp://example.com/file", ""}, + {"no host", "https:///path", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sanitizeResultURL(tt.raw)) + }) + } } func TestBuildArgsUsesLocalJSONForCustomServers(t *testing.T) { diff --git a/web/src/components/speedtest/columns.tsx b/web/src/components/speedtest/columns.tsx index 656ab5c..097b066 100644 --- a/web/src/components/speedtest/columns.tsx +++ b/web/src/components/speedtest/columns.tsx @@ -79,6 +79,7 @@ export const getSpeedTestColumns = ( rel="noopener noreferrer" onClick={(e) => e.stopPropagation()} title="View result on LibreSpeed" + aria-label="View result on LibreSpeed" > @@ -179,6 +180,7 @@ export const getSpeedTestMobileColumns = ( rel="noopener noreferrer" onClick={(e) => e.stopPropagation()} title="View result on LibreSpeed" + aria-label="View result on LibreSpeed" > From a05d01699669acde2c135d3712c05790f4c7181f Mon Sep 17 00:00:00 2001 From: soup Date: Tue, 31 Mar 2026 22:59:45 +0200 Subject: [PATCH 3/5] fix(speedtest): trim whitespace before parsing result URL --- internal/speedtest/librespeed.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/speedtest/librespeed.go b/internal/speedtest/librespeed.go index b4e8c67..8e45ca0 100644 --- a/internal/speedtest/librespeed.go +++ b/internal/speedtest/librespeed.go @@ -336,6 +336,7 @@ func parseCountryFromName(name string) string { // sanitizeResultURL validates a share URL from librespeed-cli output. // Returns the normalized URL if valid (http/https with a host), empty string otherwise. func sanitizeResultURL(raw string) string { + raw = strings.TrimSpace(raw) if raw == "" { return "" } From 2d83cd078c58811bd1176e3c21e3464c69c0372f Mon Sep 17 00:00:00 2001 From: soup Date: Tue, 31 Mar 2026 23:13:09 +0200 Subject: [PATCH 4/5] fix(speedtest): handle non-JSON output before librespeed-cli JSON array librespeed-cli prints telemetry error text to stdout before the JSON when --share is used. Find the JSON array start before unmarshaling. --- internal/speedtest/librespeed.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/speedtest/librespeed.go b/internal/speedtest/librespeed.go index 8e45ca0..b75e2b8 100644 --- a/internal/speedtest/librespeed.go +++ b/internal/speedtest/librespeed.go @@ -4,6 +4,7 @@ package speedtest import ( + "bytes" "context" "encoding/json" "fmt" @@ -128,8 +129,15 @@ func (r *LibrespeedRunner) RunTest(ctx context.Context, opts *types.TestOptions) log.Debug().Str("output", string(output)).Msg("librespeed-cli output") + // librespeed-cli may print non-JSON text (e.g. telemetry errors) before + // the JSON array when --share is used. Find the JSON array start. + jsonOutput := output + if idx := bytes.IndexByte(output, '['); idx > 0 { + jsonOutput = output[idx:] + } + var librespeedResults []LibrespeedResult - if err := json.Unmarshal(output, &librespeedResults); err != nil { + if err := json.Unmarshal(jsonOutput, &librespeedResults); err != nil { log.Error().Err(err).Str("output", string(output)).Msg("failed to parse librespeed-cli output") return nil, fmt.Errorf("failed to parse librespeed-cli output: %w", err) } From 5b9fabddc8925822f59d837d5e61a19670930f1c Mon Sep 17 00:00:00 2001 From: soup Date: Sun, 31 May 2026 19:10:54 +0200 Subject: [PATCH 5/5] fix(database): renumber result_url migration to 022 (duplicate 021 skipped on upgrade) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit develop already ships 021_packetloss_history_summary_index, and the migrator applies migrations by position relative to the applied count (it never reads migration names back). The duplicate 021_add_result_url sorted before the already-applied packetloss index, so on existing databases it was silently skipped: result_url was never created and every SaveSpeedTest/GetSpeedTests then failed on the missing column (speed test history broke after upgrade). Fresh installs applied all migrations in order, masking the bug in CI. Renumber to 022 for both dialects so the new migration always sorts last and is applied on upgrade. Add a migrations test rejecting duplicate version prefixes, plus a DB round-trip test for result_url. Also fixes other review findings for this PR: - speedtest: read librespeed-cli output from stdout instead of CombinedOutput so stderr telemetry errors (e.g. an IPv6 endpoint with '[') can't corrupt JSON parsing; extract the JSON-array trimming into a tested helper. - speedtest: replace the tautological result-URL test with one that exercises DefaultResultHandler.SaveResult; pin sanitizeResultURL normalization cases. - web: keep server-name ellipsis when a result link is present (move truncate to a shrinkable child of the inline-flex container; was clipping without '…'). - docs: document NETRONOME__LIBRESPEED_SHARE_RESULTS. --- README.md | 1 + .../database/migrations/migrations_test.go | 43 +++++++++++++++++ ...es.sql => 022_add_result_url_postgres.sql} | 0 ..._result_url.sql => 022_add_result_url.sql} | 0 .../database/speedtest_integration_test.go | 48 +++++++++++++++++++ internal/speedtest/librespeed.go | 39 ++++++++++----- internal/speedtest/librespeed_test.go | 23 +++++++++ internal/speedtest/result_handler_test.go | 45 +++++++++++++---- web/src/components/speedtest/columns.tsx | 8 ++-- 9 files changed, 180 insertions(+), 27 deletions(-) create mode 100644 internal/database/migrations/migrations_test.go rename internal/database/migrations/postgres/{021_add_result_url_postgres.sql => 022_add_result_url_postgres.sql} (100%) rename internal/database/migrations/sqlite/{021_add_result_url.sql => 022_add_result_url.sql} (100%) diff --git a/README.md b/README.md index 7b164fa..527cace 100644 --- a/README.md +++ b/README.md @@ -761,6 +761,7 @@ NETRONOME__IPERF_PING_TIMEOUT=10 # Ping timeout (seconds) # LibreSpeed settings NETRONOME__LIBRESPEED_TIMEOUT=60 # LibreSpeed timeout (seconds) +NETRONOME__LIBRESPEED_SHARE_RESULTS=false # Submit results to librespeed.org for a shareable link (sends telemetry) ``` ### Pagination diff --git a/internal/database/migrations/migrations_test.go b/internal/database/migrations/migrations_test.go new file mode 100644 index 0000000..728e35f --- /dev/null +++ b/internal/database/migrations/migrations_test.go @@ -0,0 +1,43 @@ +// Copyright (c) 2024-2026, s0up and the autobrr contributors. +// SPDX-License-Identifier: GPL-2.0-or-later + +package migrations + +import ( + "path" + "strconv" + "strings" + "testing" +) + +// TestNoDuplicateMigrationVersions guards against two migration files sharing a +// numeric version prefix. The migrator (pkg/migrator) applies migrations by +// position relative to the count already applied, so a duplicate version makes +// the sorted order ambiguous and silently skips a migration on upgrade (the new +// file gets ordered before an already-applied one and is never run). +// +// The version is parsed from the file basename here rather than via the +// production getMigrationVersion, which does not strip the directory prefix. +func TestNoDuplicateMigrationVersions(t *testing.T) { + for _, dbType := range []DatabaseType{SQLite, Postgres} { + files, err := GetMigrationFiles(dbType) + if err != nil { + t.Fatalf("%s: failed to get migration files: %v", dbType, err) + } + + seen := make(map[int]string, len(files)) + for _, file := range files { + base := path.Base(file) + prefix, _, _ := strings.Cut(base, "_") + version, err := strconv.Atoi(prefix) + if err != nil { + t.Errorf("%s: migration %q has no numeric version prefix", dbType, file) + continue + } + if prev, ok := seen[version]; ok { + t.Errorf("%s: duplicate migration version %d: %q and %q", dbType, version, prev, file) + } + seen[version] = file + } + } +} diff --git a/internal/database/migrations/postgres/021_add_result_url_postgres.sql b/internal/database/migrations/postgres/022_add_result_url_postgres.sql similarity index 100% rename from internal/database/migrations/postgres/021_add_result_url_postgres.sql rename to internal/database/migrations/postgres/022_add_result_url_postgres.sql diff --git a/internal/database/migrations/sqlite/021_add_result_url.sql b/internal/database/migrations/sqlite/022_add_result_url.sql similarity index 100% rename from internal/database/migrations/sqlite/021_add_result_url.sql rename to internal/database/migrations/sqlite/022_add_result_url.sql diff --git a/internal/database/speedtest_integration_test.go b/internal/database/speedtest_integration_test.go index 24aa27a..904f3d2 100644 --- a/internal/database/speedtest_integration_test.go +++ b/internal/database/speedtest_integration_test.go @@ -249,6 +249,54 @@ func TestSpeedTest_ScheduledVsManual(t *testing.T) { }) } +func TestSpeedTest_ResultURLRoundTrip(t *testing.T) { + RunTestWithBothDatabases(t, func(t *testing.T, td *TestDatabase) { + ctx := context.Background() + + resultURL := "https://librespeed.org/results/?id=abc123" + + withURL := types.SpeedTestResult{ + ServerName: "Shared Server", + ServerID: "librespeed-shared", + TestType: "librespeed", + DownloadSpeed: 100.0, + UploadSpeed: 50.0, + ResultURL: &resultURL, + } + savedWithURL, err := td.Service.SaveSpeedTest(ctx, withURL) + require.NoError(t, err) + + withoutURL := types.SpeedTestResult{ + ServerName: "Unshared Server", + ServerID: "librespeed-unshared", + TestType: "librespeed", + DownloadSpeed: 100.0, + UploadSpeed: 50.0, + // ResultURL left nil + } + savedWithoutURL, err := td.Service.SaveSpeedTest(ctx, withoutURL) + require.NoError(t, err) + + results, err := td.Service.GetSpeedTests(ctx, "all", 1, 10) + require.NoError(t, err) + + var foundWithURL, foundWithoutURL bool + for _, test := range results.Data { + switch test.ID { + case savedWithURL.ID: + foundWithURL = true + require.NotNil(t, test.ResultURL, "result_url should round-trip as a non-nil pointer") + assert.Equal(t, resultURL, *test.ResultURL) + case savedWithoutURL.ID: + foundWithoutURL = true + assert.Nil(t, test.ResultURL, "absent result_url should read back as nil") + } + } + assert.True(t, foundWithURL, "should find the test saved with a result URL") + assert.True(t, foundWithoutURL, "should find the test saved without a result URL") + }) +} + func TestSpeedTest_NullableFields(t *testing.T) { RunTestWithBothDatabases(t, func(t *testing.T, td *TestDatabase) { ctx := context.Background() diff --git a/internal/speedtest/librespeed.go b/internal/speedtest/librespeed.go index b75e2b8..5bde0dc 100644 --- a/internal/speedtest/librespeed.go +++ b/internal/speedtest/librespeed.go @@ -121,24 +121,26 @@ func (r *LibrespeedRunner) RunTest(ctx context.Context, opts *types.TestOptions) cmd := exec.CommandContext(ctx, "librespeed-cli", args...) - output, err := cmd.CombinedOutput() - if err != nil { - log.Error().Err(err).Str("output", string(output)).Msg("librespeed-cli failed") - return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, string(output)) + // Capture stdout and stderr separately: librespeed-cli writes the JSON + // report to stdout, while progress/UI lines and telemetry errors (emitted + // when --share is used) go to stderr. Keeping them apart avoids stderr + // noise corrupting the JSON we parse. + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + log.Error().Err(err).Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli failed") + return nil, fmt.Errorf("librespeed-cli failed: %v: %s", err, strings.TrimSpace(stderr.String())) } - log.Debug().Str("output", string(output)).Msg("librespeed-cli output") + output := stdout.Bytes() - // librespeed-cli may print non-JSON text (e.g. telemetry errors) before - // the JSON array when --share is used. Find the JSON array start. - jsonOutput := output - if idx := bytes.IndexByte(output, '['); idx > 0 { - jsonOutput = output[idx:] - } + log.Debug().Str("stdout", stdout.String()).Str("stderr", stderr.String()).Msg("librespeed-cli output") var librespeedResults []LibrespeedResult - if err := json.Unmarshal(jsonOutput, &librespeedResults); err != nil { - log.Error().Err(err).Str("output", string(output)).Msg("failed to parse librespeed-cli output") + if err := json.Unmarshal(extractJSONArray(output), &librespeedResults); err != nil { + log.Error().Err(err).Str("stdout", stdout.String()).Msg("failed to parse librespeed-cli output") return nil, fmt.Errorf("failed to parse librespeed-cli output: %w", err) } @@ -341,6 +343,17 @@ func parseCountryFromName(name string) string { return "Unknown" } +// extractJSONArray trims any leading non-JSON output (e.g. progress/log lines +// librespeed-cli may print before the JSON report) so the result can be +// unmarshalled. It returns b starting at the first '['; if there is no '[' or +// it is already at the start, b is returned unchanged. +func extractJSONArray(b []byte) []byte { + if idx := bytes.IndexByte(b, '['); idx > 0 { + return b[idx:] + } + return b +} + // sanitizeResultURL validates a share URL from librespeed-cli output. // Returns the normalized URL if valid (http/https with a host), empty string otherwise. func sanitizeResultURL(raw string) string { diff --git a/internal/speedtest/librespeed_test.go b/internal/speedtest/librespeed_test.go index 256ce92..3cc622b 100644 --- a/internal/speedtest/librespeed_test.go +++ b/internal/speedtest/librespeed_test.go @@ -68,6 +68,8 @@ func TestSanitizeResultURL(t *testing.T) { }{ {"valid https URL", "https://librespeed.org/results/?id=abc123", "https://librespeed.org/results/?id=abc123"}, {"valid http URL", "http://librespeed.org/results/?id=abc123", "http://librespeed.org/results/?id=abc123"}, + {"uppercase scheme normalized", "HTTPS://librespeed.org/results/?id=abc123", "https://librespeed.org/results/?id=abc123"}, + {"surrounding whitespace trimmed", " https://librespeed.org/results/?id=abc123 ", "https://librespeed.org/results/?id=abc123"}, {"empty string", "", ""}, {"no scheme", "librespeed.org/results", ""}, {"javascript scheme", "javascript:alert(1)", ""}, @@ -82,6 +84,27 @@ func TestSanitizeResultURL(t *testing.T) { } } +func TestExtractJSONArray(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"clean array", `[{"download":1}]`, `[{"download":1}]`}, + {"array at offset zero unchanged", `[1,2,3]`, `[1,2,3]`}, + {"leading log line", "Selected server: foo\n[{\"download\":1}]", `[{"download":1}]`}, + {"multiple leading lines", "retrieving servers\nrunning test\n[{\"download\":1}]", `[{"download":1}]`}, + {"no array present", "no json here", "no json here"}, + {"empty", "", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, string(extractJSONArray([]byte(tt.in)))) + }) + } +} + func TestBuildArgsUsesLocalJSONForCustomServers(t *testing.T) { runner := NewLibrespeedRunner(config.LibrespeedConfig{ ServersPath: "/tmp/local-servers.json", diff --git a/internal/speedtest/result_handler_test.go b/internal/speedtest/result_handler_test.go index f688a90..4f7add4 100644 --- a/internal/speedtest/result_handler_test.go +++ b/internal/speedtest/result_handler_test.go @@ -4,24 +4,45 @@ package speedtest import ( + "context" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/autobrr/netronome/internal/database" + "github.com/autobrr/netronome/internal/types" ) -func TestResultURLNormalization(t *testing.T) { +// captureService is a database.Service test double that records the +// SpeedTestResult passed to SaveSpeedTest. Embedding the interface gives nil +// implementations for every other method (none are exercised by SaveResult). +type captureService struct { + database.Service + captured types.SpeedTestResult +} + +func (c *captureService) SaveSpeedTest(_ context.Context, result types.SpeedTestResult) (*types.SpeedTestResult, error) { + c.captured = result + return &result, nil +} + +// TestSaveResultNormalizesResultURL exercises the production normalization in +// DefaultResultHandler.SaveResult: an empty ResultURL must persist as a nil +// *string, a non-empty one as a pointer to that value. +func TestSaveResultNormalizesResultURL(t *testing.T) { tests := []struct { name string resultURL string wantNil bool }{ { - name: "empty string produces nil", + name: "empty string persists as nil", resultURL: "", wantNil: true, }, { - name: "non-empty string produces pointer", + name: "non-empty string persists as pointer", resultURL: "https://librespeed.org/results/?id=abc123", wantNil: false, }, @@ -29,16 +50,20 @@ func TestResultURLNormalization(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var resultURL *string - if tt.resultURL != "" { - resultURL = &tt.resultURL - } + svc := &captureService{} + h := NewResultHandler(svc, nil) + + err := h.SaveResult(context.Background(), &Result{ + Server: "test-server", + ResultURL: tt.resultURL, + }, "librespeed", &types.TestOptions{}) + require.NoError(t, err) if tt.wantNil { - assert.Nil(t, resultURL) + assert.Nil(t, svc.captured.ResultURL) } else { - assert.NotNil(t, resultURL) - assert.Equal(t, tt.resultURL, *resultURL) + require.NotNil(t, svc.captured.ResultURL) + assert.Equal(t, tt.resultURL, *svc.captured.ResultURL) } }) } diff --git a/web/src/components/speedtest/columns.tsx b/web/src/components/speedtest/columns.tsx index 097b066..8061517 100644 --- a/web/src/components/speedtest/columns.tsx +++ b/web/src/components/speedtest/columns.tsx @@ -68,10 +68,10 @@ export const getSpeedTestColumns = ( header: "Server", cell: ({ row }) => ( - {row.getValue("serverName")} + {row.getValue("serverName")} {row.original.resultUrl && (
-
- {test.serverName} +
+ {test.serverName} {test.resultUrl && (