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:
[email protected]
