test: add comprehensive test coverage for config-cache module#217
test: add comprehensive test coverage for config-cache module#217nikolasdehor wants to merge 2 commits intoSynkraAI:mainfrom
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. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request adds two new comprehensive test suites: one for the ConfigCache module covering cache lifecycle, TTL expiration, metrics, and edge cases (592 lines); and another for the estimateTokens heuristic covering basic behavior, boundary cases, content types, and accuracy verification (215 lines). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 new Jest unit test suites to increase coverage for internal caching/utilities used across .aios-core, primarily focusing on the configuration cache behavior (TTL, lifecycle, stats) and also adding coverage for SYNAPSE token estimation.
Changes:
- Add a comprehensive
ConfigCachetest suite covering TTL expiration, invalidation, stats, serialization, and the global singleton behavior. - Add a dedicated test suite for
estimateTokens(SYNAPSE token heuristic) across boundary and content-type cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/config-cache.test.js | New unit tests for .aios-core/core/config/config-cache.js, including TTL behavior, metrics, serialization, and singleton interval cleanup behavior. |
| tests/synapse/synapse-tokens.test.js | New unit tests for .aios-core/core/synapse/utils/tokens.js estimateTokens() heuristic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| globalConfigCache = mod.globalConfigCache; | ||
| }); | ||
|
|
||
| afterAll(() => { |
There was a problem hiding this comment.
jest.useFakeTimers() is enabled for the whole file and the module under test installs a module-scope setInterval. The suite restores real timers in afterAll, but never clears pending timers/intervals, which can leak handles and cause flaky behavior in Jest. Clear timers (e.g., jest.clearAllTimers() / jest.runOnlyPendingTimers()) before switching back to real timers, and consider jest.resetModules() if you need to fully tear down the singleton interval between runs.
| afterAll(() => { | |
| afterAll(() => { | |
| jest.runOnlyPendingTimers(); | |
| jest.clearAllTimers(); |
| globalConfigCache.setTTL(1000); | ||
| globalConfigCache.set('temp', 'data'); | ||
| jest.advanceTimersByTime(1001); | ||
| jest.advanceTimersByTime(60000); | ||
| globalConfigCache.setTTL(originalTtl); | ||
| expect(globalConfigCache.size).toBe(0); | ||
| consoleSpy.mockRestore(); |
There was a problem hiding this comment.
This test mutates the global singleton’s TTL and installs a console spy, but restoration happens inline. If an assertion throws before the cleanup lines, TTL and/or the console mock can leak into subsequent tests. Use try/finally (or an afterEach) to guarantee restoring setTTL(originalTtl) and consoleSpy.mockRestore().
| globalConfigCache.setTTL(1000); | |
| globalConfigCache.set('temp', 'data'); | |
| jest.advanceTimersByTime(1001); | |
| jest.advanceTimersByTime(60000); | |
| globalConfigCache.setTTL(originalTtl); | |
| expect(globalConfigCache.size).toBe(0); | |
| consoleSpy.mockRestore(); | |
| try { | |
| globalConfigCache.setTTL(1000); | |
| globalConfigCache.set('temp', 'data'); | |
| jest.advanceTimersByTime(1001); | |
| jest.advanceTimersByTime(60000); | |
| expect(globalConfigCache.size).toBe(0); | |
| } finally { | |
| globalConfigCache.setTTL(originalTtl); | |
| consoleSpy.mockRestore(); | |
| } |
| * - Edge cases: empty keys, null/undefined values, numeric keys | ||
| * - Error handling and boundary conditions |
There was a problem hiding this comment.
The header comment claims coverage for “numeric keys” and “error handling”, but the test suite doesn’t currently include a numeric-key case (e.g. cache.set(123, 'v')) and the module doesn’t appear to throw errors. Either add the missing test(s) or adjust the comment to avoid overstating coverage.
| * - Edge cases: empty keys, null/undefined values, numeric keys | |
| * - Error handling and boundary conditions | |
| * - Edge cases: empty keys, null/undefined values | |
| * - Boundary conditions |
tests/synapse/synapse-tokens.test.js
Outdated
| /** | ||
| * SYNAPSE Token Utilities Tests | ||
| * | ||
| * Comprehensive tests for the estimateTokens heuristic used by the | ||
| * formatter and memory bridge. | ||
| * | ||
| * @module tests/synapse/synapse-tokens | ||
| * @story SYN-10 - Pro Memory Bridge (extracted from formatter.js) | ||
| */ |
There was a problem hiding this comment.
PR description/title focus on adding comprehensive tests for config-cache, but this PR also introduces a new synapse-tokens test suite. Either update the PR description (including the stated total test count) to reflect the additional coverage, or split the SYNAPSE token tests into a separate PR to keep scope aligned with the stated goal.
![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/config-cache.test.js (3)
24-29: Consider addingjest.resetModules()beforerequirefor guaranteed module isolation.The
requireinsidebeforeAllis intentional — it ensures the module'ssetIntervalis intercepted by fake timers. However, if Jest's module registry already holds a cached version ofconfig-cache(e.g., from a global setup file or a shared worker), the cached module'ssetIntervalwould use real timers, making the singleton cleanup test unreliable.jest.resetModules()forces a fresh module load unconditionally.♻️ Suggested defensive pattern
beforeAll(() => { jest.useFakeTimers(); + jest.resetModules(); const mod = require('../.aios-core/core/config/config-cache'); ConfigCache = mod.ConfigCache; globalConfigCache = mod.globalConfigCache; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/config-cache.test.js` around lines 24 - 29, Add a Jest module reset before requiring the module to ensure a fresh load: inside the beforeAll block call jest.resetModules() prior to requiring '../.aios-core/core/config/config-cache' so that ConfigCache and globalConfigCache are loaded with fake timers applied; update the beforeAll that currently sets jest.useFakeTimers() and requires the module (affecting ConfigCache and globalConfigCache) to call jest.resetModules() first to guarantee module isolation.
432-445: Tighten range assertions to exact values — fake timers make time fully deterministic.
toBeGreaterThanOrEqual(5000)(line 436) andtoBeLessThanOrEqual(290000)(line 444) are looser than necessary with fake timers.jest.advanceTimersByTimeguarantees precise control, soageis exactly5000andexpiresis exactly290000(= 300000 − 10000). UsingtoBe()would catch off-by-one errors and incorrect formulas that happen to produce values within the range.♻️ Proposed tightening
test('should calculate age correctly', () => { cache.set('key', 'value'); jest.advanceTimersByTime(5000); const entries = cache.entries(); - expect(entries[0].age).toBeGreaterThanOrEqual(5000); + expect(entries[0].age).toBe(5000); expect(entries[0].ageSeconds).toBe('5.0'); }); test('should calculate expiration time correctly', () => { cache.set('key', 'value'); jest.advanceTimersByTime(10000); const entries = cache.entries(); - expect(entries[0].expires).toBeLessThanOrEqual(290000); + expect(entries[0].expires).toBe(290000); // ttl(300000) - elapsed(10000) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/config-cache.test.js` around lines 432 - 445, Update the two tests that use fake timers to assert exact values instead of ranges: in the "should calculate age correctly" test (which calls cache.set('key','value') and jest.advanceTimersByTime(5000)), change the assertion on entries()[0].age from toBeGreaterThanOrEqual(5000) to toBe(5000) and entries()[0].ageSeconds to the expected exact string; and in the "should calculate expiration time correctly" test (which advances timers by 10000), change the assertion on entries()[0].expires from toBeLessThanOrEqual(290000) to toBe(290000) so the tests validate the deterministic timing calculations exactly.
1-20: Optional: Add'use strict'for consistency with the sibling test file.
tests/synapse/synapse-tokens.test.jsdeclares'use strict'at the top;tests/config-cache.test.jsdoes not. This is a minor inconsistency in the test suite style.♻️ Suggested addition
+ +'use strict'; + let ConfigCache; let globalConfigCache;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/config-cache.test.js` around lines 1 - 20, Add the missing strict mode declaration to the top of tests/config-cache.test.js by inserting 'use strict' as the very first statement (before the file comment header) so the file matches the sibling test tests/synapse/synapse-tokens.test.js; ensure it's placed outside any block or comment so strict mode applies to the whole module.tests/synapse/synapse-tokens.test.js (1)
130-133: Optional: Clarify thatemoji.lengthcounts UTF-16 code units, not code points.
'\uD83D\uDE80'is a surrogate pair, soemoji.length === 2(not 1). The test correctly usesMath.ceil(emoji.length / 4)dynamically, keeping it consistent with however the implementation measures string length. A brief comment would help future readers understand why emoji behave this way.💡 Suggested comment
test('handles unicode / emoji characters', () => { const emoji = 'Hello \uD83D\uDE80'; - expect(estimateTokens(emoji)).toBe(Math.ceil(emoji.length / 4)); + // '\uD83D\uDE80' is a surrogate pair → .length === 2 code units, not 1 code point + expect(estimateTokens(emoji)).toBe(Math.ceil(emoji.length / 4)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/synapse/synapse-tokens.test.js` around lines 130 - 133, Add a brief inline comment to the test case for "handles unicode / emoji characters" explaining that JavaScript's string length counts UTF-16 code units (so the surrogate pair '\uD83D\uDE80' yields length 2, not 1) and that the assertion uses emoji.length to match how estimateTokens measures length; update the test near estimateTokens usage to clarify this behavior for future readers.
🤖 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/config-cache.test.js`:
- Around line 510-513: The test currently treats cache.get('nullable') === null
as proof a null value was stored, which is ambiguous; update the test to
explicitly assert cache.has('nullable') === true before asserting
cache.get('nullable') === null so the test distinguishes a stored null from a
missing key—locate the test using cache.set('nullable', null) /
cache.get('nullable') and insert an expectation calling cache.has('nullable') to
return true.
- Around line 580-590: The test relies on an internal cleanup interval
(60_000ms) causing a fragile timing dependency; update the test to avoid that by
either exporting the cleanup interval constant from the module and using it in
the test, or by calling the public cleanup API directly: after setting
globalConfigCache.setTTL(1000) and adding the entry with
globalConfigCache.set('temp','data'), invoke globalConfigCache.clearExpired() to
force cleanup and assert globalConfigCache.size is 0, and separately assert that
a cleanup timer was registered using jest.getTimerCount() or a spy on
setInterval to verify interval registration (refer to globalConfigCache.setTTL,
globalConfigCache.clearExpired, and the module's exported cleanup interval
constant or setInterval usage).
---
Nitpick comments:
In `@tests/config-cache.test.js`:
- Around line 24-29: Add a Jest module reset before requiring the module to
ensure a fresh load: inside the beforeAll block call jest.resetModules() prior
to requiring '../.aios-core/core/config/config-cache' so that ConfigCache and
globalConfigCache are loaded with fake timers applied; update the beforeAll that
currently sets jest.useFakeTimers() and requires the module (affecting
ConfigCache and globalConfigCache) to call jest.resetModules() first to
guarantee module isolation.
- Around line 432-445: Update the two tests that use fake timers to assert exact
values instead of ranges: in the "should calculate age correctly" test (which
calls cache.set('key','value') and jest.advanceTimersByTime(5000)), change the
assertion on entries()[0].age from toBeGreaterThanOrEqual(5000) to toBe(5000)
and entries()[0].ageSeconds to the expected exact string; and in the "should
calculate expiration time correctly" test (which advances timers by 10000),
change the assertion on entries()[0].expires from toBeLessThanOrEqual(290000) to
toBe(290000) so the tests validate the deterministic timing calculations
exactly.
- Around line 1-20: Add the missing strict mode declaration to the top of
tests/config-cache.test.js by inserting 'use strict' as the very first statement
(before the file comment header) so the file matches the sibling test
tests/synapse/synapse-tokens.test.js; ensure it's placed outside any block or
comment so strict mode applies to the whole module.
In `@tests/synapse/synapse-tokens.test.js`:
- Around line 130-133: Add a brief inline comment to the test case for "handles
unicode / emoji characters" explaining that JavaScript's string length counts
UTF-16 code units (so the surrogate pair '\uD83D\uDE80' yields length 2, not 1)
and that the assertion uses emoji.length to match how estimateTokens measures
length; update the test near estimateTokens usage to clarify this behavior for
future readers.
| test('should handle null as a stored value', () => { | ||
| cache.set('nullable', null); | ||
| expect(cache.get('nullable')).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
null value test is a false positive — add has() assertion to distinguish stored null from a cache miss.
cache.get returns null for both a stored null value and a missing key. If the implementation silently discards null values (e.g., due to a falsy guard like if (!value) return), get('nullable') still returns null and the test passes incorrectly. Asserting cache.has('nullable') === true first eliminates the ambiguity.
🛡️ Proposed fix
test('should handle null as a stored value', () => {
cache.set('nullable', null);
+ expect(cache.has('nullable')).toBe(true);
expect(cache.get('nullable')).toBeNull();
});📝 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.
| test('should handle null as a stored value', () => { | |
| cache.set('nullable', null); | |
| expect(cache.get('nullable')).toBeNull(); | |
| }); | |
| test('should handle null as a stored value', () => { | |
| cache.set('nullable', null); | |
| expect(cache.has('nullable')).toBe(true); | |
| expect(cache.get('nullable')).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/config-cache.test.js` around lines 510 - 513, The test currently treats
cache.get('nullable') === null as proof a null value was stored, which is
ambiguous; update the test to explicitly assert cache.has('nullable') === true
before asserting cache.get('nullable') === null so the test distinguishes a
stored null from a missing key—locate the test using cache.set('nullable', null)
/ cache.get('nullable') and insert an expectation calling cache.has('nullable')
to return true.
| test('should auto-cleanup expired entries via setInterval', () => { | ||
| const consoleSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); | ||
| const originalTtl = globalConfigCache.ttl; | ||
| globalConfigCache.setTTL(1000); | ||
| globalConfigCache.set('temp', 'data'); | ||
| jest.advanceTimersByTime(1001); | ||
| jest.advanceTimersByTime(60000); | ||
| globalConfigCache.setTTL(originalTtl); | ||
| expect(globalConfigCache.size).toBe(0); | ||
| consoleSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the config-cache module
find . -name "config-cache.js" -o -name "config-cache*" -type f | grep -v test | grep -v node_modules | head -20Repository: SynkraAI/aios-core
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Search for setInterval calls in the config-cache related files
rg -n 'setInterval' --type js -C 5Repository: SynkraAI/aios-core
Length of output: 2960
🏁 Script executed:
#!/bin/bash
# Read the config-cache.js module to find setInterval
cat -n ./.aios-core/core/config/config-cache.js | head -100Repository: SynkraAI/aios-core
Length of output: 2832
🏁 Script executed:
#!/bin/bash
# Search for setInterval specifically in the config-cache.js file
rg -n 'setInterval' ./.aios-core/core/config/config-cache.js -C 10Repository: SynkraAI/aios-core
Length of output: 587
Test has an undocumented implicit dependency on the cleanup interval duration.
The module sets the cleanup interval to 60 * 1000 milliseconds (line 228 of config-cache.js), and the test advances timers by exactly 60000ms (line 586) to trigger that interval. While the values currently match, the test relies on an implementation detail that is not a public API. If the interval is changed in the module, the test will fail silently or give misleading results without clearly signaling why.
The interval duration should either be exported as a named constant from the module, documented explicitly in the test, or the test should verify the cleanup logic directly via clearExpired() and independently confirm that setInterval is registered (using jest.getTimerCount() or a spy). This decouples the test from a fragile internal timing assumption.
💡 Example: Decouple cleanup logic from interval timing
- test('should auto-cleanup expired entries via setInterval', () => {
- const consoleSpy = jest.spyOn(console, 'log').mockImplementation(() => {});
- const originalTtl = globalConfigCache.ttl;
- globalConfigCache.setTTL(1000);
- globalConfigCache.set('temp', 'data');
- jest.advanceTimersByTime(1001);
- jest.advanceTimersByTime(60000);
- globalConfigCache.setTTL(originalTtl);
- expect(globalConfigCache.size).toBe(0);
- consoleSpy.mockRestore();
- });
+ test('clearExpired() removes expired entries from globalConfigCache', () => {
+ globalConfigCache.setTTL(1000);
+ globalConfigCache.set('temp', 'data');
+ jest.advanceTimersByTime(1001);
+ const cleared = globalConfigCache.clearExpired();
+ expect(cleared).toBe(1);
+ expect(globalConfigCache.size).toBe(0);
+ globalConfigCache.setTTL(5 * 60 * 1000);
+ });
+
+ test('setInterval for auto-cleanup is registered on module load', () => {
+ // Verifies that a periodic cleanup timer exists
+ expect(jest.getTimerCount()).toBeGreaterThan(0);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/config-cache.test.js` around lines 580 - 590, The test relies on an
internal cleanup interval (60_000ms) causing a fragile timing dependency; update
the test to avoid that by either exporting the cleanup interval constant from
the module and using it in the test, or by calling the public cleanup API
directly: after setting globalConfigCache.setTTL(1000) and adding the entry with
globalConfigCache.set('temp','data'), invoke globalConfigCache.clearExpired() to
force cleanup and assert globalConfigCache.size is 0, and separately assert that
a cleanup timer was registered using jest.getTimerCount() or a spy on
setInterval to verify interval registration (refer to globalConfigCache.setTTL,
globalConfigCache.clearExpired, and the module's exported cleanup interval
constant or setInterval usage).
Add unit tests covering: - Cache initialization and configuration - Get/set operations with TTL support - Cache expiration and invalidation - Hit/miss metrics tracking - Edge cases and error handling Closes SynkraAI#214
nikolasdehor
force-pushed
the
test/config-cache-coverage
branch
from
7e573b6 to
da2451e
Compare
31 tests covering ConfigCache class: constructor defaults, get/set with TTL expiration, has, invalidate, clear, clearExpired, statistics tracking, keys, entries with age/expiry info, setTTL, toJSON serialization, and global singleton instance.
|
Consolidated into #424 |
Summary
.aios-core/core/config/config-cache.js)Test plan
Closes #214
cc @Pedrovaleriolopez
Summary by CodeRabbit
Release Notes