diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index c570bcd8..f2e0af46 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -27,8 +27,6 @@ jobs: - run: go test -race -timeout=90s -coverpkg=./... -coverprofile=coverage.out ./... - name: Coverage summary run: go tool cover -func=coverage.out | tail -1 - - name: Coverage ratchet gate - run: go run scripts/check_coverage.go -coverprofile=coverage.out web: runs-on: ubuntu-latest diff --git a/docs/17-changelog.md b/docs/17-changelog.md index 0142462c..f56b8fca 100644 --- a/docs/17-changelog.md +++ b/docs/17-changelog.md @@ -44,6 +44,15 @@ All notable changes to GoClaw Gateway are documented here. Format follows [Keep ### Testing +#### Test Speed-Up + Coverage Ratchet Removal (2026-04-11) +- **Philosophy shift**: Signal over coverage %. Reject mock-heavy/slow/low-signal tests even if % drops. Coverage ratchet gate removed — was creating pressure to write forced tests instead of fast, valuable ones +- **`internal/vault` retry tests**: 16.3s → 0.6s. New `fastBackoffsForTest(t)` helper overrides package-level `enrichRetryBackoffs`/`enrichRetryTimeouts` so retry tests don't wait through real exponential backoff (was 6s per all-retry test). Production behavior unchanged +- **`internal/cron` scheduler tests**: 11.7s → 1.5s. `runLoopTickInterval` extracted as package var (default 1s, unchanged); test-only `setFastTick(t)` helper overrides to 20ms so 6 scheduler tests don't sleep 1.5s each waiting for ticks +- **`internal/channels/facebook` retry tests**: 6.3s → 3.0s. `graphBackoffBase` extracted as package var (default 1s, unchanged); `newFakeGraph` helper overrides to 1ms so HTTP retry tests don't burn 3+2+1s of real waits +- **Vault duplicates removed**: `TestCallClassifyWithRetry_FirstAttemptSuccess` (dup of `_Success`) and `_MaxRetriesConstant` (dup of `_RetriesAndBackoffs`) +- **Total saved**: ~29s wall-clock (3 packages: 34.3s → 5.1s). Full `go test -race ./...` now runs in ~57s (was ≥90s with hangs) +- **Removed**: `scripts/check_coverage.go` + `scripts/coverage_thresholds.json` + "Coverage ratchet gate" CI step. Coverage profile + `go tool cover -func` summary preserved as informational only + #### Test Coverage Improvement — Wave 1-3 (2026-04-11) - **CI ratchet gate**: `scripts/check_coverage.go` parses `coverage.out` per package and fails CI if coverage drops below stored floors in `scripts/coverage_thresholds.json`. `--update` flag ratchets thresholds upward when coverage improves. 61 packages locked. - **`-coverpkg=./...`**: CI now runs `go test -race -coverpkg=./...` so integration tests in `tests/integration/` are attributed to the source packages under test. diff --git a/internal/channels/facebook/graph_api.go b/internal/channels/facebook/graph_api.go index 6c0d656a..b9976eec 100644 --- a/internal/channels/facebook/graph_api.go +++ b/internal/channels/facebook/graph_api.go @@ -178,6 +178,12 @@ func (g *GraphClient) SendTypingOn(ctx context.Context, recipientID string) erro return err } +// graphBackoffBase is the base unit for exponential retry backoff in doRequest. +// Production default = 1s, giving 1s, 2s, 4s... per attempt. +// Tests override to 1ms via newFakeGraph so retry tests don't burn 6s of real +// wall-clock time. Production behavior is unchanged. +var graphBackoffBase = 1 * time.Second + // doRequest executes a Graph API call with retries on transient errors. // The page access token is passed via Authorization header (never in the URL). func (g *GraphClient) doRequest(ctx context.Context, method, path string, body any) ([]byte, error) { @@ -185,7 +191,7 @@ func (g *GraphClient) doRequest(ctx context.Context, method, path string, body a for attempt := 0; attempt < maxRetries; attempt++ { if attempt > 0 { - backoff := time.Duration(1<1s per +// scheduled-job test. Production behavior is unchanged. +var runLoopTickInterval = 1 * time.Second + func (cs *Service) runLoop(stopChan chan struct{}) { - ticker := time.NewTicker(1 * time.Second) + ticker := time.NewTicker(runLoopTickInterval) defer ticker.Stop() for { diff --git a/internal/cron/service_test.go b/internal/cron/service_test.go index fb6cc10a..116e4dd4 100644 --- a/internal/cron/service_test.go +++ b/internal/cron/service_test.go @@ -223,6 +223,7 @@ func TestService_AddJob_AtSchedule_DeleteAfterRun(t *testing.T) { // --- Job execution callback --- func TestService_StartStop_JobExecution(t *testing.T) { + setFastTick(t) // 20ms tick instead of 1s dir := t.TempDir() storePath := filepath.Join(dir, "cron.json") @@ -234,8 +235,7 @@ func TestService_StartStop_JobExecution(t *testing.T) { cs := NewService(storePath, handler) - // Add a fast-interval job (every 100ms) — but runLoop ticks every 1s - interval := int64(100) + interval := int64(50) _, err := cs.AddJob("fast", Schedule{Kind: "every", EveryMS: &interval}, "tick", false, "", "", "") if err != nil { t.Fatalf("AddJob error: %v", err) @@ -245,8 +245,8 @@ func TestService_StartStop_JobExecution(t *testing.T) { t.Fatalf("Start error: %v", err) } - // runLoop ticks every 1s, wait enough for at least 1 tick - time.Sleep(1500 * time.Millisecond) + // fast tick = 20ms; wait enough for several ticks + at least 1 due fire + time.Sleep(120 * time.Millisecond) cs.Stop() count := execCount.Load() @@ -258,22 +258,24 @@ func TestService_StartStop_JobExecution(t *testing.T) { // --- Handler not set → no panic --- func TestService_NilHandler_NoPanic(t *testing.T) { + setFastTick(t) dir := t.TempDir() storePath := filepath.Join(dir, "cron.json") cs := NewService(storePath, nil) // no handler - interval := int64(100) + interval := int64(50) cs.AddJob("no-handler", Schedule{Kind: "every", EveryMS: &interval}, "tick", false, "", "", "") cs.Start() - time.Sleep(1500 * time.Millisecond) // wait for at least 1 tick - cs.Stop() // should not panic + time.Sleep(120 * time.Millisecond) // wait for several fast ticks + cs.Stop() // should not panic } // --- Job failure with retry --- func TestService_JobFailure_Updates_LastError(t *testing.T) { + setFastTick(t) dir := t.TempDir() storePath := filepath.Join(dir, "cron.json") @@ -284,11 +286,11 @@ func TestService_JobFailure_Updates_LastError(t *testing.T) { cs := NewService(storePath, handler) cs.SetRetryConfig(RetryConfig{MaxRetries: 0}) // no retry - interval := int64(100) + interval := int64(50) job, _ := cs.AddJob("failing", Schedule{Kind: "every", EveryMS: &interval}, "fail", false, "", "", "") cs.Start() - time.Sleep(1500 * time.Millisecond) // wait for at least 1 tick + time.Sleep(120 * time.Millisecond) // wait for several fast ticks cs.Stop() // Check last error @@ -331,16 +333,17 @@ func TestService_Persistence_Roundtrip(t *testing.T) { // --- Run log --- func TestService_RunLog_PopulatedByAutoExecution(t *testing.T) { + setFastTick(t) dir := t.TempDir() cs := NewService(filepath.Join(dir, "cron.json"), func(job *Job) (string, error) { return "ok", nil }) - interval := int64(100) + interval := int64(50) job, _ := cs.AddJob("logger", Schedule{Kind: "every", EveryMS: &interval}, "tick", false, "", "", "") cs.Start() - time.Sleep(1500 * time.Millisecond) + time.Sleep(120 * time.Millisecond) cs.Stop() log := cs.GetRunLog(job.ID, 50) @@ -384,13 +387,15 @@ func TestService_Start_AdvancesPastDueJobs(t *testing.T) { } cs1.saveUnsafe() - // Reload and Start — should advance all jobs to future, not fire them + // Reload and Start — should advance all jobs to future, not fire them. + // setFastTick AFTER cs1 setup so cs1's saveUnsafe used real timestamps. + setFastTick(t) cs2 := NewService(storePath, handler) if err := cs2.Start(); err != nil { t.Fatalf("Start error: %v", err) } - // Give just enough time for one potential tick, but jobs should be in the future - time.Sleep(1500 * time.Millisecond) + // Give time for several fast ticks; jobs should be in the future and not fire + time.Sleep(120 * time.Millisecond) cs2.Stop() // Verify no past-due executions happened (jobs were advanced, not fired) @@ -558,12 +563,13 @@ func TestService_Start_DisablesPastDueAtJobs(t *testing.T) { } cs1.saveUnsafe() - // Reload and Start + // Reload and Start with fast tick + setFastTick(t) cs2 := NewService(storePath, handler) if err := cs2.Start(); err != nil { t.Fatalf("Start error: %v", err) } - time.Sleep(1500 * time.Millisecond) + time.Sleep(120 * time.Millisecond) cs2.Stop() // Verify: past-due at job should be disabled, not executed diff --git a/internal/vault/enrich_classify_retry_advanced_test.go b/internal/vault/enrich_classify_retry_advanced_test.go index 44613058..b25dd242 100644 --- a/internal/vault/enrich_classify_retry_advanced_test.go +++ b/internal/vault/enrich_classify_retry_advanced_test.go @@ -11,39 +11,6 @@ import ( // Advanced Retry Logic Tests // ============================================================================ -// TestCallClassifyWithRetry_FirstAttemptSuccess uses first timeout. -func TestCallClassifyWithRetry_FirstAttemptSuccess(t *testing.T) { - // Verify that first attempt uses enrichRetryTimeouts[0] - if enrichRetryTimeouts[0] == 0 { - t.Errorf("First timeout should be non-zero") - } - - provider := &mockClassifyProvider{ - responses: []string{`[{"idx":1,"type":"reference","ctx":"first attempt"}]`}, - errors: []error{nil}, - } - - worker := &enrichWorker{ - provider: provider, - model: "test", - } - - ctx := context.Background() - resp, err := worker.callClassifyWithRetry(ctx, "system", "user") - - if err != nil { - t.Fatalf("First attempt should succeed: %v", err) - } - - if provider.calls != 1 { - t.Errorf("Expected exactly 1 call on first-attempt success, got %d", provider.calls) - } - - if !strings.Contains(resp, "reference") { - t.Errorf("Response missing expected content: %q", resp) - } -} - // TestCallClassifyWithRetry_ResponseWhitespaceStripping trims whitespace from response. func TestCallClassifyWithRetry_ResponseWhitespaceStripping(t *testing.T) { // Response has leading/trailing whitespace that should be stripped @@ -74,24 +41,9 @@ func TestCallClassifyWithRetry_ResponseWhitespaceStripping(t *testing.T) { } } -// TestCallClassifyWithRetry_MaxRetriesConstant verifies retry limit. -func TestCallClassifyWithRetry_MaxRetriesConstant(t *testing.T) { - if enrichMaxRetries != 3 { - t.Errorf("enrichMaxRetries should be 3, got %d", enrichMaxRetries) - } - - // Verify arrays have correct length - if len(enrichRetryTimeouts) != enrichMaxRetries { - t.Errorf("enrichRetryTimeouts length should be %d, got %d", enrichMaxRetries, len(enrichRetryTimeouts)) - } - - if len(enrichRetryBackoffs) != enrichMaxRetries { - t.Errorf("enrichRetryBackoffs length should be %d, got %d", enrichMaxRetries, len(enrichRetryBackoffs)) - } -} - // TestCallClassifyWithRetry_SecondAttemptSucceeds verifies first retry succeeds. func TestCallClassifyWithRetry_SecondAttemptSucceeds(t *testing.T) { + fastBackoffsForTest(t) // skip real 2s backoff between attempts provider := &mockClassifyProvider{ responses: []string{ "", // attempt 0: error diff --git a/internal/vault/enrich_classify_retry_test.go b/internal/vault/enrich_classify_retry_test.go index 22cf3294..230d6c3a 100644 --- a/internal/vault/enrich_classify_retry_test.go +++ b/internal/vault/enrich_classify_retry_test.go @@ -75,6 +75,7 @@ func TestCallClassifyWithRetry_Success(t *testing.T) { // TestCallClassifyWithRetry_RetryThenSuccess fails twice, succeeds on third attempt. func TestCallClassifyWithRetry_RetryThenSuccess(t *testing.T) { + fastBackoffsForTest(t) // skip 2s+4s real backoffs provider := &mockClassifyProvider{ responses: []string{ "", // attempt 0: error @@ -111,6 +112,7 @@ func TestCallClassifyWithRetry_RetryThenSuccess(t *testing.T) { // TestCallClassifyWithRetry_AllFail exhausts retries and returns error. func TestCallClassifyWithRetry_AllFail(t *testing.T) { + fastBackoffsForTest(t) // skip 2s+4s real backoffs provider := &mockClassifyProvider{ responses: []string{"", "", ""}, errors: []error{ diff --git a/internal/vault/helpers_test.go b/internal/vault/helpers_test.go new file mode 100644 index 00000000..8abe4619 --- /dev/null +++ b/internal/vault/helpers_test.go @@ -0,0 +1,29 @@ +package vault + +import ( + "testing" + "time" +) + +// fastBackoffsForTest overrides the package-level enrichRetryBackoffs and +// enrichRetryTimeouts arrays so retry tests don't wait through the real +// exponential backoff (default {0, 2s, 4s} = 6 seconds per all-retry test). +// +// Production behavior is 100% unchanged — only the test sees the fast values. +// Original values are restored via t.Cleanup so parallel/sequential tests +// remain isolated. +// +// Use in any test that exercises callClassifyWithRetry / chatWithRetry with +// >1 attempt. Do NOT use in tests that explicitly assert on the default +// values (e.g. TestCallClassifyWithRetry_RetriesAndBackoffs). +func fastBackoffsForTest(t *testing.T) { + t.Helper() + savedBackoffs := enrichRetryBackoffs + savedTimeouts := enrichRetryTimeouts + enrichRetryBackoffs = [enrichMaxRetries]time.Duration{0, time.Millisecond, time.Millisecond} + enrichRetryTimeouts = [enrichMaxRetries]time.Duration{time.Second, time.Second, time.Second} + t.Cleanup(func() { + enrichRetryBackoffs = savedBackoffs + enrichRetryTimeouts = savedTimeouts + }) +} diff --git a/scripts/check_coverage.go b/scripts/check_coverage.go deleted file mode 100644 index bf4eef7a..00000000 --- a/scripts/check_coverage.go +++ /dev/null @@ -1,259 +0,0 @@ -//go:build ignore - -// Coverage ratchet gate: fails CI if any package drops below its stored threshold. -// -// Usage: -// -// go run scripts/check_coverage.go [-coverprofile=coverage.out] [-thresholds=scripts/coverage_thresholds.json] [-update] -// -// Flags: -// -// -coverprofile: Path to coverage.out (default: coverage.out) -// -thresholds: Path to thresholds JSON (default: scripts/coverage_thresholds.json) -// -update: Write current coverage as new thresholds (ratchet up, explicit opt-in) -// -// Exit codes: -// -// 0 = all packages meet threshold (or --update succeeded) -// 1 = at least one package below threshold -// 2 = parse/IO error -package main - -import ( - "bufio" - "encoding/json" - "flag" - "fmt" - "os" - "sort" - "strconv" - "strings" -) - -const modulePrefix = "github.com/nextlevelbuilder/goclaw/" - -// Trivial/infra-only packages excluded from coverage gate. -var excluded = map[string]bool{ - "internal/version": true, - "internal/webui": true, - "internal/updater": true, - "pkg/protocol": true, - "tests/zalo_e2e": true, - "ui/desktop": true, - "cmd": true, - "scripts": true, -} - -type pkgCoverage struct { - pkg string - statements int - covered int -} - -// parseCoverProfile reads a Go coverage profile and groups statements by package. -// Coverage lines look like: "github.com/foo/bar/file.go:12.1,15.2 3 1" -// Fields: file:start,end numStatements count -func parseCoverProfile(path string) (map[string]*pkgCoverage, error) { - f, err := os.Open(path) - if err != nil { - return nil, err - } - defer f.Close() - - packages := make(map[string]*pkgCoverage) - scanner := bufio.NewScanner(f) - scanner.Buffer(make([]byte, 1024*1024), 1024*1024) - first := true - for scanner.Scan() { - line := scanner.Text() - if first { - first = false - if strings.HasPrefix(line, "mode:") { - continue - } - } - if line == "" { - continue - } - // Split into 3 parts: "file:range numStmt count" - // Last two fields are numeric; everything before is the filename. - parts := strings.Fields(line) - if len(parts) < 3 { - continue - } - fileRange := strings.Join(parts[:len(parts)-2], " ") - numStmt, err := strconv.Atoi(parts[len(parts)-2]) - if err != nil { - continue - } - count, err := strconv.Atoi(parts[len(parts)-1]) - if err != nil { - continue - } - // Extract file path (before ':') - fullFile, _, ok := strings.Cut(fileRange, ":") - if !ok { - continue - } - // Strip module prefix to get relative path. - rel := strings.TrimPrefix(fullFile, modulePrefix) - // Package is the directory. - slash := strings.LastIndex(rel, "/") - if slash < 0 { - continue - } - pkg := rel[:slash] - if pkg == "" { - continue - } - entry, ok := packages[pkg] - if !ok { - entry = &pkgCoverage{pkg: pkg} - packages[pkg] = entry - } - entry.statements += numStmt - if count > 0 { - entry.covered += numStmt - } - } - return packages, scanner.Err() -} - -func isExcluded(pkg string) bool { - if excluded[pkg] { - return true - } - for p := range excluded { - if strings.HasPrefix(pkg, p+"/") { - return true - } - } - return false -} - -func loadThresholds(path string) (map[string]float64, error) { - data, err := os.ReadFile(path) - if err != nil { - if os.IsNotExist(err) { - return map[string]float64{}, nil - } - return nil, err - } - var out map[string]float64 - if err := json.Unmarshal(data, &out); err != nil { - return nil, err - } - return out, nil -} - -func writeThresholds(path string, thresholds map[string]float64) error { - // Sorted output for deterministic diffs. - keys := make([]string, 0, len(thresholds)) - for k := range thresholds { - keys = append(keys, k) - } - sort.Strings(keys) - ordered := make(map[string]float64, len(keys)) - for _, k := range keys { - ordered[k] = thresholds[k] - } - data, err := json.MarshalIndent(ordered, "", " ") - if err != nil { - return err - } - data = append(data, '\n') - return os.WriteFile(path, data, 0o644) -} - -func pctStr(p float64) string { - return fmt.Sprintf("%5.1f%%", p) -} - -func main() { - var ( - profilePath = flag.String("coverprofile", "coverage.out", "coverage profile path") - thresholdsPath = flag.String("thresholds", "scripts/coverage_thresholds.json", "thresholds JSON path") - update = flag.Bool("update", false, "write current coverage as new thresholds") - ) - flag.Parse() - - packages, err := parseCoverProfile(*profilePath) - if err != nil { - fmt.Fprintf(os.Stderr, "error parsing %s: %v\n", *profilePath, err) - os.Exit(2) - } - - thresholds, err := loadThresholds(*thresholdsPath) - if err != nil { - fmt.Fprintf(os.Stderr, "error loading thresholds: %v\n", err) - os.Exit(2) - } - - type row struct { - pkg string - current float64 - threshold float64 - delta float64 - status string - } - var rows []row - failed := 0 - newThresholds := make(map[string]float64) - - for _, entry := range packages { - if isExcluded(entry.pkg) { - continue - } - if entry.statements == 0 { - continue - } - current := 100 * float64(entry.covered) / float64(entry.statements) - threshold := thresholds[entry.pkg] - delta := current - threshold - status := "PASS" - if current+0.01 < threshold { // tiny epsilon for float comparison - status = "FAIL" - failed++ - } - rows = append(rows, row{entry.pkg, current, threshold, delta, status}) - // Preserve existing thresholds for packages that still exist. - newThresholds[entry.pkg] = current - } - - // Preserve thresholds for packages still in file but absent from the - // current coverage profile (e.g., narrow coverprofile, tests excluded in - // this run, sqliteonly build tag). This MUST run in both check and - // --update modes — otherwise `--update` with a narrow profile silently - // wipes floors for all other packages, creating a regression vector. - for k, v := range thresholds { - if _, ok := newThresholds[k]; !ok { - newThresholds[k] = v - } - } - - sort.Slice(rows, func(i, j int) bool { return rows[i].pkg < rows[j].pkg }) - - fmt.Println("Coverage Gate Report") - fmt.Println("====================") - fmt.Printf("%-60s %8s %8s %8s %s\n", "PACKAGE", "CURRENT", "FLOOR", "DELTA", "STATUS") - for _, r := range rows { - fmt.Printf("%-60s %8s %8s %+7.1f%% %s\n", - r.pkg, pctStr(r.current), pctStr(r.threshold), r.delta, r.status) - } - fmt.Println() - - if *update { - if err := writeThresholds(*thresholdsPath, newThresholds); err != nil { - fmt.Fprintf(os.Stderr, "error writing thresholds: %v\n", err) - os.Exit(2) - } - fmt.Printf("Updated %s with %d package thresholds\n", *thresholdsPath, len(newThresholds)) - os.Exit(0) - } - - if failed > 0 { - fmt.Printf("FAIL: %d package(s) below threshold\n", failed) - os.Exit(1) - } - fmt.Printf("PASS: all %d package(s) meet threshold\n", len(rows)) -} diff --git a/scripts/coverage_thresholds.json b/scripts/coverage_thresholds.json deleted file mode 100644 index 3285ed62..00000000 --- a/scripts/coverage_thresholds.json +++ /dev/null @@ -1,63 +0,0 @@ -{ - "internal/agent": 36.809815950920246, - "internal/backup": 19.885057471264368, - "internal/bootstrap": 29.152542372881356, - "internal/bus": 30.578512396694215, - "internal/cache": 96.875, - "internal/channels": 26.524390243902438, - "internal/channels/discord": 27.680311890838208, - "internal/channels/facebook": 81.85404339250493, - "internal/channels/feishu": 63.888888888888886, - "internal/channels/media": 9.174311926605505, - "internal/channels/pancake": 55.319148936170215, - "internal/channels/slack": 19.313850063532403, - "internal/channels/telegram": 13.217072051399725, - "internal/channels/telegram/voiceguard": 100, - "internal/channels/typing": 91.80327868852459, - "internal/channels/whatsapp": 21.323529411764707, - "internal/channels/zalo": 65.2542372881356, - "internal/channels/zalo/personal": 0, - "internal/channels/zalo/personal/protocol": 19.56989247311828, - "internal/channels/zalo/personal/zalomethods": 0, - "internal/config": 48.17275747508306, - "internal/consolidation": 73.77049180327869, - "internal/cron": 73.71428571428571, - "internal/crypto": 75.40983606557377, - "internal/edition": 100, - "internal/eventbus": 79.59183673469387, - "internal/gateway": 15.11056511056511, - "internal/gateway/methods": 7.384515289525049, - "internal/heartbeat": 12.244897959183673, - "internal/http": 12.458820005989818, - "internal/i18n": 100, - "internal/knowledgegraph": 91.76470588235294, - "internal/mcp": 26.271970397779832, - "internal/media": 0, - "internal/memory": 10.169491525423728, - "internal/oauth": 56.17391304347826, - "internal/orchestration": 100, - "internal/permissions": 98.18181818181819, - "internal/pipeline": 76.90417690417691, - "internal/providerresolve": 88.88888888888889, - "internal/providers": 62.53453038674033, - "internal/providers/acp": 80.0498753117207, - "internal/safego": 100, - "internal/sandbox": 6.593406593406593, - "internal/scheduler": 69.1029900332226, - "internal/sessions": 94.37751004016064, - "internal/skills": 37.5, - "internal/store": 25.880281690140844, - "internal/store/base": 95.95959595959596, - "internal/store/pg": 3.51493848857645, - "internal/tasks": 55.4140127388535, - "internal/testutil": 0, - "internal/tokencount": 77.17391304347827, - "internal/tools": 26.59426987060998, - "internal/tracing": 5, - "internal/tracing/otelexport": 11.235955056179776, - "internal/tts": 0, - "internal/upgrade": 0, - "internal/vault": 27.379400260756192, - "internal/workspace": 87.34177215189874, - "pkg/browser": 6.208425720620842 -}