test: add unit tests for MCP modules#432
Open
nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Open
Conversation
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
This was referenced Feb 19, 2026
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive unit test coverage for the MCP (Model Context Protocol) modules, specifically targeting four core modules: os-detector, global-config-manager, symlink-manager, and the index barrel file. The tests ensure cross-platform compatibility and validate all public APIs of these modules, addressing issue #431.
Changes:
- Added 197 lines of tests for os-detector covering OS detection, path utilities, and symlink support checks
- Added 322 lines of tests for global-config-manager covering CRUD operations, server management, and directory structure creation
- Added 626 lines of tests for symlink-manager with extensive cross-platform testing for Windows junctions and Unix symlinks
- Added 117 lines of tests for the MCP index module validating both spread and namespaced exports
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/core/mcp/os-detector.test.js | Tests OS detection, platform checks, path utilities, and link type determination across Windows, macOS, and Linux |
| tests/core/mcp/global-config-manager.test.js | Tests global MCP configuration management including structure creation, config CRUD, server operations, and template handling |
| tests/core/mcp/symlink-manager.test.js | Comprehensive tests for symlink/junction creation, status checking, and removal with extensive Windows and Unix scenarios |
| tests/core/mcp/mcp-index.test.js | Tests the barrel file's export patterns, validating both spread exports and namespaced objects for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| test('getGlobalMcpDir returns ~/.aios/mcp', () => { | ||
| expect(getGlobalMcpDir()).toMatch(/\.aios.mcp$/); |
There was a problem hiding this comment.
The regular expression pattern .aios.mcp$ is missing the path separator character(s) between .aios and mcp. This test assertion will pass for paths like /home/user/.aiosFOOmcp when it should only match paths ending with .aios/mcp. The pattern should be updated to properly match the path separator, for example: /\.aios[\/\\]mcp$/ or more simply, since we know getGlobalMcpDir returns path.join(getGlobalAiosDir(), 'mcp'), just check if it includes '.aios' and ends with 'mcp'.
Suggested change
| expect(getGlobalMcpDir()).toMatch(/\.aios.mcp$/); | |
| expect(getGlobalMcpDir()).toMatch(/\.aios[\/\\]mcp$/); |
This was referenced Feb 20, 2026
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughFour new comprehensive unit test suites were added for MCP modules: global-config-manager, os-detector, symlink-manager, and mcp-index. These tests cover configuration operations, OS detection utilities, filesystem link management, and facade exports with mocked dependencies and both success and error scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 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)
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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/core/mcp/symlink-manager.test.js (2)
405-420: Force-remove-symlink and Windows junction tests are missingresult.successassertions.Both tests verify a side effect (
unlinkSync/execSynccalled) but don't assert the overall operation outcome, leaving the return value unvalidated.✨ Proposed additions
const result = createLink(projectRoot, { force: true }); expect(fs.unlinkSync).toHaveBeenCalledWith(linkPath); + expect(result.success).toBe(true);And for the Windows junction test (lines 463–468):
expect(execSync).toHaveBeenCalledWith( expect.stringContaining('mklink /J'), expect.objectContaining({ windowsHide: true }) ); + expect(result.success).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/symlink-manager.test.js` around lines 405 - 420, The tests that exercise createLink (in tests/core/mcp/symlink-manager.test.js) currently assert side-effects (fs.unlinkSync / child_process.execSync) but do not assert the returned result; update both the "force-remove-symlink" test and the "Windows junction" test to capture the call result (const result = createLink(projectRoot, { force: true }) / createLink(projectRoot, { windows: true } ) as used) and add an assertion that result.success is true so the overall operation outcome is validated in addition to the side-effect assertions.
380-403:callCount-basedlstatSyncmock is brittle and implementation-coupled.The counter assumes exactly 2
lstatSynccalls occur before removal and the third starts returning a symlink. Any refactor that changes the internal call sequence will silently cause this test to exercise a different code path without failing. Additionally, the test asserts only thatfs.rmSyncwas called, not thatresult.successistrue.♻️ Proposed refactor — use sequential mock and add outcome assertion
- let callCount = 0; - fs.lstatSync.mockImplementation(() => { - callCount++; - if (callCount <= 2) return { isSymbolicLink: () => false }; - return { isSymbolicLink: () => true }; - }); - fs.rmSync.mockReturnValue(undefined); - fs.symlinkSync.mockReturnValue(undefined); - fs.readlinkSync.mockReturnValue(globalPath); + // First lstatSync (check existing path) returns directory. + // Second lstatSync (post-creation verification) returns symlink. + fs.lstatSync + .mockReturnValueOnce({ isSymbolicLink: () => false }) + .mockReturnValue({ isSymbolicLink: () => true }); + fs.rmSync.mockReturnValue(undefined); + fs.symlinkSync.mockReturnValue(undefined); + fs.readlinkSync.mockReturnValue(globalPath); const result = createLink(projectRoot, { force: true }); expect(fs.rmSync).toHaveBeenCalledWith(linkPath, { recursive: true }); + expect(result.success).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/symlink-manager.test.js` around lines 380 - 403, The lstatSync call-count pattern is brittle—replace the callCount logic by sequencing explicit mocks (e.g., use fs.lstatSync.mockImplementationOnce(...) twice returning { isSymbolicLink: () => false } and then mockImplementationOnce(...) returning { isSymbolicLink: () => true }) so the test behavior is stable regardless of call ordering changes, and extend assertions after invoking createLink(projectRoot, { force: true }) to check result.success === true in addition to verifying fs.rmSync was calledWith(linkPath, { recursive: true }); keep references to createLink, fs.lstatSync, fs.rmSync and result.success so the change is easy to locate.tests/core/mcp/os-detector.test.js (1)
124-126: Unescaped.in regex matches any character, not specifically the path separator.
/\.aios.mcp$/passes for/home/user/.aios/mcponly because.happens to match/, but it would also match.aiosXmcpfor any characterX. Use a character class to be explicit.✨ Proposed fix
- expect(getGlobalMcpDir()).toMatch(/\.aios.mcp$/); + expect(getGlobalMcpDir()).toMatch(/\.aios[/\\]mcp$/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/os-detector.test.js` around lines 124 - 126, The test's regex `/\.aios.mcp$/` uses an unescaped dot that matches any character; update the assertion in the test for getGlobalMcpDir to require an explicit path separator between ".aios" and "mcp" by replacing the pattern with one that uses a character class for separators, e.g. change the regex to /\.aios[\/\\]mcp$/ so it matches both POSIX and Windows separators and still escapes the leading dot before "aios".tests/core/mcp/global-config-manager.test.js (1)
79-87:globalMcpDirExistsandglobalConfigExiststests lack path assertions.Unlike
globalDirExists(line 76), these tests don't verify the exact path passed toexistsSync, so a path regression in the implementation would go undetected.✨ Proposed fix
test('globalMcpDirExists checks mcp directory', () => { fs.existsSync.mockReturnValue(false); expect(globalMcpDirExists()).toBe(false); + expect(fs.existsSync).toHaveBeenCalledWith('/home/user/.aios/mcp'); }); test('globalConfigExists checks config file', () => { fs.existsSync.mockReturnValue(true); expect(globalConfigExists()).toBe(true); + expect(fs.existsSync).toHaveBeenCalledWith('/home/user/.aios/mcp/global-config.json'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/global-config-manager.test.js` around lines 79 - 87, The tests for globalMcpDirExists and globalConfigExists should also assert that fs.existsSync was called with the exact expected path: after mocking fs.existsSync return value, add expect(fs.existsSync).toHaveBeenCalledWith(expectedPath) for globalMcpDirExists and for globalConfigExists; compute expectedPath using the same path-building logic or helper used by the implementation (so refer to the module's path builder or constants) and use those values in the assertion to catch any path regressions; reference the functions globalMcpDirExists, globalConfigExists and the mocked fs.existsSync when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/mcp/mcp-index.test.js`:
- Around line 75-93: The tests for mcpIndex namespaces (mcpIndex.osDetector,
mcpIndex.globalConfigManager, mcpIndex.symlinkManager, mcpIndex.configMigrator)
currently allow null because they only use toBeDefined() and typeof ===
'object'; update each test to assert that the property is both an object and not
null—for example replace the two assertions with a single stricter assertion
such as expect(mcpIndex.osDetector).toEqual(expect.any(Object)) (and similarly
for globalConfigManager, symlinkManager, configMigrator) or assert typeof ===
'object' and expect(...).not.toBeNull() so null values will fail.
In `@tests/core/mcp/symlink-manager.test.js`:
- Around line 84-87: The trailing-separator test for getProjectMcpPath is
ineffective because path.join normalizes the trailing slash; update the 'should
handle project root with trailing separator' test to assert against a canonical
hardcoded path string (e.g., '/my/project/.aios-core/tools/mcp') or assert
equality with getProjectMcpPath('/my/project') to explicitly test idempotency of
getProjectMcpPath when given a trailing separator.
---
Nitpick comments:
In `@tests/core/mcp/global-config-manager.test.js`:
- Around line 79-87: The tests for globalMcpDirExists and globalConfigExists
should also assert that fs.existsSync was called with the exact expected path:
after mocking fs.existsSync return value, add
expect(fs.existsSync).toHaveBeenCalledWith(expectedPath) for globalMcpDirExists
and for globalConfigExists; compute expectedPath using the same path-building
logic or helper used by the implementation (so refer to the module's path
builder or constants) and use those values in the assertion to catch any path
regressions; reference the functions globalMcpDirExists, globalConfigExists and
the mocked fs.existsSync when adding these assertions.
In `@tests/core/mcp/os-detector.test.js`:
- Around line 124-126: The test's regex `/\.aios.mcp$/` uses an unescaped dot
that matches any character; update the assertion in the test for getGlobalMcpDir
to require an explicit path separator between ".aios" and "mcp" by replacing the
pattern with one that uses a character class for separators, e.g. change the
regex to /\.aios[\/\\]mcp$/ so it matches both POSIX and Windows separators and
still escapes the leading dot before "aios".
In `@tests/core/mcp/symlink-manager.test.js`:
- Around line 405-420: The tests that exercise createLink (in
tests/core/mcp/symlink-manager.test.js) currently assert side-effects
(fs.unlinkSync / child_process.execSync) but do not assert the returned result;
update both the "force-remove-symlink" test and the "Windows junction" test to
capture the call result (const result = createLink(projectRoot, { force: true })
/ createLink(projectRoot, { windows: true } ) as used) and add an assertion that
result.success is true so the overall operation outcome is validated in addition
to the side-effect assertions.
- Around line 380-403: The lstatSync call-count pattern is brittle—replace the
callCount logic by sequencing explicit mocks (e.g., use
fs.lstatSync.mockImplementationOnce(...) twice returning { isSymbolicLink: () =>
false } and then mockImplementationOnce(...) returning { isSymbolicLink: () =>
true }) so the test behavior is stable regardless of call ordering changes, and
extend assertions after invoking createLink(projectRoot, { force: true }) to
check result.success === true in addition to verifying fs.rmSync was
calledWith(linkPath, { recursive: true }); keep references to createLink,
fs.lstatSync, fs.rmSync and result.success so the change is easy to locate.
Comment on lines
+75
to
+93
| it('exports osDetector namespace object', () => { | ||
| expect(mcpIndex.osDetector).toBeDefined(); | ||
| expect(typeof mcpIndex.osDetector).toBe('object'); | ||
| }); | ||
|
|
||
| it('exports globalConfigManager namespace object', () => { | ||
| expect(mcpIndex.globalConfigManager).toBeDefined(); | ||
| expect(typeof mcpIndex.globalConfigManager).toBe('object'); | ||
| }); | ||
|
|
||
| it('exports symlinkManager namespace object', () => { | ||
| expect(mcpIndex.symlinkManager).toBeDefined(); | ||
| expect(typeof mcpIndex.symlinkManager).toBe('object'); | ||
| }); | ||
|
|
||
| it('exports configMigrator namespace object', () => { | ||
| expect(mcpIndex.configMigrator).toBeDefined(); | ||
| expect(typeof mcpIndex.configMigrator).toBe('object'); | ||
| }); |
There was a problem hiding this comment.
Namespace type assertions are too permissive — typeof null === 'object'.
Both toBeDefined() and typeof x === 'object' return truthy for null, so the four namespace tests would still pass if the facade accidentally exported null for any of those properties.
🛡️ Proposed fix for namespace type assertions
- it('exports osDetector namespace object', () => {
- expect(mcpIndex.osDetector).toBeDefined();
- expect(typeof mcpIndex.osDetector).toBe('object');
- });
+ it('exports osDetector namespace object', () => {
+ expect(mcpIndex.osDetector).toEqual(expect.any(Object));
+ });
- it('exports globalConfigManager namespace object', () => {
- expect(mcpIndex.globalConfigManager).toBeDefined();
- expect(typeof mcpIndex.globalConfigManager).toBe('object');
- });
+ it('exports globalConfigManager namespace object', () => {
+ expect(mcpIndex.globalConfigManager).toEqual(expect.any(Object));
+ });
- it('exports symlinkManager namespace object', () => {
- expect(mcpIndex.symlinkManager).toBeDefined();
- expect(typeof mcpIndex.symlinkManager).toBe('object');
- });
+ it('exports symlinkManager namespace object', () => {
+ expect(mcpIndex.symlinkManager).toEqual(expect.any(Object));
+ });
- it('exports configMigrator namespace object', () => {
- expect(mcpIndex.configMigrator).toBeDefined();
- expect(typeof mcpIndex.configMigrator).toBe('object');
- });
+ it('exports configMigrator namespace object', () => {
+ expect(mcpIndex.configMigrator).toEqual(expect.any(Object));
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('exports osDetector namespace object', () => { | |
| expect(mcpIndex.osDetector).toBeDefined(); | |
| expect(typeof mcpIndex.osDetector).toBe('object'); | |
| }); | |
| it('exports globalConfigManager namespace object', () => { | |
| expect(mcpIndex.globalConfigManager).toBeDefined(); | |
| expect(typeof mcpIndex.globalConfigManager).toBe('object'); | |
| }); | |
| it('exports symlinkManager namespace object', () => { | |
| expect(mcpIndex.symlinkManager).toBeDefined(); | |
| expect(typeof mcpIndex.symlinkManager).toBe('object'); | |
| }); | |
| it('exports configMigrator namespace object', () => { | |
| expect(mcpIndex.configMigrator).toBeDefined(); | |
| expect(typeof mcpIndex.configMigrator).toBe('object'); | |
| }); | |
| it('exports osDetector namespace object', () => { | |
| expect(mcpIndex.osDetector).toEqual(expect.any(Object)); | |
| }); | |
| it('exports globalConfigManager namespace object', () => { | |
| expect(mcpIndex.globalConfigManager).toEqual(expect.any(Object)); | |
| }); | |
| it('exports symlinkManager namespace object', () => { | |
| expect(mcpIndex.symlinkManager).toEqual(expect.any(Object)); | |
| }); | |
| it('exports configMigrator namespace object', () => { | |
| expect(mcpIndex.configMigrator).toEqual(expect.any(Object)); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/mcp/mcp-index.test.js` around lines 75 - 93, The tests for
mcpIndex namespaces (mcpIndex.osDetector, mcpIndex.globalConfigManager,
mcpIndex.symlinkManager, mcpIndex.configMigrator) currently allow null because
they only use toBeDefined() and typeof === 'object'; update each test to assert
that the property is both an object and not null—for example replace the two
assertions with a single stricter assertion such as
expect(mcpIndex.osDetector).toEqual(expect.any(Object)) (and similarly for
globalConfigManager, symlinkManager, configMigrator) or assert typeof ===
'object' and expect(...).not.toBeNull() so null values will fail.
Comment on lines
+84
to
+87
| it('should handle project root with trailing separator', () => { | ||
| const result = getProjectMcpPath('/my/project/'); | ||
| expect(result).toBe(path.join('/my/project/', '.aios-core', 'tools', 'mcp')); | ||
| }); |
There was a problem hiding this comment.
Trailing-separator test doesn't exercise distinct behavior.
path.join('/my/project/', '.aios-core', 'tools', 'mcp') normalizes to the same path as without the trailing slash, so the expected value equals the result of the first test. A stronger assertion would compare the output directly against a hardcoded canonical string, or compare to getProjectMcpPath('/my/project') to explicitly assert idempotency.
✨ Proposed alternative assertion
it('should handle project root with trailing separator', () => {
const result = getProjectMcpPath('/my/project/');
- expect(result).toBe(path.join('/my/project/', '.aios-core', 'tools', 'mcp'));
+ expect(result).toBe(getProjectMcpPath('/my/project'));
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/mcp/symlink-manager.test.js` around lines 84 - 87, The
trailing-separator test for getProjectMcpPath is ineffective because path.join
normalizes the trailing slash; update the 'should handle project root with
trailing separator' test to assert against a canonical hardcoded path string
(e.g., '/my/project/.aios-core/tools/mcp') or assert equality with
getProjectMcpPath('/my/project') to explicitly test idempotency of
getProjectMcpPath when given a trailing separator.
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
2 participants
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.
Add test coverage for all MCP modules. All tests pass locally. Closes #431
Summary by CodeRabbit