Repository navigation
UTILS/CONFIG: Make config file reading unit-testable and re-enable test. - #1742
Conversation
|
👋 Hi ColinNV! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
📝 WalkthroughWalkthroughThe PR refactors config storage to support test isolation by introducing a mutable static holder with a refresh function, then re-enables the previously disabled config file test with proper invalidation logic. ChangesTestable config reloading
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
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. 🔧 Infer (1.2.0)test/gtest/configuration.cpptest/gtest/configuration.cpp:18:10: fatal error: 'common/backend.h' file not found ... [truncated 1084 characters] ... nstall/lib/clang/18/include" 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/gtest/configuration.cpp (1)
239-283: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueTest config file is not cleaned up after test.
The test creates a temporary config file but never removes it after the test completes. Consider adding cleanup to avoid leaving test artifacts in the temp directory.
♻️ Suggested fix using RAII cleanup
TEST(Config, ReadConfigFile) { const auto pid = ::getpid(); const auto tmp_path = std::filesystem::temp_directory_path(); const auto tmp_file = "nixl_test_" + std::to_string(pid) + ".tmp"; const auto cfg_file = "nixl_test_" + std::to_string(pid) + ".cfg"; + const auto cfg_path = tmp_path / cfg_file; + + // RAII cleanup - remove config file when test exits + struct FileCleanup { + std::filesystem::path path; + ~FileCleanup() { std::filesystem::remove(path); } + } cleanup{cfg_path}; + { std::ofstream ofs(tmp_path / tmp_file, std::ios::out | std::ios::trunc); ofs << bool_name << " = true\n";🤖 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 `@test/gtest/configuration.cpp` around lines 239 - 283, The test TEST(Config, ReadConfigFile) leaves the temp config (tmp_path / cfg_file) behind; add deterministic cleanup by removing that file at test end using RAII or a scope-guard: create a small local guard object (or use std::unique_ptr with custom deleter) that calls std::filesystem::remove(tmp_path / cfg_file) in its destructor, and instantiate it right after the rename so the file is always deleted even if assertions throw; ensure the guard runs after internal::invalidateConfigFileForUnitTest() and gtest::ScopedEnv usage so teardown order is safe.
🤖 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 `@src/utils/common/configuration.cpp`:
- Around line 80-99: Rename the private members in struct configGlobal to follow
the trailing-underscore convention: change mutex to mutex_ and data to data_,
then update all usages accordingly (e.g., in getConfigGlobal(), getConfigFile(),
and any helper like invalidateConfigFileForUnitTest()) so they reference
cg.mutex_ and cg.data_ instead of cg.mutex and cg.data; ensure the lock_guard in
getConfigFile uses cg.mutex_ and the shared_ptr assignment/return uses cg.data_.
In `@src/utils/common/configuration.h`:
- Around line 59-69: Rename the private member keep_view_alive in struct
tomlFindResult to keep_view_alive_ and update its constructor initializer and
any other references accordingly; specifically change the member declaration to
keep_view_alive_ and update the member initializer in tomlFindResult(const
toml::node_view<const toml::node>& view, const std::shared_ptr<const
toml::table>& data) : view(view), keep_view_alive(data) {} to use
keep_view_alive_(data), and adjust any code that accesses keep_view_alive to use
keep_view_alive_.
---
Outside diff comments:
In `@test/gtest/configuration.cpp`:
- Around line 239-283: The test TEST(Config, ReadConfigFile) leaves the temp
config (tmp_path / cfg_file) behind; add deterministic cleanup by removing that
file at test end using RAII or a scope-guard: create a small local guard object
(or use std::unique_ptr with custom deleter) that calls
std::filesystem::remove(tmp_path / cfg_file) in its destructor, and instantiate
it right after the rename so the file is always deleted even if assertions
throw; ensure the guard runs after internal::invalidateConfigFileForUnitTest()
and gtest::ScopedEnv usage so teardown order is safe.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8ce8abe0-1ac2-4388-89fa-1b42137c11ee
📒 Files selected for processing (3)
src/utils/common/configuration.cppsrc/utils/common/configuration.htest/gtest/configuration.cpp
|
/build |
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/gtest/configuration.cpp`:
- Around line 241-252: The test TEST(Config, ReadConfigFile) creates a temp file
(variables file and path) and should remove it when done; update the test to
delete the temporary config at the end of the test (e.g., call
std::filesystem::remove(path) or use an RAII scope-guard/temporary-file helper)
so the file is cleaned up even on failure (prefer using a scope guard or
try/finally-like construct to ensure removal).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3db3f2c9-a357-4ddb-b9b8-7c9373cbe32c
📒 Files selected for processing (2)
src/utils/common/configuration.cpptest/gtest/configuration.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@test/gtest/configuration.cpp`:
- Around line 241-252: The test TEST(Config, ReadConfigFile) creates a temp file
(variables file and path) and should remove it when done; update the test to
delete the temporary config at the end of the test (e.g., call
std::filesystem::remove(path) or use an RAII scope-guard/temporary-file helper)
so the file is cleaned up even on failure (prefer using a scope guard or
try/finally-like construct to ensure removal).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3db3f2c9-a357-4ddb-b9b8-7c9373cbe32c
📒 Files selected for processing (2)
src/utils/common/configuration.cpptest/gtest/configuration.cpp
🛑 Comments failed to post (1)
test/gtest/configuration.cpp (1)
241-252: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider cleaning up the temporary config file.
The test creates a temporary file in the temp directory but never deletes it. While temp directories are eventually cleaned by the OS, explicit cleanup would be better practice.
♻️ Proposed cleanup using RAII or explicit deletion
Add cleanup at the end of the test scope:
EXPECT_THROW((void)nixl::config::getValue<int>(env2name), std::runtime_error); } +std::filesystem::remove(path);Or consider using a RAII wrapper if one exists in the codebase (similar to
gtest::ScopedEnv).📝 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(Config, ReadConfigFile) { const auto pid = ::getpid(); const auto file = "nixl_test_" + std::to_string(pid) + ".cfg"; const auto path = std::filesystem::temp_directory_path() / file; { std::ofstream ofs(path, std::ios::out | std::ios::trunc); ofs << bool_name << " = true\n"; ofs << number_name << " = 42\n"; ofs << string_name << " = \"hello\"\n"; ofs << env1name << " = \"dummy\"\n"; ofs << env2name << " = 0\n"; } // ... rest of test code ... std::filesystem::remove(path); }🤖 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 `@test/gtest/configuration.cpp` around lines 241 - 252, The test TEST(Config, ReadConfigFile) creates a temp file (variables file and path) and should remove it when done; update the test to delete the temporary config at the end of the test (e.g., call std::filesystem::remove(path) or use an RAII scope-guard/temporary-file helper) so the file is cleaned up even on failure (prefer using a scope guard or try/finally-like construct to ensure removal).
Signed-off-by: Colin Hirsch <chirsch@nvidia.com>
|
/build |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/gtest/configuration.cpp (1)
294-332: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider cleaning up the temporary config file after the test.
The test creates a temporary config file but doesn't delete it. While the OS will eventually clean up the temp directory, explicitly removing test artifacts is better practice and prevents accumulation if tests run frequently.
♻️ Suggested cleanup
Add cleanup at the end of the test:
{ // Check that conversion failure on env var does not trigger lookup in config file. EXPECT_THROW((void)nixl::config::getValue<int>(env2name), std::runtime_error); } + std::filesystem::remove(path); }🤖 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 `@test/gtest/configuration.cpp` around lines 294 - 332, The test TEST(Config, ReadConfigFile) leaves a temporary config file at path (constructed via file and path variables); remove that artifact at the end of the test by deleting the file (e.g., call std::filesystem::remove or std::filesystem::remove_if_exists) after the assertions or wrap the temp file creation in an RAII helper so the file is removed in teardown; reference the existing path variable used to write the file and perform the removal so the temp file is always cleaned even on test failure (use try/finally style or ScopedEnv teardown to ensure removal).
🤖 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.
Outside diff comments:
In `@test/gtest/configuration.cpp`:
- Around line 294-332: The test TEST(Config, ReadConfigFile) leaves a temporary
config file at path (constructed via file and path variables); remove that
artifact at the end of the test by deleting the file (e.g., call
std::filesystem::remove or std::filesystem::remove_if_exists) after the
assertions or wrap the temp file creation in an RAII helper so the file is
removed in teardown; reference the existing path variable used to write the file
and perform the removal so the temp file is always cleaned even on test failure
(use try/finally style or ScopedEnv teardown to ensure removal).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 09d0c7dc-91a0-4db5-9cc7-d0d41917fa04
📒 Files selected for processing (1)
test/gtest/configuration.cpp
…st. (ai-dynamo#1742) * UTILS/CONFIG: Make config file reading unit-testable and re-enable test. * Simplify and keep ABI. * Fix typo. * Fix order. --------- Signed-off-by: Colin Hirsch <chirsch@nvidia.com>
This branch was previously deployed
2 participants
What?
Allow the config file reading unit test to re-load config file data.
Why?
Otherwise the config file reading test only succeeds when it is the first test to run inside of a process.
Summary by CodeRabbit
Tests
Refactor