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 redundant/superseded Everest tests #8954

Merged

Conversation

StephanDeHoop
Copy link
Contributor

@StephanDeHoop StephanDeHoop commented Oct 14, 2024

Issue
Resolves #8858

Approach
It seems, after investigating, that both the test_math_func_multiobj and test_math_func_advanced cover all the functionality of the other tests. The only exception, perhaps, is in the part of the config_minimal test where we test if the following parameter is ignored when no constraints are provided in the config file: "constraint_tolerance: 0.1 # To test that it is ignored.". We could keep the minimal test and remove only the other two for this reason? But it might seem like a stretch.

(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

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.55%. Comparing base (391a25f) to head (493701d).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8954      +/-   ##
==========================================
- Coverage   91.56%   91.55%   -0.01%     
==========================================
  Files         344      345       +1     
  Lines       21295    21302       +7     
==========================================
+ Hits        19499    19504       +5     
- Misses       1796     1798       +2     
Flag Coverage Δ
cli-tests 39.71% <ø> (-0.01%) ⬇️
gui-tests 73.51% <ø> (-0.02%) ⬇️
performance-tests 50.28% <ø> (-0.03%) ⬇️
unit-tests 80.31% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@StephanDeHoop StephanDeHoop force-pushed the sted/remove_redundant_tests_everest branch from ab0e94a to 9100a86 Compare October 14, 2024 10:21
@StephanDeHoop StephanDeHoop marked this pull request as ready for review October 14, 2024 10:44
Copy link
Contributor

@verveerpj verveerpj left a comment

Choose a reason for hiding this comment

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

One question, otherwise good.

Comment on lines -15 to -17
CONFIG_FILE_MINIMAL = "config_minimal.yml"
CONFIG_FILE_IN_CONSTR = "config_in_constr.yml"
CONFIG_FILE_OUT_CONSTR = "config_out_constr.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you check if these files are used in some other test, and if not, remove them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they are (only) used in the documentation of Everest as examples for slowly building up the complexity of using Everest (at least the minimal one is for sure, maybe the in_constr and out_constr aren't)

Copy link
Contributor

Choose a reason for hiding this comment

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

But in the documentation are they explicitly referring to the file location? Because they used to be in an examples folder, but that has now become test-data, and we should not have anything there that is not used. The documentation should not refer to test-data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry you are right, they are making their own ('everest_config.yml') which happens to look exactly the same as the 'config_minimal.yml'. We can delete them I guess, and also clean up the associated 'jobs' I suppose?

Copy link
Contributor

@verveerpj verveerpj Oct 15, 2024

Choose a reason for hiding this comment

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

yes, please do. However, the associated jobs might be used by other tests. You will find out when the tests fail...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The job (DISTANCE3) for in- and output constraint was used in other tests. Also, config_minimal was used in quite a lot of other tests too, so we need to keep the config file.

@verveerpj verveerpj added release-notes:skip If there should be no mention of this in release notes everest labels Oct 15, 2024
Copy link
Contributor

@verveerpj verveerpj left a comment

Choose a reason for hiding this comment

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

You will also need to squash the two commits into a single one.

Copy link
Contributor

@verveerpj verveerpj left a comment

Choose a reason for hiding this comment

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

Looks good. Can you squash into a single commit?

@StephanDeHoop StephanDeHoop enabled auto-merge (squash) October 16, 2024 06:57
@StephanDeHoop StephanDeHoop force-pushed the sted/remove_redundant_tests_everest branch from f417ffb to 493701d Compare October 16, 2024 09:30
@StephanDeHoop StephanDeHoop merged commit 45b9a36 into equinor:main Oct 16, 2024
56 checks passed
@StephanDeHoop StephanDeHoop deleted the sted/remove_redundant_tests_everest branch October 16, 2024 13:11
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.

Remove everest redundant_tests
3 participants