szaszm commented on a change in pull request #1141:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1141#discussion_r676407725



##########
File path: extensions/standard-processors/tests/unit/HashContentTest.cpp
##########
@@ -149,17 +149,31 @@ TEST_CASE("TestingFailOnEmptyProperty", 
"[HashContentPropertiesCheck]") {
                                                                      
core::Relationship("success", "description"), true);
   plan->setProperty(md5processor, 
org::apache::nifi::minifi::processors::HashContent::HashAttribute.getName(), 
MD5_ATTR);
   plan->setProperty(md5processor, 
org::apache::nifi::minifi::processors::HashContent::HashAlgorithm.getName(), 
"MD5");
-  plan->setProperty(md5processor, 
org::apache::nifi::minifi::processors::HashContent::FailOnEmpty.getName(), 
"true");
+  std::set<core::Relationship> relationships;
+  relationships.insert(core::Relationship("success", "description"));
+  relationships.insert(core::Relationship("failure", "description"));
+  md5processor->setAutoTerminatedRelationships(relationships);
 
   std::stringstream stream_dir;
   stream_dir << tempdir << utils::file::FileUtils::get_separator() << 
TEST_FILE;
   std::string test_file_path = stream_dir.str();
   std::ofstream test_file(test_file_path, std::ios::binary);
 
+  SECTION("with an empty file and fail on empty property set to false") {
+  plan->setProperty(md5processor, 
org::apache::nifi::minifi::processors::HashContent::FailOnEmpty.getName(), 
"false");
+
   plan->runNextProcessor();
   plan->runNextProcessor();
 
-  REQUIRE(LogTestController::getInstance().contains("Failure as flow file is 
empty"));
-}
+  REQUIRE(LogTestController::getInstance().contains("attempting read"));

Review comment:
       The indentation is incorrect here, this should go one level deeper.

##########
File path: extensions/standard-processors/processors/HashContent.cpp
##########
@@ -86,6 +86,7 @@ void HashContent::onTrigger(core::ProcessContext *, 
core::ProcessSession *sessio
   if (failOnEmpty_ && flowFile->getSize() == 0) {
     logger_->log_debug("Failure as flow file is empty");
     session->transfer(flowFile, Failure);
+    return;

Review comment:
       :+1: 

##########
File path: extensions/standard-processors/tests/unit/HashContentTest.cpp
##########
@@ -149,17 +149,31 @@ TEST_CASE("TestingFailOnEmptyProperty", 
"[HashContentPropertiesCheck]") {
                                                                      
core::Relationship("success", "description"), true);
   plan->setProperty(md5processor, 
org::apache::nifi::minifi::processors::HashContent::HashAttribute.getName(), 
MD5_ATTR);
   plan->setProperty(md5processor, 
org::apache::nifi::minifi::processors::HashContent::HashAlgorithm.getName(), 
"MD5");
-  plan->setProperty(md5processor, 
org::apache::nifi::minifi::processors::HashContent::FailOnEmpty.getName(), 
"true");
+  std::set<core::Relationship> relationships;
+  relationships.insert(core::Relationship("success", "description"));
+  relationships.insert(core::Relationship("failure", "description"));
+  md5processor->setAutoTerminatedRelationships(relationships);

Review comment:
       Not sure if it's necessary, but I'd rather refer to HashContent's 
Relationship instances instead of creating new ones. Also, it's shorter (and 
nicer IMO) to use an initializer-list to define the set.
   ```suggestion
     using processors::HashContent;
     md5processor->setAutoTerminatedRelationships({HashContent::Success, 
HashContent::Failure});
   ```

##########
File path: docker/test/integration/MiNiFi_integration_test_driver.py
##########
@@ -266,3 +266,10 @@ def check_azure_storage_server_data(self, cluster_name, 
object_data):
     def wait_for_kafka_consumer_to_be_registered(self, cluster_name):
         cluster = self.acquire_cluster(cluster_name)
         assert cluster.wait_for_kafka_consumer_to_be_registered()
+
+    def check_minifi_log_contents(self, line):
+        line_found = False
+        for cluster in self.clusters.values():
+            if cluster.get_engine() == "minifi-cpp":
+                line_found = cluster.wait_for_app_logs(line, 60)
+        assert line_found

Review comment:
       All other functions have a cluster_name parameter and selects the 
cluster based on that. Shouldn't this function do the same?




-- 
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]


Reply via email to