-
Notifications
You must be signed in to change notification settings - Fork 350
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
Add pytest suite for airflow processor #1317
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
depends and based on #1316 |
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 for putting this together. I just had a comment on the fixture names.
Also, there's a typo in the test filename: test_airlfow_processory.py
e8df8f7
to
6fbf5bf
Compare
Adds a basic test suite specific to the airflow processor. Related elyra-ai#185
Will a github method test be added separately? |
@ptitzler -yes, will add tests for the git utils, but can go separately in another PR |
@ptitzler and @kevin-bates It seems that you guys have been reviewing this, so I will wait for your final approvals before merging. |
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.
LGTM
|
||
|
||
def test_create_file(monkeypatch, processor, parsed_pipeline, parsed_ordered_dict, sample_metadata): | ||
correct_hash_value = '9a15eba3337ba3c90457d5b60495720333f944fd2cec8e4ce40c32238d2a4206' |
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.
Could you add a comment indicating how this hash value is computed so folks making changes know what to do?
Frankly. I'd prefer we validate the export w/o this dependency, perhaps by looking for expected values in the output python file - or things like that. This hash stuff has proven to be quite fragile and was abandoned in kfp-notebook.
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.
+1 abandoning the hash stuff. Ill refactor to verify via expected values
Im working through parsing the generated airflow DAG file. Will have an updated PR no later than EOD today. Attempted to use the ast lib to parse but that felt heavy. |
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.
LGTM - took tests for a spin - all good. Thanks for removing the hash check - this will be much more sustainable.
See original PR #490 for more details
Developer's Certificate of Origin 1.1