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.

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

Remove clutter of configfiles in testdata mathfunc #9357

Conversation

StephanDeHoop
Copy link
Contributor

@StephanDeHoop StephanDeHoop commented Nov 26, 2024

Issue
(will) Resolve(s) #9210

DRAFT
Tested that all generated EverestConfigs are still exactly the same as the original config files in the repository (will be removed later of course). Next step is how to replace loading the config files with using the generated EverestConfig instances. The one thing to note is: we need to specify a config_path since otherwise the PyDantic validation fails when we validate the model. Right now all the tests that require a config file are copying (recursively) the math_func sub-directory in /test-data/everest. After we remove all the config files except minimal, when we copy we could then overwrite this file with our generated EverestConfig instance and set the config_path? Because as far as I understand the code now, the config file need to exists in order to succeed with the PyDantic validation. Further steps can be (might have been a good starting point too, but with my limited understanding of Everest I found it a bit too abstract to start from there): what are the tests that use the config files actually testing? Can we do away with some of the config files? Or are they necessary to have large testing coverage of all Everest functionality?

PS: reason why I didn't go for modifying one existing yaml file and instead building/generating them from dicts was that some configs contain different values for some parameters and it wasn't as simple as just inserting new lines here in there (would have to replace substrings in the existing one as well as adding them). Maybe in retrospect I should have read the yaml as a string and just replace sub-strings and write the final string back to the file? My current solution seemed cleaner, but maybe it is overkill?

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@StephanDeHoop StephanDeHoop added release-notes:skip If there should be no mention of this in release notes everest labels Nov 26, 2024
@StephanDeHoop StephanDeHoop self-assigned this Nov 26, 2024
@StephanDeHoop
Copy link
Contributor Author

Started over and will finish this issue in #9434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
everest release-notes:skip If there should be no mention of this in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant