Add unit tests for merge-utils module#271
Closed
nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Closed
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. |
WalkthroughAdds a comprehensive unit test suite for the merge-utils module with 24 tests covering isPlainObject behavior, deepMerge functionality across various scenarios, and mergeAll layered merging capabilities. No source code modifications. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 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 |
There was a problem hiding this comment.
Pull request overview
Adds Jest unit tests for the .aios-core/core/config/merge-utils.js merge strategy (ADR-PRO-002) to improve confidence in core config merging behavior.
Changes:
- Introduce a new Jest test suite for
isPlainObject,deepMerge, andmergeAll - Cover key merge rules: last-wins scalars, deep object merge, array replace,
+append, andnulldeletion - Add basic immutability and edge-case handling assertions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+10
| /** | ||
| * Unit tests for merge-utils module | ||
| * | ||
| * Tests the deep merge strategy from ADR-PRO-002: | ||
| * scalars last-wins, objects deep merge, arrays replace, | ||
| * +append modifier, null deletes key. | ||
| */ | ||
|
|
||
| const { deepMerge, mergeAll, isPlainObject } = require('../../../.aios-core/core/config/merge-utils'); | ||
|
|
There was a problem hiding this comment.
This test suite appears to duplicate existing coverage in tests/config/merge-utils.test.js (which is matched by the root jest.config.js testMatch). As-is, both suites will run, increasing maintenance cost and risking diverging expectations. Consider moving/renaming the existing tests into this location (and deleting the old file) or otherwise clarifying why both suites are needed; also update the PR description/issue claim that merge-utils had no unit tests if the existing suite is still intended to run.
20 tests covering isPlainObject, deepMerge (scalars, nested objects, arrays, +append, null delete, immutability), and mergeAll (multi-layer, null/non-object skip) per ADR-PRO-002.
nikolasdehor
force-pushed
the
test/merge-utils-coverage
branch
from
83f6e46 to
495253b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/config/merge-utils.test.js`:
- Around line 65-73: Add assertions to the '+append' tests to ensure the
suffixed key is removed from the merged output: after calling deepMerge in both
tests (the ones using 'items+append'), verify that result does not have the
literal 'items+append' property (e.g., assert result['items+append'] is
undefined or that the key is not in Object.keys(result)); update the two tests
referencing deepMerge to include this check so any leakage of the raw suffixed
key is caught.
- Around line 98-125: The tests for mergeAll are missing the single-layer and
all-invalid-layers cases: add a test that calls mergeAll({ a: 1 }) and asserts
it returns { a: 1 } (unchanged) and another test that calls mergeAll(null,
undefined, 'string') and asserts it returns {} (empty object); place both tests
inside the existing describe('mergeAll') block next to the other cases so they
cover the zero/single/all-invalid layer scenarios for the mergeAll function.
- Around line 42-96: Add two unit tests to the deepMerge suite to reach the
promised 12: one that verifies deepMerge(null, { a: 1 }) returns { a: 1 }
(exercise the null target branch), and one that verifies '+append' against a
non-array target (e.g., deepMerge({ items: 5 }, { 'items+append': [1] }) results
in items being [1] or concatenated appropriately per impl) to cover the scalar
target +append branch; place them alongside the existing tests in the
describe('deepMerge') block and use the same expect style as other tests.
Comment on lines
+42
to
+96
| describe('deepMerge', () => { | ||
| test('scalars: source overrides target', () => { | ||
| const result = deepMerge({ a: 1 }, { a: 2 }); | ||
| expect(result.a).toBe(2); | ||
| }); | ||
|
|
||
| test('adds new keys from source', () => { | ||
| const result = deepMerge({ a: 1 }, { b: 2 }); | ||
| expect(result).toEqual({ a: 1, b: 2 }); | ||
| }); | ||
|
|
||
| test('deep merges nested objects', () => { | ||
| const target = { db: { host: 'localhost', port: 5432 } }; | ||
| const source = { db: { host: 'remote', timeout: 30 } }; | ||
| const result = deepMerge(target, source); | ||
| expect(result.db).toEqual({ host: 'remote', port: 5432, timeout: 30 }); | ||
| }); | ||
|
|
||
| test('arrays: source replaces target', () => { | ||
| const result = deepMerge({ tags: ['a', 'b'] }, { tags: ['c'] }); | ||
| expect(result.tags).toEqual(['c']); | ||
| }); | ||
|
|
||
| test('+append: concatenates arrays', () => { | ||
| const result = deepMerge({ items: [1, 2] }, { 'items+append': [3, 4] }); | ||
| expect(result.items).toEqual([1, 2, 3, 4]); | ||
| }); | ||
|
|
||
| test('+append: creates new array when target key missing', () => { | ||
| const result = deepMerge({}, { 'items+append': [1, 2] }); | ||
| expect(result.items).toEqual([1, 2]); | ||
| }); | ||
|
|
||
| test('null value deletes key', () => { | ||
| const result = deepMerge({ a: 1, b: 2 }, { a: null }); | ||
| expect(result).toEqual({ b: 2 }); | ||
| expect('a' in result).toBe(false); | ||
| }); | ||
|
|
||
| test('does not mutate inputs', () => { | ||
| const target = { a: { b: 1 } }; | ||
| const source = { a: { c: 2 } }; | ||
| const result = deepMerge(target, source); | ||
| expect(target.a).toEqual({ b: 1 }); | ||
| expect(result.a).toEqual({ b: 1, c: 2 }); | ||
| }); | ||
|
|
||
| test('returns source when target not plain object', () => { | ||
| expect(deepMerge('string', { a: 1 })).toEqual({ a: 1 }); | ||
| }); | ||
|
|
||
| test('returns target when source is undefined', () => { | ||
| expect(deepMerge({ a: 1 }, undefined)).toEqual({ a: 1 }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
deepMerge block has 10 tests, not the 12 stated in the PR description and issue #270.
Counting each test() call: scalars, new keys, deep merge, array replace, +append concat, +append missing key, null delete, immutability, non-object target, undefined source = 10. The PR table and issue both promise 12.
The two most useful additions to close the gap (and cover realistic branches):
nulltarget —deepMerge(null, { a: 1 })is a distinct code path from the'string'target already tested at line 89.+appendagainst a non-array target key — what happens when the existing value is a scalar (e.g.,deepMerge({ items: 5 }, { 'items+append': [1] }))? The current suite leaves this branch untested.
🧪 Suggested additional test cases
+ test('returns source when target is null', () => {
+ expect(deepMerge(null, { a: 1 })).toEqual({ a: 1 });
+ });
+
+ test('+append: replaces scalar target with appended array', () => {
+ // Validate the defined behavior when target key is not an array
+ const result = deepMerge({ items: 5 }, { 'items+append': [1, 2] });
+ expect(result.items).toEqual([1, 2]); // adjust expectation to match actual implementation
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/config/merge-utils.test.js` around lines 42 - 96, Add two unit
tests to the deepMerge suite to reach the promised 12: one that verifies
deepMerge(null, { a: 1 }) returns { a: 1 } (exercise the null target branch),
and one that verifies '+append' against a non-array target (e.g., deepMerge({
items: 5 }, { 'items+append': [1] }) results in items being [1] or concatenated
appropriately per impl) to cover the scalar target +append branch; place them
alongside the existing tests in the describe('deepMerge') block and use the same
expect style as other tests.
Comment on lines
+65
to
+73
| test('+append: concatenates arrays', () => { | ||
| const result = deepMerge({ items: [1, 2] }, { 'items+append': [3, 4] }); | ||
| expect(result.items).toEqual([1, 2, 3, 4]); | ||
| }); | ||
|
|
||
| test('+append: creates new array when target key missing', () => { | ||
| const result = deepMerge({}, { 'items+append': [1, 2] }); | ||
| expect(result.items).toEqual([1, 2]); | ||
| }); |
There was a problem hiding this comment.
Missing assertion: the +append suffixed key should not appear in the result.
Both +append tests validate the merged array value under result.items, but neither asserts that the literal 'items+append' key is absent from the result object. If the implementation accidentally leaks the raw key, these tests would not catch it.
🧪 Suggested additional assertions
test('+append: concatenates arrays', () => {
const result = deepMerge({ items: [1, 2] }, { 'items+append': [3, 4] });
expect(result.items).toEqual([1, 2, 3, 4]);
+ expect('items+append' in result).toBe(false);
});
test('+append: creates new array when target key missing', () => {
const result = deepMerge({}, { 'items+append': [1, 2] });
expect(result.items).toEqual([1, 2]);
+ expect('items+append' in result).toBe(false);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/config/merge-utils.test.js` around lines 65 - 73, Add assertions
to the '+append' tests to ensure the suffixed key is removed from the merged
output: after calling deepMerge in both tests (the ones using 'items+append'),
verify that result does not have the literal 'items+append' property (e.g.,
assert result['items+append'] is undefined or that the key is not in
Object.keys(result)); update the two tests referencing deepMerge to include this
check so any leakage of the raw suffixed key is caught.
Comment on lines
+98
to
+125
| describe('mergeAll', () => { | ||
| test('merges multiple layers in order', () => { | ||
| const result = mergeAll( | ||
| { a: 1, b: 2 }, | ||
| { b: 3, c: 4 }, | ||
| { c: 5 }, | ||
| ); | ||
| expect(result).toEqual({ a: 1, b: 3, c: 5 }); | ||
| }); | ||
|
|
||
| test('skips null and non-object layers', () => { | ||
| const result = mergeAll({ a: 1 }, null, undefined, 'string', { b: 2 }); | ||
| expect(result).toEqual({ a: 1, b: 2 }); | ||
| }); | ||
|
|
||
| test('returns empty object when no layers', () => { | ||
| expect(mergeAll()).toEqual({}); | ||
| }); | ||
|
|
||
| test('deep merges across layers', () => { | ||
| const result = mergeAll( | ||
| { db: { host: 'localhost' } }, | ||
| { db: { port: 5432 } }, | ||
| { db: { host: 'remote' } }, | ||
| ); | ||
| expect(result.db).toEqual({ host: 'remote', port: 5432 }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
mergeAll block has 4 tests, not the 6 promised by the PR and issue #270 — and the explicitly scoped single-layer case is missing.
Issue #270 calls out "correct handling of zero/single layers" as a required scenario. Zero layers is covered at line 113; the single-layer case is absent. The overall count is 4 vs the claimed 6, leaving two gaps:
- Single layer —
mergeAll({ a: 1 })should return{ a: 1 }unchanged. - All-invalid layers —
mergeAll(null, undefined, 'string')should return{}, a distinct path from the mixed-valid test at line 108.
🧪 Suggested additional test cases
+ test('returns single layer unchanged', () => {
+ expect(mergeAll({ a: 1, b: 2 })).toEqual({ a: 1, b: 2 });
+ });
+
+ test('returns empty object when all layers are invalid', () => {
+ expect(mergeAll(null, undefined, 'string')).toEqual({});
+ });📝 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
| describe('mergeAll', () => { | |
| test('merges multiple layers in order', () => { | |
| const result = mergeAll( | |
| { a: 1, b: 2 }, | |
| { b: 3, c: 4 }, | |
| { c: 5 }, | |
| ); | |
| expect(result).toEqual({ a: 1, b: 3, c: 5 }); | |
| }); | |
| test('skips null and non-object layers', () => { | |
| const result = mergeAll({ a: 1 }, null, undefined, 'string', { b: 2 }); | |
| expect(result).toEqual({ a: 1, b: 2 }); | |
| }); | |
| test('returns empty object when no layers', () => { | |
| expect(mergeAll()).toEqual({}); | |
| }); | |
| test('deep merges across layers', () => { | |
| const result = mergeAll( | |
| { db: { host: 'localhost' } }, | |
| { db: { port: 5432 } }, | |
| { db: { host: 'remote' } }, | |
| ); | |
| expect(result.db).toEqual({ host: 'remote', port: 5432 }); | |
| }); | |
| }); | |
| describe('mergeAll', () => { | |
| test('merges multiple layers in order', () => { | |
| const result = mergeAll( | |
| { a: 1, b: 2 }, | |
| { b: 3, c: 4 }, | |
| { c: 5 }, | |
| ); | |
| expect(result).toEqual({ a: 1, b: 3, c: 5 }); | |
| }); | |
| test('skips null and non-object layers', () => { | |
| const result = mergeAll({ a: 1 }, null, undefined, 'string', { b: 2 }); | |
| expect(result).toEqual({ a: 1, b: 2 }); | |
| }); | |
| test('returns empty object when no layers', () => { | |
| expect(mergeAll()).toEqual({}); | |
| }); | |
| test('deep merges across layers', () => { | |
| const result = mergeAll( | |
| { db: { host: 'localhost' } }, | |
| { db: { port: 5432 } }, | |
| { db: { host: 'remote' } }, | |
| ); | |
| expect(result.db).toEqual({ host: 'remote', port: 5432 }); | |
| }); | |
| test('returns single layer unchanged', () => { | |
| expect(mergeAll({ a: 1, b: 2 })).toEqual({ a: 1, b: 2 }); | |
| }); | |
| test('returns empty object when all layers are invalid', () => { | |
| expect(mergeAll(null, undefined, 'string')).toEqual({}); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/config/merge-utils.test.js` around lines 98 - 125, The tests for
mergeAll are missing the single-layer and all-invalid-layers cases: add a test
that calls mergeAll({ a: 1 }) and asserts it returns { a: 1 } (unchanged) and
another test that calls mergeAll(null, undefined, 'string') and asserts it
returns {} (empty object); place both tests inside the existing
describe('mergeAll') block next to the other cases so they cover the
zero/single/all-invalid layer scenarios for the mergeAll function.
Contributor
Author
|
Consolidated into #424 |
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.
Closes #270
Summary
merge-utilsconfig moduleTest Coverage
Test plan
Summary by CodeRabbit