test: add unit tests for env-interpolator module#273
test: add unit tests for env-interpolator module#273nikolasdehor wants to merge 1 commit 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. ✨ 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 coverage for the .aios-core/core/config/env-interpolator module (Issue #272), validating env var interpolation behavior and lint detection for committed ${...} patterns.
Changes:
- Introduces 39 unit tests covering
ENV_VAR_PATTERN,interpolateString,interpolateEnvVars, andlintEnvPatterns. - Verifies
${VAR}resolution,${VAR:-default}fallback behavior, missing-var warning collection, and recursive traversal across objects/arrays. - Adds linting assertions for detecting env patterns and generating path-qualified findings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const originalEnv = process.env; | ||
|
|
||
| beforeEach(() => { | ||
| process.env = { ...originalEnv }; | ||
| }); | ||
|
|
||
| afterAll(() => { |
There was a problem hiding this comment.
In this suite, originalEnv is captured as a reference (const originalEnv = process.env) and restored in afterAll(). This is inconsistent with other test files (which snapshot via { ...process.env } and restore in afterEach()), and makes it easier for env mutations to leak if additional tests are added or if the suite is reordered. Consider snapshotting with { ...process.env } and restoring in afterEach() for better isolation.
| const originalEnv = process.env; | |
| beforeEach(() => { | |
| process.env = { ...originalEnv }; | |
| }); | |
| afterAll(() => { | |
| const originalEnv = { ...process.env }; | |
| beforeEach(() => { | |
| process.env = { ...originalEnv }; | |
| }); | |
| afterEach(() => { |
| const originalEnv = process.env; | ||
|
|
||
| beforeEach(() => { | ||
| process.env = { ...originalEnv }; | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| process.env = originalEnv; |
There was a problem hiding this comment.
Same process.env isolation issue as above: this block captures originalEnv by reference and restores only in afterAll(). Prefer const originalEnv = { ...process.env } and restore in afterEach() to match the repo’s existing test pattern and reduce cross-test leakage.
| const originalEnv = process.env; | |
| beforeEach(() => { | |
| process.env = { ...originalEnv }; | |
| }); | |
| afterAll(() => { | |
| process.env = originalEnv; | |
| const originalEnv = { ...process.env }; | |
| beforeEach(() => { | |
| process.env = { ...originalEnv }; | |
| }); | |
| afterEach(() => { | |
| process.env = { ...originalEnv }; |
| expect(findings[0]).toContain('key'); | ||
| expect(findings[0]).toContain('${SECRET}'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Because ENV_VAR_PATTERN is a global regex (/g), lintEnvPatterns() can give false negatives if ENV_VAR_PATTERN.lastIndex is non-zero before the call (e.g., after an external .exec()/.test()). There isn’t currently a test that pre-sets ENV_VAR_PATTERN.lastIndex to a non-zero value before calling lintEnvPatterns; adding one would prevent regressions and would surface the need for lintEnvPatterns() to reset lastIndex before testing.
| test('handles non-zero ENV_VAR_PATTERN.lastIndex before lint', () => { | |
| // Simulate prior use of the global regex that leaves a non-zero lastIndex | |
| ENV_VAR_PATTERN.lastIndex = 5; | |
| const findings = lintEnvPatterns({ key: '${SECRET}' }, 'config-with-lastindex.yaml'); | |
| expect(findings).toHaveLength(1); | |
| expect(findings[0]).toContain('config-with-lastindex.yaml'); | |
| expect(findings[0]).toContain('key'); | |
| expect(findings[0]).toContain('${SECRET}'); | |
| }); |
f55167b to
ea1cd5c
Compare
|
Consolidated into #424 |
Summary
core/config/env-interpolatormoduleENV_VAR_PATTERN,interpolateString,interpolateEnvVars, andlintEnvPatterns${VAR}resolution,${VAR:-default}fallback, missing var warnings, recursive object/array walks, and lint detectionTest Coverage
ENV_VAR_PATTERNinterpolateStringinterpolateEnvVarslintEnvPatternsTest Plan
Closes #272