Skip to content

Stop more slow Go tests waiting on production timers - #2399

Merged
nadaverell merged 1 commit into
mainfrom
chore/dev-loop-speed-f
Oct 28, 2026
Merged

nadaverell merged 1 commit into
mainfrom
chore/dev-loop-speed-f

Conversation

@nadaverell

@nadaverell nadaverell commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up to the internal/server timer work: the slowest Go tests in the other packages also spent most of their time waiting on real production timers. Locally, the 11 tests changed here went from 130 s to 42 s. The largest is internal/mcp's TestResourceReadsDynamicPendingAndCollectorDenial, which drops from 54 s to 15 s. That takes internal/mcp, the longest package in CI's Backend job, from about 143 s to 94 s locally. On CI, the Backend job's test step drops from 187–236 s on recent main runs to 148 s.

What changed

Same pattern as before. A production duration becomes an unexported package variable with its default unchanged. Only the tests that wait on it shorten it, restoring it with t.Cleanup, and they keep the ratios their assertions depend on.

Duration (default) Where Test value
Argo CD reachability probe timeout (8 s) internal/argocd/manager.go 1 s
Workload-attribution proof budget (8 s) internal/prometheus/workload_attribution.go 1 s
Historical-partition proof budget (8 s) internal/prometheus/workload_history.go 1 s
Dynamic-cache scope-walk base budget (5 s) pkg/k8score/dynamic_cache.go (unexported) 200 ms
  • The MCP tests reuse existing overrides for the sync wait, the stall threshold and the enrichment budget.
  • A Helm test's mock SAR client stops client-side rate limiting (QPS: -1): 4.4 s → 0.01 s.
  • Two fixtures (internal/issues, internal/k8s Traefik) start every informer before waiting for any to sync, instead of syncing them one after another.
  • The Prometheus budgets are read once, before the worker goroutines start, so a test restoring the value can't race a read.
Test Before After
mcp TestResourceReadsDynamicPendingAndCollectorDenial 54.3 s 15.0 s
issues TestStuckDeletionCleanupNeedsNoRecognizedController 10.3 s 2.1 s
prometheus TestHistoricalPartitionDiscoveryHasSeparateBudget 9.0 s 2.0 s
prometheus TestAttributionWaitsForDiscoveryBeforeProbeDeadline 8.2 s 1.2 s
argocd TestSlowRecheck_LeavesNoFailureBehind 8.1 s 1.1 s
k8score TestWalkOutOfTimeIsNotADenial 7.0 s 2.2 s
mcp TestAgentMCPListBoundsSlowOwnershipAuthorization 7.1 s 4.1 s
k8s TestDetectLiveTraefikMissingRefsShadowedRouter 5.8 s 2.9 s
mcp TestResourceReadsDynamicStallIsPerNamespace 5.6 s 2.5 s
helm TestRequireHelmWrite_ChecksCallerInNamespace 4.4 s 0.0 s
k8score TestRequestDeadlines_StalledInformerListRecovers 10.1 s 8.3 s

Left as they are

  • The 15 s that remains in the MCP pending test, plus two 4 s tests. That time is pkg/k8score's hard-coded 2 s cache-miss wait. Shortening it from internal/ needs new exported API in pkg/, which Radar Hub imports, so it isn't in this PR.
  • Four internal/cnpg log-follow tests (5–10 s each). They call t.Parallel() and share the discovery interval with other parallel tests, so overriding it isn't safe. They already overlap each other.
  • Real work. Desktop login-shell tests, 40 generated-world ownership conformance checks, a 1,000-Pod load-test retry, and two tests built from many real informer startups.

Testing

  • Every touched package passed a full run once, and go vet is clean.
  • Each changed test was run on its own before and after.
  • CI: all jobs green. The Backend test step took 148 s, against 187–236 s on the last 8 main runs. The pkg job is unchanged (70 s against 66–77 s): its long pole is elsewhere in pkg/k8score.

The same treatment as internal/server's slowest tests, for the slowest
tests elsewhere: Argo CD's 8s reachability probe timeout, Prometheus's 8s
attribution and historical-partition proof budgets and the dynamic cache's
5s scope-walk budget become unexported package variables with unchanged
defaults, shortened only by the tests that wait on them. MCP tests reuse
the existing sync-wait, stall and enrichment-budget overrides, a Helm test
client stops rate-limiting its mock SARs, and two fixtures start every
informer before waiting for any of them to sync.
@nadaverell
nadaverell requested a review from hisco as a code owner October 28, 2026 02:16
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Shorten timer-bound Go tests without changing production defaults

🧪 Tests ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Shorten test-only timer budgets while preserving production timeouts and deadline assertions.
• Start fixture informers together and remove rate limiting from a mock Helm client.
• Reduce the 11 affected tests from 130 to 42 seconds locally.
Diagram

