test: Add config helper utility (Fixes #45364)#45373
test: Add config helper utility (Fixes #45364)#45373yeelam-gordon wants to merge 1 commit intomainfrom
Conversation
yeelam-gordon
left a comment
There was a problem hiding this comment.
PR Review Summary - PR #45373
Review for: test: Add config helper utility (Fixes #45364)
Medium Severity Findings
Finding 1 - Stub Implementation (Functionality)
📁 src/common/utils/TestConfigHelper.h (lines 12-14)
The GetConfigValue function always returns the default value without reading from any actual configuration source. This is a stub implementation that could mislead callers into thinking configuration is being read.
Recommendation: Either mark this clearly as a mock/stub in the function name (e.g., GetConfigValueMock) or implement actual config reading logic.
Finding 2 - Hardcoded Return Value (Functionality)
📁 src/common/utils/TestConfigHelper.h (lines 16-18)
IsFeatureEnabled always returns true regardless of the feature name. This could mask disabled features during testing or lead to incorrect behavior if used in production code paths.
Recommendation: Consider either:
- Renaming to
IsFeatureEnabledMockto clarify it's a stub - Add a
// TODO: Implement actual feature flag checkingcomment - Or implementing actual feature flag lookup
Finding 3 - Missing include guard alternative (Repo Patterns)
📁 src/common/utils/TestConfigHelper.h (line 4)
While #pragma once is used (which is fine), the <optional> header is included but never used in the code.
Recommendation: Remove the unused #include <optional> to keep includes minimal.
Summary
This appears to be a mock/test utility file based on the comments, but the function names don't indicate this. If this is intentional test infrastructure, consider making the mock nature explicit in naming.
Overall: The PR is marked as a mock for orchestration testing, which explains the stub implementations. However, adding this file to src/common/utils/ (a production code location) without clear "Mock" naming could cause confusion.
yeelam-gordon
left a comment
There was a problem hiding this comment.
PR Review Summary for #45373
Files Changed: 1 (TestConfigHelper.h)
Findings >= Medium: 3
Summary
This PR adds a mock configuration helper. While functional for testing, there are repository pattern issues that should be addressed.
yeelam-gordon
left a comment
There was a problem hiding this comment.
Automated PR Review - Medium+ Severity Findings
This review identified 4 medium-severity issues that should be addressed:
| # | Issue | Line | Severity |
|---|---|---|---|
| 1 | Unused include (<optional>) |
7 | Medium |
| 2 | Test file in production location | 1 | Medium |
| 3 | Code formatting inconsistency (K&R vs Allman) | 13-21 | Medium |
| 4 | Hardcoded return values | 13-18 | Medium |
See inline comments for details and suggested fixes.
There was a problem hiding this comment.
Pull request overview
This PR adds a new C++ header file TestConfigHelper.h with stub configuration helper functions, claiming to fix issue #45364. However, the linked issue requests adding XML documentation comments to a C# ModuleHelper class, which is unrelated to this change. The PR description explicitly states this is a "Mock PR for orchestration test" that should not be merged.
Changes:
- Adds a new header file
src/common/utils/TestConfigHelper.hwith two inline template/function stubs for configuration management
Mock PR for orchestration test. Fixes #45364