test: add unit tests for MCP config-migrator module#502
test: add unit tests for MCP config-migrator module#502nikolasdehor wants to merge 3 commits intoSynkraAI:mainfrom
Conversation
48 tests covering: - MIGRATION_OPTION constants - detectProjectConfig: multi-path detection, legacy mcpServers format, invalid JSON fallthrough, server counting, default project root - analyzeMigration: all 5 scenarios (already_linked, fresh_install, link_only, migrate_to_global, merge_required) - mergeServers: add/skip/overwrite logic, conflict tracking, immutability of input objects, default parameters - executeMigration: full lifecycle (structure creation, config creation, server migration, symlink creation, backup), error propagation for each stage, KEEP_PROJECT short-circuit - restoreFromBackup: symlink removal, directory removal, rename errors, lstat errors, missing backup detection
nikolasdehor
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@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. |
WalkthroughAdds a comprehensive unit test suite for the config-migrator module that validates detection, analysis, merging, execution, and rollback behaviors using mocked dependencies and extensive error-path coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🧹 Nitpick comments (1)
tests/core/mcp/config-migrator.test.js (1)
40-47: UseresetAllMocks()to avoid implementation leakage between tests.
jest.clearAllMocks()resets call history only; mocked return values/implementations can persist across test cases and create order-dependent behavior in this suite.Suggested change
beforeEach(() => { - jest.clearAllMocks(); + jest.resetAllMocks(); // Default mocks getProjectMcpPath.mockReturnValue('/project/.aios-core/tools/mcp'); checkLinkStatus.mockReturnValue({ status: LINK_STATUS.NOT_LINKED }); globalConfigExists.mockReturnValue(false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/config-migrator.test.js` around lines 40 - 47, Replace jest.clearAllMocks() with jest.resetAllMocks() in the beforeEach block so mocked implementations/return values are reset between tests; after calling jest.resetAllMocks(), re-establish the default mocks for getProjectMcpPath, checkLinkStatus, and globalConfigExists (e.g., mockReturnValue calls shown) inside beforeEach so each test starts with the intended defaults.
🤖 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/config-migrator.test.js`:
- Around line 550-556: The test should not assume executeMigration throws
synchronously; instead mock checkLinkStatus (used by analyzeMigration) to return
a rejected Promise and assert executeMigration rejects (or returns a structured
failure) using async assertions. Update the test named 'propagates exceptions
thrown before try/catch (analyzeMigration)' so
checkLinkStatus.mockImplementation(() => Promise.reject(new
Error('unexpected'))) and use await
expect(executeMigration('/project')).rejects.toThrow('unexpected') (or assert
the returned failure shape if your API returns an error object), referencing
checkLinkStatus, analyzeMigration, and executeMigration.
---
Nitpick comments:
In `@tests/core/mcp/config-migrator.test.js`:
- Around line 40-47: Replace jest.clearAllMocks() with jest.resetAllMocks() in
the beforeEach block so mocked implementations/return values are reset between
tests; after calling jest.resetAllMocks(), re-establish the default mocks for
getProjectMcpPath, checkLinkStatus, and globalConfigExists (e.g.,
mockReturnValue calls shown) inside beforeEach so each test starts with the
intended defaults.
| test('propagates exceptions thrown before try/catch (analyzeMigration)', () => { | ||
| // analyzeMigration is called before the try/catch in executeMigration, | ||
| // so errors from it propagate as unhandled exceptions | ||
| checkLinkStatus.mockImplementation(() => { throw new Error('unexpected'); }); | ||
|
|
||
| expect(() => executeMigration('/project')).toThrow('unexpected'); | ||
| }); |
There was a problem hiding this comment.
Avoid locking in unhandled exceptions as the expected API contract.
This test currently validates a throw path from executeMigration, which makes error handling brittle and discourages returning structured failures from a migration API.
Suggested test direction (paired with implementation hardening)
- expect(() => executeMigration('/project')).toThrow('unexpected');
+ const result = executeMigration('/project');
+ expect(result.success).toBe(false);
+ expect(result.errors).toContainEqual(expect.stringContaining('unexpected'));As per coding guidelines, "Verify error handling is comprehensive."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/mcp/config-migrator.test.js` around lines 550 - 556, The test
should not assume executeMigration throws synchronously; instead mock
checkLinkStatus (used by analyzeMigration) to return a rejected Promise and
assert executeMigration rejects (or returns a structured failure) using async
assertions. Update the test named 'propagates exceptions thrown before try/catch
(analyzeMigration)' so checkLinkStatus.mockImplementation(() =>
Promise.reject(new Error('unexpected'))) and use await
expect(executeMigration('/project')).rejects.toThrow('unexpected') (or assert
the returned failure shape if your API returns an error object), referencing
checkLinkStatus, analyzeMigration, and executeMigration.
There was a problem hiding this comment.
Pull request overview
Adds a dedicated Jest unit test suite for the MCP config-migrator module to validate migration detection, scenario analysis, merging behavior, execution flow, and backup restore logic while mocking external dependencies.
Changes:
- Introduces 48 unit tests covering
detectProjectConfig,analyzeMigration,mergeServers,executeMigration, andrestoreFromBackup. - Adds dependency mocking for
global-config-managerandsymlink-managerto avoid filesystem/global state side effects.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test('reports errors when structure creation fails', () => { | ||
| checkLinkStatus.mockReturnValue({ status: LINK_STATUS.NOT_LINKED }); | ||
| globalConfigExists.mockReturnValue(false); | ||
| createGlobalStructure.mockReturnValue({ success: false, errors: ['disk full'] }); |
There was a problem hiding this comment.
The mocked return shape for createGlobalStructure here doesn’t match the real implementation. In .aios-core/core/mcp/global-config-manager.js, createGlobalStructure() returns errors as an array of objects like { dir, error } (and { file, error }), not strings. Using ['disk full'] risks masking a real issue in executeMigration where it does structureResult.errors.join(', ') (which would produce [object Object]). Consider mocking errors with the real object shape and asserting on the resulting message (or update production code to format the object errors).
| createGlobalStructure.mockReturnValue({ success: false, errors: ['disk full'] }); | |
| createGlobalStructure.mockReturnValue({ | |
| success: false, | |
| errors: [{ dir: '/global', error: 'disk full' }], | |
| }); |
| test('backs up project directory when DIRECTORY status', () => { | ||
| const configPath = path.join('/project', 'mcp.json'); | ||
| existsSyncSpy.mockImplementation((p) => p === configPath); | ||
| readFileSyncSpy.mockReturnValue(JSON.stringify({ servers: {} })); | ||
| checkLinkStatus.mockReturnValue({ status: LINK_STATUS.DIRECTORY }); | ||
| globalConfigExists.mockReturnValue(true); | ||
| readGlobalConfig.mockReturnValue({ version: '1.0', servers: {} }); | ||
| writeGlobalConfig.mockReturnValue({ success: true }); | ||
| createLink.mockReturnValue({ success: true }); | ||
| getProjectMcpPath.mockReturnValue('/project/.aios-core/tools/mcp'); | ||
|
|
||
| const result = executeMigration('/project', MIGRATION_OPTION.MERGE); |
There was a problem hiding this comment.
This test sets checkLinkStatus to DIRECTORY, which in the real symlink-manager implies the project MCP path actually exists as a directory. However, the fs.existsSync mock only returns true for the config file (/project/mcp.json), and fs.renameSync is stubbed to always succeed. That combination can make the backup-path test pass even if backup logic would fail in a real DIRECTORY scenario. Consider making the mocks consistent (e.g., existsSync true for the project MCP path and/or have renameSync throw when the source path is missing) and assert renameSync was called with the expected projectMcpPath and backupPath.
- Troca clearAllMocks por resetAllMocks (evita leak de implementações)
- Usa shape real de errors do createGlobalStructure ({ dir, error })
- Adiciona asserção de argumentos no renameSync do backup DIRECTORY
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/core/mcp/config-migrator.test.js (1)
559-565: Test documents brittle throw-through behavior — known concern from prior review.This test locks in the fact that
executeMigrationpropagates unhandled exceptions fromanalyzeMigration(called before the internal try/catch). While the test accurately describes current behavior, it codifies a fragile contract. Ideally,executeMigrationwould wrap the full flow in error handling and return a structured failure.Since this is a test-only PR, the implementation change is out of scope — but this should be tracked as follow-up work. As per coding guidelines, "Verify error handling is comprehensive."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/config-migrator.test.js` around lines 559 - 565, Test currently asserts that executeMigration allows exceptions from analyzeMigration to propagate (via checkLinkStatus mock), which locks in brittle behavior; since implementation changes are out of scope, mark this as a known-brittle test and refrain from enforcing the throw-through contract: change the test to test.skip (or prepend it with a clear TODO) and add a one-line comment referencing a follow-up issue to make executeMigration handle analyzeMigration errors comprehensively; mention executeMigration, analyzeMigration and checkLinkStatus in the comment so the future fix can locate the behavior to change.
🧹 Nitpick comments (3)
tests/core/mcp/config-migrator.test.js (3)
39-47: Clean mock lifecycle — good use ofresetAllMocks.The
beforeEachproperly resets all mocks and re-establishes sensible defaults, preventing state leakage between tests. This is the right approach after switching fromclearAllMocks.Minor nit: Line 43 has a Portuguese comment (
"re-estabelecidos após resetAllMocks"). Consider keeping comments in English for consistency across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/config-migrator.test.js` around lines 39 - 47, Replace the Portuguese inline comment in the beforeEach block with an English equivalent to keep code comments consistent; specifically update the comment near the mock re-establishment (adjacent to getProjectMcpPath.mockReturnValue, checkLinkStatus.mockReturnValue, and globalConfigExists.mockReturnValue) from "re-estabelecidos após resetAllMocks" to a clear English phrase like "re-established after resetAllMocks".
191-196: Default parameter test could be slightly more assertive.The comment says "Just verify it doesn't throw," but the test does assert
result.found === false. Consider also verifying thatexistsSyncSpywas called with a path derived fromprocess.cwd()to confirm the default is actually wired up:💡 Optional strengthening
test('uses process.cwd() as default project root', () => { existsSyncSpy.mockReturnValue(false); const result = detectProjectConfig(); expect(result.found).toBe(false); - // Just verify it doesn't throw + expect(existsSyncSpy).toHaveBeenCalledWith( + expect.stringContaining(process.cwd()), + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/config-migrator.test.js` around lines 191 - 196, Update the test "uses process.cwd() as default project root" to not only assert result.found is false but also verify that existsSyncSpy was invoked with a path derived from process.cwd(); call detectProjectConfig() and assert that existsSyncSpy received a path containing process.cwd() (use process.cwd(), detectProjectConfig, and existsSyncSpy as the referenced symbols) so the test confirms the default root wiring.
473-488: Portuguese comments — and a useful source-code bug observation.Lines 485–486 document an apparent bug in the production code where
errors(which are objects with{ dir, error }shape) are.join(', ')-ed, producing[object Object]. This is valuable insight, but the comments should be in English for the team:🌐 Suggested English translation
- // NOTA: config-migrator usa .join(', ') em objetos, resultando em [object Object] - // Isso é um bug no source — os errors deveriam ser mapeados antes do join + // NOTE: config-migrator calls .join(', ') on objects, producing [object Object] + // This is a source bug — errors should be mapped to strings before joinAdditionally, consider filing an issue to fix the serialization bug in the production code rather than leaving it as a test comment.
Would you like me to open an issue to track the
[object Object]serialization bug inconfig-migrator.js?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/config-migrator.test.js` around lines 473 - 488, Replace the Portuguese test comment lines that describe the serialization bug with an English comment (e.g., note that createGlobalStructure returns error objects { dir, error } and that config-migrator.js currently does .join(', ') on those objects producing “[object Object]”), and then file a tracker issue for config-migrator.js describing the bug and suggested fixes (map error objects to strings or JSON.stringify before joining); reference createGlobalStructure and executeMigration in the issue to help locate the problematic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/core/mcp/config-migrator.test.js`:
- Around line 559-565: Test currently asserts that executeMigration allows
exceptions from analyzeMigration to propagate (via checkLinkStatus mock), which
locks in brittle behavior; since implementation changes are out of scope, mark
this as a known-brittle test and refrain from enforcing the throw-through
contract: change the test to test.skip (or prepend it with a clear TODO) and add
a one-line comment referencing a follow-up issue to make executeMigration handle
analyzeMigration errors comprehensively; mention executeMigration,
analyzeMigration and checkLinkStatus in the comment so the future fix can locate
the behavior to change.
---
Nitpick comments:
In `@tests/core/mcp/config-migrator.test.js`:
- Around line 39-47: Replace the Portuguese inline comment in the beforeEach
block with an English equivalent to keep code comments consistent; specifically
update the comment near the mock re-establishment (adjacent to
getProjectMcpPath.mockReturnValue, checkLinkStatus.mockReturnValue, and
globalConfigExists.mockReturnValue) from "re-estabelecidos após resetAllMocks"
to a clear English phrase like "re-established after resetAllMocks".
- Around line 191-196: Update the test "uses process.cwd() as default project
root" to not only assert result.found is false but also verify that
existsSyncSpy was invoked with a path derived from process.cwd(); call
detectProjectConfig() and assert that existsSyncSpy received a path containing
process.cwd() (use process.cwd(), detectProjectConfig, and existsSyncSpy as the
referenced symbols) so the test confirms the default root wiring.
- Around line 473-488: Replace the Portuguese test comment lines that describe
the serialization bug with an English comment (e.g., note that
createGlobalStructure returns error objects { dir, error } and that
config-migrator.js currently does .join(', ') on those objects producing
“[object Object]”), and then file a tracker issue for config-migrator.js
describing the bug and suggested fixes (map error objects to strings or
JSON.stringify before joining); reference createGlobalStructure and
executeMigration in the issue to help locate the problematic behavior.
- Documentar no teste que analyzeMigration propaga exceções porque é chamado antes do try/catch (comportamento correto do source) - Adicionar projectMcpPath no mock de existsSync para consistência no teste de backup com status DIRECTORY
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/core/mcp/config-migrator.test.js (1)
560-567:⚠️ Potential issue | 🟠 MajorAvoid locking unhandled throw behavior into the API contract
Line 560 and Line 566 currently assert a raw synchronous throw from
executeMigration. This bakes in brittle pre-try/catchbehavior and makes future hardening (structured failure result) a breaking test change. Prefer asserting a stable failure contract instead of the unhandled throw.Suggested direction
- test('propaga exceções de analyzeMigration (chamado antes do try/catch)', () => { - checkLinkStatus.mockImplementation(() => { throw new Error('unexpected'); }); - expect(() => executeMigration('/project')).toThrow('unexpected'); - }); + test('returns failure result when analysis fails unexpectedly', () => { + checkLinkStatus.mockImplementation(() => { throw new Error('unexpected'); }); + const result = executeMigration('/project'); + expect(result.success).toBe(false); + expect(result.errors).toContainEqual(expect.stringContaining('unexpected')); + });As per coding guidelines, "Verify error handling is comprehensive."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/mcp/config-migrator.test.js` around lines 560 - 567, The test currently asserts that executeMigration('/project') throws synchronously when checkLinkStatus (used by analyzeMigration) throws, which locks brittle pre-try/catch behavior into the API; change the test to assert the stable failure contract instead—mock checkLinkStatus (or analyzeMigration) to throw, call executeMigration('/project'), and expect a structured failure result (e.g., an object like { success: false, errors: [...] } or whatever the module's error-result shape is) rather than expecting a raw throw; reference executeMigration, analyzeMigration, and checkLinkStatus to locate the behavior to mock and the return value to assert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/core/mcp/config-migrator.test.js`:
- Around line 560-567: The test currently asserts that
executeMigration('/project') throws synchronously when checkLinkStatus (used by
analyzeMigration) throws, which locks brittle pre-try/catch behavior into the
API; change the test to assert the stable failure contract instead—mock
checkLinkStatus (or analyzeMigration) to throw, call
executeMigration('/project'), and expect a structured failure result (e.g., an
object like { success: false, errors: [...] } or whatever the module's
error-result shape is) rather than expecting a raw throw; reference
executeMigration, analyzeMigration, and checkLinkStatus to locate the behavior
to mock and the return value to assert.
Resumo
.aios-core/core/mcp/config-migrator.jsglobal-config-manageresymlink-managerdetectProjectConfig,analyzeMigration,mergeServers,executeMigration,restoreFromBackupÁreas testadas
mcpServers, fallthrough em JSON inválido, contagem de servidoresPlano de teste
Summary by CodeRabbit