arpadboda commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r454431840
##########
File path: extensions/http-curl/tests/HttpGetIntegrationTest.cpp
##########
@@ -74,6 +71,8 @@ class HttpResponder : public CivetHandler {
};
int main(int argc, char **argv) {
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
I'm not sure this has anything to do in utils in case we only use it in
for verification. Should be part of a test namespace.
##########
File path: extensions/http-curl/tests/C2FailedUpdateTest.cpp
##########
@@ -201,13 +199,11 @@ int main(int argc, char **argv) {
controller->load();
controller->start();
- waitToVerifyProcessor();
+ const bool test_success =
verifyLogLinePresenceInPollTime(std::chrono::seconds(10), "Invalid
configuration payload", "update failed.");
Review comment:
Why the extra bool and why don't we do it in the proper overridden
function?
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,66 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <utility>
+#include <string>
+#include <vector>
+
+#include "../../../libminifi/test/TestBase.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template <class Rep, class Period, typename Fun>
+bool verifyEventHappenedInPollTime(const std::chrono::duration<Rep, Period>&
wait_duration, Fun&& check) {
+ std::chrono::system_clock::time_point wait_end =
std::chrono::system_clock::now() + wait_duration;
+ do {
+ if (std::forward<Fun>(check)()) {
+ return true;
+ }
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ } while (std::chrono::system_clock::now() < wait_end);
+ return false;
+}
+
+template <class Rep, class Period, typename ...String>
+bool verifyLogLinePresenceInPollTime(const std::chrono::duration<Rep, Period>&
wait_duration, String... patterns) {
+ // This helper is to be removed once we upgrade to support gcc 4.9+ only
+ std::vector<std::string> pattern_list;
+ std::initializer_list<int> {(pattern_list.push_back(patterns), 0)...};
+ auto check = [&] {
+ const std::string logs = LogTestController::getInstance().log_output.str();
+ for (const std::string& pattern : pattern_list) {
Review comment:
I think std_all is quite talkative for such cases to make the purpose
clear:
```
std::all_of(pattern_list.cbegin(), pattern_list.cend(), [&logs](const
std::string& pattern){ return logs.find(pattern) != std::string::npos; })
```
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,66 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <utility>
+#include <string>
+#include <vector>
+
+#include "../../../libminifi/test/TestBase.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+template <class Rep, class Period, typename Fun>
+bool verifyEventHappenedInPollTime(const std::chrono::duration<Rep, Period>&
wait_duration, Fun&& check) {
+ std::chrono::system_clock::time_point wait_end =
std::chrono::system_clock::now() + wait_duration;
+ do {
+ if (std::forward<Fun>(check)()) {
+ return true;
+ }
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ } while (std::chrono::system_clock::now() < wait_end);
+ return false;
+}
+
+template <class Rep, class Period, typename ...String>
+bool verifyLogLinePresenceInPollTime(const std::chrono::duration<Rep, Period>&
wait_duration, String... patterns) {
+ // This helper is to be removed once we upgrade to support gcc 4.9+ only
+ std::vector<std::string> pattern_list;
+ std::initializer_list<int> {(pattern_list.push_back(patterns), 0)...};
+ auto check = [&] {
+ const std::string logs = LogTestController::getInstance().log_output.str();
+ for (const std::string& pattern : pattern_list) {
+ if (logs.find(pattern) == std::string::npos) {
Review comment:
Inconsistent indentation here (2 vs 4)
##########
File path: extensions/http-curl/tests/ControllerServiceIntegrationTests.cpp
##########
@@ -126,34 +125,34 @@ int main(int argc, char **argv) {
// now let's disable one of the controller services.
std::shared_ptr<core::controller::ControllerServiceNode> cs_id =
controller->getControllerServiceNode("ID");
assert(cs_id != nullptr);
- {
- std::lock_guard<std::mutex> lock(control_mutex);
- controller->disableControllerService(cs_id);
- disabled = true;
- waitToVerifyProcessor();
- }
{
std::lock_guard<std::mutex> lock(control_mutex);
controller->enableControllerService(cs_id);
disabled = false;
- waitToVerifyProcessor();
}
std::shared_ptr<core::controller::ControllerServiceNode> mock_cont =
controller->getControllerServiceNode("MockItLikeIts1995");
- assert(cs_id->enabled());
-{
+ const bool test_success_01 =
verifyEventHappenedInPollTime(std::chrono::seconds(4), [&cs_id] {
+ return cs_id->enabled();
+ });
+ {
std::lock_guard<std::mutex> lock(control_mutex);
controller->disableReferencingServices(mock_cont);
disabled = true;
- waitToVerifyProcessor();
}
- assert(cs_id->enabled() == false);
-{
+ const bool test_success_02 =
verifyEventHappenedInPollTime(std::chrono::seconds(2), [&cs_id] {
+ return !cs_id->enabled();
+ });
+ {
std::lock_guard<std::mutex> lock(control_mutex);
controller->enableReferencingServices(mock_cont);
disabled = false;
- waitToVerifyProcessor();
}
- assert(cs_id->enabled() == true);
+ const bool test_success_03 =
verifyEventHappenedInPollTime(std::chrono::seconds(2), [&cs_id] {
Review comment:
This seems to be exactly the first lambda recreated, can you simplify
this code a bit? (an additional bool argument for the lambda as expected result
and we just run the same func 3 times)
##########
File path: extensions/http-curl/tests/HTTPSiteToSiteTests.cpp
##########
@@ -81,7 +81,10 @@ class SiteToSiteTestHarness : public CoapIntegrationBase {
void cleanup() override {}
- void runAssertions() override {}
+ void runAssertions() override {
+ // There is nothing to verify here, but we are expected to wait for all
paralell events to execute
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
Why here? Why don't we do this in the function designed to wait in?
(waitToVerifyProcessors)
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,66 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <utility>
+#include <string>
+#include <vector>
+
+#include "../../../libminifi/test/TestBase.h"
Review comment:
In case libminifi is added as "target_include_dir", this can be made
nice and simple.
##########
File path: libminifi/test/integration/OnScheduleErrorHandlingTests.cpp
##########
@@ -25,31 +25,45 @@
#include "core/state/ProcessorController.h"
#include "../TestBase.h"
#include "../KamikazeProcessor.h"
+#include "utils/IntegrationTestUtils.h"
/*Verify behavior in case exceptions are thrown in onSchedule or onTrigger
functions
* KamikazeProcessor is a test processor to trigger errors in these functions
*/
class KamikazeErrorHandlingTests : public IntegrationBase {
public:
void runAssertions() override {
- std::string logs = LogTestController::getInstance().log_output.str();
-
- auto result = countPatInStr(logs,
minifi::processors::KamikazeProcessor::OnScheduleExceptionStr);
- size_t last_pos = result.first;
- int occurrences = result.second;
-
- assert(occurrences > 1); // Verify retry of onSchedule and onUnSchedule
calls
+ using org::apache::nifi::minifi::utils::verifyEventHappenedInPollTime;
+
assert(verifyEventHappenedInPollTime(std::chrono::milliseconds(wait_time_), [&]
{
+ const std::string logs =
LogTestController::getInstance().log_output.str();
+ const auto result = countPatInStr(logs,
minifi::processors::KamikazeProcessor::OnScheduleExceptionStr);
+ const int occurrences = result.second;
+ if (occurrences <= 1) { // Verify retry of onSchedule and onUnSchedule
calls
Review comment:
```return result.second > 1```
?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
