From a21c9b276516ace002c5d1f39ed8d9f95b1d3f28 Mon Sep 17 00:00:00 2001 From: cplieger <917744+cplieger@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:18:25 +0200 Subject: [PATCH 1/2] test: cover the pagination-bound warnings and the conditional cache's save contract The watcher's only observable output is its log stream, so the truncation warnings were asserted only by reaching them, never by their contents. Both now pin the ceiling they report (5 pages of 100), and a healthy save of the dedup set and of the conditional cache is asserted silent on both its failure paths. Four cache behaviors gain a test: a representation validated only by Last-Modified is cached rather than dropped, an entry written by a concurrent process survives this process's save, and a persisted payload sized exactly at the slot bound is kept whole instead of evicted from. --- internal/collect/collect_test.go | 5 + internal/github/client_conditional_test.go | 123 +++++++++++++++++++++ internal/github/client_test.go | 35 +++++- 3 files changed, 160 insertions(+), 3 deletions(-) diff --git a/internal/collect/collect_test.go b/internal/collect/collect_test.go index 12cb26e..004a75c 100644 --- a/internal/collect/collect_test.go +++ b/internal/collect/collect_test.go @@ -459,6 +459,11 @@ func TestStatePersistsDedupAcrossProcesses(t *testing.T) { if got := rec1.CountExact("dedup state save failed"); got != 0 { t.Errorf("successful state save emitted %d save-failure warnings, want 0", got) } + // Nor the marshal-failure warning: the whole scan is read through its log + // stream, so a healthy save has to be silent about both halves. + if got := rec1.CountExact("dedup state marshal failed"); got != 0 { + t.Errorf("successful state save emitted %d marshal-failure warnings, want 0", got) + } c2, rec2 := mk() // fresh "process", same state file c2.Scan(t.Context()) diff --git a/internal/github/client_conditional_test.go b/internal/github/client_conditional_test.go index 8e911bd..cbfa54f 100644 --- a/internal/github/client_conditional_test.go +++ b/internal/github/client_conditional_test.go @@ -8,12 +8,14 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "sync/atomic" "testing" "time" "github.com/cplieger/github-scout/internal/ghsignal" "github.com/cplieger/httpx/v5" + "github.com/cplieger/slogx/capture" ) // condServer is an httptest server that serves body with an ETag and answers @@ -322,3 +324,124 @@ func TestCondCache_persistEvictsOldestWhenOverBound(t *testing.T) { t.Error("newest entry was evicted; eviction must drop oldest-first") } } + +// TestCondCache_lastModifiedOnlyIsRevalidatable pins that a representation +// validated only by Last-Modified is cached like an ETag'd one. Several GitHub +// endpoints answer without an ETag, and treating those as unrevalidatable +// would spend a full-price GET on every scan forever. +func TestCondCache_lastModifiedOnlyIsRevalidatable(t *testing.T) { + const ( + reqURL = "https://example.test/user/repos" + lastModified = "Fri, 21 Aug 2026 15:35:00 GMT" + ) + c := newCondCache("", slog.Default()) + c.store(reqURL, httpx.Validators{LastModified: lastModified}, []string{"keep"}) + + got := c.validators(reqURL) + if got.LastModified != lastModified { + t.Errorf("validators(%q).LastModified = %q, want %q", reqURL, got.LastModified, lastModified) + } + if got.ETag != "" { + t.Errorf("validators(%q).ETag = %q, want empty", reqURL, got.ETag) + } + var items []string + if !c.decodeInto(reqURL, &items) { + t.Fatalf("decodeInto(%q) = false, want the stored items re-servable on a 304", reqURL) + } + if len(items) != 1 || items[0] != "keep" { + t.Errorf("decodeInto(%q) items = %v, want [keep]", reqURL, items) + } +} + +// TestCondCache_successfulSaveIsSilent pins that a healthy save emits nothing. +// The watcher is read entirely through its log stream, so a line on the +// success path would report a broken cache on every single scan. +func TestCondCache_successfulSaveIsSilent(t *testing.T) { + logger, rec := capture.New() + c := newCondCache(filepath.Join(t.TempDir(), "cond-cache.json"), logger) + c.store("https://example.test/user/repos", httpx.Validators{ETag: `W/"v1"`}, []string{"keep"}) + + if rec.Len() != 0 { + t.Errorf("a successful cache save logged %d line(s) %v, want none", rec.Len(), rec.Messages()) + } +} + +// TestCondCache_saveKeepsAnotherProcessesEntries pins the merge under the +// slot's flock: an entry written by a concurrent process after this cache +// loaded — the scheduled daemon racing a hand-exec'd trigger — must survive +// this process's save instead of being overwritten away. +func TestCondCache_saveKeepsAnotherProcessesEntries(t *testing.T) { + const ( + mine = "https://example.test/mine" + other = "https://example.test/other" + ) + path := filepath.Join(t.TempDir(), "cond-cache.json") + c := newCondCache(path, slog.Default()) // loads cold: memory holds nothing + + // The other process writes its entry after this cache loaded, so the only + // copy of it is the one on disk. + persisted, err := json.Marshal(map[string]cacheEntry{ + other: {ETag: `W/"other"`, Items: json.RawMessage(`["o"]`), UsedAt: time.Now().Add(-time.Hour)}, + }) + if err != nil { + t.Fatalf("setup: marshal the other process's entry: %v", err) + } + if err := os.WriteFile(path, persisted, 0o600); err != nil { + t.Fatalf("setup: %v", err) + } + + c.store(mine, httpx.Validators{ETag: `W/"mine"`}, []string{"m"}) + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read the cache file back: %v", err) + } + var merged map[string]cacheEntry + if err := json.Unmarshal(data, &merged); err != nil { + t.Fatalf("cache file does not parse after the save: %v (%s)", err, data) + } + if _, ok := merged[other]; !ok { + t.Errorf("save dropped the concurrently written entry %q; file holds %d entry/entries", other, len(merged)) + } + if _, ok := merged[mine]; !ok { + t.Errorf("save did not persist this process's entry %q", mine) + } +} + +// TestCondCache_payloadExactlyAtTheBoundIsKeptWhole pins the inclusive edge of +// the persisted-size cap: a payload marshaling to exactly the bound still +// reads back whole, so evicting from it would discard a usable validator for +// no gain. +func TestCondCache_payloadExactlyAtTheBoundIsKeptWhole(t *testing.T) { + // Sized so the marshaled map lands exactly on the cap. UsedAt is fixed + // and carries no trailing-zero nanoseconds, so its encoding is a stable + // width; the guard below fails loudly if the entry shape ever changes. + const padding = 61339 + const reqURL = "https://example.test/p" + entries := map[string]cacheEntry{ + reqURL: { + ETag: `W/"x"`, + Items: json.RawMessage(`["` + strings.Repeat("a", padding) + `"]`), + UsedAt: time.Date(2026, 8, 21, 15, 35, 0, 123456789, time.UTC), + }, + } + raw, err := json.Marshal(entries) + if err != nil { + t.Fatalf("setup: marshal fixture: %v", err) + } + if len(raw) != condCacheMaxBytes { + t.Fatalf("fixture drifted: entry marshals to %d bytes, want exactly %d (adjust padding by %d)", + len(raw), condCacheMaxBytes, condCacheMaxBytes-len(raw)) + } + + data, err := marshalBounded(entries) + if err != nil { + t.Fatalf("marshalBounded: %v", err) + } + if _, ok := entries[reqURL]; !ok { + t.Errorf("a payload of exactly %d bytes was evicted, want it retained", condCacheMaxBytes) + } + if len(data) != condCacheMaxBytes { + t.Errorf("marshalBounded returned %d bytes, want the whole %d-byte payload", len(data), condCacheMaxBytes) + } +} diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 892fef1..3240dea 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -14,13 +14,21 @@ import ( "github.com/cplieger/github-scout/internal/ghsignal" "github.com/cplieger/httpx/v5" + "github.com/cplieger/slogx/capture" ) // newTestClient wires a Client at the test server's URL with a short-timeout // http.Client so tests never hang. func newTestClient(t *testing.T, srv *httptest.Server) *Client { t.Helper() - c := NewClient(Options{HTTP: httpx.NewClient(5 * time.Second), Token: "test-token", Logger: slog.Default()}) + return newTestClientWithLogger(t, srv, slog.Default()) +} + +// newTestClientWithLogger is newTestClient with a caller-supplied logger, for +// the paths whose only observable output is the log line they emit. +func newTestClientWithLogger(t *testing.T, srv *httptest.Server, logger *slog.Logger) *Client { + t.Helper() + c := NewClient(Options{HTTP: httpx.NewClient(5 * time.Second), Token: "test-token", Logger: logger}) c.baseURL = srv.URL return c } @@ -479,7 +487,8 @@ func TestListReposStopsAtMaxPages(t *testing.T) { `{"name":"r%d","owner":{"login":"cplieger"}}`)) defer srv.Close() - repos, err := newTestClient(t, srv).ListRepos(t.Context(), "cplieger") + logger, rec := capture.New() + repos, err := newTestClientWithLogger(t, srv, logger).ListRepos(t.Context(), "cplieger") if err != nil { t.Fatalf("ListRepos: %v", err) } @@ -489,6 +498,16 @@ func TestListReposStopsAtMaxPages(t *testing.T) { if len(repos) != maxPages*perPage { t.Errorf("got %d repos, want %d", len(repos), maxPages*perPage) } + // The warning is the only signal an operator gets that the scan universe + // was capped, so it must name the ceiling actually scanned: 5 pages of + // 100 repos. + const warning = "repo listing hit pagination bound; scan universe may be truncated" + if got := rec.CountExact(warning); got != 1 { + t.Errorf("ListRepos over the page cap emitted %d truncation warnings, want 1", got) + } + if got, ok := rec.AttrValueExact(warning, "repo_cap"); !ok || got != "500" { + t.Errorf("truncation warning repo_cap = %q (present=%v), want %q", got, ok, "500") + } } func TestListRunsStopsAtMaxPages(t *testing.T) { @@ -518,7 +537,8 @@ func TestSearchStopsAtMaxPages(t *testing.T) { `{"number":%d,"repository_url":"https://api.github.com/repos/cplieger/a","user":{"login":"cplieger"}}`)) defer srv.Close() - prs, err := newTestClient(t, srv).SearchOpenPRs(t.Context(), "cplieger", "") + logger, rec := capture.New() + prs, err := newTestClientWithLogger(t, srv, logger).SearchOpenPRs(t.Context(), "cplieger", "") if err != nil { t.Fatalf("SearchOpenPRs: %v", err) } @@ -528,6 +548,15 @@ func TestSearchStopsAtMaxPages(t *testing.T) { if len(prs) != maxPages*perPage { t.Errorf("got %d PRs, want %d", len(prs), maxPages*perPage) } + // A partial snapshot is only distinguishable from a complete one by this + // warning, so it must name the ceiling actually searched: 5 pages of 100. + const warning = "search hit pagination bound; snapshot may be truncated" + if got := rec.CountExact(warning); got != 1 { + t.Errorf("search over the page cap emitted %d truncation warnings, want 1", got) + } + if got, ok := rec.AttrValueExact(warning, "item_cap"); !ok || got != "500" { + t.Errorf("truncation warning item_cap = %q (present=%v), want %q", got, ok, "500") + } } func TestListCodeScanningAlertsStopsAtMaxPages(t *testing.T) { From be4aa6f6c20793f93731bce90675455793b22318 Mon Sep 17 00:00:00 2001 From: cplieger <917744+cplieger@users.noreply.github.com> Date: Sat, 22 Aug 2026 10:51:44 +0200 Subject: [PATCH 2/2] test(config): assert default and clamp values, not the constants Several assertions compared a loaded value against the very constant the loader returns (`cfg.ScanInterval != DefaultScanInterval`), so they held for whatever value the constant had and pinned nothing. Measured: editing DefaultScanInterval, DefaultLookbackHours, minScanInterval or maxScanInterval to a different valid value left the suite green. Expectations are now literals, so each assertion states the cadence, window and bound the compose contract and bundled dashboard assume: 15m default scan interval, 72h default lookback, a 1m floor, an 8760h (365-day) ceiling and a 720h (30-day) lookback cap. The GitHub API version header assertion gets the same treatment. maxScanInterval was the one bound nothing else pinned: halving it via its own initializer left every test in the module passing. --- internal/config/config_test.go | 69 +++++++++++++++++++++------------- internal/github/client_test.go | 4 +- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 9348b20..5615dcb 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -21,11 +21,16 @@ func TestLoadDefaults(t *testing.T) { t.Setenv("LOG_LEVEL", "") cfg := Load() - if cfg.ScanInterval != DefaultScanInterval { - t.Errorf("ScanInterval = %v, want %v", cfg.ScanInterval, DefaultScanInterval) + + // Expected values are written out rather than read back from the + // constants they check: an assertion against DefaultScanInterval moves + // with any edit to it and so pins nothing. 15m and 72h are the cadence + // and window the compose contract and the bundled dashboard assume. + if cfg.ScanInterval != 15*time.Minute { + t.Errorf("ScanInterval = %v, want 15m0s", cfg.ScanInterval) } - if cfg.Lookback != DefaultLookbackHours*time.Hour { - t.Errorf("Lookback = %v, want %v", cfg.Lookback, DefaultLookbackHours*time.Hour) + if cfg.Lookback != 72*time.Hour { + t.Errorf("Lookback = %v, want 72h0m0s", cfg.Lookback) } if cfg.LogLevel != slog.LevelInfo { t.Errorf("LogLevel = %v, want Info", cfg.LogLevel) @@ -82,8 +87,8 @@ func TestScanIntervalSentinelsFallBackToDefault(t *testing.T) { t.Run(v, func(t *testing.T) { rec := captureDefaultSlog(t) t.Setenv("SCAN_INTERVAL", v) - if got := Load().ScanInterval; got != DefaultScanInterval { - t.Errorf("SCAN_INTERVAL=%q ScanInterval = %v, want default %v", v, got, DefaultScanInterval) + if got := Load().ScanInterval; got != 15*time.Minute { + t.Errorf("SCAN_INTERVAL=%q ScanInterval = %v, want default 15m0s", v, got) } if n := rec.CountExact("invalid SCAN_INTERVAL, using default"); n != 1 { t.Errorf("SCAN_INTERVAL=%q warned %d times, want exactly 1", v, n) @@ -99,6 +104,10 @@ func TestScanIntervalParsesDuration(t *testing.T) { } } +// TestClampingAndFallbacks pins every out-of-range and unparseable input to the +// duration it actually yields. Each want is a literal, never the constant the +// clamp reads: a want written as `maxScanInterval` holds however that constant +// is edited, so the bound it claims to check is unpinned. func TestClampingAndFallbacks(t *testing.T) { tests := []struct { selector func(Config) time.Duration @@ -107,14 +116,14 @@ func TestClampingAndFallbacks(t *testing.T) { val string want time.Duration }{ - {name: "scan negative falls back to default", key: "SCAN_INTERVAL", val: "-5m", want: DefaultScanInterval, selector: func(c Config) time.Duration { return c.ScanInterval }}, - {name: "scan garbage falls back to default", key: "SCAN_INTERVAL", val: "abc", want: DefaultScanInterval, selector: func(c Config) time.Duration { return c.ScanInterval }}, - {name: "scan over max is clamped", key: "SCAN_INTERVAL", val: "10000h", want: maxScanInterval, selector: func(c Config) time.Duration { return c.ScanInterval }}, + {name: "scan negative falls back to default", key: "SCAN_INTERVAL", val: "-5m", want: 15 * time.Minute, selector: func(c Config) time.Duration { return c.ScanInterval }}, + {name: "scan garbage falls back to default", key: "SCAN_INTERVAL", val: "abc", want: 15 * time.Minute, selector: func(c Config) time.Duration { return c.ScanInterval }}, + {name: "scan over max is clamped", key: "SCAN_INTERVAL", val: "10000h", want: 8760 * time.Hour, selector: func(c Config) time.Duration { return c.ScanInterval }}, // 365 days {name: "lookback zero floors to lo=1", key: "LOOKBACK_HOURS", val: "0", want: 1 * time.Hour, selector: func(c Config) time.Duration { return c.Lookback }}, {name: "lookback at lo boundary is kept", key: "LOOKBACK_HOURS", val: "1", want: 1 * time.Hour, selector: func(c Config) time.Duration { return c.Lookback }}, - {name: "lookback negative falls back to default", key: "LOOKBACK_HOURS", val: "-1", want: DefaultLookbackHours * time.Hour, selector: func(c Config) time.Duration { return c.Lookback }}, - {name: "lookback at hi boundary is kept", key: "LOOKBACK_HOURS", val: "720", want: maxLookbackHours * time.Hour, selector: func(c Config) time.Duration { return c.Lookback }}, - {name: "lookback over max is clamped", key: "LOOKBACK_HOURS", val: "100000", want: maxLookbackHours * time.Hour, selector: func(c Config) time.Duration { return c.Lookback }}, + {name: "lookback negative falls back to default", key: "LOOKBACK_HOURS", val: "-1", want: 72 * time.Hour, selector: func(c Config) time.Duration { return c.Lookback }}, + {name: "lookback at hi boundary is kept", key: "LOOKBACK_HOURS", val: "720", want: 720 * time.Hour, selector: func(c Config) time.Duration { return c.Lookback }}, // 30 days + {name: "lookback over max is clamped", key: "LOOKBACK_HOURS", val: "100000", want: 720 * time.Hour, selector: func(c Config) time.Duration { return c.Lookback }}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -131,7 +140,7 @@ func TestClampingAndFallbacks(t *testing.T) { // remaining conditional is `clamped != v`, which gates this warning. Asserting // the warning fires when (and only when) the value is clamped down makes that // guard's mutants (==, removal) killable — the return-value table tests alone -// cannot see a log-only branch. Guards against the L163 "living mutant". +// cannot see a log-only branch. func TestClampedIntWarnsOverMax(t *testing.T) { capture := func(val string) string { var buf bytes.Buffer @@ -184,11 +193,15 @@ func TestExcludeQueriesDefaultWhenUnset(t *testing.T) { cfg := Load() - if cfg.PRExclude != DefaultPRExclude { - t.Errorf("PRExclude = %q, want default %q", cfg.PRExclude, DefaultPRExclude) + const ( + wantPR = "-author:app/renovate" + wantIssue = "-author:app/renovate -label:renovate -label:auto-generated" + ) + if cfg.PRExclude != wantPR { + t.Errorf("PRExclude = %q, want default %q", cfg.PRExclude, wantPR) } - if cfg.IssueExclude != DefaultIssueExclude { - t.Errorf("IssueExclude = %q, want default %q", cfg.IssueExclude, DefaultIssueExclude) + if cfg.IssueExclude != wantIssue { + t.Errorf("IssueExclude = %q, want default %q", cfg.IssueExclude, wantIssue) } } @@ -236,11 +249,11 @@ func TestLookbackAtMaxIsAcceptedWithoutWarning(t *testing.T) { // (a v >= hi mutant would warn spuriously at the legal maximum). rec := captureDefaultSlog(t) - t.Setenv("LOOKBACK_HOURS", "720") // == maxLookbackHours + t.Setenv("LOOKBACK_HOURS", "720") // the maximum, 30 days cfg := Load() - if cfg.Lookback != maxLookbackHours*time.Hour { - t.Errorf("Lookback = %v, want %v (max accepted as-is)", cfg.Lookback, maxLookbackHours*time.Hour) + if cfg.Lookback != 720*time.Hour { + t.Errorf("Lookback = %v, want 720h0m0s (max accepted as-is)", cfg.Lookback) } if n := rec.CountExact("env value clamped"); n != 0 { t.Errorf("value at exactly the max should not warn; got %d clamp warnings", n) @@ -255,8 +268,8 @@ func TestLookbackAboveMaxIsClampedWithWarning(t *testing.T) { t.Setenv("LOOKBACK_HOURS", "721") cfg := Load() - if cfg.Lookback != maxLookbackHours*time.Hour { - t.Errorf("Lookback = %v, want clamped to %v", cfg.Lookback, maxLookbackHours*time.Hour) + if cfg.Lookback != 720*time.Hour { + t.Errorf("Lookback = %v, want clamped to 720h0m0s", cfg.Lookback) } if n := rec.CountExact("env value clamped"); n != 1 { t.Errorf("value over the max should warn once; got %d clamp warnings", n) @@ -274,20 +287,22 @@ func captureDefaultSlog(t *testing.T) *capture.Recorder { } // TestScanIntervalBelowMinimumClamped pins the minScanInterval floor: a positive -// sub-minute SCAN_INTERVAL is clamped up to minScanInterval (1m) so a too-frequent -// scan of a multi-repo account can't exhaust GitHub's 5000 req/hour budget. The +// sub-minute SCAN_INTERVAL is clamped up to 1m so a too-frequent scan of a +// multi-repo account can't exhaust GitHub's 5000 req/hour budget. The // existing TestClampingAndFallbacks covers negative/garbage/over-max but no // sub-minute case. The "exactly 1m" row is kept via the default branch, pinning // the strict `<` lower edge; "2m" confirms an above-floor value passes through. +// The floor is written as a literal, not as minScanInterval, so the row states +// what the floor IS rather than restating whatever it has become. func TestScanIntervalBelowMinimumClamped(t *testing.T) { tests := []struct { name string val string want time.Duration }{ - {"sub-minute clamps up to the floor", "30s", minScanInterval}, - {"one second below the floor clamps", "59s", minScanInterval}, - {"exactly at the floor is kept", "1m", minScanInterval}, + {"sub-minute clamps up to the floor", "30s", time.Minute}, + {"one second below the floor clamps", "59s", time.Minute}, + {"exactly at the floor is kept", "1m", time.Minute}, {"above the floor is kept", "2m", 2 * time.Minute}, } for _, tt := range tests { diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 3240dea..49f209a 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -39,8 +39,8 @@ func TestListReposFiltersOwnerAndArchived(t *testing.T) { if got := r.Header.Get("Authorization"); got != "Bearer test-token" { t.Errorf("Authorization = %q, want Bearer test-token", got) } - if got := r.Header.Get("X-GitHub-Api-Version"); got != apiVersion { - t.Errorf("api version header = %q, want %q", got, apiVersion) + if got := r.Header.Get("X-GitHub-Api-Version"); got != "2022-11-28" { + t.Errorf("api version header = %q, want %q", got, "2022-11-28") } w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`[