[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest

2020-08-06 Thread GitBox 


fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r466255118



##
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##
@@ -79,6 +79,17 @@ Flow Controller:
 Remote Processing Groups:
 )";
 
+template
+bool verifyWithBusyWait(std::chrono::milliseconds timeout, Fn&& fn) {
+  auto start = std::chrono::steady_clock::now();
+  while 
(std::chrono::duration_cast(std::chrono::steady_clock::now()
 - start) < timeout) {

Review comment:
   minor, but I don't think you need the `duration_cast`: durations in 
different units can be compared





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest

2020-08-05 Thread GitBox 


fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465615443



##
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", 
"[TestFlow2]") {
   testController.startFlow();
 
   // wait for the source processor to enqueue its flowFiles
-  int tryCount = 0;
-  while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
-std::this_thread::sleep_for(std::chrono::milliseconds{20});
-  }
+  busy_wait(std::chrono::milliseconds{50}, [&] {return 
root->getTotalFlowFileCount() != 0;});

Review comment:
   @adebreceni It wasn't really a proposal, but I would prefer two separate 
functions `verifyEventHappenedInPollTime()` and 
`verifyEventHappenedInPollTimeWithBusyWait()` (just an example, feel free to 
find a shorter name) rather than a single function where an argument value of 
-1 means "use the busy version".
   
   [TBH, I am not a fan of the `verifyEventHappenedInPollTime` name, as 
something like `REQUIRE(happensBeforeTimeout(...))` would read better, but 
renaming it is probably not in scope 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:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest

2020-08-04 Thread GitBox 


fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465088044



##
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", 
"[TestFlow2]") {
   testController.startFlow();
 
   // wait for the source processor to enqueue its flowFiles
-  int tryCount = 0;
-  while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
-std::this_thread::sleep_for(std::chrono::milliseconds{20});
-  }
+  busy_wait(std::chrono::milliseconds{50}, [&] {return 
root->getTotalFlowFileCount() != 0;});

Review comment:
   I would prefer a separate additional function (possibly with both the 
old and the new function calling a common 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:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest

2020-08-04 Thread GitBox 


fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465088044



##
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", 
"[TestFlow2]") {
   testController.startFlow();
 
   // wait for the source processor to enqueue its flowFiles
-  int tryCount = 0;
-  while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
-std::this_thread::sleep_for(std::chrono::milliseconds{20});
-  }
+  busy_wait(std::chrono::milliseconds{50}, [&] {return 
root->getTotalFlowFileCount() != 0;});

Review comment:
   I would prefer a function without a timeout parameter (possibly with 
both the timeout-y and timeout-less version calling a common 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:
us...@infra.apache.org

[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #861: MINIFICPP-1312 - Fix flaky FlowControllerTest

2020-08-04 Thread GitBox 


fgerlits commented on a change in pull request #861:
URL: https://github.com/apache/nifi-minifi-cpp/pull/861#discussion_r465013502



##
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", 
"[TestFlow2]") {
   testController.startFlow();
 
   // wait for the source processor to enqueue its flowFiles
-  int tryCount = 0;
-  while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
-std::this_thread::sleep_for(std::chrono::milliseconds{20});
-  }
+  busy_wait(std::chrono::milliseconds{50}, [&] {return 
root->getTotalFlowFileCount() != 0;});
 
-  REQUIRE(root->getTotalFlowFileCount() == 3);
+  REQUIRE(root->getTotalFlowFileCount() != 0);

Review comment:
   why do we need this REQUIRE? `busy_wait` (or 
`verifyEventHappenedInPollTime` if you change to that) already checked this 
condition in the previous line

##
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##
@@ -215,20 +220,19 @@ TEST_CASE("Extend the waiting period during shutdown", 
"[TestFlow4]") {
   testController.startFlow();
 
   // wait for the source processor to enqueue its flowFiles
-  int tryCount = 0;
-  while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
-std::this_thread::sleep_for(std::chrono::milliseconds{20});
-  }
+  busy_wait(std::chrono::milliseconds{50}, [&] {return 
root->getTotalFlowFileCount() != 0;});
 
-  REQUIRE(root->getTotalFlowFileCount() == 3);
+  REQUIRE(root->getTotalFlowFileCount() != 0);
   REQUIRE(sourceProc->trigger_count.load() == 1);
 
   std::thread shutdownThread([&]{
 execSinkPromise.set_value();
 controller->stop(true);
   });
 
-  while (controller->isRunning()) {
+  int extendCount = 0;
+  while (extendCount++ < 5 && controller->isRunning()) {
+testController.logger_->log_info("Controller still running, extend the 
waiting period, ff count: %d", static_cast(root->getTotalFlowFileCount()));

Review comment:
   it would be useful to include the value of `extendCount ` in this log

##
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", 
"[TestFlow2]") {
   testController.startFlow();
 
   // wait for the source processor to enqueue its flowFiles
-  int tryCount = 0;
-  while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
-std::this_thread::sleep_for(std::chrono::milliseconds{20});
-  }
+  busy_wait(std::chrono::milliseconds{50}, [&] {return 
root->getTotalFlowFileCount() != 0;});

Review comment:
   You could use `verifyEventHappenedInPollTime` in IntegrationTestUtils.h, 
it is almost the same as `busy_wait`.
   
   ```suggestion
 REQUIRE(verifyEventHappenedInPollTime(std::chrono::milliseconds{50}, [&] 
{return root->getTotalFlowFileCount() != 0;}));
   ```

##
File path: libminifi/test/flow-tests/FlowControllerTests.cpp
##
@@ -133,12 +144,9 @@ TEST_CASE("Flow shutdown waits for a while", 
"[TestFlow2]") {
   testController.startFlow();
 
   // wait for the source processor to enqueue its flowFiles
-  int tryCount = 0;
-  while (tryCount++ < 10 && root->getTotalFlowFileCount() != 3) {
-std::this_thread::sleep_for(std::chrono::milliseconds{20});
-  }
+  busy_wait(std::chrono::milliseconds{50}, [&] {return 
root->getTotalFlowFileCount() != 0;});

Review comment:
   Is there no way to avoid changing `== 3` to `!= 0`?  The test condition 
is much weaker now.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org