Fix check_writeable_path to validate the target folder itself - #14065
Open
Sanjays2402 wants to merge 1 commit into
Open
Fix check_writeable_path to validate the target folder itself#14065Sanjays2402 wants to merge 1 commit into
Sanjays2402 wants to merge 1 commit into
Conversation
check_writeable_path walks up from the configured path to the lowest existing directory, but the loop only stopped on a directory that was already writeable. An existing but unwritable simulation_folder therefore did not terminate the walk: it kept climbing to an ancestor that happened to be writeable and validation passed, so the error only surfaced when every directory on the path was unwritable. The walk now stops at the first existing directory, and the access check after the loop decides whether to raise. A writeable folder still passes, an unwritable one now raises the intended ValueError, and a non-existent path still falls back to its nearest existing ancestor. Removes the xfail from test_that_simulation_folder_without_write_access_raises_validation_error, which was added as a placeholder for this bug and now passes.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Everest config validation so environment.simulation_folder (and other paths using check_writeable_path) fail fast when the target directory itself exists but is not writable, instead of incorrectly passing by walking up to a writable ancestor. This aligns validation with the intended behavior in issue #14056 and activates the previously xfailed regression test from #14055.
Changes:
- Update
check_writeable_pathto stop walking at the first existing directory and perform the write/execute access check after the walk. - Remove
xfailfromtest_that_simulation_folder_without_write_access_raises_validation_errorso it now enforces the regression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/everest/test_everlint.py | Un-xfails the regression test asserting unwritable simulation_folder triggers a validation error. |
| src/everest/config/validation_utils.py | Fixes directory walk logic so an existing but unwritable target directory correctly fails validation. |
Collaborator
|
Screenshots differ from baselines. A baseline update PR has been prepared: equinor/ert-testdata#78 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14065 +/- ##
==========================================
- Coverage 91.83% 91.82% -0.01%
==========================================
Files 482 482
Lines 33444 33443 -1
==========================================
- Hits 30713 30709 -4
- Misses 2731 2734 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Resolves #14056
Approach
check_writeable_pathwalks up from the configured path to the lowest existing directory, but the loop only stopped on a directory that was already writeable. An existing but unwritablesimulation_foldertherefore did not terminate the walk — it kept climbing to a writeable ancestor and validation passed, so the error only surfaced when every directory on the path was unwritable.The walk now stops at the first existing directory and the
os.accesscheck after the loop decides whether to raise. A writeable folder still passes, a non-existent path still falls back to its nearest existing ancestor, and an existing unwritable folder now raises the intendedValueError.This also un-xfails
test_that_simulation_folder_without_write_access_raises_validation_error, the placeholder added in #14055 for exactly this bug; it fails onmainand passes with the fix.git rebase -i main --exec 'just rapid-tests')When applicable
merge screenshot-PR in ert-testdata before merging this PR.
Verified locally:
tests/everest/test_everlint.py+tests/everest/test_config_validation.py— 247 passed (the one failure intest_config_validation.pyis pre-existing onmain). Labels left for a maintainer.This change was prepared with AI assistance; the regression test was run locally and fails without the fix.