From ff69504f898e37907382062f331eb359e6123156 Mon Sep 17 00:00:00 2001 From: tw93 Date: Wed, 4 Mar 2026 16:09:13 +0800 Subject: [PATCH] fix: harden CI test stability and status collector resilience --- .gitignore | 1 + cmd/analyze/analyze_test.go | 5 +--- cmd/analyze/delete_test.go | 10 ++----- cmd/analyze/test_helpers_test.go | 44 +++++++++++++++++++++++++++++ cmd/status/metrics.go | 15 ++++++++++ cmd/status/metrics_memory.go | 3 ++ cmd/status/metrics_network.go | 20 +++++++++++-- cmd/status/metrics_network_test.go | 45 +++++++++++++++++++++++++++++- lib/clean/brew.sh | 5 ++++ scripts/test.sh | 29 ++++++++++++++++--- 10 files changed, 158 insertions(+), 19 deletions(-) create mode 100644 cmd/analyze/test_helpers_test.go diff --git a/.gitignore b/.gitignore index d245179..451942f 100644 --- a/.gitignore +++ b/.gitignore @@ -43,6 +43,7 @@ tests/tmp-* # AI Assistant Instructions .claude/ +.agents/ .gemini/ .kiro/ CLAUDE.md diff --git a/cmd/analyze/analyze_test.go b/cmd/analyze/analyze_test.go index f8a4b84..6361ff9 100644 --- a/cmd/analyze/analyze_test.go +++ b/cmd/analyze/analyze_test.go @@ -92,10 +92,7 @@ func TestScanPathConcurrentBasic(t *testing.T) { } func TestDeletePathWithProgress(t *testing.T) { - // Skip in CI environments where Finder may not be available. - if os.Getenv("CI") != "" { - t.Skip("Skipping Finder-dependent test in CI") - } + skipIfFinderUnavailable(t) parent := t.TempDir() target := filepath.Join(parent, "target") diff --git a/cmd/analyze/delete_test.go b/cmd/analyze/delete_test.go index 9e48b22..8d89ae1 100644 --- a/cmd/analyze/delete_test.go +++ b/cmd/analyze/delete_test.go @@ -7,10 +7,7 @@ import ( ) func TestTrashPathWithProgress(t *testing.T) { - // Skip in CI environments where Finder may not be available. - if os.Getenv("CI") != "" { - t.Skip("Skipping Finder-dependent test in CI") - } + skipIfFinderUnavailable(t) parent := t.TempDir() target := filepath.Join(parent, "target") @@ -42,10 +39,7 @@ func TestTrashPathWithProgress(t *testing.T) { } func TestDeleteMultiplePathsCmdHandlesParentChild(t *testing.T) { - // Skip in CI environments where Finder may not be available. - if os.Getenv("CI") != "" { - t.Skip("Skipping Finder-dependent test in CI") - } + skipIfFinderUnavailable(t) base := t.TempDir() parent := filepath.Join(base, "parent") diff --git a/cmd/analyze/test_helpers_test.go b/cmd/analyze/test_helpers_test.go new file mode 100644 index 0000000..9490833 --- /dev/null +++ b/cmd/analyze/test_helpers_test.go @@ -0,0 +1,44 @@ +package main + +import ( + "context" + "os" + "os/exec" + "strings" + "testing" + "time" +) + +func skipIfFinderUnavailable(t *testing.T) { + t.Helper() + + if os.Getenv("CI") != "" { + t.Skip("Skipping Finder-dependent test in CI") + } + if os.Getenv("MOLE_SKIP_FINDER_TESTS") == "1" { + t.Skip("Skipping Finder-dependent test via MOLE_SKIP_FINDER_TESTS") + } + if _, err := exec.LookPath("osascript"); err != nil { + t.Skipf("Skipping Finder-dependent test, osascript unavailable: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, "osascript", "-e", `tell application "Finder" to get name`) + output, err := cmd.CombinedOutput() + text := strings.ToLower(string(output)) + if ctx.Err() == context.DeadlineExceeded { + t.Skip("Skipping Finder-dependent test, Finder probe timed out") + } + if strings.Contains(text, "connection invalid") || strings.Contains(text, "can’t get application \"finder\"") || strings.Contains(text, "can't get application \"finder\"") { + t.Skipf("Skipping Finder-dependent test, Finder probe indicates unavailable session: %s", strings.TrimSpace(string(output))) + } + if err != nil { + reason := strings.TrimSpace(string(output)) + if reason == "" { + reason = err.Error() + } + t.Skipf("Skipping Finder-dependent test, Finder unavailable: %s", reason) + } +} diff --git a/cmd/status/metrics.go b/cmd/status/metrics.go index b0c01c7..75ecd13 100644 --- a/cmd/status/metrics.go +++ b/cmd/status/metrics.go @@ -230,6 +230,9 @@ func (c *Collector) Collect() (MetricsSnapshot, error) { // Host info is cached by gopsutil; fetch once. hostInfo, _ := host.Info() + if hostInfo == nil { + hostInfo = &host.InfoStat{} + } var ( wg sync.WaitGroup @@ -255,6 +258,18 @@ func (c *Collector) Collect() (MetricsSnapshot, error) { wg.Add(1) go func() { defer wg.Done() + defer func() { + if r := recover(); r != nil { + errMu.Lock() + panicErr := fmt.Errorf("collector panic: %v", r) + if mergeErr == nil { + mergeErr = panicErr + } else { + mergeErr = fmt.Errorf("%v; %w", mergeErr, panicErr) + } + errMu.Unlock() + } + }() if err := fn(); err != nil { errMu.Lock() if mergeErr == nil { diff --git a/cmd/status/metrics_memory.go b/cmd/status/metrics_memory.go index 0ffe644..3e61631 100644 --- a/cmd/status/metrics_memory.go +++ b/cmd/status/metrics_memory.go @@ -17,6 +17,9 @@ func collectMemory() (MemoryStatus, error) { } swap, _ := mem.SwapMemory() + if swap == nil { + swap = &mem.SwapMemoryStat{} + } pressure := getMemoryPressure() // On macOS, vm.Cached is 0, so we calculate from file-backed pages. diff --git a/cmd/status/metrics_network.go b/cmd/status/metrics_network.go index d0cd469..e3c0c46 100644 --- a/cmd/status/metrics_network.go +++ b/cmd/status/metrics_network.go @@ -2,6 +2,7 @@ package main import ( "context" + "fmt" "net/url" "os" "runtime" @@ -13,10 +14,25 @@ import ( "github.com/shirou/gopsutil/v4/net" ) +var ioCountersFunc = net.IOCounters + +func collectIOCountersSafely(pernic bool) (stats []net.IOCountersStat, err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("panic collecting network counters: %v", r) + } + }() + return ioCountersFunc(pernic) +} + func (c *Collector) collectNetwork(now time.Time) ([]NetworkStatus, error) { - stats, err := net.IOCounters(true) + stats, err := collectIOCountersSafely(true) if err != nil { - return nil, err + // Some restricted environments can break netstat-backed collectors. + // Degrade gracefully to keep status output available. + c.rxHistoryBuf.Add(0) + c.txHistoryBuf.Add(0) + return nil, nil } // Map interface IPs. diff --git a/cmd/status/metrics_network_test.go b/cmd/status/metrics_network_test.go index 640675a..51e47cc 100644 --- a/cmd/status/metrics_network_test.go +++ b/cmd/status/metrics_network_test.go @@ -1,6 +1,11 @@ package main -import "testing" +import ( + "strings" + "testing" + + gopsutilnet "github.com/shirou/gopsutil/v4/net" +) func TestCollectProxyFromEnvSupportsAllProxy(t *testing.T) { env := map[string]string{ @@ -58,3 +63,41 @@ func TestCollectProxyFromScutilOutputHTTPHostPort(t *testing.T) { t.Fatalf("unexpected host: %s", got.Host) } } + +func TestCollectIOCountersSafelyRecoversPanic(t *testing.T) { + original := ioCountersFunc + ioCountersFunc = func(bool) ([]gopsutilnet.IOCountersStat, error) { + panic("boom") + } + t.Cleanup(func() { ioCountersFunc = original }) + + stats, err := collectIOCountersSafely(true) + if err == nil { + t.Fatalf("expected error from panic recovery") + } + if !strings.Contains(err.Error(), "panic collecting network counters") { + t.Fatalf("unexpected error: %v", err) + } + if len(stats) != 0 { + t.Fatalf("expected empty stats when panic recovered") + } +} + +func TestCollectIOCountersSafelyReturnsData(t *testing.T) { + original := ioCountersFunc + want := []gopsutilnet.IOCountersStat{ + {Name: "en0", BytesRecv: 1, BytesSent: 2}, + } + ioCountersFunc = func(bool) ([]gopsutilnet.IOCountersStat, error) { + return want, nil + } + t.Cleanup(func() { ioCountersFunc = original }) + + got, err := collectIOCountersSafely(true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 1 || got[0].Name != "en0" { + t.Fatalf("unexpected stats: %+v", got) + } +} diff --git a/lib/clean/brew.sh b/lib/clean/brew.sh index cf16da4..202c45a 100644 --- a/lib/clean/brew.sh +++ b/lib/clean/brew.sh @@ -13,6 +13,11 @@ clean_homebrew() { fi return 0 fi + # Keep behavior consistent with dry-run preview. + if is_path_whitelisted "$HOME/Library/Caches/Homebrew"; then + echo -e " ${GREEN}${ICON_SUCCESS}${NC} Homebrew · skipped whitelist" + return 0 + fi # Skip if cleaned recently to avoid repeated heavy operations. local brew_cache_file="${HOME}/.cache/mole/brew_last_cleanup" local cache_valid_days=7 diff --git a/scripts/test.sh b/scripts/test.sh index d42905c..2cf6fab 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -150,7 +150,11 @@ echo "" echo "3. Running Go tests..." if command -v go > /dev/null 2>&1; then - if go build ./... > /dev/null 2>&1 && go vet ./cmd/... > /dev/null 2>&1 && go test ./cmd/... > /dev/null 2>&1; then + GO_TEST_CACHE="${MOLE_GO_TEST_CACHE:-/tmp/mole-go-build-cache}" + mkdir -p "$GO_TEST_CACHE" + if GOCACHE="$GO_TEST_CACHE" go build ./... > /dev/null 2>&1 && + GOCACHE="$GO_TEST_CACHE" go vet ./cmd/... > /dev/null 2>&1 && + GOCACHE="$GO_TEST_CACHE" go test ./cmd/... > /dev/null 2>&1; then printf "${GREEN}${ICON_SUCCESS} Go tests passed${NC}\n" else printf "${RED}${ICON_ERROR} Go tests failed${NC}\n" @@ -182,9 +186,23 @@ echo "" echo "6. Testing installation..." # Skip if Homebrew mole is installed (install.sh will refuse to overwrite) -if brew list mole &> /dev/null; then +install_test_home="" +if command -v brew > /dev/null 2>&1 && brew list mole &> /dev/null; then printf "${GREEN}${ICON_SUCCESS} Installation test skipped, Homebrew${NC}\n" -elif ./install.sh --prefix /tmp/mole-test > /dev/null 2>&1; then +else + install_test_home="$(mktemp -d /tmp/mole-test-home.XXXXXX 2> /dev/null || true)" + if [[ -z "$install_test_home" ]]; then + install_test_home="/tmp/mole-test-home" + mkdir -p "$install_test_home" + fi +fi +if [[ -z "$install_test_home" ]]; then + : +elif HOME="$install_test_home" \ + XDG_CONFIG_HOME="$install_test_home/.config" \ + XDG_CACHE_HOME="$install_test_home/.cache" \ + MO_NO_OPLOG=1 \ + ./install.sh --prefix /tmp/mole-test > /dev/null 2>&1; then if [ -f /tmp/mole-test/mole ]; then printf "${GREEN}${ICON_SUCCESS} Installation test passed${NC}\n" else @@ -195,7 +213,10 @@ else printf "${RED}${ICON_ERROR} Installation test failed${NC}\n" ((FAILED++)) fi -safe_remove "/tmp/mole-test" true || true +MO_NO_OPLOG=1 safe_remove "/tmp/mole-test" true || true +if [[ -n "$install_test_home" ]]; then + MO_NO_OPLOG=1 safe_remove "$install_test_home" true || true +fi echo "" echo "==============================="