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/07/23 11:53:41 UTC
[GitHub] [nifi-minifi-cpp] fgerlits opened a new pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
fgerlits opened a new pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139
https://issues.apache.org/jira/browse/MINIFICPP-1600
Remove all variants of `createTempDir`/`createTempDirectory` except one, and use that one in every test.
---
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676654446
##########
File path: extensions/standard-processors/tests/unit/RetryFlowFileTests.cpp
##########
@@ -86,7 +85,7 @@ class RetryFlowFileTest {
optional<bool> processor_uuid_matches_flowfile) {
reInitialize();
- const std::string output_dir = createTempDir(testController_.get());
+ const std::string output_dir = testController_->createTempDirectory();
Review comment:
This `output_dir` seems to be unused
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676654446
##########
File path: extensions/standard-processors/tests/unit/RetryFlowFileTests.cpp
##########
@@ -86,7 +85,7 @@ class RetryFlowFileTest {
optional<bool> processor_uuid_matches_flowfile) {
reInitialize();
- const std::string output_dir = createTempDir(testController_.get());
+ const std::string output_dir = testController_->createTempDirectory();
Review comment:
This `output_dir` seems to be unused 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
aminadinari19 commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r677165587
##########
File path: libminifi/test/bustache-tests/ApplyTemplateTests.cpp
##########
@@ -85,13 +85,13 @@ TEST_CASE("Test usage of ApplyTemplate", "[ApplyTemplateTest]") {
std::shared_ptr<core::Processor> atprocessor = plan->addProcessor("ApplyTemplate", "testApplyTemplate", core::Relationship("success", "description"), true);
std::shared_ptr<core::Processor> putfile = plan->addProcessor("PutFile", "putfile", core::Relationship("success", "description"), true);
- plan->setProperty(putfile, org::apache::nifi::minifi::processors::PutFile::Directory.getName(), dir3);
+ plan->setProperty(putfile, org::apache::nifi::minifi::processors::PutFile::Directory.getName(), put_file_destination_dir);
plan->setProperty(putfile, org::apache::nifi::minifi::processors::PutFile::ConflictResolution.getName(),
Review comment:
I like how the variables are way more meaningful 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
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 #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676537081
##########
File path: extensions/sftp/tests/FetchSFTPTests.cpp
##########
@@ -75,10 +73,10 @@ class FetchSFTPTestsFixture {
LogTestController::getInstance().setDebug<SFTPTestServer>();
// Create temporary directories
- testController.createTempDirectory(src_dir);
- REQUIRE(src_dir != nullptr);
- testController.createTempDirectory(dst_dir);
- REQUIRE(dst_dir != nullptr);
+ src_dir = testController.createTempDirectory();
+ REQUIRE_FALSE(src_dir.empty());
+ dst_dir = testController.createTempDirectory();
+ REQUIRE_FALSE(dst_dir.empty());
Review comment:
I did not consider it, but it is a good idea, thanks! Done in https://github.com/apache/nifi-minifi-cpp/pull/1139/commits/555d7b367f11d7750d594f2c0076b1e4c4144cdd.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
aminadinari19 commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r677165587
##########
File path: libminifi/test/bustache-tests/ApplyTemplateTests.cpp
##########
@@ -85,13 +85,13 @@ TEST_CASE("Test usage of ApplyTemplate", "[ApplyTemplateTest]") {
std::shared_ptr<core::Processor> atprocessor = plan->addProcessor("ApplyTemplate", "testApplyTemplate", core::Relationship("success", "description"), true);
std::shared_ptr<core::Processor> putfile = plan->addProcessor("PutFile", "putfile", core::Relationship("success", "description"), true);
- plan->setProperty(putfile, org::apache::nifi::minifi::processors::PutFile::Directory.getName(), dir3);
+ plan->setProperty(putfile, org::apache::nifi::minifi::processors::PutFile::Directory.getName(), put_file_destination_dir);
plan->setProperty(putfile, org::apache::nifi::minifi::processors::PutFile::ConflictResolution.getName(),
Review comment:
I like how the variable names are way more meaningful 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
aminadinari19 commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676622831
##########
File path: extensions/http-curl/tests/unit/InvokeHTTPTests.cpp
##########
@@ -47,8 +47,7 @@ TEST_CASE("HTTPTestsWithNoResourceClaimPOST", "[httptest1]") {
std::shared_ptr<core::Processor> logAttribute = std::make_shared<org::apache::nifi::minifi::processors::LogAttribute>("logattribute");
- char format[] = "/tmp/gt.XXXXXX";
- auto dir = testController.createTempDirectory(format);
+ auto dir = testController.createTempDirectory();
Review comment:
Maybe I am wrong but the dir variable doesn't seem to be used anywhere in this test.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
aminadinari19 commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r677162855
##########
File path: extensions/sftp/tests/FetchSFTPTests.cpp
##########
@@ -74,18 +72,15 @@ class FetchSFTPTestsFixture {
LogTestController::getInstance().setDebug<processors::LogAttribute>();
LogTestController::getInstance().setDebug<SFTPTestServer>();
- // Create temporary directories
- testController.createTempDirectory(src_dir);
- REQUIRE(src_dir != nullptr);
- testController.createTempDirectory(dst_dir);
- REQUIRE(dst_dir != nullptr);
+ REQUIRE_FALSE(src_dir.empty());
+ REQUIRE_FALSE(dst_dir.empty());
+ REQUIRE(plan);
Review comment:
Got it. Thanks :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
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 #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676740661
##########
File path: extensions/sftp/tests/FetchSFTPTests.cpp
##########
@@ -74,18 +72,15 @@ class FetchSFTPTestsFixture {
LogTestController::getInstance().setDebug<processors::LogAttribute>();
LogTestController::getInstance().setDebug<SFTPTestServer>();
- // Create temporary directories
- testController.createTempDirectory(src_dir);
- REQUIRE(src_dir != nullptr);
- testController.createTempDirectory(dst_dir);
- REQUIRE(dst_dir != nullptr);
+ REQUIRE_FALSE(src_dir.empty());
+ REQUIRE_FALSE(dst_dir.empty());
+ REQUIRE(plan);
Review comment:
To make sure that the pointer is not null, since we dereference it later. An assertion failure here is better than a segmentation fault later.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
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 #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676536348
##########
File path: encrypt-config/tests/CMakeLists.txt
##########
@@ -32,7 +32,7 @@ foreach(testfile ${ENCRYPT_CONFIG_TESTS})
target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/libminifi/test")
target_wholearchive_library(${testfilename} minifi)
- target_link_libraries(${testfilename} ${CATCH_MAIN_LIB})
+ target_link_libraries(${testfilename} ${CATCH_MAIN_LIB} ${TEST_BASE_LIB})
Review comment:
It's needed because `encrypt-config/tests/ConfigFileTests.cpp` now uses `TestController`, which is in `test_base` (= `${TEST_BASE_LIB}`). Before this change, `ConfigFileTests` created the temporary directory directly using `utils::file::create_temp_directory()` -- which also means that this test directory was not cleaned up after the test.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
aminadinari19 commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676682671
##########
File path: extensions/sftp/tests/FetchSFTPTests.cpp
##########
@@ -74,18 +72,15 @@ class FetchSFTPTestsFixture {
LogTestController::getInstance().setDebug<processors::LogAttribute>();
LogTestController::getInstance().setDebug<SFTPTestServer>();
- // Create temporary directories
- testController.createTempDirectory(src_dir);
- REQUIRE(src_dir != nullptr);
- testController.createTempDirectory(dst_dir);
- REQUIRE(dst_dir != nullptr);
+ REQUIRE_FALSE(src_dir.empty());
+ REQUIRE_FALSE(dst_dir.empty());
+ REQUIRE(plan);
Review comment:
What is the purpose of the last REQUIRE statement?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
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 #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676536348
##########
File path: encrypt-config/tests/CMakeLists.txt
##########
@@ -32,7 +32,7 @@ foreach(testfile ${ENCRYPT_CONFIG_TESTS})
target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/libminifi/test")
target_wholearchive_library(${testfilename} minifi)
- target_link_libraries(${testfilename} ${CATCH_MAIN_LIB})
+ target_link_libraries(${testfilename} ${CATCH_MAIN_LIB} ${TEST_BASE_LIB})
Review comment:
It's needed because `encrypt-config/tests/ConfigFileTests.cpp` now uses `TestController`, which is in `test_base` (= `${TEST_BASE_LIB}`. Before this change, `ConfigFileTests` created the temporary directory directly using `utils::file::create_temp_directory()` -- which also means that this test directory was not cleaned up after the test.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
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 #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676579897
##########
File path: encrypt-config/tests/CMakeLists.txt
##########
@@ -32,7 +32,7 @@ foreach(testfile ${ENCRYPT_CONFIG_TESTS})
target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/libminifi/test")
target_wholearchive_library(${testfilename} minifi)
- target_link_libraries(${testfilename} ${CATCH_MAIN_LIB})
+ target_link_libraries(${testfilename} ${CATCH_MAIN_LIB} ${TEST_BASE_LIB})
Review comment:
I see, thanks.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
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 #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676681090
##########
File path: extensions/standard-processors/tests/unit/RetryFlowFileTests.cpp
##########
@@ -86,7 +85,7 @@ class RetryFlowFileTest {
optional<bool> processor_uuid_matches_flowfile) {
reInitialize();
- const std::string output_dir = createTempDir(testController_.get());
+ const std::string output_dir = testController_->createTempDirectory();
Review comment:
Yes, it is. Removed it in https://github.com/apache/nifi-minifi-cpp/pull/1139/commits/4aae1742792f939879a50a211fb39877c068925a.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
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 #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676488130
##########
File path: extensions/sftp/tests/FetchSFTPTests.cpp
##########
@@ -75,10 +73,10 @@ class FetchSFTPTestsFixture {
LogTestController::getInstance().setDebug<SFTPTestServer>();
// Create temporary directories
- testController.createTempDirectory(src_dir);
- REQUIRE(src_dir != nullptr);
- testController.createTempDirectory(dst_dir);
- REQUIRE(dst_dir != nullptr);
+ src_dir = testController.createTempDirectory();
+ REQUIRE_FALSE(src_dir.empty());
+ dst_dir = testController.createTempDirectory();
+ REQUIRE_FALSE(dst_dir.empty());
Review comment:
Did you consider moving the initializers to the member initializer list or to default member initializers?
##########
File path: encrypt-config/tests/CMakeLists.txt
##########
@@ -32,7 +32,7 @@ foreach(testfile ${ENCRYPT_CONFIG_TESTS})
target_include_directories(${testfilename} BEFORE PRIVATE "${CMAKE_SOURCE_DIR}/libminifi/test")
target_wholearchive_library(${testfilename} minifi)
- target_link_libraries(${testfilename} ${CATCH_MAIN_LIB})
+ target_link_libraries(${testfilename} ${CATCH_MAIN_LIB} ${TEST_BASE_LIB})
Review comment:
Is this a new library, or why is this addition 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
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 #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676667357
##########
File path: extensions/http-curl/tests/unit/InvokeHTTPTests.cpp
##########
@@ -47,8 +47,7 @@ TEST_CASE("HTTPTestsWithNoResourceClaimPOST", "[httptest1]") {
std::shared_ptr<core::Processor> logAttribute = std::make_shared<org::apache::nifi::minifi::processors::LogAttribute>("logattribute");
- char format[] = "/tmp/gt.XXXXXX";
- auto dir = testController.createTempDirectory(format);
+ auto dir = testController.createTempDirectory();
Review comment:
Good catch, thanks! I have removed the unused variables in https://github.com/apache/nifi-minifi-cpp/pull/1139/commits/d61c4e651a4e3b1c36db8da6f8352ec65e76505a.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] aminadinari19 commented on a change in pull request #1139: MINIFICPP-1600 Unify temporary directory creation in tests
Posted by GitBox <gi...@apache.org>.
aminadinari19 commented on a change in pull request #1139:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1139#discussion_r676624081
##########
File path: extensions/http-curl/tests/unit/InvokeHTTPTests.cpp
##########
@@ -168,8 +167,7 @@ TEST_CASE("HTTPTestsWithResourceClaimPOST", "[httptest1]") {
std::shared_ptr<core::Processor> logAttribute = std::make_shared<org::apache::nifi::minifi::processors::LogAttribute>("logattribute");
- char format[] = "/tmp/gt.XXXXXX";
- auto dir = testController.createTempDirectory(format);
+ auto dir = testController.createTempDirectory();
Review comment:
Same concern as the 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org