Copilot commented on code in PR #2282:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2282#discussion_r4248609438
##########
cmake/valgrind.supp:
##########
@@ -40,14 +40,15 @@
# the pointer is still reachable at exit and valgrind flags it "possibly
# lost". Not a real leak.
{
- pthread_create_tls_dtv_LogCompressorSink
+ pthread_create_tls_dtv_any_thread
Memcheck:Leak
match-leak-kinds: possible
fun:calloc
...
+ fun:_dl_allocate_tls
+ ...
fun:pthread_create*
...
- fun:*LogCompressorSink*
}
Review Comment:
This suppression is now very broad (applies to any `pthread_create*` path)
and could hide new 'possible' leak regressions introduced elsewhere during
thread creation. Suggestion (mandatory): tighten the suppression match (e.g.,
include a more specific call chain frame, object/module match, or additional
distinguishing frames) so it targets only the known glibc TLS/DTV false
positive without covering unrelated allocations.
##########
extensions/pdh/tests/CMakeLists.txt:
##########
@@ -32,6 +32,6 @@ FOREACH(testfile ${PDH_TESTS})
target_link_libraries(${testfilename} minifi-standard-processors)
MATH(EXPR PDH_TEST_COUNT "${PDH_TEST_COUNT}+1")
add_test(NAME "${testfilename}" COMMAND "${testfilename}"
WORKING_DIRECTORY ${TEST_DIR})
- set_tests_properties("${testfilename}" PROPERTIES LABELS "phd;memchecked")
+ set_tests_properties("${testfilename}" PROPERTIES LABELS "phd")
Review Comment:
The label string looks misspelled: the extension/test directory is `pdh` but
the label is `phd`. This makes label-based selection inconsistent. Suggestion
(mandatory): rename the label to `pdh`.
##########
extensions/standard-processors/tests/unit/AttributeRollingWindowTests.cpp:
##########
@@ -29,10 +32,12 @@ using AttributeRollingWindow =
processors::AttributeRollingWindow;
bool checkAttributes(const std::map<std::string, std::string>& expected, const
std::map<std::string, std::string>& actual) {
// expected may be incomplete, but if something is specified in expected,
they also need to be in the actual
// set of attributes
- return std::all_of(std::begin(expected), std::end(expected), [&actual](const
auto& kvpair) {
+ return std::ranges::all_of(expected, [&actual](const auto& kvpair) {
const auto& key = kvpair.first;
- const auto& value = kvpair.second;
- return actual.contains(key) && actual.at(key) == value;
+ if (!actual.contains(key)) { return false; }
+ double expected_value = std::stod(kvpair.second);
+ double actual_value = std::stod(actual.at(key));
+ return std::abs(expected_value - actual_value) < 0.000001;
});
Review Comment:
`checkAttributes` now unconditionally parses attribute values as doubles via
`std::stod`. If any expected/actual attribute value is non-numeric, this will
throw and fail the test for the wrong reason. Suggestion (mandatory): only use
floating-point comparison when both strings are numeric (or when the specific
keys under test are known numeric), otherwise fall back to exact string
comparison. Also consider including `<cmath>` explicitly for `std::abs(double)`
to avoid relying on transitive includes.
##########
extensions/standard-processors/tests/unit/PutTCPTests.cpp:
##########
@@ -105,7 +106,7 @@ class PutTCPTestFixture {
LogTestController::getInstance().setInfo<core::ProcessSession>();
LogTestController::getInstance().setTrace<utils::net::Server>();
REQUIRE(put_tcp_->setProperty(PutTCP::Hostname.name,
"${literal('localhost')}"));
- REQUIRE(put_tcp_->setProperty(PutTCP::Timeout.name, "200 ms"));
+ REQUIRE(put_tcp_->setProperty(PutTCP::Timeout.name, fmt::format("{}",
minifi::test::utils::timeout(200ms))));
Review Comment:
This changes the timeout property string from the previously explicit \"200
ms\" to whatever `fmt` prints for the duration (commonly \"200ms\" without a
space). If the property parser expects a space-separated format, this will
break the test (or behavior) under Valgrind. Suggestion (mandatory): format the
timeout in the same syntax the processor property parser is guaranteed to
accept (e.g., format count + explicit unit).
##########
extensions/standard-processors/tests/unit/GetTCPTests.cpp:
##########
@@ -155,9 +156,9 @@ TEST_CASE("GetTCP test with delimiter", "[GetTCP]") {
SingleProcessorTestController
controller{minifi::test::utils::make_processor<GetTCP>("GetTCP")};
const auto get_tcp = controller.getProcessor();
LogTestController::getInstance().setTrace<GetTCP>();
+ REQUIRE(get_tcp->setProperty(GetTCP::Timeout.name, fmt::format("{}",
minifi::test::utils::timeout(1s))));
Review Comment:
Same concern as in PutTCP tests: `fmt`’s default chrono formatting may
produce a string (e.g. \"1s\") that is not in the exact format the property
parser expects. Suggestion (mandatory): format the value in a parser-guaranteed
format (e.g., numeric + explicit unit string) while still applying
`minifi::test::utils::timeout(...)` scaling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
