Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates tests/config to avoid creating or relying on files inside the repository source tree, aligning with the goal of keeping the working tree clean during test runs (Related: AAP-65204).
Changes:
- Move
ansible-mcp-servertool tests to create playbook fixtures in a per-run OS temp directory and clean up the directory afterward. - Centralize
ansible-language-serverfixture base path resolution via an exportedFIXTURES_BASE_PATHtest helper constant. - Simplify
eslint.config.mjsby removing manual__dirnamederivation and droppingtsconfigRootDir.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts | Writes playbooks into a temp directory instead of the test source directory; cleans up temp directory after tests. |
| packages/ansible-mcp-server/test/tools/ansibleLint.test.ts | Same temp-directory approach for ansible-lint tests; cleans up after tests. |
| packages/ansible-language-server/test/utils/pathUtils.test.ts | Uses shared FIXTURES_BASE_PATH to locate fixtures. |
| packages/ansible-language-server/test/utils/getAnsibleMetaData.test.ts | Uses shared FIXTURES_BASE_PATH for fixture paths instead of per-file __dirname resolution. |
| packages/ansible-language-server/test/helper.ts | Exports FIXTURES_BASE_PATH for reuse across tests. |
| eslint.config.mjs | Removes manual __dirname setup and tsconfigRootDir configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+39
to
41
| afterAll(() => { | ||
| rmSync(tempDir, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
afterAll unconditionally calls rmSync(tempDir, ...). If beforeAll throws before tempDir is assigned (e.g., mkdtemp/writeFile failure), afterAll will run with tempDir undefined and throw, masking the original failure. Consider initializing tempDir as string | undefined and guarding in afterAll (or wrapping setup/teardown in try/finally).
Comment on lines
+37
to
39
| afterAll(() => { | ||
| rmSync(tempDir, { recursive: true, force: true }); | ||
| }); |
There was a problem hiding this comment.
afterAll unconditionally calls rmSync(tempDir, ...). If beforeAll fails before assigning tempDir (mkdtemp/writeFile errors), cleanup will throw with an invalid path and can hide the real failure. Consider making tempDir optional and guarding the rmSync call.
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
📝 WalkthroughWalkthroughThis pull request removes ESM module boilerplate ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts (1)
39-40: Guard teardown to avoid masking setup failures.Line 40 assumes
tempDiris always initialized. IfbeforeAllfails early, teardown can throw and hide the original failure.Suggested hardening
-import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; ... - let tempDir: string; + let tempDir = ""; ... afterAll(() => { - rmSync(tempDir, { recursive: true, force: true }); + if (tempDir && existsSync(tempDir)) { + rmSync(tempDir, { recursive: true, force: true }); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts` around lines 39 - 40, The afterAll teardown calls rmSync(tempDir, { recursive: true, force: true }) unguarded which can throw if beforeAll failed to set tempDir; update the afterAll handler to first verify tempDir is defined and optionally that it exists (e.g., using fs.existsSync) and only then call rmSync, or wrap the rmSync call in a try/catch to swallow cleanup errors so setup failures aren't masked; reference the afterAll block, tempDir variable and rmSync call when making this change.packages/ansible-mcp-server/test/tools/ansibleLint.test.ts (1)
37-38: Make cleanup resilient to partial setup failures.Line 38 can throw if
beforeAllfails beforetempDiris assigned. Guarding teardown prevents secondary failures from obscuring the primary error.Suggested hardening
-import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; ... - let tempDir: string; + let tempDir = ""; ... afterAll(() => { - rmSync(tempDir, { recursive: true, force: true }); + if (tempDir && existsSync(tempDir)) { + rmSync(tempDir, { recursive: true, force: true }); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ansible-mcp-server/test/tools/ansibleLint.test.ts` around lines 37 - 38, The teardown in afterAll currently calls rmSync(tempDir, { recursive: true, force: true }) which can throw if tempDir was never set (due to beforeAll failing); update afterAll to guard the cleanup by checking that tempDir is defined and non-empty (and/or exists) before calling rmSync, and wrap the rmSync call in a try/catch to swallow/ log any cleanup errors so they don't mask the original failure — locate the afterAll block and the tempDir variable referenced there and add the guard and error handling around rmSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ansible-mcp-server/test/tools/ansibleLint.test.ts`:
- Around line 37-38: The teardown in afterAll currently calls rmSync(tempDir, {
recursive: true, force: true }) which can throw if tempDir was never set (due to
beforeAll failing); update afterAll to guard the cleanup by checking that
tempDir is defined and non-empty (and/or exists) before calling rmSync, and wrap
the rmSync call in a try/catch to swallow/ log any cleanup errors so they don't
mask the original failure — locate the afterAll block and the tempDir variable
referenced there and add the guard and error handling around rmSync.
In `@packages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts`:
- Around line 39-40: The afterAll teardown calls rmSync(tempDir, { recursive:
true, force: true }) unguarded which can throw if beforeAll failed to set
tempDir; update the afterAll handler to first verify tempDir is defined and
optionally that it exists (e.g., using fs.existsSync) and only then call rmSync,
or wrap the rmSync call in a try/catch to swallow cleanup errors so setup
failures aren't masked; reference the afterAll block, tempDir variable and
rmSync call when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32e813e0-7f47-4216-b119-e459ac330778
📒 Files selected for processing (6)
eslint.config.mjspackages/ansible-language-server/test/helper.tspackages/ansible-language-server/test/utils/getAnsibleMetaData.test.tspackages/ansible-language-server/test/utils/pathUtils.test.tspackages/ansible-mcp-server/test/tools/ansibleLint.test.tspackages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts
💤 Files with no reviewable changes (1)
- eslint.config.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: AAP-65204
The PR avoids creating temporary files inside the source tree during test execution by consolidating fixture path references and migrating tests to use system temporary directories. This ensures test artifacts don't pollute the repository, improving build hygiene and compatibility with different file system layouts.
FIXTURES_BASE_PATHconstant from test helper for shared fixture location reference__dirnamederivations from eslint.config.mjs, simplifying parser configurationFIXTURES_BASE_PATHinstead of deriving paths from__dirnamemkdtempSyncinstead of writing to source treerelated: AAP-65204