feat: Enhance unit test coverage and improve file system utility tests #220
Conversation
📝 WalkthroughWalkthroughThis PR significantly expands test coverage across the codebase by introducing comprehensive test suites for OpenAPI adapters, CLI utilities, cryptographic functions, and storage adapters. Jest coverage thresholds are adjusted across multiple libraries to reflect incremental testing progress, and documentation metadata is updated with icon changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 9
🤖 Fix all issues with AI agents
In `@libs/adapters/src/openapi/__tests__/openapi-branch-coverage.spec.ts`:
- Around line 51-61: The helpers use `any` and a non‑null assertion; replace
them with explicit types and a small type guard: change getToolMeta to accept
something like Record<string, unknown> and return Record<string, unknown> using
FrontMcpToolTokens.metadata lookup safely, and change getFirstTool to accept
result: { tools?: unknown[] } and add a local type guard (e.g. isToolArray(obj):
obj is unknown[] validating Array.isArray(obj) && obj.length>0) so you can
assert tools exist via the guard (no `!`) and return the first element with
correct typed return (unknown or a proper Tool type if available). Ensure you
remove eslint any disables and the non‑null assertion.
- Around line 88-157: The tests currently only check spy existence rather than
verifying the console methods were invoked; update the first test (using
OpenapiAdapter and debugSpy) to assert the debug spy was called (e.g., replace
expect(debugSpy).toBeDefined() with expect(debugSpy).toHaveBeenCalled()) and
update the second test to assert each spy was invoked as intended
(expect(debugSpy).toHaveBeenCalled(), expect(infoSpy).toHaveBeenCalled(),
expect(warnSpy).toHaveBeenCalled(), expect(errorSpy).toHaveBeenCalled()) before
calling mockRestore; keep references to OpenapiAdapter, fetch, and the spy
variables (debugSpy, infoSpy, warnSpy, errorSpy) so the assertions target the
correct spies.
- Around line 406-445: The test is missing an assertion that no debug log was
emitted when schema transforms return the same schema; update the test to assert
the mockLogger.debug function was not called after calling adapter.fetch() (use
the existing mockLogger variable and the adapter.fetch() result) so the behavior
is locked in for OpenapiAdapter schemaTransforms (input.global and
output.global) no-op cases.
In `@libs/sdk/src/direct/__tests__/direct-server.spec.ts`:
- Around line 29-37: The test for DirectMcpServerImpl is missing an await on the
async assertion; update the test that constructs server (using createMockScope
and DirectMcpServerImpl) to await the Jest resolves matcher on server.ready
(e.g., await expect(server.ready).resolves.toBeUndefined()) so Jest properly
waits for the promise to settle.
In `@libs/utils/src/crypto/__tests__/crypto.spec.ts`:
- Around line 692-696: The test for EncryptedBlobError needs to assert the
concrete error type in addition to Error: in the test case named 'should export
EncryptedBlobError' (the one that constructs new EncryptedBlobError('test')),
add an assertion that the created error is instanceof EncryptedBlobError (e.g.,
expect(error).toBeInstanceOf(EncryptedBlobError)) so the error hierarchy is
explicitly verified.
- Around line 594-599: The test "should export PkceError class" currently
asserts the error is an instance of Error but misses asserting the specific
class; after creating the error with new PkceError('test error') add an
assertion that the instance is instanceof PkceError (e.g.,
expect(error).toBeInstanceOf(PkceError)) to validate the error hierarchy and
ensure PkceError is the actual exported type.
In `@libs/utils/src/fs/fs.spec.ts`:
- Around line 274-281: The test "should throw for existing directory without
recursive" is flaky because mkdir(tempDir) may succeed on some OSes; update the
test for stability by either removing it or making it conditional: locate the
spec around the it(...) block referencing mkdir and tempDir and either delete
the test or change it to check behavior based on feature detection (call
mkdir(tempDir) and assert throws only if a prior capability check indicates the
platform throws, otherwise assert it resolves), or skip the test via test.skip
when platform detection (e.g., process.platform) shows non-throwing behavior.
In `@libs/utils/src/storage/__tests__/factory.test.ts`:
- Around line 280-291: Rename the test to accurately reflect behavior: it
currently asserts that createStorage({ type: 'auto' }) in production logs a
warning and returns a usable storage (memory fallback) rather than throwing an
error; update the test description string (the it(...) title) to something like
"should fall back to memory with a warning in production by default" and keep
references to createStorage and consoleWarnSpy unchanged so the assertion and
cleanup (storage.disconnect()) remain valid.
- Around line 246-255: The test name/comment mismatch: the spec mentions "auto"
but the code passes 'invalid' and duplicates another test; either remove this
duplicate test or change it to actually exercise auto-resolution. Fix by
updating the test to call createStorage({ type: 'auto', fallback: 'error' }) (or
the equivalent code path that triggers auto resolution) and assert the expected
outcome for auto (e.g., resolves to a specific adapter or throws a clear
"Unknown storage type" if auto cannot be resolved), and update the it(...)
description and comment to match; if no auto scenario is needed, simply delete
this redundant test to avoid duplication with the test covering invalid types.
🧹 Nitpick comments (10)
libs/adapters/src/openapi/__tests__/openapi-spec-utils.spec.ts (1)
254-278: Consider adding a test for appending to existing security array.The current test at lines 254-278 verifies that duplicate security entries are not added. However, there's no test verifying behavior when an operation has existing security with a different scheme name. Based on the implementation (which pushes to existing
operation.securityarray), it would be valuable to verify that new security requirements are appended to existing ones rather than replacing them.📝 Suggested additional test case
it('should append security requirement to operation with different existing security', () => { const spec = createMinimalSpec({ '/users': { get: { operationId: 'getUsers', security: [{ ApiKeyAuth: [] }], responses: { '200': { description: 'Success' } }, }, }, }); const result = forceJwtSecurity(spec); const getUsers = (result.paths?.['/users'] as OpenAPIV3.PathItemObject)?.get as OpenAPIV3.OperationObject; expect(getUsers.security).toHaveLength(2); expect(getUsers.security).toContainEqual({ ApiKeyAuth: [] }); expect(getUsers.security).toContainEqual({ BearerAuth: [] }); });libs/ui/jest.config.ts (1)
18-25: Coverage thresholds set to 0% without explanation.Setting all coverage thresholds to 0 effectively disables coverage enforcement for
libs/ui. Based on learnings, this library requires 95%+ test coverage across all metrics. Unlike other Jest configs in this PR (e.g.,libs/auth,libs/sdk,libs/testing), this file lacks explanatory comments or a TODO for incremental improvement.Consider either:
- Adding a comment explaining the rationale and a TODO to track progress
- Setting non-zero thresholds that reflect current coverage as a baseline
Suggested improvement
+ // UI package has coverage gaps - using lower threshold for incremental improvement + // TODO: Increase thresholds as more tests are added coverageThreshold: { global: { - statements: 0, - branches: 0, - functions: 0, - lines: 0, + statements: 10, // Set to current coverage baseline + branches: 10, + functions: 10, + lines: 10, }, },libs/uipack/jest.config.ts (1)
18-25: Coverage thresholds set to 0% without explanation.Similar to
libs/ui, setting all thresholds to 0 disables coverage enforcement for@frontmcp/uipack. Based on learnings, this library requires 95%+ code coverage. Adding explanatory comments and a TODO (as done inlibs/auth,libs/sdk, andlibs/testing) would provide accountability for improvement.Suggested improvement
+ // UIpack has coverage gaps - using lower threshold for incremental improvement + // TODO: Increase thresholds as more tests are added coverageThreshold: { global: { - statements: 0, - branches: 0, - functions: 0, - lines: 0, + statements: 10, // Set to current coverage baseline + branches: 10, + functions: 10, + lines: 10, }, },libs/utils/src/storage/__tests__/factory.test.ts (3)
184-188: Consider addinginstanceofcheck for error verification.Per coding guidelines, tests for error classes should include
instanceofchecks to verify proper error hierarchy. This helps ensure the thrown error is of the expected type.Suggested improvement
it('should throw for unknown storage type when fallback is error', async () => { - await expect(createStorage({ type: 'unknown-type' as never, fallback: 'error' })).rejects.toThrow( - 'Unknown storage type', - ); + await expect(createStorage({ type: 'unknown-type' as never, fallback: 'error' })).rejects.toThrow( + 'Unknown storage type', + ); + + try { + await createStorage({ type: 'unknown-type' as never, fallback: 'error' }); + } catch (error) { + expect(error).toBeInstanceOf(Error); + } });Alternatively, use a more concise pattern if the codebase defines a custom error class:
const error = await createStorage({ type: 'unknown-type' as never, fallback: 'error' }).catch(e => e); expect(error).toBeInstanceOf(Error); expect(error.message).toContain('Unknown storage type');Based on learnings, tests should include
instanceofchecks for error classes to verify proper error hierarchy.
303-310: Duplicate test case.This test ("should handle explicit error fallback option") tests the same scenario as the test at lines 237-244 ("should throw error for unknown type with error fallback"). Consider consolidating these tests to reduce redundancy and improve maintainability.
293-301: Duplicate test case.This test ("should handle explicit memory fallback option") duplicates the test at lines 225-235 ("should not fallback to memory when already using memory type"). Both test
type: 'memory'withfallback: 'memory'and verify that ping returns true.libs/utils/src/fs/fs.spec.ts (2)
227-235: Weak assertion for file mode verification.The assertion
expect(stats.mode & 0o600).toBeTruthy()will pass for any file with owner read or write permissions, not specifically mode0o600. This doesn't verify the mode option was applied correctly.Consider a more precise check (accounting for umask effects):
♻️ Suggested improvement
await writeFile(filePath, 'secret content', { mode: 0o600 }); const stats = await fs.promises.stat(filePath); - // Check write permission for owner (mode & 0o200) - expect(stats.mode & 0o600).toBeTruthy(); + // Check that only owner read/write bits are set (no group/other permissions) + const permBits = stats.mode & 0o777; + expect(permBits & 0o077).toBe(0); // No group/other permissions + expect(permBits & 0o600).toBe(0o600); // Owner read/write set
494-500: Consider usingfs.constants.R_OKinstead of magic number.Using the named constant improves readability and makes the intent clearer.
♻️ Suggested improvement
it('should check specific access mode', async () => { const filePath = path.join(tempDir, 'mode-check.txt'); await fs.promises.writeFile(filePath, 'content'); - // Check for read access (fs.constants.R_OK = 4) - await expect(access(filePath, 4)).resolves.toBeUndefined(); + // Check for read access + await expect(access(filePath, fs.constants.R_OK)).resolves.toBeUndefined(); });libs/sdk/src/direct/__tests__/direct-server.spec.ts (1)
17-23: Consider creating a typed mock interface for the Scope.The repeated use of
as anyfor the mock scope (15+ occurrences) works but reduces type safety. Creating a typed mock interface would catch breaking changes if theScopetype evolves.♻️ Suggested improvement
+// Define a typed mock interface matching expected Scope shape +interface MockScope { + runFlowForOutput: jest.Mock; + transportService?: { + destroy: jest.Mock; + }; +} + - const createMockScope = () => ({ + const createMockScope = (): MockScope => ({ runFlowForOutput: jest.fn(), transportService: { destroy: jest.fn().mockResolvedValue(undefined), }, });Then cast with
as unknown as Scopeinstead ofas anyfor slightly better type checking:const server = new DirectMcpServerImpl(mockScope as unknown as Scope);Also applies to: 32-32, 45-45, 61-61, 86-86, 90-90, 106-106, 109-109, 126-126, 130-130, 144-144, 149-149, 164-164, 168-168, 183-183, 187-187, 202-202, 206-206, 224-224, 228-228, 235-235, 239-239, 247-247, 250-250, 256-256, 260-260, 268-268, 269-269, 277-277, 281-281, 287-287, 291-291, 299-299, 300-300, 309-309, 310-310, 321-321, 324-324, 340-340, 343-343, 366-366, 369-369, 389-389, 392-392
libs/utils/src/crypto/__tests__/crypto.spec.ts (1)
926-1084: Consider exporting RSA-PSS constants from the utils node module to avoid test importingnode:cryptodirectly.
ThersaSignfunction already accepts padding/salt options via itsoptionsparameter, providing proper API exposure. However, the test could be improved by importing the constants fromnode.tsinstead of directly fromnode:crypto. Since the production code innode.tsuses these constants internally, exporting them would keep the test aligned with the guideline to use utils wrappers and eliminate the separatenode:cryptoimport.
Summary by CodeRabbit
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.