-
Notifications
You must be signed in to change notification settings - Fork 236
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
Added validation checks for import_tasks_from #686
Added validation checks for import_tasks_from #686
Conversation
thanks for your contribution! can you add some tests? please add them here In the first case, we wanna check that an empty file does not break initializing the DAGSpec object, in the second case, we want to test that an appropriate error is shown let me know if you encounter issues or have questions! |
Hi @LawrenceCheng1570 , thanks for the contribution! I reproduced the error. The problem is that a downstream function doesn't work with empty lists, so I think the original logic needs to change a bit instead of this: if imported is None:
imported = [] it should be: if not imported:
path = str(self.data['meta']['import_tasks_from'])
raise ValueError(f'expected import_tasks_from file ({path!r}) to return a list of tasks, got: {imported}') Note that this conditional will take care of Please also update the test so it checks that the appropriate error message is shown: see here the condition can be something like: |
…-list import_tasks_from tests
Hi @edublancas, thanks for the suggestions! It fixed my error and I just pushed my changes. Please let me know if I need to make any other edits. |
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.
almost there! just minor observations
'Expected list when loading YAML file from ' | ||
'import_tasks_from: file.yaml, ' | ||
f'but got {type(imported)}') | ||
|
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.
looks good!
tests/spec/test_dagspec.py
Outdated
spec_d = yaml.safe_load(Path('pipeline.yaml').read_text()) | ||
spec_d['meta']['import_tasks_from'] = 'some_tasks.yaml' | ||
|
||
spec = DAGSpec(spec_d) |
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.
as a best practice when writing tests that evaluate error messages. it's best to only include the statement that throws the error, in this case:
with pytest.raises(ValueError) as excinfo:
DAGSpec(spec_d)
Please move everything the spec = ...
statement outside the with pytest.raises
line, and you can remove the spec.to_dag().render()
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.
Thanks @edublancas! Just pushed my changes
wooo great job! thanks a lot for contributing! |
Thanks so much! Really appreciate all the help and support for my first time contributing :) |
* Added validation checks for import_tasks_from * Removed unnecessary blank line in DAGSpecPartial * Added tests for empty yaml and non-list yaml file * Changed logic for handling empty YAML files and updated empty and non-list import_tasks_from tests * Removed unnecessary blank lines * Updated import_tasks_from tests for empty and non-list YAML file
Issue #669
Changes:
When loading
import_tasks_from: file.yaml
yaml.safe_load
is None, replace it with an empty listyaml.safe_load
returns something other than a list, raise an error saying we were expecting a list