Improve exception handling around init/loading of enkf nodes #4938

valentin-krasontovitsch · 2023-02-22T14:01:54Z

Issue
Resolves #4900

OBS
we're using a kind of integration test approach. it takes like 2 minutes to run one of the tests, which is quite long. i've tried looking at this before, but it seems like we might not be able to do much about it because of how we set up different components of ert in threads, and their communication with each other.

Approach
See commit messages.

Pre review checklist

Added appropriate release note label
SKIPPED! PR title captures the intent of the changes, and is fitting for release notes.
Commit history is consistent and clean, in line with the contribution guidelines.
IRRELEVANT Updated documentation
Ensured new behaviour is tested

codecov-commenter · 2023-02-22T14:15:27Z

Codecov Report

Merging #4938 (c2ae423) into main (7bdf7d3) will decrease coverage by 0.06%.
The diff coverage is 23.43%.

@@            Coverage Diff             @@
##             main    #4938      +/-   ##
==========================================
- Coverage   63.03%   62.97%   -0.06%     
==========================================
  Files         438      438              
  Lines       30683    30731      +48     
  Branches     3127     3175      +48     
==========================================
+ Hits        19341    19354      +13     
- Misses      10551    10569      +18     
- Partials      791      808      +17

Impacted Files	Coverage Δ
src/clib/lib/enkf/enkf_node.cpp	`0.00% <0.00%> (ø)`
src/clib/lib/enkf/field.cpp	`0.00% <0.00%> (ø)`
src/clib/lib/enkf/gen_kw_config.cpp	`0.00% <0.00%> (ø)`
src/clib/lib/config/config_parser.cpp	`43.65% <50.00%> (-4.05%)`	⬇️
src/ert/_c_wrappers/enkf/data/enkf_node.py	`91.56% <50.00%> (ø)`
src/ert/_c_wrappers/enkf/enkf_obs.py	`84.33% <100.00%> (+0.58%)`	⬆️
src/ert/ensemble_evaluator/_wait_for_evaluator.py	`97.22% <100.00%> (+0.25%)`	⬆️
src/ert/shared/models/base_run_model.py	`93.07% <0.00%> (+2.07%)`	⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

tests/unit_tests/c_wrappers/res/enkf/test_field_export.py

src/clib/lib/config/config_parser.cpp

src/clib/lib/enkf/enkf_node.cpp

src/clib/lib/enkf/field.cpp

tests/unit_tests/cli/test_integration_cli.py

src/clib/lib/config/config_parser.cpp

eivindjahren · 2023-02-24T14:11:24Z

src/clib/lib/enkf/gen_kw_config.cpp

@@ -102,6 +104,14 @@ void gen_kw_config_set_parameter_file(gen_kw_config_type *config,
        config_content_type *content =
            config_parse(parser, parameter_file, "--", NULL, NULL, NULL,
                         CONFIG_UNRECOGNIZED_ADD, false);
+        if (!content->valid) {
+            logger->warning(


This shows up as multiple logging statements in azure, which is difficult to deal with. Could we make it into one statement?

valentin-krasontovitsch · 2023-02-24T14:14:55Z

@eivindjahren the three new tests run in 18 seconds (with -n 3 on my machine)

The function `util_file_readable` does not function as expected, checking only if a file is readable by its owner. We replace by our own check.

There is a block of code in the forward_model_ok callback that was not covered by tests. We rectify this here.

… of enkf nodes)

…_KW parsing errors log! two for the price of one! genous!! fixup

oyvindeide

Please refrain from merging this in its current state, this causes all kinds of conflicts with our feature branch which we are aiming to merge next week. Making tests is fine, but enkf_node.cpp and field.cpp in particular will just create a lot of extra work.

eivindjahren · 2023-02-27T06:49:17Z

Please refrain from merging this in its current state, this causes all kinds of conflicts with our feature branch which we are aiming to merge next week. Making tests is fine, but enkf_node.cpp and field.cpp in particular will just create a lot of extra work.

@oyvindeide This fixes a bug in the existing code and adds a regression test. This therefore creates more safety for you to merge.

valentin-krasontovitsch added the release-notes:skip If there should be no mention of this in release notes label Feb 22, 2023

valentin-krasontovitsch self-assigned this Feb 22, 2023

eivindjahren reviewed Feb 22, 2023

View reviewed changes

tests/unit_tests/c_wrappers/res/enkf/test_field_export.py Outdated Show resolved Hide resolved

valentin-krasontovitsch force-pushed the segfault-troll-get-the-juices branch from 0f22f25 to 7998fac Compare February 23, 2023 12:48

valentin-krasontovitsch marked this pull request as ready for review February 23, 2023 14:29

eivindjahren added release-notes:bug-fix Automatically categorise as bug fix in release notes and removed release-notes:bug-fix Automatically categorise as bug fix in release notes labels Feb 23, 2023

eivindjahren reviewed Feb 23, 2023

View reviewed changes

src/clib/lib/config/config_parser.cpp Outdated Show resolved Hide resolved