Replace some integration tests with unit tests to reduce reliance on requireUncached#862
Conversation
- 2-config-test.js - 20-multiple.js - 22-files-order.js To reduce use of requireUncached for node-config#860
- 4-array-merging.js - 7-unicode-situations.js Note: this fixes a test that was silently failing after earlier refactoring - x-config-js-test-transpiled.js - x-config-test-ts.js - x-config-test-ts-module-exports.js To reduce use of requireUncached for node-config#860
📝 WalkthroughWalkthroughThis PR consolidates test coverage by removing multiple individual test files and a configuration fixture, relocating their test cases into a comprehensive unified test suite in test/0-util.js. A minor production code change removes automatic absolute path conversion for NODE_CONFIG_DIR in Load.fromEnvironment. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
| } | ||
|
|
||
| let configDir = env.initParam('NODE_CONFIG_DIR'); | ||
| configDir = configDir && _toAbsolutePath(configDir); |
There was a problem hiding this comment.
This code is 1) incorrect because this can be a colon-separated value, and 2) duplicates work that is repeated in the next phase of the load operation anyway, after the paths have been split for iterating.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @test/0-util.js:
- Around line 1246-1266: The tests expect util.loadFileConfigs to treat
appInstance values (including numeric 3) and to load JSON files before YAML so
YAML can override JSON; update the load logic in util.loadFileConfigs (and any
helper that builds filenames or iterates extensions) to coerce appInstance to
string when constructing instance-specific filenames and to enforce an extension
load order that loads .json (and .js) before .yml/.yaml so YAML overrides take
effect; ensure the code path that merges configs preserves later-file precedence
so values from YAML replace earlier JSON values.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
lib/util.jstest/0-util.jstest/10-config/default.jsontest/10-gitcrypt-test.jstest/14-node_env-load-multiple-files.jstest/17-toml.jstest/19-config/custom-environment-variables.jsontest/19-config/default.jstest/19-custom-environment-variables.jstest/2-config-test.jstest/20-multiple-config.jstest/22-files-order.jstest/4-array-merging.jstest/7-unicode-situations.jstest/config/bare-metal.jsontest/config/cloud.jsontest/config/development.jsontest/config/encrypted.jsontest/config/git-crypt-symmetric-keytest/config/local-cloud.jsontest/config/local-development.jsontest/config/test-bare-metal.jsontest/config/test-development.jsontest/x-config-js-test-transpiled.jstest/x-config-test-ts-module-exports.jstest/x-config-test-ts.js
💤 Files with no reviewable changes (16)
- test/14-node_env-load-multiple-files.js
- test/19-config/default.js
- test/10-gitcrypt-test.js
- lib/util.js
- test/2-config-test.js
- test/22-files-order.js
- test/10-config/default.json
- test/x-config-test-ts-module-exports.js
- test/20-multiple-config.js
- test/19-custom-environment-variables.js
- test/x-config-test-ts.js
- test/x-config-js-test-transpiled.js
- test/7-unicode-situations.js
- test/4-array-merging.js
- test/17-toml.js
- test/19-config/custom-environment-variables.json
🔇 Additional comments (9)
test/0-util.js (9)
373-393: Array replacement tests in extendDeep are well-designed.The new tests properly verify that arrays are replaced wholesale (not merged element-by-element) and that both empty-to-full and full-to-empty replacements work correctly. The assertions confirm the result is correct and that the original object remains unmodified.
795-805: Test verifies fromEnvironment ignores process.env when argument provided.This test correctly validates the new behavior: when an explicit argument is passed to
Load.fromEnvironment(), it should use that value instead of reading fromNODE_ENV. The test properly cleans up via try/finally.
807-819: Multiple nodeEnv enumeration test is correct.Test verifies that comma-separated
NODE_ENVvalues like'mercury,apollo'are properly split into an array['mercury', 'apollo']. The structure and assertions are sound.
1268-1293: Environment and local file loading tests are comprehensive.Tests verify that nodeEnv-specific files (e.g.,
config/test.*), local files, and local-env combinations load correctly with expected merging. Assertions target specific fields that should come from different file sources, validating proper precedence.
1442-1483: Multiple nodeEnv tests validate enumeration, local variants, and left-right load order.Tests verify that multiple nodeEnv values (e.g.,
['development', 'cloud']) cause corresponding files to load in order, that local variants are included, and that values are merged with correct precedence (right-most wins, validated at line 1474-1482). The hostname-specific loading test (line 1463-1472) verifies that env-hostname combinations are resolved.
1485-1523: Multiple config directories tests validate path handling and merge order.Tests verify that configs from multiple directories (delimited by
Path.delimiter) are loaded and merged correctly, that both absolute and relative paths work together, and that empty entries don't cause errors. Assertions confirm values come from the correct directory and that resolution order is correct.
725-748: Encrypted file tests validate both success and error paths.Tests verify that encrypted files return
nullwhen gitCrypt is disabled (lines 726-735) and throw an appropriate error when gitCrypt is not configured (lines 737-747). The regex match/Cannot parse config file/is appropriately loose to avoid brittleness.
1295-1439: Filetypes coverage is extensive and validates format-specific parsing.The new "filetypes" describe block tests loading from JS, JSON, JSONC, JSON5, CSON, YAML (.yaml/.yml), CoffeeScript, transpiled ESM, Hjson, XML, MJS, TypeScript, TOML, and .properties files. Each test loads a config and asserts a format-specific value exists with the correct data, validating that parsers work correctly.
Notable: The nested "for .ts files" describe block includes defensive testing for TypeScript—verifying that an existing
.tshandler is not overwritten, which is good practice when dynamically registering require extensions.
703-707: BOM Unicode test added; fixture file verified.The test correctly uses
doesNotThrowto verify that config files with UTF-8 BOM prefixes parse without error. The fixture file attest/7-config/defaultWithUnicodeBOM.jsonexists and contains a UTF-8 BOM marker as expected.
1 participant
Work for #860
This changeset fixes a silently broken test for Unicode BOM characters, adds coverage for the optional parameter to
fromEnvironment(), and moves a heap of tests still being run on the Config singleton to run on loadFile() or loadFileConfigs() as appropriate. A few of these were already doing this but had been modified in-place.This PR reduces the number of uses of
requireUncachedin the tests from 80 to 46. I believe all of the remaining cases arefor functionality that was not migrated to lib/util.js
Summary by CodeRabbit
Configuration Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.