Copilot commented on code in PR #2284:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2284#discussion_r4258390654
##########
extensions/kafka/tests/CMakeLists.txt:
##########
@@ -25,6 +25,7 @@ FOREACH (testfile ${KAFKA_INTEGRATION_TESTS})
add_minifi_executable("${testfilename}" "${testfile}")
target_include_directories(${testfilename} BEFORE PRIVATE
"${CMAKE_SOURCE_DIR}/extensions/kafka")
target_compile_definitions(${testfilename} PRIVATE
"MINIFI_EXTENSION_GROUP_NAME=org.apache.nifi.minifi.test")
+ target_compile_definitions(${testfilename} PRIVATE
TEST_RESOURCES="${TEST_RESOURCES}")
Review Comment:
Passing filesystem paths via preprocessor definitions can break on some
platforms/toolchains when the path contains backslashes or spaces
(quoting/escaping differs across compilers). Consider ensuring the value is
passed as a properly escaped C string literal across generators (e.g., using a
shell/path-safe transform or explicit escaping), so `TEST_RESOURCES` reliably
expands to a valid string literal everywhere.
##########
extensions/kafka/tests/PublishKafkaTests.cpp:
##########
@@ -55,4 +60,39 @@ TEST_CASE("Trigger without valid broker", "[PublishKafka]") {
CHECK_FALSE(logger->logs_.empty());
}
+namespace {
+bool hasDebugLog(const mock::MockLogger& logger, std::string_view message) {
+ const auto debug_logs = logger.logs_.find(core::logging::LOG_LEVEL::debug);
+ return debug_logs != logger.logs_.end() &&
std::ranges::any_of(debug_logs->second, [&](const auto& log) { return
log.contains(message); });
Review Comment:
`log.contains(message)` requires C++23 (`std::string::contains` /
`std::string_view::contains`). If this project is built with C++20 (common for
MiNiFi C++), this will fail to compile. Use a C++20-compatible substring check
(e.g., `log.find(message) != std::string::npos`) instead.
##########
extensions/kafka/tests/PublishKafkaTests.cpp:
##########
@@ -55,4 +60,39 @@ TEST_CASE("Trigger without valid broker", "[PublishKafka]") {
CHECK_FALSE(logger->logs_.empty());
}
+namespace {
+bool hasDebugLog(const mock::MockLogger& logger, std::string_view message) {
+ const auto debug_logs = logger.logs_.find(core::logging::LOG_LEVEL::debug);
+ return debug_logs != logger.logs_.end() &&
std::ranges::any_of(debug_logs->second, [&](const auto& log) { return
log.contains(message); });
+}
+} // namespace
+
+TEST_CASE("Security properties are used for SSL connection when no SSL Context
Service is set", "[PublishKafka]") {
+ const auto logger = std::make_shared<mock::MockLogger>();
+ auto publish_kafka = PublishKafka(mock::metadataWithLogger(logger));
+ auto context = mock::MockProcessContext{};
+ const auto ca_cert_path = (std::filesystem::path(TEST_RESOURCES) /
"nifi-cert.pem").string();
+ context.properties_.emplace(PublishKafka::ClientName.name, "test_client");
+ context.properties_.emplace(PublishKafka::SeedBrokers.name,
"test_seedbroker");
+ context.properties_.emplace(PublishKafka::SecurityProtocol.name, "ssl");
+ context.properties_.emplace(PublishKafka::SecurityCA.name, ca_cert_path);
+
+ REQUIRE_NOTHROW(publish_kafka.onScheduleImpl(context));
+
+ CHECK(hasDebugLog(*logger, "Kafka ssl.ca.location [" + ca_cert_path + "]"));
Review Comment:
These assertions are tightly coupled to the exact log formatting (including
brackets and full message text), which makes the tests brittle to harmless log
wording/format changes. Consider loosening the checks to validate key
substrings (e.g., `\"Kafka ssl.ca.location\"` plus the expected value), or
asserting on the effective configuration value directly if the config object is
accessible in tests.
##########
extensions/kafka/tests/PublishKafkaTests.cpp:
##########
@@ -55,4 +60,39 @@ TEST_CASE("Trigger without valid broker", "[PublishKafka]") {
CHECK_FALSE(logger->logs_.empty());
}
+namespace {
+bool hasDebugLog(const mock::MockLogger& logger, std::string_view message) {
+ const auto debug_logs = logger.logs_.find(core::logging::LOG_LEVEL::debug);
+ return debug_logs != logger.logs_.end() &&
std::ranges::any_of(debug_logs->second, [&](const auto& log) { return
log.contains(message); });
+}
+} // namespace
+
+TEST_CASE("Security properties are used for SSL connection when no SSL Context
Service is set", "[PublishKafka]") {
+ const auto logger = std::make_shared<mock::MockLogger>();
+ auto publish_kafka = PublishKafka(mock::metadataWithLogger(logger));
+ auto context = mock::MockProcessContext{};
+ const auto ca_cert_path = (std::filesystem::path(TEST_RESOURCES) /
"nifi-cert.pem").string();
+ context.properties_.emplace(PublishKafka::ClientName.name, "test_client");
+ context.properties_.emplace(PublishKafka::SeedBrokers.name,
"test_seedbroker");
+ context.properties_.emplace(PublishKafka::SecurityProtocol.name, "ssl");
+ context.properties_.emplace(PublishKafka::SecurityCA.name, ca_cert_path);
+
+ REQUIRE_NOTHROW(publish_kafka.onScheduleImpl(context));
+
+ CHECK(hasDebugLog(*logger, "Kafka ssl.ca.location [" + ca_cert_path + "]"));
+}
+
+TEST_CASE("System CA certificate locations are probed when no CA certificate
is set for SSL connection", "[PublishKafka]") {
+ const auto logger = std::make_shared<mock::MockLogger>();
+ auto publish_kafka = PublishKafka(mock::metadataWithLogger(logger));
+ auto context = mock::MockProcessContext{};
+ context.properties_.emplace(PublishKafka::ClientName.name, "test_client");
+ context.properties_.emplace(PublishKafka::SeedBrokers.name,
"test_seedbroker");
+ context.properties_.emplace(PublishKafka::SecurityProtocol.name, "ssl");
+
+ REQUIRE_NOTHROW(publish_kafka.onScheduleImpl(context));
+
+ CHECK(hasDebugLog(*logger, "Kafka ssl.ca.location [probe]"));
Review Comment:
These assertions are tightly coupled to the exact log formatting (including
brackets and full message text), which makes the tests brittle to harmless log
wording/format changes. Consider loosening the checks to validate key
substrings (e.g., `\"Kafka ssl.ca.location\"` plus the expected value), or
asserting on the effective configuration value directly if the config object is
accessible in tests.
--
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]
