-
Notifications
You must be signed in to change notification settings - Fork 555
Fix workflow tests that are failing #410
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
Conversation
…macrocyclic_monomer.sh
…l to res in test_diffusion.py
…chunks to speed up the workflow.
…is a class method so examples run only once
…o not fail before running examples
rclune
left a comment
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 reformatting so much stuff to make it more readable!
rclune
left a comment
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
lyskov
left a comment
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! Thank you for taking care of of this @woodsh17 !
| TestSubmissionCommands.total_chunks = args.total_chunks | ||
| TestSubmissionCommands.chunk_index = args.chunk_index | ||
|
|
||
| unittest.main(argv=[sys.argv[0]] + remaining) |
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.
@woodsh17 I am assuming that this was simply reformatted with PEP8 settings, - is that so?
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.
Are you referencing changing unittest.main() to unittest.main(argv=[sys.argv[0]] + remaining)? These arguments were added so we don't pass the custom arguments --total_chunks and --chunk_index to unittest, which would be unrecognized.
| else | ||
| echo "A chunk (PID $pid) passed" | ||
| fi | ||
| done |
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! I would personally would start to consider moving this logic into Python/shell script that live inside repository so it could be run/tested without the need to trigger the GH action. But for simple cases (like above) current approach is also works. 👍
After running the test, I found that this was needed This reverts commit ead721f.
rclune
left a comment
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
This PR updates the tests so that all the examples run and if an example fails then the test results in a failure as well. Changes include:
Currently design_ppi_scaffolded, design_timbarrel, and design_ppi_flexible_peptide_with_secondarystructure_specification are failing which should be addressed in other future PRs.