From a5794d64e359c2543265d53038a1849a8e9cb5c2 Mon Sep 17 00:00:00 2001 From: Bjarne Date: Thu, 25 Nov 2021 16:20:37 +0100 Subject: [PATCH] Replace old GENERAL_OBSERVATION error message Replaced to dynamically report keywords for which files cannot be located, topped off with an extensive unit-test. The method conf_instance_has_path_error() is neither used not defined anywhere so declaration is removed from conf.hpp --- libres/lib/config/conf.cpp | 51 +++++- libres/lib/enkf/enkf_obs.cpp | 25 ++- libres/lib/include/ert/config/conf.hpp | 5 +- libres/tests/CMakeLists.txt | 4 +- libres/tests/enkf/enkf_obs_paths_detailed.cpp | 170 ++++++++++++++++++ 5 files changed, 231 insertions(+), 24 deletions(-) create mode 100644 libres/tests/enkf/enkf_obs_paths_detailed.cpp diff --git a/libres/lib/config/conf.cpp b/libres/lib/config/conf.cpp index 1d8b388c8db..8882c39565f 100644 --- a/libres/lib/config/conf.cpp +++ b/libres/lib/config/conf.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -1037,8 +1038,24 @@ conf_instance_validate_sub_instances(const conf_instance_type *conf_instance) { return ok; } -bool conf_instance_get_path_error(const conf_instance_type *conf_instance) { - bool path_errors = false; +/** + * This function resolves paths for each file-keyword in the given + * configuration. It returns a set of keywords with corresponding + * path for each non-existing path. + * + * The return type is std::set because this is a simple and efficient + * way to avoid duplicates in the end-result: Note that the method + * recurses and combines results in the last part of the code. + * + * Elements in the set are the keyword concatenated with "=>" and its + * resolved path. For example + * + * "OBS_FILE=>/var/tmp/obs_path/obs.txt" + * + */ +static std::set +get_path_errors(const conf_instance_type *conf_instance) { + std::set path_errors; { int num_items = hash_get_size(conf_instance->items); char **item_keys = hash_alloc_keylist(conf_instance->items); @@ -1050,7 +1067,8 @@ bool conf_instance_get_path_error(const conf_instance_type *conf_instance) { conf_item->conf_item_spec; if (conf_item_spec->dt == DT_FILE) { if (!fs::exists(conf_item->value)) - path_errors = true; + path_errors.insert(std::string(conf_item_spec->name) + + "=>" + std::string(conf_item->value)); } } util_free_stringlist(item_keys, num_items); @@ -1067,8 +1085,8 @@ bool conf_instance_get_path_error(const conf_instance_type *conf_instance) { const conf_instance_type *sub_conf_instance = (const conf_instance_type *)hash_get( conf_instance->sub_instances, sub_instances_key); - if (conf_instance_get_path_error(sub_conf_instance)) - path_errors = true; + std::set sub = get_path_errors(sub_conf_instance); + path_errors.insert(sub.begin(), sub.end()); } util_free_stringlist(sub_instance_keys, num_sub_instances); @@ -1077,6 +1095,29 @@ bool conf_instance_get_path_error(const conf_instance_type *conf_instance) { return path_errors; } +/** + * This method returns a single C-style string (zero-terminated char *) + * describing keywords and paths to which these resolve, and which does + * not exist. For example + * + * "OBS_FILE=>/var/tmp/obs_path/obs.txt\nINDEX_FILE=>/var/notexisting/file.txt" + * + * Note newlines - this string is intended for printing. + */ +char *conf_instance_get_path_error(const conf_instance_type *conf_instance) { + std::set errors = get_path_errors(conf_instance); + + if (errors.size() == 0) + return NULL; + + std::string retval; + for (std::string s : errors) { + retval.append(s); + retval.append("\n"); + } + return strdup(retval.data()); +} + bool conf_instance_validate(const conf_instance_type *conf_instance) { return conf_instance && conf_instance_has_required_items(conf_instance) && conf_instance_has_valid_mutexes(conf_instance) && diff --git a/libres/lib/enkf/enkf_obs.cpp b/libres/lib/enkf/enkf_obs.cpp index d9ede82ee0b..91f7ff4ecf1 100644 --- a/libres/lib/enkf/enkf_obs.cpp +++ b/libres/lib/enkf/enkf_obs.cpp @@ -659,18 +659,14 @@ static void handle_general_observation(enkf_obs_type *enkf_obs, stringlist_free(block_obs_keys); } -static void enkf_obs_reinterpret_DT_FILE(const char *config_file) { +static void enkf_obs_reinterpret_DT_FILE(const char *errors) { // clang-format off - fprintf(stderr, "**********************************************************************\n"); - fprintf(stderr, "* In ert version 2.3 we have changed how filepaths are interpreted *\n"); - fprintf(stderr, "* in the observation file. When using the keywords OBS_FILE, *\n"); - fprintf(stderr, "* ERROR_COVAR and INDEX_FILE in the category GENERAL_OBSERVATION *\n"); - fprintf(stderr, "* the filenames will be interpreted relative to the main observation *\n"); - fprintf(stderr, "* file. *\n"); - fprintf(stderr, "* *\n"); - fprintf(stderr, "* Please update the OBS_FILE, ERROR_COVAR and INDEX_FILE keywords *\n"); - fprintf(stderr, "* by removing the path to the main observation file. *\n"); - fprintf(stderr, "**********************************************************************\n"); + fprintf(stderr, "*****************************************\n"); + fprintf(stderr, "The following keywords in your configuration did not resolve to a valid path: \n"); + fprintf(stderr, "\n"); + fprintf(stderr, "%s\n", errors); + fprintf(stderr, "\n"); + fprintf(stderr, "*****************************************\n"); // clang-format on } @@ -693,9 +689,10 @@ void enkf_obs_load(enkf_obs_type *enkf_obs, const char *config_file, conf_instance_type *enkf_conf = conf_instance_alloc_from_file( enkf_conf_class, "enkf_conf", config_file); - if (conf_instance_get_path_error(enkf_conf)) { - enkf_obs_reinterpret_DT_FILE(config_file); - exit(1); + const char *errors = conf_instance_get_path_error(enkf_conf); + if (errors) { + enkf_obs_reinterpret_DT_FILE(errors); + exit(1); // No need to free errors... } if (!conf_instance_validate(enkf_conf)) diff --git a/libres/lib/include/ert/config/conf.hpp b/libres/lib/include/ert/config/conf.hpp index afe48991ef2..5467355661a 100644 --- a/libres/lib/include/ert/config/conf.hpp +++ b/libres/lib/include/ert/config/conf.hpp @@ -102,7 +102,6 @@ #ifdef __cplusplus extern "C" { #endif - #include #include @@ -247,14 +246,12 @@ time_t conf_instance_get_item_value_time_t(const conf_instance_type *conf_instance, const char *item_name); -bool conf_instance_get_path_error(const conf_instance_type *conf_instance); +char *conf_instance_get_path_error(const conf_instance_type *conf_instance); /** V A L I D A T O R S */ bool conf_instance_validate(const conf_instance_type *conf_instance); -bool conf_instance_has_path_error(const conf_instance_type *conf_instance); - /** A L L O C F R O M F I L E */ conf_instance_type * diff --git a/libres/tests/CMakeLists.txt b/libres/tests/CMakeLists.txt index f901bb4adc7..ef620af5050 100644 --- a/libres/tests/CMakeLists.txt +++ b/libres/tests/CMakeLists.txt @@ -9,7 +9,9 @@ include(Catch) add_executable(ert_test_suite tmpdir.cpp analysis/modules/test_ies_enkf_main.cpp analysis/test_enkf_linalg.cpp - res_util/test_matrix.cpp) + res_util/test_matrix.cpp + enkf/enkf_obs_paths_detailed.cpp + ) target_link_libraries(ert_test_suite res Catch2::Catch2WithMain fmt::fmt) catch_discover_tests(ert_test_suite) diff --git a/libres/tests/enkf/enkf_obs_paths_detailed.cpp b/libres/tests/enkf/enkf_obs_paths_detailed.cpp new file mode 100644 index 00000000000..7ed12d24164 --- /dev/null +++ b/libres/tests/enkf/enkf_obs_paths_detailed.cpp @@ -0,0 +1,170 @@ +#include +#include +#include + +#include + +#include +#include "../tmpdir.hpp" + +/* + * Write conf-file with given keywords, then parse it. + * Returns pointer to the parsed configuration + */ +conf_instance_type *write_conf(std::vector keyword_lines) { + std::string buf("GENERAL_OBSERVATION WPR_DIFF_1 {\n"); + buf += " DATA = SNAKE_OIL_WPR_DIFF;\n"; + buf += " INDEX_LIST = 400,800,1200,1800;\n"; + buf += " RESTART = 199;\n"; + for (auto s : keyword_lines) + buf += s + "\n"; + buf += "};\n"; + + std::ofstream stream("obs_path/conf.txt", std::ostream::out); + stream.write(buf.data(), buf.size()); + stream.close(); // close before reading in next step! + + conf_class_type *enkf_conf_class = enkf_obs_get_obs_conf_class(); + return conf_instance_alloc_from_file(enkf_conf_class, "enkf_conf", + "obs_path/conf.txt"); +} + +/* + * Just a convenient wrapper from char* to std::string + */ +std::string get_path_error(conf_instance_type *enkf_conf) { + char *errors = conf_instance_get_path_error(enkf_conf); + if (errors) { + std::string s(errors); + free(errors); + return s; + } else + return std::string(""); +} + +/* + * Syntactic sugar for checking if a string contains another string + * The memmber-function ::contains() is introduced in C++23 + */ +bool contains(std::string errors, std::string kw) { + return errors.find(kw) != std::string::npos; +} + +/** + * Syntactic sugar for making sure a file exists ("touching" it) + * + * Note that std::ofstream is a proper RAII object which means + * neither flush nor close is required. + */ +void touch_file(std::string name) { + std::ofstream stream(name.data(), std::ostream::out); +} + +TEST_CASE("Test parsing keywords in configuration", "[unittest]") { + GIVEN("A workearea") { + WITH_TMPDIR; + std::filesystem::create_directory("obs_path"); + + GIVEN("A conf file with no relevant keywords") { + conf_instance_type *enkf_conf = write_conf({}); + THEN("There are no errors") { + std::string errors = get_path_error(enkf_conf); + REQUIRE(!contains(errors, "OBS_FILE")); + REQUIRE(!contains(errors, "ERROR_COVAR")); + REQUIRE(!contains(errors, "INDEX_FILE")); + } + conf_instance_free(enkf_conf); + } + + GIVEN("A conf file with OBS_FILE keyword in it") { + conf_instance_type *enkf_conf = + write_conf({"OBS_FILE = obs.txt;"}); + WHEN("There is no OBS_FILE present") { + THEN("Error refers to missing file") { + std::string errors = get_path_error(enkf_conf); + REQUIRE(contains( + errors, "OBS_FILE=>" + + std::filesystem::current_path().native() + + "/obs_path/obs.txt")); + REQUIRE(!contains(errors, "ERROR_COVAR")); + REQUIRE(!contains(errors, "INDEX_FILE")); + } + } + WHEN("The obs file is present") { + touch_file("obs_path/obs.txt"); + THEN("There are no errors") { + std::string errors = get_path_error(enkf_conf); + REQUIRE(!contains(errors, "OBS_FILE")); + REQUIRE(!contains(errors, "ERROR_COVAR")); + REQUIRE(!contains(errors, "INDEX_FILE")); + } + } + conf_instance_free(enkf_conf); + } + + GIVEN("A conf file with OBS_FILE in subdir obs_path") { + conf_instance_type *enkf_conf = + write_conf({"OBS_FILE = obs_path/obs.txt;"}); + + WHEN("The OBS_FILE is in the wrong location") { + touch_file("obs_path/obs.txt"); + THEN("Error refers to missing file") { + std::string errors = get_path_error(enkf_conf); + REQUIRE(contains( + errors, "OBS_FILE=>" + + std::filesystem::current_path().native() + + "/obs_path/obs_path/obs.txt")); + REQUIRE(!contains(errors, "ERROR_COVAR")); + REQUIRE(!contains(errors, "INDEX_FILE")); + } + } + WHEN("The OBS_FILE is in the subdirectory") { + util_make_path("obs_path/obs_path"); + touch_file("obs_path/obs_path/obs.txt"); + THEN("There are no errors") { + std::string errors = get_path_error(enkf_conf); + REQUIRE(!contains(errors, "OBS_FILE")); + REQUIRE(!contains(errors, "ERROR_COVAR")); + REQUIRE(!contains(errors, "INDEX_FILE")); + } + } + conf_instance_free(enkf_conf); + } + + GIVEN( + "A conf file with OBS_FILE, ERROR_COVAR and INDEX_FILE keywords") { + conf_instance_type *enkf_conf = + write_conf({"OBS_FILE = obs.txt;" + "ERROR_COVAR = covar.txt;" + "INDEX_FILE = index.txt;"}); + + WHEN("Only OBS_FILE exists") { + touch_file("obs_path/obs.txt"); + THEN("Error refer to missing covar and index file") { + std::string errors = get_path_error(enkf_conf); + REQUIRE(!contains(errors, "OBS_FILE")); + REQUIRE(contains( + errors, "ERROR_COVAR=>" + + std::filesystem::current_path().native() + + "/obs_path/covar.txt")); + REQUIRE(contains( + errors, "INDEX_FILE=>" + + std::filesystem::current_path().native() + + "/obs_path/index.txt")); + } + } + WHEN("covar, obs, and index files exist") { + touch_file("obs_path/obs.txt"); + touch_file("obs_path/covar.txt"); + touch_file("obs_path/index.txt"); + THEN("There are no errors") { + std::string errors = get_path_error(enkf_conf); + REQUIRE(!contains(errors, "OBS_FILE")); + REQUIRE(!contains(errors, "ERROR_COVAR")); + REQUIRE(!contains(errors, "INDEX_FILE")); + } + } + conf_instance_free(enkf_conf); + } + } +}