Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make sure loading gui produces just one storage folder #4053

Merged
merged 1 commit into from
Oct 13, 2022
Merged

Make sure loading gui produces just one storage folder #4053

merged 1 commit into from
Oct 13, 2022

Conversation

dafeda
Copy link
Contributor

@dafeda dafeda commented Oct 13, 2022 •
edited

Issue
Resolves #4054

Pre review checklist

  • Added appropriate release note label
  • 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.

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@dafeda dafeda self-assigned this Oct 13, 2022
@dafeda dafeda added the release-notes:bug-fix Automatically categorise as bug fix in release notes label Oct 13, 2022
DanSava
DanSava reviewed Oct 13, 2022
View reviewed changes
src/clib/lib/enkf/model_config.cpp
@@ -239,8 +239,7 @@ model_config_type *model_config_alloc_empty() {
model_config->num_realizations = 0;
model_config->obs_config_file = NULL;

model_config_set_enspath(model_config,
fs::absolute(DEFAULT_ENSPATH).c_str());
model_config_set_enspath(model_config, DEFAULT_ENSPATH);
Copy link
Contributor

@DanSava DanSava Oct 13, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably cause the test test_default_model_config_ens_path to fail.

So as a quick fix I would suggest also replacing

    if (config_content_has_item(config, ENSPATH_KEY))
        model_config_set_enspath(
            model_config,
            config_content_get_value_as_abspath(config, ENSPATH_KEY));

with

    if (config_content_has_item(config, ENSPATH_KEY))
        model_config_set_enspath(
            model_config,
            config_content_get_value_as_abspath(config, ENSPATH_KEY));
    else
        model_config_set_enspath(
            model_config,
            config_content_get_value_as_abspath(config, DEFAULT_ENSPATH));

I know this makes model_config_set_enspath(model_config, DEFAULT_ENSPATH); on ln 242 (what a nice palindromic number) a bit useless but it might be ok to just remove that line altogether

Copy link
Contributor

@DanSava DanSava Oct 13, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace fs = std::filesystem; can be removed also

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we instead fix the test to assert that assert default_ens_path == set_in_file_ens_path but where we add res_config.config_path to default_ens_path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure the test needs a fix. Are you suggesting fixing getEnspath from ModelConfig to make sure it contains res_config.config_path ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed ⬆️ replaced config_content_get_value_as_abspath(config, DEFAULT_RUNPATH)); with config_content_get_value_as_abspath(config, DEFAULT_ENSPATH));

@dafeda dafeda force-pushed the gui_load branch 2 times, most recently from b9f264f to 60055bb Compare October 13, 2022 09:39
@dafeda dafeda changed the title Test that loading gui creates single storage folder Make sure loading gui produces just one storage folder Oct 13, 2022
@dafeda dafeda marked this pull request as ready for review October 13, 2022 10:38
DanSava
DanSava approved these changes Oct 13, 2022
View reviewed changes
Copy link
Contributor

@DanSava DanSava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dafeda @DanSava @frode-aarstad
Test that loading gui creates single storage folder
0498dcf 
Here we remove the unwanted change but keep test introduced in equinor#3982.

Replace tmpdir with tmp_path in test_gui_load.py

Do not mock resconfig and enkfmain to get more realistic tests

Co-authored-by: Dan Sava <dan.sava42@gmail.com>
Co-authored-by: Frode Aarstad <frodeaarstad@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #4053 (0498dcf) into main (4d32873) will increase coverage by 0.16%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #4053      +/-   ##
==========================================
+ Coverage   57.71%   57.87%   +0.16%     
==========================================
  Files         523      523              
  Lines       39525    39526       +1     
  Branches     3592     3591       -1     
==========================================
+ Hits        22811    22876      +65     
+ Misses      15782    15720      -62     
+ Partials      932      930       -2
Impacted Files Coverage Δ
src/clib/lib/enkf/model_config.cpp 30.73% <25.00%> (-0.13%) ⬇️
src/ert/gui/gert_main.py 95.65% <100.00%> (ø)
src/ert/_c_wrappers/enkf/enkf_main.py 98.66% <0.00%> (+0.33%) ⬆️
src/ert/libres_facade.py 88.37% <0.00%> (+0.91%) ⬆️
src/ert/services/storage_service.py 77.35% <0.00%> (+1.88%) ⬆️
src/ert/_c_wrappers/job_queue/workflow_job.py 85.23% <0.00%> (+2.68%) ⬆️
src/ert/_c_wrappers/enkf/ert_workflow_list.py 83.09% <0.00%> (+4.22%) ⬆️
src/ert/shared/exporter.py 64.28% <0.00%> (+7.14%) ⬆️
src/ert/gui/ertwidgets/models/targetcasemodel.py 74.07% <0.00%> (+7.40%) ⬆️
... and 7 more

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

@dafeda dafeda merged commit 374ff52 into equinor:main Oct 13, 2022
@dafeda dafeda deleted the gui_load branch October 13, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanSava DanSava DanSava approved these changes

@frode-aarstad frode-aarstad Awaiting requested review from frode-aarstad

Assignees

@dafeda dafeda

Labels
release-notes:bug-fix Automatically categorise as bug fix in release notes
Projects
SCOUT
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Loading gui produces two storage folders
3 participants
@dafeda @codecov-commenter @DanSava