test: add environment variable parsing edge case tests#1073
Conversation
Add comprehensive tests for environment variable handling edge cases: - Missing required env var produces clear error - Empty string env var handling - Whitespace-only env var handling - Boolean env vars (true/false/1/0) - Unicode and special characters - Path and URL values Risk: LOW - tests only
WalkthroughA comprehensive Jest test suite added for the ChangesgetEnvVariable Test Suite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/utils/__tests__/env-var.test.ts`:
- Around line 61-65: Remove the duplicate unit test that sets
process.env.TEST_VAR = ' ' and asserts getEnvVariable('TEST_VAR') returns '
' (the test with description "should handle whitespace-only environment variable
with no default"); locate the redundant it(...) block in
server/src/utils/__tests__/env-var.test.ts and delete that second occurrence so
only one identical test for getEnvVariable handling whitespace remains.
- Around line 129-136: The inline comment incorrectly claims process.env returns
undefined for empty vs empty string; update the comment in the test around
getEnvVariable to say that process.env can contain an empty string and that
getEnvVariable treats empty strings as falsy (due to using the ||/coalescing
logic), so an empty string falls through to the provided default; reference the
test name and getEnvVariable so it's clear which behavior is being described.
- Around line 138-142: Remove the duplicate test block that checks default
behavior for an undefined env var: delete the second "it('should use default
when env var is undefined', ...)" test that deletes process.env.TEST_VAR and
asserts getEnvVariable('TEST_VAR', 'my-default') returns 'my-default', leaving
only the original occurrence to avoid redundant assertions.
- Around line 41-46: getEnvVariable currently uses falsy checks
(process.env[key] || defaultValue and if (!value)) which treats an empty string
as "missing" and causes incorrect throws when defaultValue === ''. Update
getEnvVariable to use nullish-coalescing or explicit undefined checks so only
undefined counts as missing: compute value = process.env[key] ?? defaultValue
and replace the if (!value) guard with if (value === undefined) (or equivalent)
so empty-string env values and empty-string defaults are returned rather than
treated as absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a5dd6fe-675f-4720-a57a-1c1d1f64e3b3
📒 Files selected for processing (1)
server/src/utils/__tests__/env-var.test.ts
| it('should handle empty string environment variable', () => { | ||
| process.env.TEST_VAR = ''; | ||
|
|
||
| // Empty string is falsy, so it should throw when no default | ||
| expect(() => getEnvVariable('TEST_VAR')).toThrow(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the implementation of getEnvVariable to verify empty string handling
# Find and display the implementation
rg -A 20 'function getEnvVariable|const getEnvVariable|export.*getEnvVariable' --type=ts server/src/utils/env.tsRepository: getmaxun/maxun
Length of output: 306
🏁 Script executed:
#!/bin/bash
# Print the relevant test sections to confirm empty-string expectations
sed -n '1,220p' server/src/utils/__tests__/env-var.test.ts | nl -ba | sed -n '30,170p'Repository: getmaxun/maxun
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Show the relevant test blocks with line numbers (cat -n)
cat -n server/src/utils/__tests__/env-var.test.ts | sed -n '35,170p'Repository: getmaxun/maxun
Length of output: 5505
Fix getEnvVariable falsy handling to match tests (esp. defaultValue === '').
server/src/utils/env.ts uses process.env[key] || defaultValue and throws on if (!value), which matches the “empty env var without default should throw” (41-46) and “empty env var with default should use default” (129-136).
However, it will throw when defaultValue is '' and the env var is missing, contradicting the test at 48-52 expecting ''.
Adjust the “missing” check to treat only undefined as missing (so empty string defaults are returned).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/utils/__tests__/env-var.test.ts` around lines 41 - 46,
getEnvVariable currently uses falsy checks (process.env[key] || defaultValue and
if (!value)) which treats an empty string as "missing" and causes incorrect
throws when defaultValue === ''. Update getEnvVariable to use nullish-coalescing
or explicit undefined checks so only undefined counts as missing: compute value
= process.env[key] ?? defaultValue and replace the if (!value) guard with if
(value === undefined) (or equivalent) so empty-string env values and
empty-string defaults are returned rather than treated as absent.
| it('should handle whitespace-only environment variable with no default', () => { | ||
| process.env.TEST_VAR = ' '; | ||
|
|
||
| expect(getEnvVariable('TEST_VAR')).toBe(' '); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test.
This test is identical to the test on lines 54-59. Both set TEST_VAR to ' ' with no default and expect the whitespace to be returned.
🧹 Proposed fix to remove duplicate
- it('should handle whitespace-only environment variable with no default', () => {
- process.env.TEST_VAR = ' ';
-
- expect(getEnvVariable('TEST_VAR')).toBe(' ');
- });
-📝 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.
| it('should handle whitespace-only environment variable with no default', () => { | |
| process.env.TEST_VAR = ' '; | |
| expect(getEnvVariable('TEST_VAR')).toBe(' '); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/utils/__tests__/env-var.test.ts` around lines 61 - 65, Remove the
duplicate unit test that sets process.env.TEST_VAR = ' ' and asserts
getEnvVariable('TEST_VAR') returns ' ' (the test with description "should
handle whitespace-only environment variable with no default"); locate the
redundant it(...) block in server/src/utils/__tests__/env-var.test.ts and delete
that second occurrence so only one identical test for getEnvVariable handling
whitespace remains.
| it('should prioritize actual env var over default when actual is empty string', () => { | ||
| process.env.TEST_VAR = ''; | ||
|
|
||
| // Empty string is falsy so will fall through to default | ||
| // This is because process.env returns undefined for empty vs empty string | ||
| // But our implementation uses || which treats empty string as falsy | ||
| expect(getEnvVariable('TEST_VAR', 'default')).toBe('default'); | ||
| }); |
There was a problem hiding this comment.
Misleading comment about process.env behavior.
The comment on line 133 states "This is because process.env returns undefined for empty vs empty string", but this is inaccurate. process.env can store and return empty strings—the actual behavior is that getEnvVariable treats empty strings as falsy (likely using || or similar), causing them to fall through to the default value.
📝 Proposed fix for the comment
// Empty string is falsy so will fall through to default
- // This is because process.env returns undefined for empty vs empty string
- // But our implementation uses || which treats empty string as falsy
+ // The implementation treats empty string as falsy (using || or similar logic)
expect(getEnvVariable('TEST_VAR', 'default')).toBe('default');📝 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.
| it('should prioritize actual env var over default when actual is empty string', () => { | |
| process.env.TEST_VAR = ''; | |
| // Empty string is falsy so will fall through to default | |
| // This is because process.env returns undefined for empty vs empty string | |
| // But our implementation uses || which treats empty string as falsy | |
| expect(getEnvVariable('TEST_VAR', 'default')).toBe('default'); | |
| }); | |
| it('should prioritize actual env var over default when actual is empty string', () => { | |
| process.env.TEST_VAR = ''; | |
| // Empty string is falsy so will fall through to default | |
| // The implementation treats empty string as falsy (using || or similar logic) | |
| expect(getEnvVariable('TEST_VAR', 'default')).toBe('default'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/utils/__tests__/env-var.test.ts` around lines 129 - 136, The
inline comment incorrectly claims process.env returns undefined for empty vs
empty string; update the comment in the test around getEnvVariable to say that
process.env can contain an empty string and that getEnvVariable treats empty
strings as falsy (due to using the ||/coalescing logic), so an empty string
falls through to the provided default; reference the test name and
getEnvVariable so it's clear which behavior is being described.
| it('should use default when env var is undefined', () => { | ||
| delete process.env.TEST_VAR; | ||
|
|
||
| expect(getEnvVariable('TEST_VAR', 'my-default')).toBe('my-default'); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test.
This test duplicates the test on lines 35-39. Both tests delete TEST_VAR and verify that the default value is returned when the environment variable is undefined.
🧹 Proposed fix to remove duplicate
- it('should use default when env var is undefined', () => {
- delete process.env.TEST_VAR;
-
- expect(getEnvVariable('TEST_VAR', 'my-default')).toBe('my-default');
- });
-📝 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.
| it('should use default when env var is undefined', () => { | |
| delete process.env.TEST_VAR; | |
| expect(getEnvVariable('TEST_VAR', 'my-default')).toBe('my-default'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/utils/__tests__/env-var.test.ts` around lines 138 - 142, Remove
the duplicate test block that checks default behavior for an undefined env var:
delete the second "it('should use default when env var is undefined', ...)" test
that deletes process.env.TEST_VAR and asserts getEnvVariable('TEST_VAR',
'my-default') returns 'my-default', leaving only the original occurrence to
avoid redundant assertions.
2 participants
Summary
Problem
Solution
Tests
npm test -- --testPathPattern=env-varRisk / Compatibility
Summary by CodeRabbit