fgerlits commented on code in PR #2022:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2022#discussion_r2336450231
##########
libminifi/test/libtest/integration/IntegrationBase.cpp:
##########
@@ -28,9 +29,16 @@
namespace org::apache::nifi::minifi::test {
-IntegrationBase::IntegrationBase(std::chrono::milliseconds waitTime)
+IntegrationBase::IntegrationBase(const std::optional<std::filesystem::path>&
test_file_location, const std::optional<std::filesystem::path>& home_path,
std::chrono::milliseconds waitTime)
: configuration(std::make_shared<minifi::ConfigureImpl>()),
- wait_time_(waitTime) {
+ wait_time_(waitTime),
+ home_path_(home_path) {
+ flow_config_path_.config_path = test_file_location;
+ if (test_file_location && std::filesystem::exists(*test_file_location) &&
std::filesystem::is_regular_file(*test_file_location)) {
Review Comment:
I think we can throw here if this condition is false; this should never
happen.
##########
extensions/standard-processors/tests/integration/TailFileIntegrationTest.cpp:
##########
@@ -36,7 +36,8 @@ namespace org::apache::nifi::minifi::test {
class TailFileTestHarness : public IntegrationBase {
public:
- TailFileTestHarness() {
+ explicit TailFileTestHarness(const std::filesystem::path& test_file_location)
+ : IntegrationBase(test_file_location, {}, 2s) {
Review Comment:
is the addition of the `2s` argument here intentional?
##########
libminifi/test/libtest/integration/IntegrationBase.h:
##########
@@ -31,26 +31,35 @@
#include "utils/file/AssetManager.h"
#include "utils/file/FileUtils.h"
#include "core/BulletinStore.h"
+#include "unit/TestBase.h"
namespace minifi = org::apache::nifi::minifi;
namespace core = minifi::core;
namespace utils = minifi::utils;
namespace org::apache::nifi::minifi::test {
+struct FlowConfigPath {
+ std::unique_ptr<TempDirectory> temp_dir;
+ std::optional<std::filesystem::path> config_path;
+};
+
class IntegrationBase {
public:
- explicit IntegrationBase(std::chrono::milliseconds waitTime =
std::chrono::milliseconds(DEFAULT_WAITTIME_MSECS));
+ explicit IntegrationBase(const std::optional<std::filesystem::path>&
test_file_location = {}, const std::optional<std::filesystem::path>& home_path
= {},
Review Comment:
Does `test_file_location` (and `flow_config_path_.config_path`) still need
to be an optional? We seem to set it everywhere. Could we do this?
```suggestion
explicit IntegrationBase(const std::filesystem::path& test_file_location,
const std::optional<std::filesystem::path>& home_path = {},
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
