test(cicero-core): 100% coverage for CompositeArchiveLoader#829
Conversation
Signed-off-by: Aadityavardhan Singh <singhrashmi018@gmail.com>
…le recursive testing Signed-off-by: Aadityavardhan Singh <singhrashmi018@gmail.com>
Signed-off-by: Aadityavardhan Singh <singhrashmi018@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit tests for CompositeArchiveLoader in cicero-core, increasing coverage from ~5% to 100%. The PR introduces a new test file that uses sinon stubs to mock sub-loaders and verify all code paths including error handling and empty states. Additionally, it updates the Mocha test configuration to recursively discover tests in subdirectories.
Changes:
- Added comprehensive unit tests for CompositeArchiveLoader covering constructor, adding/clearing loaders, URL acceptance, and loading functionality
- Updated package.json to add the
--recursiveflag to Mocha, enabling test discovery in subdirectories liketest/loaders/
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/cicero-core/test/loaders/compositearchiveloader.js | New test file with comprehensive unit tests for CompositeArchiveLoader, testing all public methods and error paths |
| packages/cicero-core/package.json | Added --recursive flag to Mocha configuration to discover and run tests in subdirectories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('#accepts', () => { | ||
| it('should return true if one of the loaders accepts the URL', () => { | ||
| // Mock: Loader 1 says NO, Loader 2 says YES | ||
| mockLoader1.accepts.returns(false); | ||
| mockLoader2.accepts.returns(true); | ||
| compositeLoader.addArchiveLoader(mockLoader1); | ||
| compositeLoader.addArchiveLoader(mockLoader2); | ||
|
|
||
| const result = compositeLoader.accepts('http://test.url'); | ||
| result.should.be.true; | ||
| }); | ||
|
|
||
| it('should return false if NONE of the loaders accept the URL', () => { | ||
| mockLoader1.accepts.returns(false); | ||
| mockLoader2.accepts.returns(false); | ||
| compositeLoader.addArchiveLoader(mockLoader1); | ||
| compositeLoader.addArchiveLoader(mockLoader2); | ||
|
|
||
| const result = compositeLoader.accepts('http://test.url'); | ||
| result.should.be.false; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test case to verify early exit behavior when the first loader accepts the URL. Add a test where mockLoader1.accepts returns true to ensure that mockLoader2.accepts is never called, demonstrating the early exit optimization in the accepts method.
| mockLoader1 = { | ||
| accepts: sinon.stub(), | ||
| load: sinon.stub() | ||
| }; | ||
| mockLoader2 = { | ||
| accepts: sinon.stub(), | ||
| load: sinon.stub() | ||
| }; |
There was a problem hiding this comment.
Consider using simple mock objects instead of sinon stubs to avoid introducing a new dependency. The codebase uses mock-require for mocking (see httparchiveloader.js), and for this test case, plain JavaScript objects with methods would suffice. For example: mockLoader1 = { accepts: () => false, load: () => {} }. If you still prefer sinon for its assertion capabilities, make sure to add it to devDependencies.
| describe('#load', () => { | ||
| it('should delegate load to the first accepting loader', async () => { | ||
| mockLoader1.accepts.returns(false); | ||
| mockLoader2.accepts.returns(true); | ||
| mockLoader2.load.resolves('ARCHIVE_DATA'); // Simulate successful load | ||
|
|
||
| compositeLoader.addArchiveLoader(mockLoader1); | ||
| compositeLoader.addArchiveLoader(mockLoader2); | ||
|
|
||
| const result = await compositeLoader.load('http://test.url', {}); | ||
| result.should.equal('ARCHIVE_DATA'); | ||
| sinon.assert.calledWith(mockLoader2.load, 'http://test.url', {}); | ||
| }); | ||
|
|
||
| it('should throw an error if NO loader accepts the URL', () => { | ||
| mockLoader1.accepts.returns(false); | ||
| mockLoader2.accepts.returns(false); | ||
|
|
||
| compositeLoader.addArchiveLoader(mockLoader1); | ||
| compositeLoader.addArchiveLoader(mockLoader2); | ||
|
|
||
| (() => { | ||
| compositeLoader.load('http://unknown.url', {}); | ||
| }).should.throw(/Failed to find a model file loader/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test case for the load method when the first loader accepts the URL. Add a test where mockLoader1.accepts returns true to ensure that mockLoader1.load is called and mockLoader2 is never checked, demonstrating the early exit optimization in the load method.
| describe('#accepts', () => { | ||
| it('should return true if one of the loaders accepts the URL', () => { | ||
| // Mock: Loader 1 says NO, Loader 2 says YES | ||
| mockLoader1.accepts.returns(false); | ||
| mockLoader2.accepts.returns(true); | ||
| compositeLoader.addArchiveLoader(mockLoader1); | ||
| compositeLoader.addArchiveLoader(mockLoader2); | ||
|
|
||
| const result = compositeLoader.accepts('http://test.url'); | ||
| result.should.be.true; | ||
| }); | ||
|
|
||
| it('should return false if NONE of the loaders accept the URL', () => { | ||
| mockLoader1.accepts.returns(false); | ||
| mockLoader2.accepts.returns(false); | ||
| compositeLoader.addArchiveLoader(mockLoader1); | ||
| compositeLoader.addArchiveLoader(mockLoader2); | ||
|
|
||
| const result = compositeLoader.accepts('http://test.url'); | ||
| result.should.be.false; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test case for the accepts method when there are no loaders registered. This should verify that accepts returns false when the loaders array is empty, ensuring the edge case of an uninitialized CompositeArchiveLoader is handled correctly.
| (() => { | ||
| compositeLoader.load('http://unknown.url', {}); | ||
| }).should.throw(/Failed to find a model file loader/); | ||
| }); |
There was a problem hiding this comment.
Missing test case for the load method when there are no loaders registered. This should verify that load throws an error when the loaders array is empty, ensuring the edge case of an uninitialized CompositeArchiveLoader is handled correctly.
| }); | |
| }); | |
| it('should throw an error if there are no loaders registered', () => { | |
| (() => { | |
| compositeLoader.load('http://empty.url', {}); | |
| }).should.throw(/Failed to find a model file loader/); | |
| }); |
|
|
||
| beforeEach(() => { | ||
| compositeLoader = new CompositeArchiveLoader(); | ||
| // Create dummy loaders (The "Employees") |
There was a problem hiding this comment.
The comment "The Employees" is unclear and doesn't add value to the test code. Consider removing it or replacing it with a more descriptive comment about what these mock loaders represent in the context of the CompositeArchiveLoader pattern.
| // Create dummy loaders (The "Employees") | |
| // Create mock archive loaders used by the CompositeArchiveLoader in these tests |
Description
This PR addresses the low test coverage for
CompositeArchiveLoaderin cicero-core.Previously,
compositearchiveloader.jsjust had ~5% coverage. The functions inside the file were not being touched by mocha. This PR adds a comprehensive unit test suite using 'sinon' that mocks sub-loaders to verify all branches, including error handling and empty states.Changes:
packages/cicero-core/test/loaders/compositearchiveloader.js(New unit test).packages/cicero-core/package.jsonto include the--recursiveflag for the Mocha test runner.test/loaders/), effectively skipping them.Results
Coverage for
compositearchiveloader.jsis now 100%.Before:
After
