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