Skip to content

fix(watcher-tests): await ready + harden Windows cleanup#55

Merged
PatrickSys merged 2 commits intomasterfrom
fix/phase2d-correction-watch-tests
Mar 1, 2026
Merged

fix(watcher-tests): await ready + harden Windows cleanup#55
PatrickSys merged 2 commits intomasterfrom
fix/phase2d-correction-watch-tests

Conversation

@PatrickSys
Copy link
Owner

@PatrickSys PatrickSys commented Mar 1, 2026

What / Why

Phase 2d (auto-refresh file watcher) is merged, but a full-suite run on Windows exposed intermittent flakes that make CI/local verification unreliable:

  • Watcher readiness race: writes can occur before chokidar is fully ready, causing missed change events.
  • Windows cleanup race: temp directory deletion can intermittently fail with ENOTEMPTY / EPERM / EBUSY.

This PR hardens the watcher tests (and adds a tiny optional hook) so the test suite is deterministic and accurately validates 2d behavior.

Changes

  • src/core/file-watcher.ts: add optional onReady?: () => void and fire it on chokidar 'ready'.
  • tests/file-watcher.test.ts: wait for watcher readiness; adjust debounce test timing to avoid double-fire flakes.
  • tests/auto-refresh-e2e.test.ts: wait for watcher readiness instead of sleeping.
  • tests/search-snippets.test.ts: retry fs.rm on Windows + bump hook timeouts.
  • tests/impact-2hop.test.ts: retry fs.rm + use os.tmpdir() for temp roots (avoid repo/gitrepo side effects + sporadic EPERM lstat).

Non-goals / Notes

  • No functional/product behavior changes intended (existing watcher consumers remain compatible; onReady is optional).
  • Does not address the separate Phase 2e follow-up: some --json CLI commands still print extra non-JSON to stdout.

Validation

  • CI checks: Quality Checks, Functional Tests, GitGuardian all green.
  • Local: pnpm test (full suite) green on Windows after these changes.

Release notes

  • Minor: stabilize watcher-related tests on Windows (no user-facing change).

@greptile-apps
Copy link

greptile-apps bot commented Mar 1, 2026

Greptile Summary

This PR improves test reliability by replacing arbitrary sleep timings with deterministic ready callbacks and adding retry logic for Windows filesystem cleanup issues.

  • Replaced sleep() calls with onReady callback waits in all file watcher tests
  • Added rmWithRetries helper with exponential backoff for ENOTEMPTY/EPERM/EBUSY errors in 2 test files
  • Adjusted debounce timings in tests for better reliability

Issues found:

  • file-watcher.test.ts and auto-refresh-e2e.test.ts are missing the Windows cleanup hardening despite creating file watchers that can cause lock issues
  • rmWithRetries is duplicated across multiple test files instead of being extracted to a shared utility

Confidence Score: 3/5

  • Safe to merge but incomplete - Windows cleanup hardening only applied to 2 of 4 test files
  • The changes are well-intentioned and improve test determinism, but the Windows cleanup hardening is incomplete. Two test files that create file watchers (file-watcher.test.ts and auto-refresh-e2e.test.ts) still use direct fs.rm calls instead of rmWithRetries, which means they could still experience flaky failures on Windows. The onReady callback implementation is solid.
  • Pay close attention to tests/file-watcher.test.ts and tests/auto-refresh-e2e.test.ts - both need Windows cleanup hardening

Important Files Changed

Filename Overview
tests/file-watcher.test.ts Updated tests to await watcher ready event, but missing Windows cleanup hardening
tests/auto-refresh-e2e.test.ts Replaced sleep with ready callback, but missing Windows cleanup hardening
tests/impact-2hop.test.ts Added retry logic with exponential backoff for Windows filesystem cleanup

Last reviewed commit: 87e7434

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +21 to +37
async function rmWithRetries(targetPath: string): Promise<void> {
const maxAttempts = 8;
let delayMs = 25;

for (let attempt = 1; attempt <= maxAttempts; attempt++) {
try {
await fs.rm(targetPath, { recursive: true, force: true });
return;
} catch (error) {
const code = (error as { code?: string }).code;
const retryable = code === 'ENOTEMPTY' || code === 'EPERM' || code === 'EBUSY';
if (!retryable || attempt === maxAttempts) throw error;
await new Promise((r) => setTimeout(r, delayMs));
delayMs *= 2;
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider extracting rmWithRetries to a shared test utility file since it's duplicated in search-snippets.test.ts and will likely be needed in other test files

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@greptile-apps
Copy link

greptile-apps bot commented Mar 1, 2026

Additional Comments (2)

tests/file-watcher.test.ts
missing Windows cleanup hardening - this test creates a file watcher and should use rmWithRetries like impact-2hop.test.ts and search-snippets.test.ts to handle Windows filesystem lock issues (ENOTEMPTY, EPERM, EBUSY)


tests/auto-refresh-e2e.test.ts
missing Windows cleanup hardening - this test creates a file watcher and should use rmWithRetries like impact-2hop.test.ts and search-snippets.test.ts to handle Windows filesystem lock issues (ENOTEMPTY, EPERM, EBUSY)

@PatrickSys
Copy link
Owner Author

Greptile feedback addressed in ebf15db: extracted
mWithRetries into ests/test-helpers.ts and used it in ests/file-watcher.test.ts + ests/auto-refresh-e2e.test.ts (and de-duplicated from the other suites). Watcher-related tests re-run locally and passing.

@PatrickSys PatrickSys merged commit 9929bb0 into master Mar 1, 2026
3 checks passed
@PatrickSys PatrickSys deleted the fix/phase2d-correction-watch-tests branch March 2, 2026 13:09
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