graph TD
  Tests["Go tests"] --> Budgets["Test budgets"] --> Operations["Timed operations"] --> Fakes["Fake backends"]
  Tests --> Fixtures["Fixture setup"] --> Informers["Informer cache"] --> Fakes
  Tests --> HelmClient["Helm mock client"] --> Fakes
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Inject timer configuration into components
  • ➕ Avoids mutable package globals and permits independently configured concurrent tests.
  • ➖ Requires broader constructor and call-site changes across unrelated packages solely to speed these tests.

Recommendation: Keep the package-local overrides for this targeted test-speed change: defaults and external APIs stay intact, and the asynchronous Prometheus paths snapshot their budgets before starting workers. Timer injection would be preferable if parallel tests or more extensive timing configuration become necessary.

Files changed (14) +80 / -29

Enhancement (4) +24 / -16
manager.goMake the reachability probe timeout test-adjustable +10/-10

Make the reachability probe timeout test-adjustable

• Changes the endpoint timeout from a constant to an unexported variable. Its production default remains eight seconds.

internal/argocd/manager.go

workload_attribution.goMake the attribution probe budget test-adjustable +5/-2

Make the attribution probe budget test-adjustable

• Introduces an unexported eight-second default budget for attribution probes. The budget is captured before the worker starts and reused for its outer and probe deadlines.

internal/prometheus/workload_attribution.go

workload_history.goMake the historical-partition proof budget test-adjustable +5/-2

Make the historical-partition proof budget test-adjustable

• Introduces an unexported eight-second default proof budget, captured before the singleflight worker uses it for discovery and probing deadlines.

internal/prometheus/workload_history.go

dynamic_cache.goMake the scope-walk base budget test-adjustable +4/-2

Make the scope-walk base budget test-adjustable

• Extracts the five-second base budget into an unexported variable while retaining candidate-count scaling and the 15-second cap.

pkg/k8score/dynamic_cache.go

Tests (10) +56 / -13
background_probe_test.goShorten the slow recheck test's probe timeout +4/-1

Shorten the slow recheck test's probe timeout

• Temporarily sets the endpoint probe timeout to one second and restores it during cleanup, preserving the test's timeout behavior without an eight-second wait.

internal/argocd/background_probe_test.go

helm_write_gate_test.goRemove throttling from the Helm authorization test client +1/-1

Remove throttling from the Helm authorization test client

• Sets negative QPS on the client targeting the mock server so client-side rate limiting does not delay authorization checks.

internal/helm/helm_write_gate_test.go

finalizers_test.goStart all finalizer-fixture watches before syncing +2/-0

Start all finalizer-fixture watches before syncing

• Separates informer startup from synchronization waits, allowing the fixture's watches to make progress together.

internal/issues/finalizers_test.go

detect_traefik_refs_test.goStart Traefik-fixture watches before syncing +2/-0

Start Traefik-fixture watches before syncing

• Starts every required dynamic informer before waiting for individual syncs, avoiding sequential startup delays.

internal/k8s/detect_traefik_refs_test.go

ownership_test.goScale down the ownership-enrichment timing test +5/-2

Scale down the ownership-enrichment timing test

• Uses the existing enrichment-budget test override with a shorter mock authorization delay. The test still checks that enrichment stops within its budget.

internal/mcp/ownership_test.go

resource_reads_test.goShorten dynamic-read synchronization and stall waits +4/-1

Shorten dynamic-read synchronization and stall waits

• Temporarily reduces the sync wait in the pending-read test and lowers the stall threshold in the per-namespace test, retaining their behavioral checks.

internal/mcp/resource_reads_test.go

workload_attribution_test.goTest attribution discovery with a shorter probe budget +9/-1

Test attribution discovery with a shorter probe budget

• Temporarily uses a one-second budget and waits for discovery to start before checking that its duration is not mistaken for a probe failure.

internal/prometheus/workload_attribution_test.go

workload_history_test.goRelease historical discovery after a shortened proof budget +24/-5

Release historical discovery after a shortened proof budget

• Temporarily uses a one-second proof budget, observes discovery starting, then releases its gate after that budget has elapsed. The test continues to verify that discovery does not consume the proof budget.

internal/prometheus/workload_history_test.go

dynamic_cache_ratelimit_test.goShorten the incomplete scope-walk test +4/-1

Shorten the incomplete scope-walk test

• Uses a 200-millisecond base budget and scales the simulated slow response relative to the two-candidate walk budget, preserving the incomplete-walk assertion.

pkg/k8score/dynamic_cache_ratelimit_test.go

request_deadlines_informer_test.goReduce stalled-informer test transport deadlines +1/-1

Reduce stalled-informer test transport deadlines

• Halves the mock transport's idle and header timeouts to 500 milliseconds so the recovery test detects a stalled request sooner.

pkg/k8score/request_deadlines_informer_test.go

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@nadaverell
nadaverell merged commit 7b29b40 into main Oct 28, 2026
12 checks passed
@nadaverell
nadaverell deleted the chore/dev-loop-speed-f branch October 28, 2026 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant