-
Notifications
You must be signed in to change notification settings - Fork 3
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 test scripts #70
Add test scripts #70
Conversation
The tests are failing as expected, as the sage config.json is not available on main yet. |
test_workflows.sh
Outdated
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.
Do we want to have this file in the repo? If I understood correctly, actions using act
do not work locally, right?
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.
I removed it, since it's mainly for convenience and only one command.
I guess in theory it could work, but it failed because of Docker in Docker limitations. You'd have to mount the docker socket inside the first container somehow, but I didn't manage to get it working.
test.sh
Outdated
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.
Does it make sense to move this script to .github
? I am not sure what is the policy of .github
content. It would also be nice to rename the script to something a bit more descriptive.
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 idea is that the script is also runnable locally (it is) if you want to run the tests, so I wouldn't put it in .github. What would you suggest for a name? Maybe we should move it to a test folder or something
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.
I see, in that case I would suggest maybe naming it test_cwl_annotations.sh
, what do you think? If the idea is that you can test it locally, we should add a line in the readme that describes how (and why) to run it locally.
I guess the following 3 would be sufficient:
- a line describing the script
- mention the
cwltool
dependency - provide a command on how to run it
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.
I agree with a more specific name, since there are probably many test.sh
scripts around.
Co-authored-by: Vedran Kasalica <[email protected]>
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 added tests are really useful, and the only thing that I would recommend updating is the test.sh
and the readme to document it (as discussed in the comments). After that, the PR can be merged.
This adds mainly: