You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/07/08 10:54:06 UTC
[GitHub] [nifi-minifi-cpp] hunyadi-dev opened a new pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
hunyadi-dev opened a new pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835
**Decreases the avg. test execution time by 57% (from 326 seconds to 139 seconds).**
Measurements on test times of master (on MacBook Pro 16-inch, 2019):
```
➜ for repetition in {1..5}; do; time ninja test 2>&1 >> test_logs.txt; done;
ninja test 2>&1 >> test_logs.txt 46.05s user 19.32s system 18% cpu 5:58.81 total
ninja test 2>&1 >> test_logs.txt 45.81s user 13.33s system 18% cpu 5:16.95 total
ninja test 2>&1 >> test_logs.txt 46.10s user 13.22s system 18% cpu 5:18.84 total
ninja test 2>&1 >> test_logs.txt 46.08s user 13.39s system 18% cpu 5:17.21 total
ninja test 2>&1 >> test_logs.txt 46.16s user 13.51s system 18% cpu 5:16.61 total
➜ grep "Total Test time" test_logs.txt
Total Test time (real) = 358.74 sec
Total Test time (real) = 316.88 sec
Total Test time (real) = 318.78 sec
Total Test time (real) = 317.14 sec
Total Test time (real) = 316.57 sec
```
On the source branch of this PR:
```
➜ for repetition in {1..5}; do; time ninja test 2>&1 >> test_logs.txt; done;
ninja test 2>&1 >> test_logs.txt 24.04s user 9.62s system 24% cpu 2:19.84 total
ninja test 2>&1 >> test_logs.txt 23.98s user 9.17s system 24% cpu 2:17.74 total
ninja test 2>&1 >> test_logs.txt 23.89s user 9.44s system 24% cpu 2:18.17 total
ninja test 2>&1 >> test_logs.txt 23.82s user 9.53s system 24% cpu 2:18.72 total
ninja test 2>&1 >> test_logs.txt 24.04s user 9.56s system 23% cpu 2:20.81 total
➜ grep "Total Test time" test_logs.txt
Total Test time (real) = 139.77 sec
Total Test time (real) = 137.68 sec
Total Test time (real) = 138.10 sec
Total Test time (real) = 138.66 sec
Total Test time (real) = 140.75 sec
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r453682665
##########
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
Review comment:
what is the gcc issue here? https://godbolt.org/z/hbMvss compiles fine on gcc 4.8.5 (and even on 4.7.4), at least in this sandbox
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458830746
##########
File path: extensions/http-curl/tests/VerifyInvokeHTTPTest.cpp
##########
@@ -183,7 +193,7 @@ int main(int argc, char ** argv) {
}
{
- TimeoutingHTTPHandler handler({std::chrono::seconds(4)});
+ TimeoutingHTTPHandler handler({std::chrono::milliseconds(2000)});
Review comment:
Changed to 2 seconds.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r456372190
##########
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:
You may want to use [`std::not1`](https://en.cppreference.com/w/cpp/utility/functional/not1) (<=C++17)/[`std::not_fn`](https://en.cppreference.com/w/cpp/utility/functional/not_fn) (>=C++17) for the second test after extracting the lambda.
edit: outdated
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r457270674
##########
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:
No longer applicable.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458082474
##########
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:
Updated according to the suggestion, I did not realize that the suggestion is actually combining the return statements. I completely agree that it is the sane thing to do, I do not know how I missed it :D
Updated the PublishKafkaOnScheduleTests as well that follows a similar pattern.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463583784
##########
File path: extensions/standard-processors/tests/integration/TailFileTest.cpp
##########
@@ -69,9 +70,11 @@ class TailFileTestHarness : public IntegrationBase {
}
void runAssertions() override {
- assert(LogTestController::getInstance().contains("5 flowfiles were received from TailFile input") == true);
- assert(LogTestController::getInstance().contains("Looking for delimiter 0xA") == true);
- assert(LogTestController::getInstance().contains("li\\ne5") == true);
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
I agree that having this function in our namespace is good, but in this case the `26` function level `using`s make me question whether we should drop a `using` at the top of the file, or go with the practice of having test utils at the top level
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463595314
##########
File path: extensions/standard-processors/tests/integration/TailFileTest.cpp
##########
@@ -69,9 +70,11 @@ class TailFileTestHarness : public IntegrationBase {
}
void runAssertions() override {
- assert(LogTestController::getInstance().contains("5 flowfiles were received from TailFile input") == true);
- assert(LogTestController::getInstance().contains("Looking for delimiter 0xA") == true);
- assert(LogTestController::getInstance().contains("li\\ne5") == true);
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
I don't see any harm in having multiple using statements on demand, but if you like, I can move this into the integration testharness base class.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r451599406
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
Can the sleep go into `waitToVerifyProcessor()` and `runAssertion()` be empty in this case? I think that would say in code what you say in the comment above.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458821992
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,64 @@
+/**
+ *
+ * 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) {
+ // gcc before 4.9 does not support capturing parameter packs in lambdas: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41933
+ // Once we support gcc >= 4.9 only, this vector will no longer be necessary, we'll be able to iterate on the parameter pack directly.
+ std::vector<std::string> pattern_list{std::forward<String>(patterns)...};
Review comment:
I prefer this name as it clearly indicates, that we are periodically polling for events and not just handling signals. Also, I am not completely convinced, but initializer lists would also copy the argument strings in order to construct a vector out of them and adding a workaround later is less convenient in that case. I don't think it is worth the effort to change the implementation.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458089850
##########
File path: extensions/http-curl/tests/C2UpdateTest.cpp
##########
@@ -174,15 +174,19 @@ int main(int argc, char **argv) {
controller->load();
controller->start();
- waitToVerifyProcessor();
+
+ const bool test_success = verifyEventHappenedInPollTime(std::chrono::seconds(10), [] {
+ std::string logs = LogTestController::getInstance().log_output.str();
+ return logs.find("Starting to reload Flow Controller with flow control name MiNiFi Flow, version") != std::string::npos;
+ });
Review comment:
I think the final version does just that. You probably reviewed an earlier iteration, where this function did not exist yet, but I think I went and replaced all similar occurences.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r455554339
##########
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
Review comment:
Sure, I will improve the comment with details :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458822571
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,64 @@
+/**
+ *
+ * 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) {
+ // gcc before 4.9 does not support capturing parameter packs in lambdas: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41933
+ // Once we support gcc >= 4.9 only, this vector will no longer be necessary, we'll be able to iterate on the parameter pack directly.
+ std::vector<std::string> pattern_list{std::forward<String>(patterns)...};
+ auto check = [&] {
+ const std::string logs = LogTestController::getInstance().log_output.str();
+ if (std::all_of(pattern_list.cbegin(), pattern_list.cend(), [&logs] (const std::string& pattern) { return logs.find(pattern) != std::string::npos; })) {
+ return true;
+ }
+ return false;
Review comment:
Yep, good idea.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r457948107
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,68 @@
+/**
+ *
+ * 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 function contains an extra copy on the parameter pack unpacking
+ // However, since we gcc 4.9 does not support forwarding parameter packs we are pretty much forced to do this
+ // 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)...};
Review comment:
```suggestion
bool verifyLogLinePresenceInPollTime(const std::chrono::duration<Rep, Period>& wait_duration, String&&... patterns) {
// gcc before 4.9 does not support capturing parameter packs in lambdas: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41933
// Once we support gcc >= 4.9 only, this vector will no longer be necessary, we'll be able to iterate on the parameter pack directly.
std::vector<std::string> pattern_list{std::forward<String>(patterns)...};
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458844014
##########
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:
Maybe you misunderstand what is going on here. It is not my implementation that suddenly started to misuse the tesrunner, it was misused before as well. This change just made it glaringly obvious, which is I think a good thing to say the least.
I am fine with someone having this refactored properly, I just do not want to do the extra work of reorganizing the structure for a test I do not even knows what it does. As for `waitToVerifyProcessors` it was clearly a poor decision to have this function lying around with a generic implementation forcing to wait pretty much all the tests.
Ideally, one could go and?
1. decouple the assertions from the test-runners if it is not the testrunners responsibility to run them **OR**
1. move them inside and don't just use a testrunner for its byproduct of setting up some objects.
I would go with the latter.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r459973870
##########
File path: extensions/librdkafka/tests/PublishKafkaOnScheduleTests.cpp
##########
@@ -23,26 +23,36 @@
#include "../../../libminifi/test/TestBase.h"
#include "../PublishKafka.h"
#include "utils/StringUtils.h"
+#include "utils/IntegrationTestUtils.h"
class PublishKafkaOnScheduleTests : public IntegrationBase {
public:
virtual void runAssertions() {
- std::string logs = LogTestController::getInstance().log_output.str();
-
- auto result = utils::StringUtils::countOccurrences(logs, "value 1 is outside allowed range 1000..1000000000");
- size_t last_pos = result.first;
- int occurrences = result.second;
-
- assert(occurrences > 1); // Verify retry of onSchedule and onUnSchedule calls
-
- std::vector<std::string> must_appear_byorder_msgs = {"notifyStop called",
- "Successfully configured PublishKafka",
- "PublishKafka onTrigger"};
+ 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 = utils::StringUtils::countOccurrences(logs, "value 1 is outside allowed range 1000..1000000000");
+ const int occurrences = result.second;
+ return 1 < occurrences;
+ }));
+ flowController_->updatePropertyValue("kafka", minifi::processors::PublishKafka::MaxMessageSize.getName(), "1999");
Review comment:
What's the reason for the change of MaxMessageSize?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r451561893
##########
File path: extensions/http-curl/tests/VerifyInvokeHTTPTest.cpp
##########
@@ -93,47 +95,55 @@ class VerifyInvokeHTTP : public CoapIntegrationBase {
flowController_->unload();
flowController_->stopC2();
- runAssertions();
cleanup();
}
};
class VerifyInvokeHTTPOKResponse : public VerifyInvokeHTTP {
-public:
- virtual void runAssertions() override {
- assert(LogTestController::getInstance().contains("key:invokehttp.status.code value:201"));
- assert(LogTestController::getInstance().contains("response code 201"));
+ public:
+ void runAssertions() override {
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
+ assert(verifyLogLinePresenceInPollTime(std::chrono::seconds(6),
Review comment:
This is a maximum value, and the test would probably run for far less time. `contains` was running for 3 seconds, but the `waitForVerifyProcessor` added an extra 6 seconds (line 35 of this file). I went with keeping the flat 6 seconds, because this is seems like the time frame these logs should have been produced, and not the 3 seconds the log checking introduced.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r454463047
##########
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:
I like naming this occurences, it will probably get optimized anyway, but I wouldn't know what it means otherwise. Also I almost always prefer relation operators `<` and `<=` to `>` and `>=`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r454491636
##########
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
Review comment:
Sorry for being dense, but if I find this comment later, after we have upgraded to gcc >= 4.9 only, I'll have no idea how I am supposed to rewrite it. Maybe include the better (post gcc-4.9) version of the code in the comment?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r455547307
##########
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:
Did extract them out into a single lambda function.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458095121
##########
File path: extensions/http-curl/tests/ControllerServiceIntegrationTests.cpp
##########
@@ -15,6 +15,23 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+/**
+ *
+ * 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.
+ */
Review comment:
Removed the duplicate.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r456369013
##########
File path: extensions/http-curl/tests/C2UpdateTest.cpp
##########
@@ -174,15 +174,19 @@ int main(int argc, char **argv) {
controller->load();
controller->start();
- waitToVerifyProcessor();
+
+ const bool test_success = verifyEventHappenedInPollTime(std::chrono::seconds(10), [] {
+ std::string logs = LogTestController::getInstance().log_output.str();
+ return logs.find("Starting to reload Flow Controller with flow control name MiNiFi Flow, version") != std::string::npos;
+ });
Review comment:
why not `verifyLogLinePresenceInPollTime` here?
##########
File path: extensions/http-curl/tests/C2JstackTest.cpp
##########
@@ -29,7 +30,8 @@ class VerifyC2DescribeJstack : public VerifyC2Describe {
}
virtual void runAssertions() {
- assert(LogTestController::getInstance().contains("SchedulingAgent"));
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
+ assert(verifyLogLinePresenceInPollTime(std::chrono::milliseconds(wait_time_), "SchedulingAgent"));
Review comment:
We didn't have to wait before. Why do we allow wait time now?
##########
File path: extensions/http-curl/tests/C2UpdateAgentTest.cpp
##########
@@ -34,6 +34,6 @@ int main(int argc, char **argv) {
const auto then = std::chrono::system_clock::now();
const auto seconds = std::chrono::duration_cast<std::chrono::seconds>(then - start).count();
- assert(handler.calls_ <= (seconds) + 1);
+ assert(handler.calls_ <= (seconds) + 2);
Review comment:
What's the reason for this change?
##########
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:
You may want to use [`std::not1`](https://en.cppreference.com/w/cpp/utility/functional/not1) (<=C++17)/[`std::not_fn`](https://en.cppreference.com/w/cpp/utility/functional/not_fn) (>=C++17) for the second test after extracting the lambda.
##########
File path: extensions/http-curl/tests/ControllerServiceIntegrationTests.cpp
##########
@@ -15,6 +15,23 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+/**
+ *
+ * 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.
+ */
Review comment:
Duplicate license header
##########
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:
I agree that this sleep shouldn't be part of `runAssertions`.
The cure is worse than the disease, IMO. We shouldn't abuse `runAssertions` to prevent the abuse of `waitToVerifyProcessors`.
##########
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:
You can usually choose to use `<`/`<=` without changing the semantics. But why does it matter, except when writing generic code that must only depend on `operator<`?
What about this?
```
const int occurrences = result.second;
return 1 < occurrences;
```
Or possibly change `countOccurrences` to return a struct instead of a pair.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463640905
##########
File path: extensions/standard-processors/tests/integration/TailFileTest.cpp
##########
@@ -69,9 +70,11 @@ class TailFileTestHarness : public IntegrationBase {
}
void runAssertions() override {
- assert(LogTestController::getInstance().contains("5 flowfiles were received from TailFile input") == true);
- assert(LogTestController::getInstance().contains("Looking for delimiter 0xA") == true);
- assert(LogTestController::getInstance().contains("li\\ne5") == true);
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
feel free to mark this as resolved
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r460706666
##########
File path: extensions/http-curl/tests/C2UpdateAgentTest.cpp
##########
@@ -34,6 +34,6 @@ int main(int argc, char **argv) {
const auto then = std::chrono::system_clock::now();
const auto seconds = std::chrono::duration_cast<std::chrono::seconds>(then - start).count();
- assert(handler.calls_ <= (seconds) + 1);
+ assert(handler.calls_ <= (seconds) + 2);
Review comment:
One of the CI jobs of @fgerlits had this issue earlier as well: https://travis-ci.org/github/apache/nifi-minifi-cpp/jobs/711468287
Seems like it is indeed the case that we had this error before, but masked it away with the long waiting.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463533330
##########
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:
See the Jira created here:
https://issues.apache.org/jira/secure/RapidBoard.jspa?rapidView=405&view=detail&selectedIssue=MINIFICPP-1311
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463608524
##########
File path: extensions/standard-processors/tests/integration/TailFileTest.cpp
##########
@@ -69,9 +70,11 @@ class TailFileTestHarness : public IntegrationBase {
}
void runAssertions() override {
- assert(LogTestController::getInstance().contains("5 flowfiles were received from TailFile input") == true);
- assert(LogTestController::getInstance().contains("Looking for delimiter 0xA") == true);
- assert(LogTestController::getInstance().contains("li\\ne5") == true);
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
I don't think it is rebased to `main`, keep it open until the CI finishes
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r451597749
##########
File path: extensions/http-curl/tests/VerifyInvokeHTTPTest.cpp
##########
@@ -93,47 +95,55 @@ class VerifyInvokeHTTP : public CoapIntegrationBase {
flowController_->unload();
flowController_->stopC2();
- runAssertions();
cleanup();
}
};
class VerifyInvokeHTTPOKResponse : public VerifyInvokeHTTP {
-public:
- virtual void runAssertions() override {
- assert(LogTestController::getInstance().contains("key:invokehttp.status.code value:201"));
- assert(LogTestController::getInstance().contains("response code 201"));
+ public:
+ void runAssertions() override {
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
+ assert(verifyLogLinePresenceInPollTime(std::chrono::seconds(6),
Review comment:
👍
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r452128161
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
I do not want to bring it back. The model this test follows is a bad one, using a testharness not to actually run tests but just to set up a `FlowController` and block until other assertions happen. A good refactor would be to rewrite the logic used here, but that would have been too big of a change for this PR. This comment is a good indicator of what is actually going on (`runAssertions` only waits for assertions to be run).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458093833
##########
File path: extensions/http-curl/tests/ControllerServiceIntegrationTests.cpp
##########
@@ -15,6 +15,23 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+/**
+ *
+ * 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.
+ */
Review comment:
Good catch, I don't know how this happened. I do not remembere adding any license headers, maybe I messed this up on rebasing.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458096075
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,68 @@
+/**
+ *
+ * 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 function contains an extra copy on the parameter pack unpacking
+ // However, since we gcc 4.9 does not support forwarding parameter packs we are pretty much forced to do this
+ // 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)...};
Review comment:
Suggestion applied.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458823883
##########
File path: extensions/http-curl/tests/VerifyInvokeHTTPTest.cpp
##########
@@ -183,7 +193,7 @@ int main(int argc, char ** argv) {
}
{
- TimeoutingHTTPHandler handler({std::chrono::seconds(4)});
+ TimeoutingHTTPHandler handler({std::chrono::milliseconds(2000)});
Review comment:
:tashikani:
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r452101640
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
That's because you have just deleted it :)
My suggestion was to keep it, and put waiting code in `waitToVerifyProcessor()` and assertion code in `runAssertion()`. I don't insist on this, but I think it would make the code clearer, and some of these comments would not be necessary.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r455551303
##########
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 don't think we have a test namespace. Some test utility are in `utils` and some are (eg. our whole test-plan) in global scope.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r457270213
##########
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
Review comment:
Updated the comment.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458691107
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,64 @@
+/**
+ *
+ * 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) {
+ // gcc before 4.9 does not support capturing parameter packs in lambdas: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41933
+ // Once we support gcc >= 4.9 only, this vector will no longer be necessary, we'll be able to iterate on the parameter pack directly.
+ std::vector<std::string> pattern_list{std::forward<String>(patterns)...};
Review comment:
Thinking about it, I don't see the reason for using variadic templates over a simple `std::vector<std::string>`.
I would also suggest a shorter name: "wait for log messages", and the first parameter name would become "timeout". Sounds more natural IMO and follows the STL convention of "wait for" meaning waiting for a duration/timeout.
usage: `waitForLogMessages(2s, {"started Example", "stopped Example"});`
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,64 @@
+/**
+ *
+ * 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) {
+ // gcc before 4.9 does not support capturing parameter packs in lambdas: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41933
+ // Once we support gcc >= 4.9 only, this vector will no longer be necessary, we'll be able to iterate on the parameter pack directly.
+ std::vector<std::string> pattern_list{std::forward<String>(patterns)...};
+ auto check = [&] {
+ const std::string logs = LogTestController::getInstance().log_output.str();
+ if (std::all_of(pattern_list.cbegin(), pattern_list.cend(), [&logs] (const std::string& pattern) { return logs.find(pattern) != std::string::npos; })) {
+ return true;
+ }
+ return false;
Review comment:
```suggestion
return std::all_of(pattern_list.cbegin(), pattern_list.cend(), [&logs] (const std::string& pattern) { return logs.find(pattern) != std::string::npos; });
```
If you squint your eyes a bit, it almost reads as "return whether all [elements] of pattern_list are in the logs".
Another implementation idea as an example to the other function.
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,64 @@
+/**
+ *
+ * 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) {
Review comment:
Suggesting a new name here as well: "wait for condition".
Example usage:
```
waitForCondition(timeout, []{
const auto can_be_found_in = [](const std::string& haystack) { return [&haystack](const std::string& needle) { return StringUtils::contains(haystack, needle); }; };
const auto logs = ...;
return std::all_of(std::cbegin(messages), std::cend(messages), can_be_found_in(logs));
});
```
##########
File path: extensions/http-curl/tests/VerifyInvokeHTTPTest.cpp
##########
@@ -183,7 +193,7 @@ int main(int argc, char ** argv) {
}
{
- TimeoutingHTTPHandler handler({std::chrono::seconds(4)});
+ TimeoutingHTTPHandler handler({std::chrono::milliseconds(2000)});
Review comment:
That would be `std::chrono::seconds{2}`
##########
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:
I intend to reopen this discussion. IMO, the root of the problem is that test utilities are in the utils directory instead of somewhere under libminifi/test/.
The same applies to TestUtils.h, but it's not related to this PR.
##########
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:
linking related discussion: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r452132951
3/3 reviewers were against the misuse of `runAssertions`. Please reconsider bringing back the old function (without the widespread usage, of course) or find another solution that doesn't misuse `runAssertions`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r455548728
##########
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:
Because we don't want to bring that function back, because it would be abused again. It would also probably wait at an incorrect place with this PR-s design. Hope we can spare these declarations after rebasing on Adam D.'s PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r452001585
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
We no longer have `waitToVerifyProcessor()` though...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r452101640
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
That's because you have just deleted it :)
My suggestion was to keep it, and put waiting code in `waitToVerifyProcessor()` and assertion code in `runAssertions()`. I don't insist on this, but I think it would make the code clearer, and some of these comments would not be necessary.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r455551303
##########
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 don't think we have a test namespace. Some test utility are in `utils` and some are in global scope.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r455547307
##########
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:
Will extract them out into a single lambda function.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r455554004
##########
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:
Yes, most of the time it is, but now always. There other places includes happen like this, might worth the investigation why, but I don't think it is relevant for this PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r452132951
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
ok
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r451558272
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
It is the same as before. Maybe I can rename `runAssertions` to something more appropriate, if you have suggestions.
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
It is the same as before. Maybe I can rename `runAssertions` globally to something more appropriate, if you have suggestions.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463604597
##########
File path: extensions/standard-processors/tests/integration/TailFileTest.cpp
##########
@@ -69,9 +70,11 @@ class TailFileTestHarness : public IntegrationBase {
}
void runAssertions() override {
- assert(LogTestController::getInstance().contains("5 flowfiles were received from TailFile input") == true);
- assert(LogTestController::getInstance().contains("Looking for delimiter 0xA") == true);
- assert(LogTestController::getInstance().contains("li\\ne5") == true);
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
I counterpropose: This PR has been waiting for merge for quite a long time, we should finally merge it unless there isa major flaw to be found in it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458087051
##########
File path: extensions/http-curl/tests/C2JstackTest.cpp
##########
@@ -29,7 +30,8 @@ class VerifyC2DescribeJstack : public VerifyC2Describe {
}
virtual void runAssertions() {
- assert(LogTestController::getInstance().contains("SchedulingAgent"));
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
+ assert(verifyLogLinePresenceInPollTime(std::chrono::milliseconds(wait_time_), "SchedulingAgent"));
Review comment:
We actually did. The implementation changed for `libminifi/test/integration/IntegrationBase.h`. In addition, `contains` actually used to wait a bit as well.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463527871
##########
File path: extensions/librdkafka/tests/PublishKafkaOnScheduleTests.cpp
##########
@@ -23,26 +23,36 @@
#include "../../../libminifi/test/TestBase.h"
#include "../PublishKafka.h"
#include "utils/StringUtils.h"
+#include "utils/IntegrationTestUtils.h"
class PublishKafkaOnScheduleTests : public IntegrationBase {
public:
virtual void runAssertions() {
- std::string logs = LogTestController::getInstance().log_output.str();
-
- auto result = utils::StringUtils::countOccurrences(logs, "value 1 is outside allowed range 1000..1000000000");
- size_t last_pos = result.first;
- int occurrences = result.second;
-
- assert(occurrences > 1); // Verify retry of onSchedule and onUnSchedule calls
-
- std::vector<std::string> must_appear_byorder_msgs = {"notifyStop called",
- "Successfully configured PublishKafka",
- "PublishKafka onTrigger"};
+ 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 = utils::StringUtils::countOccurrences(logs, "value 1 is outside allowed range 1000..1000000000");
+ const int occurrences = result.second;
+ return 1 < occurrences;
+ }));
+ flowController_->updatePropertyValue("kafka", minifi::processors::PublishKafka::MaxMessageSize.getName(), "1999");
Review comment:
There is no change, it was previously in the `waitToVerifyProcessor()` on line 57.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r454491636
##########
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
Review comment:
Sorry for being dense, but if I find this comment later, after we have upgraded to gcc >= 4.9 only, I'll have no idea how I am supposed to rewrite the code. Maybe include the better (post gcc-4.9) version of the code in the comment?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463528023
##########
File path: extensions/librdkafka/tests/PublishKafkaOnScheduleTests.cpp
##########
@@ -23,26 +23,36 @@
#include "../../../libminifi/test/TestBase.h"
#include "../PublishKafka.h"
#include "utils/StringUtils.h"
+#include "utils/IntegrationTestUtils.h"
class PublishKafkaOnScheduleTests : public IntegrationBase {
public:
virtual void runAssertions() {
- std::string logs = LogTestController::getInstance().log_output.str();
-
- auto result = utils::StringUtils::countOccurrences(logs, "value 1 is outside allowed range 1000..1000000000");
- size_t last_pos = result.first;
- int occurrences = result.second;
-
- assert(occurrences > 1); // Verify retry of onSchedule and onUnSchedule calls
-
- std::vector<std::string> must_appear_byorder_msgs = {"notifyStop called",
- "Successfully configured PublishKafka",
- "PublishKafka onTrigger"};
+ 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 = utils::StringUtils::countOccurrences(logs, "value 1 is outside allowed range 1000..1000000000");
+ const int occurrences = result.second;
+ return 1 < occurrences;
+ }));
+ flowController_->updatePropertyValue("kafka", minifi::processors::PublishKafka::MaxMessageSize.getName(), "1999");
Review comment:
At least I hope there is none :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463608524
##########
File path: extensions/standard-processors/tests/integration/TailFileTest.cpp
##########
@@ -69,9 +70,11 @@ class TailFileTestHarness : public IntegrationBase {
}
void runAssertions() override {
- assert(LogTestController::getInstance().contains("5 flowfiles were received from TailFile input") == true);
- assert(LogTestController::getInstance().contains("Looking for delimiter 0xA") == true);
- assert(LogTestController::getInstance().contains("li\\ne5") == true);
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
I don't think it is rebased to `main`, keep it open until the CI is finished
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458822826
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,64 @@
+/**
+ *
+ * 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) {
Review comment:
As above.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458078586
##########
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:
Changed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r454459865
##########
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:
Yup, good idea.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r451559061
##########
File path: libminifi/test/0-9
##########
@@ -0,0 +1 @@
+C API raNdOMcaSe test d4t4 th1s is!
Review comment:
Oh, thanks for spotting it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463610959
##########
File path: extensions/standard-processors/tests/integration/TailFileTest.cpp
##########
@@ -69,9 +70,11 @@ class TailFileTestHarness : public IntegrationBase {
}
void runAssertions() override {
- assert(LogTestController::getInstance().contains("5 flowfiles were received from TailFile input") == true);
- assert(LogTestController::getInstance().contains("Looking for delimiter 0xA") == true);
- assert(LogTestController::getInstance().contains("li\\ne5") == true);
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
There is no conflict with main and all the jobs passed on Travis and Appveyor except from OSX 10.14.4. There is no compelling reason to have the GH Actions checks run them.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458091803
##########
File path: extensions/http-curl/tests/C2UpdateAgentTest.cpp
##########
@@ -34,6 +34,6 @@ int main(int argc, char **argv) {
const auto then = std::chrono::system_clock::now();
const auto seconds = std::chrono::duration_cast<std::chrono::seconds>(then - start).count();
- assert(handler.calls_ <= (seconds) + 1);
+ assert(handler.calls_ <= (seconds) + 2);
Review comment:
Some libcurl black magic we do not understand. I could not figure it out but shared the problem with a group of contributors on Slack. I verified the logs and the events happening are what we expect, but curl immediately triggers the POST handle on the Civet Server twice. It was probably an issue beforehand masked by the long waiting time.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r452128161
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
I do not want to bring it back. The model this test follows is a bad one, using a testharness not to actually run tests but just to set up a `FlowController` and block until other assertions happen. A good refactor would be to rewrite the logic used here, but that would have been too big of a change for this PR.
The added comment should a good indicator of what is actually going on (`runAssertions` only waits for assertions to be run).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r463602029
##########
File path: extensions/standard-processors/tests/integration/TailFileTest.cpp
##########
@@ -69,9 +70,11 @@ class TailFileTestHarness : public IntegrationBase {
}
void runAssertions() override {
- assert(LogTestController::getInstance().contains("5 flowfiles were received from TailFile input") == true);
- assert(LogTestController::getInstance().contains("Looking for delimiter 0xA") == true);
- assert(LogTestController::getInstance().contains("li\\ne5") == true);
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
Review comment:
I propose we leave this comment open a little and see if others have a preference, if nobody is for or against, we could leave it as is
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r451477045
##########
File path: extensions/http-curl/tests/HTTPIntegrationBase.h
##########
@@ -135,6 +135,9 @@ class VerifyC2Describe : public VerifyC2Base {
}
void runAssertions() override {
+ // This class is never used for running assertions, but we are forced to wait for DescribeManifestHandler to verifyJsonHasAgentManifest
+ // if we were to log something on finished verification, we could poll on finding it
+ std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_));
Review comment:
A `runAssertions()` function that doesn't run assertions looks weird (as you clearly felt, too, hence the comment). It may be better to keep `waitToVerifyProcessor()` and `runAssertions()` separate, but make their default implementations no-op.
##########
File path: libminifi/test/0-9
##########
@@ -0,0 +1 @@
+C API raNdOMcaSe test d4t4 th1s is!
Review comment:
garbage left over by a test run
##########
File path: libminifi/include/utils/IntegrationTestUtils.h
##########
@@ -0,0 +1,65 @@
+/**
+ *
+ * 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 "../../../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) {
Review comment:
You could replace the `becomesTrueWithinTimeout()` calls in `libminifi/test/unit/MinifiConcurrentQueueTests.cpp` with calls to this function, or make `becomesTrueWithinTimeout()` call `verifyEventHappenedInPollTime()`, as they are almost identical.
##########
File path: extensions/http-curl/tests/VerifyInvokeHTTPTest.cpp
##########
@@ -93,47 +95,55 @@ class VerifyInvokeHTTP : public CoapIntegrationBase {
flowController_->unload();
flowController_->stopC2();
- runAssertions();
cleanup();
}
};
class VerifyInvokeHTTPOKResponse : public VerifyInvokeHTTP {
-public:
- virtual void runAssertions() override {
- assert(LogTestController::getInstance().contains("key:invokehttp.status.code value:201"));
- assert(LogTestController::getInstance().contains("response code 201"));
+ public:
+ void runAssertions() override {
+ using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
+ assert(verifyLogLinePresenceInPollTime(std::chrono::seconds(6),
Review comment:
Why did the timeouts in this test increase to 6 seconds (from the default 3 seconds which they were before)?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r458844014
##########
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:
Maybe you misunderstand what is going on here. It is not my implementation that suddenly started to misuse the tesrunner, it was misused before as well. This change just made it glaringly obvious, which is I think a good thing to say the least.
I am fine with someone having this refactored properly, I just do not want to do the extra work of reorganizing the structure for a test I do not even knows what it does. As for `waitToVerifyProcessors` it was clearly a poor decision to have this function lying around with a generic implementation forcing to wait pretty much all the tests.
Ideally, one could go and:
1. decouple the assertions from the test-runners if it is not the testrunners responsibility to run them **OR**
1. move them inside and don't just use a testrunner for its byproduct of setting up some objects.
I think the latter is a better approach, but it is up for whoever decides to spend time on the refactor.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #835: MINIFICPP-1281 - Improve test performance by using event polling instead of sleep by sync
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #835:
URL: https://github.com/apache/nifi-minifi-cpp/pull/835#discussion_r454460253
##########
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
Review comment:
Ideally you should not need to unpact the arguments into a new vector. However, unpacking does not properly work on gcc 4.8.5 when defining a variadic check.
----------------------------------------------------------------
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:
users@infra.apache.org