-
Notifications
You must be signed in to change notification settings - Fork 121
Detect missing arguments to DESIGN_MATRIX #12571
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
Conversation
5ee71fb to
118aad8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12571 +/- ##
==========================================
- Coverage 90.58% 90.56% -0.02%
==========================================
Files 435 435
Lines 29929 29931 +2
==========================================
- Hits 27110 27108 -2
- Misses 2819 2823 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @berland ! 🚀
CodSpeed Performance ReportMerging #12571 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is a bug in lark Schema config. Let's fix that instead:
ert/src/ert/config/parsing/config_schema.py
Line 268 in 78cae26
| def design_matrix_keyword() -> SchemaItem: |
You mean it is not respecting the |
89938f9 to
b8389b2
Compare
Hopefully fixed now! |
| self, line: Sequence[FileContextToken] | ||
| ) -> Sequence[FileContextToken | dict[FileContextToken, FileContextToken]]: | ||
| n = self.options_after | ||
| if not line: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
|
Related bug: #12578 |
| from ert.config.parsing.config_schema_item import SchemaItem | ||
|
|
||
|
|
||
| def test_argc_min(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_schema_argc_min_invalid?
| parse_contents("OPTIONS", schema, "dummy_filename") | ||
|
|
||
|
|
||
| def test_argc_min_with_options_after(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_schema_argc_min_with_options_after?
xjules
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestion for test names improvements, but otherwise 🏅
f8c5290 to
3283353
Compare
This solves a missing error message when e.g. DESIGN_MATRIX keyword is used with no arguments - a list with an empty dict was returned in the parser, leading to argc_min=1 to be accepted when it should not.
3283353 to
b6f070c
Compare
|
Successfully created backport PR for |
If no arguments are given in the config file to DESIGN_MATRIX, a TypeError would be displayed:
TypeError: argument should be a str or an os.PathLike object where __fspath__ returns a str, not 'dict'This commit adds detection and a test for this case.
Issue
Resolves #12570
Approach
🧠
git rebase -i main --exec 'just rapid-tests')When applicable