-
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
Fix KFP component without inputs compilation bug #2646
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
The failing test |
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.
The failing test test_directory_based_component_catalog doesn't take into account that unrelated tests might add other resources (like it is done in this PR) and needs to be reworked to assure its original assumptions remain correct over time.
Each time a test uses the component_cache
fixture (which the directory-based catalog test does), the component cache instance should be cleared both before and after the test completes (and hence not side-effect other tests). Obviously, that isn't happening here, so I'll keep looking into a lightweight solution.
Thoughts on going back to a 'greater than' comparison for the |
Updates on the CI test failures: When the entire test is run, the two cases fail because the component cache is not empty at the start
When only the two test cases are run, they succeed because its setup is not tainted:
|
It'd be possible. The test would have to take an inventory of the existing components in the cache and eliminate duplicates from its catalog directory list for this to work. However, there's also a good chance that the cache already includes all of the pre-selected components, which would render the test result almost useless because it would only validate that adding components from an empty directory doesn't add any components. |
Right. And as of the last commit and my latest testing regarding cache cleanup in certain tests, it seems that we have a larger issue on our hands here. |
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
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!
Closes #2644
What changes were proposed in this pull request?
elyra/tests/pipeline/kfp/test_processor_kfp.py
(including resource)How was this pull request tested?
pytest -v elyra/tests/pipeline/kfp/test_processor_kfp.py
Developer's Certificate of Origin 1.1