Skip to content
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

test(pythonapi): add initial tests for Python API #153

Closed
wants to merge 4 commits into from

Conversation

camilovelezr
Copy link
Member

To be merged after #152
This PR adds an initial set of tests for the Python API

@jfennick
Copy link
Contributor

https://workflow-inference-compiler.readthedocs.io/en/latest/dev/gitetiquette.html#pull-requests
Please see the WIC policy w.r.t. pull request size.
This PR is very large (even ignoring the ~250 line diff from the stacked PR), and more importantly I believe there is a fundamentally better approach.
Please give me some time to demonstrate my preferred approach, and then we can revisit this PR.

@agerardin
Copy link

On my end, I am ok with the general approach of having specific test fixtures and unit tests.
However I would be great to provide a bit more structure and clarify test intents.
A conftest.py with fixtures
One or more directories if needed but at least several files for different tests (ex: testing the linking logic, the step input validation etc..)
Also better names to clarify the test intent (along the docstring)... but you knew I would mention that 😄

@jfennick
Copy link
Contributor

PR #165 is now merged. Please take a look at what it does and how it works. In short:

It clones the image-workflows repo, which allows us to clearly separate both

  1. the workflows
  2. the underlying CWL CommandLineTools
    from the code which tests them.

This allows us to do property-based testing
on the entire WIC language, for every single tool and workflow simultaneously.
This in turn reduces the need for unit-tests.

So similar to how we have done it in the mm-workflows repo

  1. We should not be storing workflows in this repository.
  2. We should not be storing CWL CommandLineTools in this repository.

Also notice that the CWL CommandLineTools in this PR are subtly different from the ones in my image-workflows PR. Just two examples (but there are more):

  1. this line demonstrates that there are additional considerations for specific inputs for legacy tools.
  2. these lines demonstrate that CWL has a different philosophy when it comes to environment variables.

Most importantly, since this information is simply not present in the plugin manifest / ICT spec,
we cannot auto-generate CWL CommandLineTools.

Finally, since we are still overwriting the existing hand-crafted CWL CommandLineTools (i.e. from image-workflows PR 4) then these incorrect auto-generated CWL CommandLineTools are still causing problems for our internal users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants