fix: make hook-project, embedding, and mesh tests hermetic#734
Conversation
These three suites coupled to the developer's environment and failed outside a specific setup:
- hook-project.test.ts hardcoded the resolved project basename as "agentmemory", so the suite failed for anyone who cloned the repo into a directory with a different name. Derive the expected basename from the git toplevel the same way resolveProject does, and fix a Windows-only assertion that used dir.split('/') instead of path.basename.
- embedding-provider.test.ts asserted "returns null when no API keys are set" and default embedding dimensions, but the provider resolves keys through getMergedEnv(), which re-reads ~/.agentmemory/.env on every call. A developer with a real key on disk leaked it into those assertions. Mock the config env-merge so the tests see only process.env.
- mesh.test.ts let isAllowedUrl() run a real dns.lookup() on *.example.com hostnames, which hangs to the vitest timeout on DNS-restricted CI hosts. Mock node:dns/promises so it resolves instantly while isAllowedUrl's own logic still runs.
Test-only; no production code changes.
Signed-off-by: MarvinFS <MarvinFS@users.noreply.github.com>
|
@MarvinFS is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR updates three test files to improve test isolation by mocking or deriving environmental factors at runtime. The embedding provider test now mocks the config module to prevent ChangesTest Hermiticity Isolation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Three test suites coupled to the developer's environment and failed outside a specific setup. All three fixes are test-only; no production code changes.
test/hook-project.test.tsHardcoded the resolved project basename as
"agentmemory", so the suite fails for anyone who clones the repo into a directory with a different name (e.g. a fork checked out asagentmemory-lancedb). Now derives the expected basename from the git toplevel the same wayresolveProjectdoes. Also fixes a Windows-only assertion that useddir.split('/')instead ofpath.basename.test/embedding-provider.test.tsAsserts "returns null when no API keys are set" and the default embedding dimensions, but the provider resolves keys through
getMergedEnv(), which re-reads~/.agentmemory/.envon every call. A developer with a real key on disk leaks it into those assertions and the tests fail. Now mocks the config env-merge so the tests see onlyprocess.env.test/mesh.test.tsLet
isAllowedUrl()run a realdns.lookup()on*.example.comhostnames, which hangs to the vitest timeout on DNS-restricted CI hosts. Now mocksnode:dns/promisesso it resolves instantly whileisAllowedUrl's own logic still runs.Verified
npm run buildclean andnpm testgreen on a freshmaincheckout.Summary by CodeRabbit