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 2021/02/18 18:30:51 UTC

[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1009: MINIFICPP-1497: Remove misleading ThreadPoolAdjust integration test

martinzink commented on a change in pull request #1009:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1009#discussion_r578654097



##########
File path: extensions/http-curl/tests/CMakeLists.txt
##########
@@ -95,7 +95,6 @@ add_test(NAME HTTPSiteToSiteTests COMMAND HTTPSiteToSiteTests "${TEST_RESOURCES}
 add_test(NAME TimeoutHTTPSiteToSiteTests COMMAND TimeoutHTTPSiteToSiteTests "${TEST_RESOURCES}/TestTimeoutHTTPSiteToSite.yml" "${TEST_RESOURCES}/" "http://localhost:8098/nifi-api")
 add_test(NAME SiteToSiteRestTest COMMAND SiteToSiteRestTest "${TEST_RESOURCES}/TestSite2SiteRest.yml" "${TEST_RESOURCES}/" "http://localhost:8077/nifi-api/site-to-site")
 add_test(NAME ControllerServiceIntegrationTests COMMAND ControllerServiceIntegrationTests "${TEST_RESOURCES}/TestControllerServices.yml" "${TEST_RESOURCES}/")
-add_test(NAME ThreadPoolAdjust COMMAND ThreadPoolAdjust "${TEST_RESOURCES}/ThreadPoolAdjust.yml" "${TEST_RESOURCES}/")

Review comment:
       Based on the name ThreadPoolAdjust, it is implied it tests the adjustments to the thread-pool or something along those lines, however extensions/http-curl/tests/ThreadPoolAdjust.cpp is just a copy of extensions/http-curl/tests/HttpPostIntegrationTest.cpp (only difference is that in the configuration nifi.flow.engine.threads is set to 1 instead of 8).
   The configuration yml (libminifi/test/resources/ThreadPoolAdjust.yml) is also almost the same as to what the HttpPostIntegrationTest uses (libminifi/test/resources/TestHTTPPost.yml)
   The test itself only runs assertion whether the InvokeHTTP processor is successfully started, nothing related to the threadpool.
   
   Having this test is currently misleading, we could just rename it but since it doesn't give anymore coverage than the aforementioned HttpPostIntegrationTest, I think we should just remove 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